Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
[3.14] GH-135171: Revert async generator expressions behavior#135352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
efimov-mikhail commented Jun 10, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
| else{ | ||
| /* Sub-iter - calculate on the fly */ | ||
| VISIT(c, expr, gen->iter); | ||
| ADDOP(c, LOC(gen->iter), GET_AITER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, is this change necessary? I think the test passes without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. There was a bug in previous commit. I've reverted related logic in codegen.c.
Uh oh!
There was an error while loading. Please reload this page.
efimov-mikhail commented Jun 10, 2025
I've reverted all generator expressions related changes in |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Yhg1s commented Jun 10, 2025
@markshannon Do you want to take a look at this fix? Do you have time to do? |
markshannon commented Jun 11, 2025
@efimov-mikhail are you sure that this isn't adding back too many We should have tests for generator expressions and list comprehensions, both sync and async that |
markshannon commented Jun 11, 2025
I've a had a quick look at the generated bytecode and looks like this PR is OK. @Yhg1s if you need this in ASAP, I think it's good to merge. |
markshannon left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like tests to check that __iter__/__aiter__ is only called once in inlined comprehensions, but that can wait for another PR.
serhiy-storchaka commented Jun 11, 2025
efimov-mikhail commented Jun 11, 2025
Yes, changes in these two PRs are very similar. Although, there aren't exactly the same. But on 3.13 with #135390 being merged there is a different solution (from #125846). So, I have a question why we don't revert #125846? |
serhiy-storchaka commented Jun 11, 2025
Was not it reverted as the result of all these changes? |
efimov-mikhail commented Jun 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
It was reverted here. |
serhiy-storchaka commented Jun 11, 2025
Thank you, this is a great observation. |
Yhg1s commented Jun 11, 2025
I don't think I was aware of bytecode changes in 31.1. I tested the stdlib's compiled bytecode (including Lib/test) and saw no differences between 13.0, 13.3 and my rollback (and saw many changes between 13.3, 13.4 and 13.4 + the other fix-forward, but I didn't bother trying with this one applied). In any case it's too late to fix a change in bytecode that slipped into 13.1, if 13.1-13.3 are all the same we should stick with that. |
serhiy-storchaka commented Jun 11, 2025
It is not only change in bytecode generation. It is a behavior change in comparison with 3.12, 3.13.0 and 3.13.4. |
efimov-mikhail commented Jun 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
To be precise, there is a bytecode change: ->% ./pythonPython3.13.0 (tags/v3.13.0:60403a5409, Jun112025, 19:23:44) [GCC12.2.0] onlinuxType"help", "copyright", "credits"or"license"formoreinformation. >>>importdis>>>dis.dis('(x for x in range(10))') 0RESUME01LOAD_CONST0 (<codeobject<genexpr>at0x7fa6c309f450, file"<dis>", line1>) MAKE_FUNCTIONLOAD_NAME0 (range) PUSH_NULLLOAD_CONST1 (10) CALL1GET_ITERCALL0RETURN_VALUEDisassemblyof<codeobject<genexpr>at0x7fa6c309f450, file"<dis>", line1>: 1RETURN_GENERATORPOP_TOPL1: RESUME0LOAD_FAST0 (.0) L2: FOR_ITER6 (toL3) STORE_FAST_LOAD_FAST17 (x, x) YIELD_VALUE0RESUME5POP_TOPJUMP_BACKWARD8 (toL2) L3: END_FORPOP_TOPRETURN_CONST0 (None) --L4: CALL_INTRINSIC_13 (INTRINSIC_STOPITERATION_ERROR) RERAISE1ExceptionTable: L1toL4->L4 [0] lastiand ->% ./pythonPython3.13.1 (tags/v3.13.1:0671451779, Jun112025, 19:25:28) [GCC12.2.0] onlinuxType"help", "copyright", "credits"or"license"formoreinformation. >>>importdis>>>dis.dis('(x for x in range(10))') 0RESUME01LOAD_CONST0 (<codeobject<genexpr>at0x7f4332397bc0, file"<dis>", line1>) MAKE_FUNCTIONLOAD_NAME0 (range) PUSH_NULLLOAD_CONST1 (10) CALL1GET_ITERCALL0RETURN_VALUEDisassemblyof<codeobject<genexpr>at0x7f4332397bc0, file"<dis>", line1>: 1RETURN_GENERATORPOP_TOPL1: RESUME0LOAD_FAST0 (.0) GET_ITERL2: FOR_ITER6 (toL3) STORE_FAST_LOAD_FAST17 (x, x) YIELD_VALUE0RESUME5POP_TOPJUMP_BACKWARD8 (toL2) L3: END_FORPOP_TOPRETURN_CONST0 (None) --L4: CALL_INTRINSIC_13 (INTRINSIC_STOPITERATION_ERROR) RERAISE1ExceptionTable: L1toL4->L4 [0] lasti>>>You can see additional GET_ITER instruction before FOR_ITER. IMO, it's better to revert this change and fix regression in #127682. |
efimov-mikhail commented Jun 11, 2025
I've created new PR for 3.13: #135403 |
efimov-mikhail commented Jun 11, 2025
There were 3 tests related to this topic, and I've added the fourth. |
efimov-mikhail commented Jun 13, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
We should remove And what about changes on |
hugovk commented Jun 16, 2025
Thanks, this reverts the 3.14 async generator back to 3.12/3.13 behaviour: >>> (x async for x in None) Traceback (most recent call last): File "<python-input-1>", line 1, in <module> (x asyncfor x inNone) ^^^^TypeError: 'async for' requires an object with __aiter__ method, got NoneType
Yes, please do for |
2ef78a8 into python:3.14Uh oh!
There was an error while loading. Please reload this page.
efimov-mikhail commented Jun 22, 2025
I've started related d.p.o. thread here: https://discuss.python.org/t/generator-expressions-behavior-with-non-iterable-objects/96329. |
This PR could be a part of #135225.
Changes in behavior of async generator expressions are also reverted.
@serhiy-storchaka@markshannon@Yhg1s