Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
gh-74929: Implement PEP 667#115153
Changes from1 commit
42d71867eeab1b60e70e71454ce40045274de73bc9ca92393ff886ff9690a2d6e9848ab84b0dfbebff28d846de9d00a7421a4344df720e1264d37722eadbf0cbae1999e7edf8523cb754b83311ae2db7cbf45c02026e15ef42980de693ad05dd045b30ecd4df35c5e35844fb406277f9e1c3f56b672d843e325728dc4664652f641f29e6a3e0ca4fef78156a4503145cdac22c49287ff378aacfFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -610,10 +610,10 @@ def test_exception_in_post_comp_call(self): | ||
| def test_frame_locals(self): | ||
| code = """ | ||
| val ="a" in[sys._getframe().f_locals for a in [0]][0] | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 To cover both aspects, I think we need two test cases (first one shows that deftest_frame_locals_in_listcomp(self):# Iteration variable is accessible via f_locals proxy while the listcomp is runningcode=""" val = [sys._getframe().f_locals["a"] for a in [0]][0] """importsysself._check_in_scopes(code, {"val":0},ns={"sys":sys})deftest_frame_locals_after_listcomp(self):# Iteration variable is no longer accessible via f_locals proxy after listcomp finishescode=""" val = "a" in [sys._getframe().f_locals for a in [0]][0] """importsysself._check_in_scopes(code, {"val":False},ns={"sys":sys}) MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 val= [sys._getframe().f_locals["a"]forain [0]][0] What should the Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Of course if you access the frame outside the comprehension, the comprehension's hidden fast locals should not be present in its Currently in main, module globals are present in the
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. Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
I don't think this is correct. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. For class and module level comprehensions, can we return a proxy if Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 I don't know why someone relying on that couldn't just use
Certainly the dict returned when 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. Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| """ | ||
| import sys | ||
| self._check_in_scopes(code, {"val":False}, ns={"sys": sys}) | ||
| def _recursive_replace(self, maybe_code): | ||
| if not isinstance(maybe_code, types.CodeType): | ||