Skip to content

[3.14] GH-135171: Revert async generator expressions behavior - #135352

Merged
hugovk merged 4 commits into
python:3.14from
efimov-mikhail:issue_135171_async_genexpr
Jun 16, 2025
Merged

[3.14] GH-135171: Revert async generator expressions behavior#135352
hugovk merged 4 commits into
python:3.14from
efimov-mikhail:issue_135171_async_genexpr

Conversation

@efimov-mikhail

@efimov-mikhailefimov-mikhail commented Jun 10, 2025

Copy link
Copy Markdown
Member

This PR could be a part of #135225.
Changes in behavior of async generator expressions are also reverted.

@serhiy-storchaka@markshannon@Yhg1s

Comment threadPython/codegen.c
else {
/* Sub-iter - calculate on the fly */
VISIT(c, expr, gen->iter);
ADDOP(c, LOC(gen->iter), GET_AITER);

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadLib/test/test_asyncgen.py Outdated
@efimov-mikhail
efimov-mikhail marked this pull request as ready for review June 10, 2025 15:29
@efimov-mikhail
efimov-mikhail marked this pull request as draft June 10, 2025 15:32
@efimov-mikhail
efimov-mikhail marked this pull request as ready for review June 10, 2025 16:19
@efimov-mikhail

Copy link
Copy Markdown
MemberAuthor

I've reverted all generator expressions related changes in codegen.c.
And for now I can see no problems with tests, included those which are added in related PR's.
Could you please review this again?
And backport for 3.13 is also neccessary.

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Yhg1s

Copy link
Copy Markdown
Member

@markshannon Do you want to take a look at this fix? Do you have time to do?

@markshannon

Copy link
Copy Markdown
Member

@efimov-mikhail are you sure that this isn't adding back too many GET_ITER/GET_AITER instructions?
Particularly, for inlined list comprehensions, this might add an extra GET_[A]ITER.

We should have tests for generator expressions and list comprehensions, both sync and async that __iter__ is called only once.
It seems that we only have test for sync gen expressions. The test is called test_genexpr_only_calls_dunder_iter_once, can you extend it to the other cases?

@markshannon

Copy link
Copy Markdown
Member

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.
We can add those tests to main in another PR and backport them later.

@markshannonmarkshannon left a comment

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

Changes in this PR look similar to the full rollback in #135390, so I think it adds the right number of GET_ITER/GET_AITER at right place. The total effect of #135293 and this PR should restore the old bytecode generation.

@efimov-mikhail

Copy link
Copy Markdown
MemberAuthor

Changes in this PR look similar to the full rollback in #135390, so I think it adds the right number of GET_ITER/GET_AITER at right place. The total effect of #135293 and this PR should restore the old bytecode generation.

Yes, changes in these two PRs are very similar. Although, there aren't exactly the same.
In fact, on 3.14 after this PR will be merged we will have issue #125038 fixed w/o additional GET_ITER instruction, but with null pointer checks in FOR_ITER.

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?
Do we want to preserve bytecode changes in 3.13.1 vs 3.13.0?

@serhiy-storchaka

Copy link
Copy Markdown
Member

Was not it reverted as the result of all these changes?

@efimov-mikhail

efimov-mikhail commented Jun 11, 2025

Copy link
Copy Markdown
MemberAuthor

Was not it reverted as the result of all these changes?

It was reverted here.
But it wasn't on 3.13 even after #135390 is being merged.

@serhiy-storchaka

Copy link
Copy Markdown
Member

Thank you, this is a great observation.

@Yhg1s

Copy link
Copy Markdown
Member

Do we want to preserve bytecode changes in 3.13.1 vs 3.13.0?

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

Copy link
Copy Markdown
Member

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

efimov-mikhail commented Jun 11, 2025

Copy link
Copy Markdown
MemberAuthor

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] lasti

and

->% ./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.
This change is made in #125846.

IMO, it's better to revert this change and fix regression in #127682.

@efimov-mikhail

Copy link
Copy Markdown
MemberAuthor

I've created new PR for 3.13: #135403

@efimov-mikhail

Copy link
Copy Markdown
MemberAuthor

I'd like tests to check that __iter__/__aiter__ is only called once in inlined comprehensions, but that can wait for another PR.

There were 3 tests related to this topic, and I've added the fourth.
Is it enough?
Or I miss something?

@efimov-mikhail

efimov-mikhail commented Jun 13, 2025

Copy link
Copy Markdown
MemberAuthor

We should remove backport-3.13 label.

And what about changes on main branch?
Maybe some thread on d.p.o. should be started?

@serhiy-storchakaserhiy-storchaka removed the needs backport to 3.13 bugs and security fixes label Jun 14, 2025
@hugovk

Copy link
Copy Markdown
Member

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

And what about changes on main branch? Maybe some thread on d.p.o. should be started?

Yes, please do for main/3.15.

@hugovk
hugovk merged commit 2ef78a8 into python:3.14Jun 16, 2025
@efimov-mikhail

Copy link
Copy Markdown
MemberAuthor

I've started related d.p.o. thread here: https://discuss.python.org/t/generator-expressions-behavior-with-non-iterable-objects/96329.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@efimov-mikhail@Yhg1s@markshannon@serhiy-storchaka@hugovk