FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-33346: Allow async comprehensions inside implicit async comprehensions. by serhiy-storchaka · Pull Request #6766 · python/cpython · GitHub

/ cpython Public

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
ConversationCommits (14)ChecksFiles changed
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 commented May 11, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

gvanrossum reviewed May 11, 2018
View reviewed changes

gvanrossum 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 commented Jun 1, 2019

Copy link
Copy Markdown
Member

Can we include this in beta1?

pablogsal force-pushed the nested-async-comp branch from de5d817 to 6039db7 Compare July 13, 2021 20:07
pablogsal force-pushed the nested-async-comp branch from 6039db7 to f3beb18 Compare July 13, 2021 20:52

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 merged commit 054e9c8 into python:main Jul 13, 2021

serhiy-storchaka commented Jul 14, 2021

Copy link
Copy Markdown
Member Author

Wow!

serhiy-storchaka deleted the nested-async-comp branch July 14, 2021 04:32
facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request Sep 26, 2022
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 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 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.

iritkatriel mentioned this pull request Jul 2, 2024
Merged
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gvanrossum gvanrossum left review comments
pablogsal pablogsal approved these changes
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

Back | FazBrowse Home | New Git URL