Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
markshannon merged 44 commits into from
May 4, 2024
Merged

gh-74929: Implement PEP 667 #115153

markshannon merged 44 commits into from
May 4, 2024

Conversation

@gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Feb 8, 2024 •
edited by bedevere-app bot
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 - The pop method is not implemented yet
  • test_listcomps - the behavior of frame.f_locals changed so the corresponding test is not valid anymore

@bedevere-app bedevere-app bot mentioned this pull request Feb 8, 2024
Closed
markshannon
markshannon reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@markshannon markshannon 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 commented Feb 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 that test_frame_locals in test_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 in f_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 that f_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, and a 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 of f_locals within the comprehension.

@gaogaotiantian
Copy link
Member Author

gaogaotiantian commented Feb 9, 2024

EDIT: on second thought, I realized the problem is likely that f_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, and a 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 of f_locals within the comprehension.

Yes, that's why the test fails. The test is val = [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 to f_locals.

@gaogaotiantian
Copy link
Member Author

gaogaotiantian commented Feb 13, 2024

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

markshannon commented Feb 20, 2024

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
Member Author

gaogaotiantian commented Feb 20, 2024

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.