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.
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
gh-74929: Implement PEP 667 #115153
gh-74929: Implement PEP 667 #115153
Changes from all commits
42d7186
7eeab1b
60e70e7
1454ce4
0045274
de73bc9
ca92393
ff886ff
9690a2d
6e9848a
b84b0df
bebff28
d846de9
d00a742
1a4344d
f720e12
64d3772
2eadbf0
cbae199
9e7edf8
523cb75
4b83311
ae2db7c
bf45c02
026e15e
f42980d
e693ad0
5dd045b
30ecd4d
f35c5e3
5844fb4
06277f9
e1c3f56
b672d84
3e32572
8dc4664
652f641
f29e6a3
e0ca4fe
f78156a
4503145
cdac22c
49287ff
378aacf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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
a
was added to thef_locals
snapshot 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_locals
while 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_locals
when it's module/class scope?What should the
sys._getframe().f_locals
inside 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_locals
with hidden fast-locals should mirror how it would behave if the comprehension were a function.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_locals
on 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_locals
of 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_locals
of 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_locals
is 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_locals
is 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.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_locals
be 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_locals
is 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_locals
are better off being shown implementation details that may vary by Python version than a costly fiction.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.