bpo-40636: PEP 618 implementation #20921

gvanrossum · 2020-06-16T18:40:03Z

https://bugs.python.org/issue40636

gvanrossum · 2020-06-16T18:51:11Z

Python/bltinmodule.c

+    }
+    if (i) {
+        PyErr_Clear();
+        return PyErr_Format(PyExc_ValueError, "%s() argument %d is too short",


It bothers me that the implementation gives different errors depending on whether it's the first iterator that's longer or any of the later ones. It seems to assume that the first iterator is "right" and the others are either too short or too long. But isn't it more the case that all we know is that they aren't all of the same length -- we shouldn't judge which one is too long or too short. Perhaps the errors could be changed to something like "iterator N is shorter/longer than the preceding iterators"?

Yeah, perhaps these are a bit too terse. I'll think on some other options (I also agree that unifying the messages is probably a good idea).

gvanrossum · 2020-06-16T23:14:29Z

I removed the "do not merge" from the title because we don't need three ways to indicate this -- two ways (draft mode and the do-not-merge label) seen quite sufficient.

Also one reason not to merge is now gone -- I accepted the PEP.

gvanrossum · 2020-06-16T23:15:04Z

Oh, you do need to add a bpo issue number and a news blurb.

vstinner

A new section is needed in https://docs.python.org/dev/whatsnew/3.10.html doc.
https://docs.python.org/dev/library/functions.html#zip should be updated: `.. versionchanged:: 3.10`` markup is needed to describe what changed.

Note: I'm kind of confused. @gvanrossum created the PR, but @brandtbucher implemented it?

Python/bltinmodule.c

bedevere-bot · 2020-06-17T08:25:07Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

brandtbucher · 2020-06-17T14:52:51Z

A new section is needed in https://docs.python.org/dev/whatsnew/3.10.html doc.

https://docs.python.org/dev/library/functions.html#zip should be updated: ``.. versionchanged:: 3.10`` markup is needed to describe what changed.

@cool-RR volunteered to take the lead on documentation and testing, which can probably be made in separate PRs. I'm happy to monitor the BPO issue until everything is done.

Ram: be sure to include some round-trip pickle tests for zip(..., strict=True) as well. We didn't discuss it before, but it's a decent chunk of what changed here.

Note: I'm kind of confused. @gvanrossum created the PR, but @brandtbucher implemented it?

He opened the PR from my branch in order to provide review comments on it. I was confused at first, too!

brandtbucher · 2020-06-17T14:53:55Z

@gvanrossum it looks like I don't have permissions to change the title of the PR or un-draft it. Can you do so, linking bpo-40636?

gvanrossum · 2020-06-17T15:04:24Z

Sorry about the unusual workflow. Next time I'll just ask the author to create the PR.

vstinner · 2020-06-17T15:22:04Z

Well, if there are permission issues, maybe @brandtbucher can create a new PR.

Lib/test/test_builtin.py

vstinner · 2020-06-18T08:55:48Z

Misc/NEWS.d/next/Core and Builtins/2020-06-17-10-27-17.bpo-40636.MYaCIe.rst

@@ -0,0 +1,3 @@
+:func:`zip` now supports :pep:`618`'s ``strict`` parameter, which raises a
+:exc:`ValueError` if the arguments are exhausted at differing lengths.


(English is not my first language.) Is "differing" correct here?

Suggested change

:exc:`ValueError` if the arguments are exhausted at differing lengths.

:exc:`ValueError` if the arguments are exhausted at different lengths.

Pretty sure "differing" is correct here (I think a strict reading of "different" may mean that no two lengths were the same).

I'll just change it anyways, since "different" probably makes more sense to more people.

vstinner · 2020-06-18T09:23:59Z

Python/bltinmodule.c

                return NULL;
            }
            PyTuple_SET_ITEM(result, i, item);
        }
    }
    return result;
+check:
+    if (PyErr_Occurred() && !PyErr_ExceptionMatches(PyExc_StopIteration)) {


The code in the error label is non-trivial. You may add comments. Here I propose:

// next() on argument i raised an exception (not StopIteration)

Python/bltinmodule.c

vstinner · 2020-06-18T09:26:31Z

Python/bltinmodule.c

                return NULL;
            }
            PyTuple_SET_ITEM(result, i, item);
        }
    }
    return result;
+check:
+    if (PyErr_Occurred() && !PyErr_ExceptionMatches(PyExc_StopIteration)) {


This error path is not tested: would you mind to write an unit test for it?

vstinner · 2020-06-18T09:27:15Z

Python/bltinmodule.c

+                                i + 1, plural, i);
+        }
+        if (PyErr_Occurred()) {
+            return NULL;


This errror path is not tested. Would you mind to add two unit tests for it?

One test where an iterator raises StopIteration

One test where an iterator raises an exception other than StopIteration

... This is bug in this code: if StopIteration is raised, ValueError is not raised whereas a following argument can be not exhausted yet.

zip(a, b,c, strict=True): if a is exhausted, b raises StopIteration, but c is not exhausted => StopIteration is passed through, whereas a ValueError must be raised because c is not exhausted.

I always trip on the fact that Python code raises StopIteration but C code usually doesn't. Nice catch.

Python/bltinmodule.c

gvanrossum · 2020-06-18T14:37:44Z

(...and that's why I am not allowed to approve this PR. :-)

brandtbucher · 2020-06-18T18:40:27Z

Thanks @vstinner, I think I addressed all of your comments.

The macOS test_resource_tracker_reused failure is unreleated (it's currently failing on other PRs).

vstinner

Thanks, the update test suite seems to have a better code coverage ;-)

vstinner · 2020-06-18T21:45:09Z

Lib/test/test_builtin.py

+
+    def test_zip_strict_error_handling(self):
+
+        class E(Exception):


Please use a name longer than a single letter. MyError; Bug, etc. Whatever works :-)

Ditto for "class I:": Iterator, MyIterator, BuggyIter, etc.

vstinner · 2020-06-18T21:47:35Z

Python/bltinmodule.c

+    if (i) {
+        // ValueError: zip() argument 2 is shorter than argument 1
+        // ValueError: zip() argument 3 is shorter than arguments 1-2
+        const char* plural = i == 1 ? " " : "s 1-";  //         ^^^^


Suggested change

const char* plural = i == 1 ? " " : "s 1-"; // ^^^^

const char* plural = i == 1 ? " " : "s 1-";

vstinner · 2020-06-18T21:49:40Z

Lib/test/test_builtin.py

+                    raise E
+                return self.size
+
+        l1 = self.iter_error(zip("AB", I(1), strict=True), E)


Can you check l1 immediately? Currently, it's not trivial to see the relationship between l1 and the expected value. Maybe write assertEqual(self.iter..., [...]) (on multiple lines).

vstinner

LGTM. Thanks for the better code coverage and additional comments.

I also checked for refleak: ./python -m test test_builtin -R 3:3 pass successfully (but I had to disable PtyTests, it's unrelated to this PR.)

brandtbucher · 2020-06-19T13:19:59Z

Thanks!

cool-RR · 2020-06-19T18:47:39Z

Congrats on the merge. If there are any more tests that you'd like me to write, let me know, with details.

cool-RR · 2020-06-20T11:06:51Z

Do you think we should look for places where zip is used in the stdlib that could benefit from strict=True? If I remember correctly there's at least one example in the PEP.

gvanrossum · 2020-06-20T14:48:56Z

I would be very conservative with such changes.

zip() now supports PEP 618's strict parameter, which raises a ValueError if the arguments are exhausted at different lengths. Patch by Brandt Bucher. Co-authored-by: Brandt Bucher <brandtbucher@gmail.com> Co-authored-by: Ram Rachum <ram@rachum.com>

brandtbucher and others added 10 commits April 29, 2020 15:16

Implement zip(..., total=True)

a36a6e5

total -> strict

b59fe70

Tests for zip(total=True)

3ed7f0a

assert -> self.assertEqual, total -> strict

3955ca7

Support all pickle protocols.

e4dfe05

Use __reduce__/__setstate__ for pickling.

4d41e21

Clean up.

892961c

Catch up with master

f13a3a5

Handle StopIteration raised from Pythonland

2d19de5

Clean up error handling

f2235f2

the-knights-who-say-ni added the CLA signed label Jun 16, 2020

bedevere-bot added the awaiting core review label Jun 16, 2020

gvanrossum marked this pull request as draft June 16, 2020 18:40

gvanrossum commented Jun 16, 2020

View reviewed changes

brandtbucher added the DO-NOT-MERGE label Jun 16, 2020

gvanrossum changed the title ~~PEP 618 implementation (don't merge yet)~~ PEP 618 implementation Jun 16, 2020

brandtbucher added the type-feature A feature request or enhancement label Jun 17, 2020

vstinner requested changes Jun 17, 2020

View reviewed changes

Python/bltinmodule.c Show resolved Hide resolved

bedevere-bot removed the awaiting core review label Jun 17, 2020

bedevere-bot added the awaiting changes label Jun 17, 2020

brandtbucher removed the DO-NOT-MERGE label Jun 17, 2020

gvanrossum changed the title ~~PEP 618 implementation~~ bpo-40636: PEP 618 implementation Jun 17, 2020

gvanrossum marked this pull request as ready for review June 17, 2020 15:03

vstinner reviewed Jun 18, 2020

View reviewed changes

cool-RR mentioned this pull request Jun 18, 2020

bpo-40636: Documentation for zip-strict #20961

Merged

brandtbucher added 5 commits June 18, 2020 10:16

Add comments and improve error handling.

baff9c8

Add tests for error handling

2ac3036

Reword NEWS

ea2a7cf

Refactor tests

3613e34

Clean up tests

329afd9

brandtbucher requested a review from vstinner June 18, 2020 18:40

brandtbucher added awaiting change review and removed awaiting changes labels Jun 18, 2020

vstinner reviewed Jun 18, 2020

View reviewed changes

brandtbucher added 2 commits June 18, 2020 15:56

Remove comment

e86daab

Rename/reorder tests

d77134e

brandtbucher requested a review from vstinner June 19, 2020 00:14

vstinner approved these changes Jun 19, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 19, 2020

vstinner merged commit 310f6aa into python:master Jun 19, 2020

bedevere-bot removed the awaiting merge label Jun 19, 2020

gvanrossum deleted the zip-strict branch June 20, 2020 14:49

Snowapril mentioned this pull request Oct 6, 2021

Add length check, reduce, setstate on zip from cpython 3.10 RustPython/RustPython#3235

Merged

		@@ -0,0 +1,3 @@
		:func:`zip` now supports :pep:`618`'s ``strict`` parameter, which raises a
		:exc:`ValueError` if the arguments are exhausted at differing lengths.


		def test_zip_strict_error_handling(self):

		class E(Exception):

	const char* plural = i == 1 ? " " : "s 1-"; // ^^^^
	const char* plural = i == 1 ? " " : "s 1-";

Uh oh!

bpo-40636: PEP 618 implementation #20921

bpo-40636: PEP 618 implementation #20921

Uh oh!

Conversation

gvanrossum commented Jun 16, 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!

gvanrossum commented Jun 16, 2020

Uh oh!

gvanrossum commented Jun 16, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Jun 17, 2020

Uh oh!

brandtbucher commented Jun 17, 2020

Uh oh!

brandtbucher commented Jun 17, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jun 17, 2020

Uh oh!

vstinner commented Jun 17, 2020

Uh oh!

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!

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!

Uh oh!

gvanrossum commented Jun 18, 2020

Uh oh!

brandtbucher commented Jun 18, 2020

Uh oh!

vstinner left a comment

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!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented Jun 19, 2020

Uh oh!

cool-RR commented Jun 19, 2020

Uh oh!

cool-RR commented Jun 20, 2020

Uh oh!

gvanrossum commented Jun 20, 2020

Uh oh!

Uh oh!

gvanrossum commented Jun 16, 2020 •

edited by bedevere-bot

Loading

brandtbucher commented Jun 17, 2020 •

edited by bedevere-bot

Loading