Patchwork py3: catch StopIteration from next() in generatorset

login
register
mail settings
Submitter via Mercurial-devel
Date June 20, 2017, 9:45 p.m.
Message ID <f86d21c457209f5f5a13.1497995157@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/21545/
State Accepted
Headers show

Comments

via Mercurial-devel - June 20, 2017, 9:45 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1497992441 25200
#      Tue Jun 20 14:00:41 2017 -0700
# Node ID f86d21c457209f5f5a139bcc808be33fbd930424
# Parent  0906385672d754f13a512fe70759f0463a069f1e
py3: catch StopIteration from next() in generatorset

IIUC, letting the StopIteration through would not cause any bugs, but
not doing it makes the test-py3-commands.t pass.

I have also diligently gone through all uses of next() in our code
base. They either:

 * are not called from a generator
 * pass a default value to next()
 * catch StopException
 * work on infinite iterators
 * request a fixed number of items that matches the generated number
 * are about batching in wireproto which I didn't quite follow

I'd appreciate if Augie or someone else could take a look at the
wireproto batching and convince themselves that the next(batchable)
calls there will not raise a StopIteration.
Sean Farley - June 20, 2017, 9:53 p.m.
Martin von Zweigbergk via Mercurial-devel
<mercurial-devel@mercurial-scm.org> writes:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1497992441 25200
> #      Tue Jun 20 14:00:41 2017 -0700
> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
> # Parent  0906385672d754f13a512fe70759f0463a069f1e
> py3: catch StopIteration from next() in generatorset
>
> IIUC, letting the StopIteration through would not cause any bugs, but
> not doing it makes the test-py3-commands.t pass.
>
> I have also diligently gone through all uses of next() in our code
> base. They either:
>
>  * are not called from a generator
>  * pass a default value to next()
>  * catch StopException
>  * work on infinite iterators
>  * request a fixed number of items that matches the generated number
>  * are about batching in wireproto which I didn't quite follow
>
> I'd appreciate if Augie or someone else could take a look at the
> wireproto batching and convince themselves that the next(batchable)
> calls there will not raise a StopIteration.

I was just thinking of doing something like this. Shouldn't we just
'return None' in Mercurial instead of 'raise StopIteration'?
via Mercurial-devel - June 20, 2017, 10:24 p.m.
On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
> Martin von Zweigbergk via Mercurial-devel
> <mercurial-devel@mercurial-scm.org> writes:
>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1497992441 25200
>> #      Tue Jun 20 14:00:41 2017 -0700
>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>> py3: catch StopIteration from next() in generatorset
>>
>> IIUC, letting the StopIteration through would not cause any bugs, but
>> not doing it makes the test-py3-commands.t pass.
>>
>> I have also diligently gone through all uses of next() in our code
>> base. They either:
>>
>>  * are not called from a generator
>>  * pass a default value to next()
>>  * catch StopException
>>  * work on infinite iterators
>>  * request a fixed number of items that matches the generated number
>>  * are about batching in wireproto which I didn't quite follow
>>
>> I'd appreciate if Augie or someone else could take a look at the
>> wireproto batching and convince themselves that the next(batchable)
>> calls there will not raise a StopIteration.
>
> I was just thinking of doing something like this. Shouldn't we just
> 'return None' in Mercurial instead of 'raise StopIteration'?

I thought "return" was just short for "return None". Are you just
saying that we prefer "return None" in Mercurial? Should I send a v2?
via Mercurial-devel - June 20, 2017, 10:26 p.m.
On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>> Martin von Zweigbergk via Mercurial-devel
>> <mercurial-devel@mercurial-scm.org> writes:
>>
>>> # HG changeset patch
>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>> # Date 1497992441 25200
>>> #      Tue Jun 20 14:00:41 2017 -0700
>>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>>> py3: catch StopIteration from next() in generatorset
>>>
>>> IIUC, letting the StopIteration through would not cause any bugs, but
>>> not doing it makes the test-py3-commands.t pass.
>>>
>>> I have also diligently gone through all uses of next() in our code
>>> base. They either:
>>>
>>>  * are not called from a generator
>>>  * pass a default value to next()
>>>  * catch StopException
>>>  * work on infinite iterators
>>>  * request a fixed number of items that matches the generated number
>>>  * are about batching in wireproto which I didn't quite follow
>>>
>>> I'd appreciate if Augie or someone else could take a look at the
>>> wireproto batching and convince themselves that the next(batchable)
>>> calls there will not raise a StopIteration.
>>
>> I was just thinking of doing something like this. Shouldn't we just
>> 'return None' in Mercurial instead of 'raise StopIteration'?
>
> I thought "return" was just short for "return None". Are you just
> saying that we prefer "return None" in Mercurial? Should I send a v2?

Oh, never mind. You're of course talking about not using "raise
StopIteration" in general, not specifically about this patch. Yes,
that's probably true. I think there were just a few more cases of
"raise StopIteration" to deal with.

Anyway, that won't fix this case, because next() itself raise StopIteration.
Sean Farley - June 20, 2017, 10:27 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>> Martin von Zweigbergk via Mercurial-devel
>> <mercurial-devel@mercurial-scm.org> writes:
>>
>>> # HG changeset patch
>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>> # Date 1497992441 25200
>>> #      Tue Jun 20 14:00:41 2017 -0700
>>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>>> py3: catch StopIteration from next() in generatorset
>>>
>>> IIUC, letting the StopIteration through would not cause any bugs, but
>>> not doing it makes the test-py3-commands.t pass.
>>>
>>> I have also diligently gone through all uses of next() in our code
>>> base. They either:
>>>
>>>  * are not called from a generator
>>>  * pass a default value to next()
>>>  * catch StopException
>>>  * work on infinite iterators
>>>  * request a fixed number of items that matches the generated number
>>>  * are about batching in wireproto which I didn't quite follow
>>>
>>> I'd appreciate if Augie or someone else could take a look at the
>>> wireproto batching and convince themselves that the next(batchable)
>>> calls there will not raise a StopIteration.
>>
>> I was just thinking of doing something like this. Shouldn't we just
>> 'return None' in Mercurial instead of 'raise StopIteration'?
>
> I thought "return" was just short for "return None". Are you just
> saying that we prefer "return None" in Mercurial? Should I send a v2?

Sorry, no, I was just being sloppy ;-P I meant going through and
replacing 'raise StopIteration' with 'return' (e.g. return None) and
therefore we shouldn't need to catch StopIteration, correct? Here are
the calls I found:

../mercurial/commandserver.py
142:            raise StopIteration

../mercurial/manifest.py
124:            raise StopIteration
146:            raise StopIteration

../mercurial/patch.py
151:                raise StopIteration
Sean Farley - June 20, 2017, 10:30 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>>> Martin von Zweigbergk via Mercurial-devel
>>> <mercurial-devel@mercurial-scm.org> writes:
>>>
>>>> # HG changeset patch
>>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>>> # Date 1497992441 25200
>>>> #      Tue Jun 20 14:00:41 2017 -0700
>>>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>>>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>>>> py3: catch StopIteration from next() in generatorset
>>>>
>>>> IIUC, letting the StopIteration through would not cause any bugs, but
>>>> not doing it makes the test-py3-commands.t pass.
>>>>
>>>> I have also diligently gone through all uses of next() in our code
>>>> base. They either:
>>>>
>>>>  * are not called from a generator
>>>>  * pass a default value to next()
>>>>  * catch StopException
>>>>  * work on infinite iterators
>>>>  * request a fixed number of items that matches the generated number
>>>>  * are about batching in wireproto which I didn't quite follow
>>>>
>>>> I'd appreciate if Augie or someone else could take a look at the
>>>> wireproto batching and convince themselves that the next(batchable)
>>>> calls there will not raise a StopIteration.
>>>
>>> I was just thinking of doing something like this. Shouldn't we just
>>> 'return None' in Mercurial instead of 'raise StopIteration'?
>>
>> I thought "return" was just short for "return None". Are you just
>> saying that we prefer "return None" in Mercurial? Should I send a v2?
>
> Oh, never mind. You're of course talking about not using "raise
> StopIteration" in general, not specifically about this patch. Yes,
> that's probably true. I think there were just a few more cases of
> "raise StopIteration" to deal with.
>
> Anyway, that won't fix this case, because next() itself raise StopIteration.

Seems I've had two race conditions today. Ah, I see. I'll be quiet now :-)
Gregory Szorc - June 21, 2017, 3:38 a.m.
On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk via Mercurial-devel <
mercurial-devel@mercurial-scm.org> wrote:

> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
> > Martin von Zweigbergk via Mercurial-devel
> > <mercurial-devel@mercurial-scm.org> writes:
> >
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1497992441 25200
> >> #      Tue Jun 20 14:00:41 2017 -0700
> >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
> >> # Parent  0906385672d754f13a512fe70759f0463a069f1e
> >> py3: catch StopIteration from next() in generatorset
> >>
> >> IIUC, letting the StopIteration through would not cause any bugs, but
> >> not doing it makes the test-py3-commands.t pass.
> >>
> >> I have also diligently gone through all uses of next() in our code
> >> base. They either:
> >>
> >>  * are not called from a generator
> >>  * pass a default value to next()
> >>  * catch StopException
> >>  * work on infinite iterators
> >>  * request a fixed number of items that matches the generated number
> >>  * are about batching in wireproto which I didn't quite follow
> >>
> >> I'd appreciate if Augie or someone else could take a look at the
> >> wireproto batching and convince themselves that the next(batchable)
> >> calls there will not raise a StopIteration.
> >
> > I was just thinking of doing something like this. Shouldn't we just
> > 'return None' in Mercurial instead of 'raise StopIteration'?
>
> I thought "return" was just short for "return None". Are you just
> saying that we prefer "return None" in Mercurial? Should I send a v2?
>

I'm not sure what the Python language itself specifies, but from the
CPython API, you need to return a PyObject from every function and NULL is
interpreted as an exception has been raised. If you have nothing to return,
you Py_RETURN_NONE to return None as a PyObject.

Yes, the return value of "return" is None. However, "return" and "return
None" aren't semantically identical. For generators (functions containing
"yield"), it is illegal to have "yield" and "return x" in the same
function: you can only use bareword "return" for control flow purposes. So
for this patch, "return None" will raise SyntaxError since the function has
"yield."
via Mercurial-devel - June 21, 2017, 6:09 a.m.
On Tue, Jun 20, 2017 at 8:38 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk via Mercurial-devel
> <mercurial-devel@mercurial-scm.org> wrote:
>>
>> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>> > Martin von Zweigbergk via Mercurial-devel
>> > <mercurial-devel@mercurial-scm.org> writes:
>> >
>> >> # HG changeset patch
>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> # Date 1497992441 25200
>> >> #      Tue Jun 20 14:00:41 2017 -0700
>> >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>> >> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>> >> py3: catch StopIteration from next() in generatorset
>> >>
>> >> IIUC, letting the StopIteration through would not cause any bugs, but
>> >> not doing it makes the test-py3-commands.t pass.
>> >>
>> >> I have also diligently gone through all uses of next() in our code
>> >> base. They either:
>> >>
>> >>  * are not called from a generator
>> >>  * pass a default value to next()
>> >>  * catch StopException
>> >>  * work on infinite iterators
>> >>  * request a fixed number of items that matches the generated number
>> >>  * are about batching in wireproto which I didn't quite follow
>> >>
>> >> I'd appreciate if Augie or someone else could take a look at the
>> >> wireproto batching and convince themselves that the next(batchable)
>> >> calls there will not raise a StopIteration.
>> >
>> > I was just thinking of doing something like this. Shouldn't we just
>> > 'return None' in Mercurial instead of 'raise StopIteration'?
>>
>> I thought "return" was just short for "return None". Are you just
>> saying that we prefer "return None" in Mercurial? Should I send a v2?
>
>
> I'm not sure what the Python language itself specifies, but from the CPython
> API, you need to return a PyObject from every function and NULL is
> interpreted as an exception has been raised. If you have nothing to return,
> you Py_RETURN_NONE to return None as a PyObject.
>
> Yes, the return value of "return" is None. However, "return" and "return
> None" aren't semantically identical. For generators (functions containing
> "yield"), it is illegal to have "yield" and "return x" in the same function:
> you can only use bareword "return" for control flow purposes. So for this
> patch, "return None" will raise SyntaxError since the function has "yield."

So, in summary, you both think the patch is good? If so, could one of
you queue it? :-)
via Mercurial-devel - June 21, 2017, 6:13 a.m.
On Tue, Jun 20, 2017 at 11:09 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Tue, Jun 20, 2017 at 8:38 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk via Mercurial-devel
>> <mercurial-devel@mercurial-scm.org> wrote:
>>>
>>> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>>> > Martin von Zweigbergk via Mercurial-devel
>>> > <mercurial-devel@mercurial-scm.org> writes:
>>> >
>>> >> # HG changeset patch
>>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>>> >> # Date 1497992441 25200
>>> >> #      Tue Jun 20 14:00:41 2017 -0700
>>> >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>>> >> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>>> >> py3: catch StopIteration from next() in generatorset
>>> >>
>>> >> IIUC, letting the StopIteration through would not cause any bugs, but
>>> >> not doing it makes the test-py3-commands.t pass.
>>> >>
>>> >> I have also diligently gone through all uses of next() in our code
>>> >> base. They either:
>>> >>
>>> >>  * are not called from a generator
>>> >>  * pass a default value to next()
>>> >>  * catch StopException
>>> >>  * work on infinite iterators
>>> >>  * request a fixed number of items that matches the generated number
>>> >>  * are about batching in wireproto which I didn't quite follow
>>> >>
>>> >> I'd appreciate if Augie or someone else could take a look at the
>>> >> wireproto batching and convince themselves that the next(batchable)
>>> >> calls there will not raise a StopIteration.
>>> >
>>> > I was just thinking of doing something like this. Shouldn't we just
>>> > 'return None' in Mercurial instead of 'raise StopIteration'?
>>>
>>> I thought "return" was just short for "return None". Are you just
>>> saying that we prefer "return None" in Mercurial? Should I send a v2?
>>
>>
>> I'm not sure what the Python language itself specifies, but from the CPython
>> API, you need to return a PyObject from every function and NULL is
>> interpreted as an exception has been raised. If you have nothing to return,
>> you Py_RETURN_NONE to return None as a PyObject.
>>
>> Yes, the return value of "return" is None. However, "return" and "return
>> None" aren't semantically identical. For generators (functions containing
>> "yield"), it is illegal to have "yield" and "return x" in the same function:
>> you can only use bareword "return" for control flow purposes. So for this
>> patch, "return None" will raise SyntaxError since the function has "yield."
>
> So, in summary, you both think the patch is good? If so, could one of
> you queue it? :-)

Sorry, I realized after sending that that may have sounded pushy. I
also realize you may simply not have had time. I just meant to check
if either of you still head reservations about the patch (potential
wireproto changes is out of scope for this patch, IMO).
Yuya Nishihara - June 21, 2017, 3:29 p.m.
On Tue, 20 Jun 2017 14:45:57 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1497992441 25200
> #      Tue Jun 20 14:00:41 2017 -0700
> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
> # Parent  0906385672d754f13a512fe70759f0463a069f1e
> py3: catch StopIteration from next() in generatorset

Looks good as this _next() exists inside a generator function.
Queued, thanks.
Yuya Nishihara - June 21, 2017, 3:36 p.m.
On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote:
> Martin von Zweigbergk <martinvonz@google.com> writes:
> 
> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
> >> Martin von Zweigbergk via Mercurial-devel
> >> <mercurial-devel@mercurial-scm.org> writes:
> >>
> >>> # HG changeset patch
> >>> # User Martin von Zweigbergk <martinvonz@google.com>
> >>> # Date 1497992441 25200
> >>> #      Tue Jun 20 14:00:41 2017 -0700
> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
> >>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
> >>> py3: catch StopIteration from next() in generatorset
> >>>
> >>> IIUC, letting the StopIteration through would not cause any bugs, but
> >>> not doing it makes the test-py3-commands.t pass.
> >>>
> >>> I have also diligently gone through all uses of next() in our code
> >>> base. They either:
> >>>
> >>>  * are not called from a generator
> >>>  * pass a default value to next()
> >>>  * catch StopException
> >>>  * work on infinite iterators
> >>>  * request a fixed number of items that matches the generated number
> >>>  * are about batching in wireproto which I didn't quite follow
> >>>
> >>> I'd appreciate if Augie or someone else could take a look at the
> >>> wireproto batching and convince themselves that the next(batchable)
> >>> calls there will not raise a StopIteration.
> >>
> >> I was just thinking of doing something like this. Shouldn't we just
> >> 'return None' in Mercurial instead of 'raise StopIteration'?
> >
> > I thought "return" was just short for "return None". Are you just
> > saying that we prefer "return None" in Mercurial? Should I send a v2?
> 
> Sorry, no, I was just being sloppy ;-P I meant going through and
> replacing 'raise StopIteration' with 'return' (e.g. return None) and
> therefore we shouldn't need to catch StopIteration, correct? Here are
> the calls I found:
> 
> ../mercurial/commandserver.py
> 142:            raise StopIteration
> 
> ../mercurial/manifest.py
> 124:            raise StopIteration
> 146:            raise StopIteration
> 
> ../mercurial/patch.py
> 151:                raise StopIteration

IIUC, these StopIteration are valid since they are iterator/generator types.
On the other hand, generatorset._iterator.gen() is a generator function, which
should just end (without raising StopIteration) when all items are yielded.
Sean Farley - June 21, 2017, 5:35 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote:
>> Martin von Zweigbergk <martinvonz@google.com> writes:
>> 
>> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>> >> Martin von Zweigbergk via Mercurial-devel
>> >> <mercurial-devel@mercurial-scm.org> writes:
>> >>
>> >>> # HG changeset patch
>> >>> # User Martin von Zweigbergk <martinvonz@google.com>
>> >>> # Date 1497992441 25200
>> >>> #      Tue Jun 20 14:00:41 2017 -0700
>> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>> >>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>> >>> py3: catch StopIteration from next() in generatorset
>> >>>
>> >>> IIUC, letting the StopIteration through would not cause any bugs, but
>> >>> not doing it makes the test-py3-commands.t pass.
>> >>>
>> >>> I have also diligently gone through all uses of next() in our code
>> >>> base. They either:
>> >>>
>> >>>  * are not called from a generator
>> >>>  * pass a default value to next()
>> >>>  * catch StopException
>> >>>  * work on infinite iterators
>> >>>  * request a fixed number of items that matches the generated number
>> >>>  * are about batching in wireproto which I didn't quite follow
>> >>>
>> >>> I'd appreciate if Augie or someone else could take a look at the
>> >>> wireproto batching and convince themselves that the next(batchable)
>> >>> calls there will not raise a StopIteration.
>> >>
>> >> I was just thinking of doing something like this. Shouldn't we just
>> >> 'return None' in Mercurial instead of 'raise StopIteration'?
>> >
>> > I thought "return" was just short for "return None". Are you just
>> > saying that we prefer "return None" in Mercurial? Should I send a v2?
>> 
>> Sorry, no, I was just being sloppy ;-P I meant going through and
>> replacing 'raise StopIteration' with 'return' (e.g. return None) and
>> therefore we shouldn't need to catch StopIteration, correct? Here are
>> the calls I found:
>> 
>> ../mercurial/commandserver.py
>> 142:            raise StopIteration
>> 
>> ../mercurial/manifest.py
>> 124:            raise StopIteration
>> 146:            raise StopIteration
>> 
>> ../mercurial/patch.py
>> 151:                raise StopIteration
>
> IIUC, these StopIteration are valid since they are iterator/generator types.
> On the other hand, generatorset._iterator.gen() is a generator function, which
> should just end (without raising StopIteration) when all items are yielded.

The reason I brought it up is that python3.6 now emits a warning about
'raise StopIteration' being deprecated:

https://www.python.org/dev/peps/pep-0479/

I'll send an RFC series and see what the feedback is.
via Mercurial-devel - June 21, 2017, 5:40 p.m.
On Wed, Jun 21, 2017 at 10:35 AM, Sean Farley <sean@farley.io> wrote:
> Yuya Nishihara <yuya@tcha.org> writes:
>
>> On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote:
>>> Martin von Zweigbergk <martinvonz@google.com> writes:
>>>
>>> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>>> >> Martin von Zweigbergk via Mercurial-devel
>>> >> <mercurial-devel@mercurial-scm.org> writes:
>>> >>
>>> >>> # HG changeset patch
>>> >>> # User Martin von Zweigbergk <martinvonz@google.com>
>>> >>> # Date 1497992441 25200
>>> >>> #      Tue Jun 20 14:00:41 2017 -0700
>>> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>>> >>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>>> >>> py3: catch StopIteration from next() in generatorset
>>> >>>
>>> >>> IIUC, letting the StopIteration through would not cause any bugs, but
>>> >>> not doing it makes the test-py3-commands.t pass.
>>> >>>
>>> >>> I have also diligently gone through all uses of next() in our code
>>> >>> base. They either:
>>> >>>
>>> >>>  * are not called from a generator
>>> >>>  * pass a default value to next()
>>> >>>  * catch StopException
>>> >>>  * work on infinite iterators
>>> >>>  * request a fixed number of items that matches the generated number
>>> >>>  * are about batching in wireproto which I didn't quite follow
>>> >>>
>>> >>> I'd appreciate if Augie or someone else could take a look at the
>>> >>> wireproto batching and convince themselves that the next(batchable)
>>> >>> calls there will not raise a StopIteration.
>>> >>
>>> >> I was just thinking of doing something like this. Shouldn't we just
>>> >> 'return None' in Mercurial instead of 'raise StopIteration'?
>>> >
>>> > I thought "return" was just short for "return None". Are you just
>>> > saying that we prefer "return None" in Mercurial? Should I send a v2?
>>>
>>> Sorry, no, I was just being sloppy ;-P I meant going through and
>>> replacing 'raise StopIteration' with 'return' (e.g. return None) and
>>> therefore we shouldn't need to catch StopIteration, correct? Here are
>>> the calls I found:
>>>
>>> ../mercurial/commandserver.py
>>> 142:            raise StopIteration
>>>
>>> ../mercurial/manifest.py
>>> 124:            raise StopIteration
>>> 146:            raise StopIteration
>>>
>>> ../mercurial/patch.py
>>> 151:                raise StopIteration
>>
>> IIUC, these StopIteration are valid since they are iterator/generator types.
>> On the other hand, generatorset._iterator.gen() is a generator function, which
>> should just end (without raising StopIteration) when all items are yielded.
>
> The reason I brought it up is that python3.6 now emits a warning about
> 'raise StopIteration' being deprecated:
>
> https://www.python.org/dev/peps/pep-0479/

But that's *inside generators* and Yuya was saying that the remaining
few cases in our code base are not inside generator (functions). Maybe
I misunderstood, but my impression was that that would still be
allowed.

>
> I'll send an RFC series and see what the feedback is.
Sean Farley - June 21, 2017, 5:42 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Wed, Jun 21, 2017 at 10:35 AM, Sean Farley <sean@farley.io> wrote:
>> Yuya Nishihara <yuya@tcha.org> writes:
>>
>>> On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote:
>>>> Martin von Zweigbergk <martinvonz@google.com> writes:
>>>>
>>>> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote:
>>>> >> Martin von Zweigbergk via Mercurial-devel
>>>> >> <mercurial-devel@mercurial-scm.org> writes:
>>>> >>
>>>> >>> # HG changeset patch
>>>> >>> # User Martin von Zweigbergk <martinvonz@google.com>
>>>> >>> # Date 1497992441 25200
>>>> >>> #      Tue Jun 20 14:00:41 2017 -0700
>>>> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424
>>>> >>> # Parent  0906385672d754f13a512fe70759f0463a069f1e
>>>> >>> py3: catch StopIteration from next() in generatorset
>>>> >>>
>>>> >>> IIUC, letting the StopIteration through would not cause any bugs, but
>>>> >>> not doing it makes the test-py3-commands.t pass.
>>>> >>>
>>>> >>> I have also diligently gone through all uses of next() in our code
>>>> >>> base. They either:
>>>> >>>
>>>> >>>  * are not called from a generator
>>>> >>>  * pass a default value to next()
>>>> >>>  * catch StopException
>>>> >>>  * work on infinite iterators
>>>> >>>  * request a fixed number of items that matches the generated number
>>>> >>>  * are about batching in wireproto which I didn't quite follow
>>>> >>>
>>>> >>> I'd appreciate if Augie or someone else could take a look at the
>>>> >>> wireproto batching and convince themselves that the next(batchable)
>>>> >>> calls there will not raise a StopIteration.
>>>> >>
>>>> >> I was just thinking of doing something like this. Shouldn't we just
>>>> >> 'return None' in Mercurial instead of 'raise StopIteration'?
>>>> >
>>>> > I thought "return" was just short for "return None". Are you just
>>>> > saying that we prefer "return None" in Mercurial? Should I send a v2?
>>>>
>>>> Sorry, no, I was just being sloppy ;-P I meant going through and
>>>> replacing 'raise StopIteration' with 'return' (e.g. return None) and
>>>> therefore we shouldn't need to catch StopIteration, correct? Here are
>>>> the calls I found:
>>>>
>>>> ../mercurial/commandserver.py
>>>> 142:            raise StopIteration
>>>>
>>>> ../mercurial/manifest.py
>>>> 124:            raise StopIteration
>>>> 146:            raise StopIteration
>>>>
>>>> ../mercurial/patch.py
>>>> 151:                raise StopIteration
>>>
>>> IIUC, these StopIteration are valid since they are iterator/generator types.
>>> On the other hand, generatorset._iterator.gen() is a generator function, which
>>> should just end (without raising StopIteration) when all items are yielded.
>>
>> The reason I brought it up is that python3.6 now emits a warning about
>> 'raise StopIteration' being deprecated:
>>
>> https://www.python.org/dev/peps/pep-0479/
>
> But that's *inside generators* and Yuya was saying that the remaining
> few cases in our code base are not inside generator (functions). Maybe
> I misunderstood, but my impression was that that would still be
> allowed.

Yeah, I was confused; sorry!

Patch

diff --git a/mercurial/smartset.py b/mercurial/smartset.py
--- a/mercurial/smartset.py
+++ b/mercurial/smartset.py
@@ -871,7 +871,10 @@ 
                 if i < _len(genlist):
                     yield genlist[i]
                 else:
-                    yield _next(nextgen)
+                    try:
+                        yield _next(nextgen)
+                    except StopIteration:
+                        return
                 i += 1
         return gen()