bpo-33346: Allow async comprehensions inside implicit async comprehensions. by serhiy-storchaka · Pull Request #6766 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't merge this yet. I think we need more discussion about the desired behavior. And we need docs to match.
bpo-33346: Allow async comprehensions inside implicit asyns comprehensions. bpo-33346: Allow async comprehensions inside implicit async comprehensions.
Can we include this in beta1?
I have updated the PR to match the main branch and I am landing this as the discussion in bpo has been quiet for several years and everyone seems to be on the same page.
@serhiy-storchaka, Дякую за чудову роботу! 😃
facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request
Summary: In cinder 3.8 we unintentionally allowed async comprehensions to nest inside non-async ones, due to comprehension inlining. When we ported comprehension inlining to 3.10, we closed this hole for better fidelity to upstream compiler behavior, but that means we have to fix the places in IGSRV that now break this rule. And this doesn't seem worth it considering the restriction is lifted in Python 3.11 anyway: python/cpython#6766
This diff just restores the 3.8 behavior of comprehension inlining allowing async comprehensions nested inside non-async ones.
Reviewed By: itamaro
Differential Revision: D39825885
fbshipit-source-id: bd2f27a
I noticed that no tests fail if I comment out this line:
| c->u->u_ste->ste_coroutine = 1; |
|---|
and I bisected it to this PR.
(1) Does this make sense?
(2) Is there another tests that will show where this line is needed, or can we remove it?
I noticed that no tests fail if I comment out this line:
c->u->u_ste->ste_coroutine = 1; and I bisected it to this PR.
(1) Does this make sense? (2) Is there another tests that will show where this line is needed, or can we remove it?
I think it's fine because this PR added in symtable.c code that propagates the value of ste_coroutine from a comprehension to the enclosing scope (unless it's a generator, which the compile.c code excludes as well). So this is now happening already during symtable construction (as it should be), and we don't need the assignment in the compiler anymore.