Skip to content

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

Merged
pablogsal merged 14 commits into
python:mainfrom
serhiy-storchaka:nested-async-comp
Jul 13, 2021
Merged

bpo-33346: Allow async comprehensions inside implicit async comprehensions.#6766
pablogsal merged 14 commits into
python:mainfrom
serhiy-storchaka:nested-async-comp

Conversation

@serhiy-storchaka

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

Copy link
Copy Markdown
Member

gvanrossum
gvanrossum reviewed May 11, 2018
View reviewed changes

@gvanrossumgvanrossum 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.

Don't merge this yet. I think we need more discussion about the desired behavior. And we need docs to match.

@merwokmerwok 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

pablogsal commented Jun 1, 2019

Copy link
Copy Markdown
Member

Can we include this in beta1?

@pablogsal

pablogsal commented Jul 13, 2021

Copy link
Copy Markdown
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:mainJul 13, 2021
@serhiy-storchaka

serhiy-storchaka commented Jul 14, 2021

Copy link
Copy Markdown
MemberAuthor

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
Carl Meyer@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

iritkatriel commented Jul 2, 2024

Copy link
Copy Markdown
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

iritkatriel commented Jul 2, 2024

Copy link
Copy Markdown
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.

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

Reviewers

@gvanrossumgvanrossumgvanrossum left review comments
@pablogsalpablogsalpablogsal approved these changes
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon 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