Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-33346: Allow async comprehensions inside implicit async comprehensions. #6766

Merged
merged 14 commits into from
Jul 13, 2021
Merged

bpo-33346: Allow async comprehensions inside implicit async comprehensions. #6766

merged 14 commits into from
Jul 13, 2021

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 11, 2018 •
edited by bedevere-bot
Loading

gvanrossum
gvanrossum reviewed May 11, 2018
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

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.

@merwok merwok changed the title bpo-33346: Allow async comprehensions inside implicit asyns comprehensions. bpo-33346: Allow async comprehensions inside implicit async comprehensions. Mar 5, 2019
@pablogsal
Copy link
Member

Can we include this in beta1?

@pablogsal
Copy link
Member

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, Дякую за чудову роботу! 😃

@pablogsal pablogsal merged commit 054e9c8 into python:main Jul 13, 2021
@serhiy-storchaka
Copy link
Member Author

Wow!

@serhiy-storchaka serhiy-storchaka deleted the nested-async-comp branch July 14, 2021 04:32
facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request Sep 26, 2022
@facebook-github-bot
relax async comprehension nesting when inlined
fd180dd 
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
@iritkatriel
Copy link
Member

I noticed that no tests fail if I comment out this line:

cpython/Python/compile.c

Line 5785 in 0898354

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?

@iritkatriel
Copy link
Member

I noticed that no tests fail if I comment out this line:

cpython/Python/compile.c

Line 5785 in 0898354

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.

@iritkatriel iritkatriel mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@pablogsal pablogsal pablogsal approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@serhiy-storchaka @pablogsal @iritkatriel @gvanrossum @the-knights-who-say-ni @bedevere-bot