-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-74929: Implement PEP 667 #115153
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
Merged
Merged
gh-74929: Implement PEP 667 #115153
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
42d7186
Basic prototype for frame proxy
gaogaotiantian 7eeab1b
Fix some lint and remove oprun check
gaogaotiantian 60e70e7
Not entirely work yet
gaogaotiantian 1454ce4
Fix a bug
gaogaotiantian 0045274
Change code style and add GC
gaogaotiantian de73bc9
Clean up code
gaogaotiantian ca92393
Disable all fast/local functions
gaogaotiantian ff886ff
Update tests for the new f_locals
gaogaotiantian 9690a2d
Comment out the pop for now
gaogaotiantian 6e9848a
Convert f_locals to dict first
gaogaotiantian b84b0df
Add static to static functions, add interface for new C API
gaogaotiantian bebff28
Add some tests and a few methods
gaogaotiantian d846de9
Implement all methods
gaogaotiantian d00a742
Make f_extra_locals extra lazy
gaogaotiantian 1a4344d
Merge branch 'main' into pep667
gaogaotiantian f720e12
Fix typo
gaogaotiantian 64d3772
Remove print debugging
gaogaotiantian 2eadbf0
Fix some styling issue
gaogaotiantian cbae199
Update generated files for cAPI
gaogaotiantian 9e7edf8
Remove f_fast_as_locals and useless calls for sys.settrace
gaogaotiantian 523cb75
📜🤖 Added by blurb_it.
blurb-it[bot] 4b83311
Add the new type to static types
gaogaotiantian ae2db7c
Remove internal APIs for fast locals
gaogaotiantian bf45c02
Add extra tests for closure
gaogaotiantian 026e15e
Add the type to globals-to-fix
gaogaotiantian f42980d
Add CAapi test
gaogaotiantian e693ad0
Polish lint
gaogaotiantian 5dd045b
Apply some simple changes
gaogaotiantian 30ecd4d
Update Misc/NEWS.d/next/Core and Builtins/2024-04-27-21-44-40.gh-issu…
gaogaotiantian f35c5e3
Abstract the key index part
gaogaotiantian 5844fb4
Fix error handling
gaogaotiantian 06277f9
Make key index work better
gaogaotiantian e1c3f56
Add comments for GetHiddenLocals
gaogaotiantian b672d84
Add global test
gaogaotiantian 3e32572
Remove unsupported methods
gaogaotiantian 8dc4664
Support non-string keys
gaogaotiantian 652f641
Use static function for setitem
gaogaotiantian f29e6a3
Fix the list comp
gaogaotiantian e0ca4fe
Fix mapping check
gaogaotiantian f78156a
Fix frame_getlocals
gaogaotiantian 4503145
Fix test error
gaogaotiantian cdac22c
Change the new ref for getcode
gaogaotiantian 49287ff
Avoid creating the frame object if possible
gaogaotiantian 378aacf
Remove a single blank line
gaogaotiantian File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Update tests for the new f_locals
- Loading branch information
commit ff886ffe922d4ae29aeaca6e38ad806fede1cc9e
There are no files selected for viewing
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've changed the intent of the test here. It was previously checking that
awas added to thef_localssnapshot during the listcomp, and now it's checking that it is not visible through the proxy afterwards.To cover both aspects, I think we need two test cases (first one shows that
"a"is accessible in thef_localswhile the listcomp is running, second shows that it is gone afterwards):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very interesting point because it introduced a new issue - should we include the hidden fast local in
f_localswhen it's module/class scope?What should the
sys._getframe().f_localsinside the list comprehension return? It's it a dict or a proxy? Do we consider that "within" the function scope and return a proxy? Will that include the module-level variables as well? Do we return a dict? Should we include variablea? If so, writing to the key'a'won't have any effects because it's a fast variable, how do we deal with that inconsistency?@markshannon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hidden fast-locals were designed to mimic the prior behavior when comprehensions were previously implemented as one-shot functions. So as much as is feasible, the behavior of
f_localswith hidden fast-locals should mirror how it would behave if the comprehension were a function.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second proposed test in @ncoghlan 's comment is not correct. Nor is the test as currently modified in the PR.
If you access
f_localson a frame you got while inside the comprehension, it should include the hidden fast-local (i.e. the comprehension iteration variable), and that variable should still be present (in thef_localsof the frame you got while inside the comprehension) even after the comprehension is done. This behavior is the same as getting a frame (or itsf_locals) inside a function and returning it from the function. This is the current behavior inmain, and this behavior is important to keep in PEP 667.Of course if you access the frame outside the comprehension, the comprehension's hidden fast locals should not be present in its
f_locals.Currently in main, module globals are present in the
f_localsof a frame accessed inside a module-level comprehension. This differs from a function, but this difference was called out in PEP 709 and determined to be OK, and I think it's still OK.I suspect this is also not a problem in practice, though if we can reasonably return a proxy that can support it, it would be even better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct.
f_localsis a proxy, so as the frame changes, so will the proxy. Inside the comprehension, the comprehension variables will be visible. After the comprehension has executed, the comprehension variables no longer exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the key here is "as if it's a one-shot function". That's infeasible with the current compiler because it's not a function anymore. For a function, we have a dedicated frame object that keeps the local variables, which can last longer than the intepreter frame as long as there are references on it. In that way, we can always access the variables on that frame even when the actual interpreter frame is gone.
However, with inline comprehension, it fakes the "function call" and clear the hidden variable - there's no mechanism currently to prevent the variable from being cleared. If we want this, we'll probably need to change the compiler and the interpreter which will definitely postpone this PEP to 3.14 (and would probably make the compiler code more complicated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For class and module level comprehensions, can we return a proxy if
f_localsis accessed inside the comprehension and a dict if it's accessed outside which does not have the hidden variable? I feel like that's more consistent.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends whether we are aiming for "correctness" for the actual situation (that the comprehension doesn't have its own frame), or for the backwards-compatible fiction (which PEP 709 only partially maintained, but did jump through hoops to maintain in terms of visibility of class-scoped variables) that comprehensions still behave as if they have their own frame. But as @gaogaotiantian points out, it may be very hard or impossible to maintain that fiction in this case, while still having
f_localsbe a proxy.TBH in the long term I think it would be better to move away from that fiction anyway, but it will have backwards-compatibility implications. Today in
main,sys._getframe.f_locals()inside a module-level comprehension returns a dictionary that includes the comprehension iteration variable, and that variable is still visible in that dictionary after the comprehension is done.I don't know why someone relying on that couldn't just use
locals()instead, though, andlocals()should be fine for backwards-compatibility; it remains a dict. So I don't know how significant that backwards-incompatibility will be in practice.Certainly the dict returned when
f_localsis accessed outside the comprehension should not have the hidden local in it.I think returning a proxy inside the module- or class-scoped comprehension is more internally consistent for PEP 667; returning a dict (that includes the comprehension iteration variable) would be a bit more backward-compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow the details, but I agree that people who access
f_localsare better off being shown implementation details that may vary by Python version than a costly fiction.