Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
markshannon merged 44 commits intopython:mainfromgaogaotiantian:pep667
May 4, 2024
Merged

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantiangaogaotiantian commentedFeb 8, 2024
edited by bedevere-appbot
Loading

This is a prototype implementation of PEP 667.
All the examples given in PEP 667 passed.

The code only serves as a prototype, the proxy object is not even tracked by GC. However, the most essential features are implemented.

frame.f_locals is now a real-time proxy for the actual local variables in the frame (there's no mapping yet but that's an implementation detail).

locals() behaves basically like before.

All tests passed except for 3 - all expected

  • test_sys for frame size - yes it changed
  • test_frame - Thepop method is not implemented yet
  • test_listcomps - the behavior offrame.f_locals changed so the corresponding test is not valid anymore

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks so much for doing this.

I've a few comments, but nothing major.

@carljm
Copy link
Member

carljm commentedFeb 9, 2024
edited
Loading

Thanks for doing this! I was hoping this would get done for 3.13 but hadn't found time to do it yet.

I don't agree thattest_frame_locals intest_listcomps should be expected to fail under PEP 667.f_locals is supposed to be a real-time proxy for the live variables in the frame. Within a comprehension, comprehension-scoped variables are live and should be present inf_locals.The fact that this test is failing suggests that the PR does not yet correctly handle "fast-hidden" locals.

EDIT: on second thought, I realized the problem is likely thatf_locals is now a live view instead of a snapshot, so by the time we look for the key, the comprehension is no longer running, anda is no longer a live name. So you are right that the test as written should be expected to fail; it should be updated to take a copy off_locals within the comprehension.

@gaogaotiantian
Copy link
MemberAuthor

EDIT: on second thought, I realized the problem is likely thatf_locals is now a live view instead of a snapshot, so by the time we look for the key, the comprehension is no longer running, anda is no longer a live name. So you are right that the test as written should be expected to fail; it should be updated to take a copy off_locals within the comprehension.

Yes, that's why the test fails. The test isval = [sys._getframe().f_locals for a in [0]][0]["a"] so when it tries to access["a"], it's already out of the comprehension scope. This actually represents one of the changes we made tof_locals.

carljm reacted with thumbs up emoji

@gaogaotiantian
Copy link
MemberAuthor

I cleaned up the code a bit to address the review and added the GC part in. This is still in draft state, which only aims to demostrate the possibility of PEP 667. There's plenty of work to be done before this can be merged in. The current goal is to make it enough for the PSF to decide whether to accept PEP 667. Let me know if there's still missing pieces (like do we want to actually remove all the FAST/LOCAL conversion calls to prove the point).

@markshannon
Copy link
Member

Removing the fast locals <--> slow locals conversion is key to fixing#74929. I think that is worth doing before submitting PEP 667 to the SC.

@gaogaotiantian since you are doing most of the work on this, would you like to be added as co-author of PEP 667?

@gaogaotiantian
Copy link
MemberAuthor

Sure, I'm happy to be listed as the co-author. I'll work on the fast vs local thing and check if#74929 is fixed.

@gvanrossum
Copy link
Member

FWIW the discussion has been started athttps://discuss.python.org/t/pep-667-consistent-views-of-namespaces/46631.

We now need at least the following updates to the PEP:

  • Add Discussion-To header pointing to that Discourse thread
  • Add Tian Gao to list of Authors
  • Link to implementation prototype PR from the Implementation section

@gaogaotiantian
Copy link
MemberAuthor

I've already added myself to the list of authors with Mark's guidance. Let me know if you want me to make the PR to update the PEP.

@gvanrossum
Copy link
Member

Go right ahead.

@gaogaotiantian
Copy link
MemberAuthor

@markshannon okay I made all fast/local related functions no-op. It did not break anything new and#74929 (comment) is not an issue anymore. The original test case in#74929 has been accidentally fixed in 3.11, is there any repro case that I can test against?

@carljm
Copy link
Member

Is there a reason you're leaving the failing tests as failing? I think it is easier for reviewers to evaluate the compatibility impact on tests by seeing the required changes to tests in the PR, rather than by having to go look at the CI test results.

@gaogaotiantian
Copy link
MemberAuthor

The current implementation is just a prototype and far from being done. I can make some modifications to the test so they can pass with the change if that's preferred.

carljm reacted with thumbs up emoji

@gaogaotiantian
Copy link
MemberAuthor

We have two more months until feature freeze. If we want to land this on 3.13, we probably should push it forward soon.

@gvanrossum
Copy link
Member

How is the Discourse discussion on this topic going? Is it substantial enough to send the PEP to the SC? Do we need to make any PEP updates first?

@gaogaotiantian
Copy link
MemberAuthor

I made a PR to the PEP and it's approved but not merged (I think Mark has to review and approve it?), if you meant the link changes mentioned before. I'm not sure if the discussion thread attracts enough attention. What point should we get for the Disclosure discussion?

@markshannon
Copy link
Member

markshannon commentedMar 5, 2024
edited
Loading

We need to decide which mapping methods we wantFrameLocalsProxy to support.
Listing all the methods onUserDict:

Dunder methods

  • Comparison operators,__le__, etc.
  • __or__,__ior__,__ror__
  • __contains__
  • __copy__
  • __getitem__
  • __setitem__
  • __delitem__ (see comment below under normal methods)
  • __format__
  • __len__
  • __iter__
  • __getitem__
  • __reversed__
  • __sizeof__
  • __repr__,__str__
  • Pickling support,__reduce__, etc

I think we need to support all the above, except pickling, and that__copy__ should return a normal__dict__, not a shallow copy.

Normal methods

  • clear,
  • copy,
  • get,
  • items,
  • keys,
  • pop,
  • popitem,
  • setdefault,
  • update,
  • values

Methods that remove items are tricky as we cannot properly delete fast locals; we just set them toNone and emit a warning.
So, I think it best not to implementclear,pop, orpopitem.

@markshannon
Copy link
Member

We don't need to implement them all before submitting the PEP though, just decide on which methods to support.

@gaogaotiantian
Copy link
MemberAuthor

Should we follow some standard for the methods we should implement? The original PEP mentioned that we should do aMutableMapping, which probably lists all the methods that need to be implement.

For the remove related methods - should the debugger be able to decref the local variable? Just likedel val. I think making that equivalent tof.locals.pop("val") is reasonable. Should we even emit a warning for that? Did we do that now? I think we emit a warning when we have to assignNone to unassigned local variables.

@gaogaotiantian
Copy link
MemberAuthor

gaogaotiantian commentedMay 2, 2024
edited
Loading

Okay I resolved most of the comments, and I believe there's one thing left here - comprehensions.

I'm just going to jump to my proposal and see if that makes sense. If not, we can discuss it more.

importsysdeff():x=1    [print(sys._getframe().f_locals)forain [0]]print([sys._getframe().f_localsforain [0]][0])print(sys._getframe().f_locals)f()

will print

# Inside comprehension, show hidden variable a and locals in the outside frame{'x': 1, 'a': 0}# f_locals from inside but read outside, no hidden variable, only locals.# This does not match the fiction because the variable should "exist" if it's a frame, but in reality it's gone{'x': 1}# f_locals outside, just local variables{'x': 1}

Notice all the values above are proxies.

importsysclassC:x=1    [print(sys._getframe().f_locals)forain [0]]print([sys._getframe().f_localsforain [0]][0])print(sys._getframe().f_locals)

will print (ignore__module__ and__qualname__)

# This is a proxy without class-level variable{'a': 0}# Empty proxy{}# A dict with class level variable{'x': 1}

An alternative would be:

# A dict, but you can't write to variable a to change the local value{'a': 0, 'x': 1}# A dict{'x': 1}# Still a dict{'x': 1}

The latter seems more consistent with the function level result, and it makeslocals() backwards compatible, but it creates a horrible exemption where thef_locals is not "write through".

Maybe we could stick to a simple rule here -f_locals will always return something that's write through.locals() always returndict(proxy) or the dict itself (in class/module level, outside of the comprehension).

However, this will break the backwards compatibility forlocals() in comprehension in class/module level.

carljm reacted with thumbs up emoji

@carljm
Copy link
Member

I think that proposal looks reasonable.

I think your first proposal for the class behavior is better. Comprehensions in class scope cannot see class-level variables (as if the comprehension were still a function, since functions defined in a class scope can't see variables in the enclosing class scope either), so those class variables should not show up whenf_locals is accessed inside the comprehension.

@gaogaotiantian
Copy link
MemberAuthor

gaogaotiantian commentedMay 2, 2024
edited
Loading

I think your first proposal for the class behavior is better. Comprehensions in class scope cannot see class-level variables (as if the comprehension were still a function, since functions defined in a class scope can't see variables in the enclosing class scope either), so those class variables should not show up whenf_locals is accessed inside the comprehension.

But we will probably treat module-level the same as class-level. Do you think it's reasonable to have a module-level comprehension to return a proxy without the global variables?

edit: I guess that makes sense as well. Even in a function in a module,f_locals should not return any global variables.

carljm and ncoghlan reacted with thumbs up emoji

@gaogaotiantian
Copy link
MemberAuthor

Okay I finished the last piece and cleaned up the code a bit.

Now we have some very consistent behavior forf_locals andlocals().

locals() will be either

  • dict(f_locals) for function scope frames andall comprehensions
  • orf_locals for class and module frames

f_locals will be

  • a proxy for function scope frames andall comprehensions (comprehension variables are not accessible outside of the comprehension)
  • or a dict for class and module frames

f_locals willalways be write through, no exceptions.

This breaks the behavior introduced by pep 709 that[locals() for a in [0]] will include outside variables if it's under a module or class level scope, but I think this specific behavior is not desired nor intentional, it's a compromise to begin with. If we consider the comprehension as a "function", thelocals() should never include the outside variables.

locals() in a function scope will include the cell variables as expected.

Let me know if there are missing pieces that I don't know of.

ncoghlan, carljm, gvanrossum, and zetaloop reacted with thumbs up emoji

@markshannonmarkshannon self-requested a reviewMay 3, 2024 20:21
Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This looks good. Thanks for your perseverance.

last chance for comments.
I'm going to merge this in 12 hours or so, if no one objects.

gvanrossum reacted with thumbs up emojincoghlan and CoolCat467 reacted with hooray emoji
@markshannonmarkshannon merged commitb034f14 intopython:mainMay 4, 2024
@gaogaotiantiangaogaotiantian deleted the pep667 branchMay 6, 2024 19:42
SonicField pushed a commit to SonicField/cpython that referenced this pull requestMay 8, 2024
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull requestMay 13, 2024
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Bug:python/cpython#115153
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull requestMay 13, 2024
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Per the documentation for locals():> The contents of this dictionary should not be modified; changes may> not affect the values of local and free variables used by the> interpreter.So... lesson learned? :) This was introduced in commitc34ee37; before that, we still usedlocals() but only to pass local variables *in*.Bug:python/cpython#115153
@henryiii
Copy link
Contributor

henryiii commentedMay 23, 2024
edited
Loading

FYI, docs are missing here:https://docs.python.org/3.13/c-api/reflection.html

The back-compat functions were causing a few destructors to not be called properly in three tests in pybind11; switching to the new functions fixed it, though. Not sure if that's expected. I believe it’s because the back compat function for eval frame locals is basically the same as the new function and returns a new reference.

eli-schwartz added a commit to eli-schwartz/meson that referenced this pull requestJun 23, 2024
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Per the documentation for locals():> The contents of this dictionary should not be modified; changes may> not affect the values of local and free variables used by the> interpreter.So... lesson learned? :) This was introduced in commitc34ee37; before that, we still usedlocals() but only to pass local variables *in*.Bug:python/cpython#115153
Volker-Weissmann pushed a commit to Volker-Weissmann/meson that referenced this pull requestMay 1, 2025
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Per the documentation for locals():> The contents of this dictionary should not be modified; changes may> not affect the values of local and free variables used by the> interpreter.So... lesson learned? :) This was introduced in commitc34ee37; before that, we still usedlocals() but only to pass local variables *in*.Bug:python/cpython#115153
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@carljmcarljmcarljm left review comments

@ncoghlanncoghlanncoghlan left review comments

@markshannonmarkshannonmarkshannon approved these changes

@encukouencukouAwaiting requested review from encukouencukou is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@gvanrossumgvanrossumAwaiting requested review from gvanrossum

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
@gaogaotiantian@carljm@markshannon@gvanrossum@ncoghlan@henryiii

[8]ページ先頭

©2009-2025 Movatter.jp