gh-83791: Raise TypeError for len(memoryview_0d) #18463

eric-wieser · 2020-02-11T15:51:33Z

This makes memoryview.__len__ consistent with memoryview.__getitem__.

https://bugs.python.org/issue39610

Issue: memoryview.__len__ should raise an exception for 0d buffers #83791

eric-wieser · 2020-02-11T16:47:27Z

Lib/ctypes/test/test_pep3118.py

-                if v.shape:
-                    n = 1
-                    for dim in v.shape:
-                        n = n * dim
-                    self.assertEqual(n * v.itemsize, len(v.tobytes()))
+                n = 1
+                for dim in v.shape:
+                    n = n * dim
+                self.assertEqual(n * v.itemsize, len(v.tobytes()))


This if was a pointless special case

eric-wieser · 2020-02-11T16:48:00Z

Lib/ctypes/test/test_pep3118.py

-                if v.shape:
-                    n = 1
-                    for dim in v.shape:
-                        n = n * dim
-                    self.assertEqual(n, len(v))
+                n = 1
+                for dim in v.shape:
+                    n = n * dim
+                self.assertEqual(n * v.itemsize, len(v.tobytes()))


This if always evaluated to False, so the garbage code within it never ran. Fixed to be identical to the above.

Lib/ctypes/test/test_pep3118.py

This makes `memoryview.__len__` consistent with `memoryview.__getitem__`. Also activates and fixes a bug in dead code in the `test_endian` test, which was only being run on 0d arrays.

codecov · 2020-03-02T11:58:23Z

Codecov Report

Merging #18463 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18463      +/-   ##
==========================================
+ Coverage   82.11%   82.13%   +0.02%     
==========================================
  Files        1955     1955              
  Lines      588958   584775    -4183     
  Branches    44428    44488      +60     
==========================================
- Hits       483632   480330    -3302     
+ Misses      95677    94794     -883     
- Partials     9649     9651       +2

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Modules/_decimal/libmpdec/umodarith.h	`80.76% <0.00%> (-19.24%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
... and 399 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e7ea5...c210029. Read the comment docs.

serhiy-storchaka · 2020-03-14T13:14:58Z

Doc/library/stdtypes.rst

-   the length is equal to the length of the nested list representation of
-   the view. The :class:`~memoryview.itemsize` attribute will give you the
-   number of bytes in a single element.
+   ``len(view)`` is equal to the length of :class:`~memoryview.tolist`, which


Since the documented behavior was changed, it needs a versionchanged directive and an entry in What's New.

Should I put this in the 3.9 what's new? This sounds like a recipe for merge conflicts, until the rest of the patch has general approval.

Just checking in on this - what's the procedure for adding a what's new entry if I have no idea which python version this will end up being merged into?

You can use version 3.10. It cannot go into earlier versions since it's a behavior change. And we have ample time for deliberation on how the documented behavior emerged.

I've update it to say 3.11, since it's been two years since I changed it to say 3.10. Let me know if it should be 3.12 instead.

serhiy-storchaka · 2020-03-14T13:18:23Z

Misc/NEWS.d/next/Core and Builtins/2020-02-11-15-54-40.bpo-39610.fvgsCl.rst

@@ -0,0 +1,8 @@
+0d ``memoryview`` objects no longer have a ``len``::


AFAIK there are issues with multi-paragraph NEWS entry.

Would you prefer me to keep just the first line here?

I am not sure that the meaning of words "have a len()" is clear.

Maybe something like "len() for 0-dimensional memoryview objects raises now a TypeError"?

I've changed to your proposed wording.

Doc/library/stdtypes.rst

…__-issue39610

ghost · 2022-07-07T20:52:40Z

All commit authors signed the Contributor License Agreement.

Doc/library/stdtypes.rst

eric-wieser · 2023-02-21T18:10:42Z

Misc/NEWS.d/next/Core and Builtins/2020-02-11-15-54-40.bpo-39610.fvgsCl.rst

+``len()`` for 0-dimensional ``memoryview`` objects (such as ``memoryview(ctypes.c_uint8(42))``) now raises a ``TypeError``.
+Previously this returned ``1``, which was not consistent with ``mem_0d[0]`` raising an ``IndexError``.


Presumably there should be a :class: or :type: or :exc: in here somewhere; can a reviewer advise what the convention is?

eric-wieser · 2023-04-12T22:43:10Z

@mdickinson: any chance you're willing to review one more buffer-related PR? This one isn't quite as old as the other two of mine you rescued, but it's getting close!

mdickinson

The change LGTM in principle, and code looks fine, too.

Question: did you consider using ValueError instead of TypeError? Given that len works for most memoryview objects, the failure is arguably due to value rather than to type. It doesn't seem clear cut: I know NumPy raises TypeError for len(np.array(some_scalar)), and it would be annoying to be inconsistent with that.

I'll leave this open for a few days in case anyone else previously involved in the discussion wants to chip in, and then merge. (@eric-wieser Feel free to ping me if this remains unmerged by April 22nd. The beta 1 cutoff is May 8th.)

eric-wieser · 2023-04-13T07:09:33Z

I'm afraid I don't remember what I considered at the time (which is obviously a lesson in writing better commit messages). I'll see if I can come up with an argument either way.

eric-wieser · 2023-04-19T23:34:45Z

I suspect consistency with numpy was the main argument; along with consistency with to_list:

>>> import numpy as np
>>> arr_0d = np.array(42)
>>> mem_0d = memoryview(arr_0d)

>>> len(arr_0d)
TypeError: len() of unsized object
>>> len(mem_0d.tolist())
TypeError: object of type 'int' has no len()
>>> len(mem_0d)
TypeError: 0-dim memory has no length  # with this PR

the-knights-who-say-ni added the CLA signed label Feb 11, 2020

bedevere-bot added the awaiting review label Feb 11, 2020

eric-wieser force-pushed the fix-memorview-__len__-issue39610 branch from 530d5f3 to 5200b72 Compare February 11, 2020 16:44

eric-wieser commented Feb 11, 2020

View reviewed changes

Lib/ctypes/test/test_pep3118.py Outdated Show resolved Hide resolved

eric-wieser force-pushed the fix-memorview-__len__-issue39610 branch from 5200b72 to b897e68 Compare February 11, 2020 17:15

pitrou requested a review from skrah February 26, 2020 23:46

bpo-39610: Raise TypeError for len(memoryview_0d)

c210029

This makes `memoryview.__len__` consistent with `memoryview.__getitem__`. Also activates and fixes a bug in dead code in the `test_endian` test, which was only being run on 0d arrays.

eric-wieser force-pushed the fix-memorview-__len__-issue39610 branch from b897e68 to c210029 Compare March 2, 2020 11:26

serhiy-storchaka reviewed Mar 14, 2020

View reviewed changes

eric-wieser commented May 30, 2020

View reviewed changes

Doc/library/stdtypes.rst Show resolved Hide resolved

eric-wieser added 2 commits August 29, 2020 13:36

Update Doc/library/stdtypes.rst

a352888

Merge remote-tracking branch 'upstream/main' into fix-memorview-__len…

209644d

…__-issue39610

Fix documentation build

dbad24a

eric-wieser force-pushed the fix-memorview-__len__-issue39610 branch from 34d362d to dbad24a Compare July 7, 2022 22:32

ezio-melotti removed the CLA signed label Jul 13, 2022

eric-wieser requested review from serhiy-storchaka and removed request for skrah August 15, 2022 22:54

Merge branch 'main' into fix-memorview-__len__-issue39610

6efc1c6

eric-wieser commented Feb 21, 2023

View reviewed changes

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved

arhadthedev changed the title ~~bpo-39610: Raise TypeError for len(memoryview_0d)~~ gh-83791: Raise TypeError for len(memoryview_0d) Feb 21, 2023

arhadthedev added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 21, 2023

Bump version in versionchanged

d6b38b9

eric-wieser commented Feb 21, 2023

View reviewed changes

eric-wieser added 2 commits April 12, 2023 23:38

Sphinx formatting

4e24952

Merge branch 'main' into fix-memorview-__len__-issue39610

31ca563

mdickinson approved these changes Apr 13, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 13, 2023

mdickinson merged commit 3d2a468 into python:main Apr 22, 2023

bedevere-bot removed the awaiting merge label Apr 22, 2023

mdickinson mentioned this pull request Apr 22, 2023

memoryview.__len__ should raise an exception for 0d buffers #83791

Closed

		@@ -0,0 +1,8 @@
		0d ``memoryview`` objects no longer have a ``len``::

		``len()`` for 0-dimensional ``memoryview`` objects (such as ``memoryview(ctypes.c_uint8(42))``) now raises a ``TypeError``.
		Previously this returned ``1``, which was not consistent with ``mem_0d[0]`` raising an ``IndexError``.

Uh oh!

gh-83791: Raise TypeError for len(memoryview_0d) #18463

gh-83791: Raise TypeError for len(memoryview_0d) #18463

Uh oh!

Conversation

eric-wieser commented Feb 11, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Jul 7, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Apr 12, 2023

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Apr 13, 2023

Uh oh!

eric-wieser commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eric-wieser commented Feb 11, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Mar 2, 2020 •

edited

Loading

ghost commented Jul 7, 2022 •

edited by ghost

Loading

eric-wieser commented Apr 19, 2023 •

edited

Loading