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.

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

PEP 709, for the third and (hopefully) final time #192

Closed
carljm opened this issue May 16, 2023 · 5 comments
Closed

PEP 709, for the third and (hopefully) final time #192

carljm opened this issue May 16, 2023 · 5 comments

Comments

@carljm
Copy link
Member

carljm commented May 16, 2023

In light of the SC feedback that the change to class-scope name visibility in comprehensions discussed in https://discuss.python.org/t/pep-709-one-behavior-change-that-was-missed-in-the-pep/26691 is not acceptable for 3.12, @JelleZijlstra and I have been exploring options B and C as outlined in that post for keeping the optimization with the originally-approved semantics, and no additional behavior changes.

Jelle has a PR which preserves inlining of all list/dict/set comprehensions, while also preserving the previous scoping rules for names in class-scope comprehensions, so it exactly implements the originally-approved semantics of PEP 709. It turns out to be a pretty simple change after all: python/cpython#104528.

I checked with @Yhg1s as 3.12 release manager, and he is OK with moving ahead with Jelle's PR for 3.12.

But he asked me to also check here to see if anyone on the SC has a strong preference for the alternative fix, which is to disable inlining altogether in module and class scopes, where it is unlikely to be performance sensitive. I also have a PR up for that: python/cpython#104519. Relative to Jelle's PR, this approach keeps the compiler a bit simpler, but it means that the subtle behavior differences discussed in PEP 709 (regarding tracebacks and locals()) would be different for function-scoped comprehensions than for class/module-scoped ones.

@brettcannon
Copy link
Member

If the semantics are fixed then I'm fine with the change (the beta will help tell us if it isn't enough).

@gpshead
Copy link
Member

gpshead commented May 17, 2023

Same here, I'm in favor of Jelle's PR (nice work!) and agree with Brett and Thomas.

@emilyemorehouse
Copy link
Member

I'm strongly in favor of the updated approach to preserve inlining of all comprehensions as well as preserve the previous scoping rules (for now, until a greater conversation around class scopes and generators can be had).

Glad there was a good solution here, thanks to both @carljm and @JelleZijlstra for the great work!

@gpshead
Copy link
Member

gpshead commented May 17, 2023

I believe @Yhg1s already told you go move forward with @JelleZijlstra's PR. So lets make that the plan. If @pablogsal who hasn't had a chance to weigh has issues with the rest of us on the steering council for this, or other surprises are revealed during the Beta, we can figure out if anything needs to be reconsidered. :)

@gpshead gpshead closed this as completed May 17, 2023
@pablogsal
Copy link
Member

Sorry for the late replay, I had a nightmare day and couldn't get to this until now. I am very much in favour of the change and I want to thank both @carljm and @JelleZijlstra for their hard work, understanding and patience dealing with this issue. You rock 🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants