-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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-120906: Support arbitrary hashable keys in FrameLocalsProxy #122309
Conversation
So for the first glance, there are two questions:
I did no go through the whole change because I don't believe we've reached to the point where we agree how to do this. |
|
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.
General approach LGTM, some suggestions inline regarding test case details.
Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
|
||
// We do 2 loops here because it's highly possible the key is interned | ||
// and we can do a pointer comparison. | ||
for (int i = 0; i < co->co_nlocalsplus; i++) { | ||
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); | ||
if (name == key) { | ||
found_key = true; |
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.
If you removed found_key
check here, maybe just return -1
if the condition does not meet. Otherwise it will go through the loop again and check something we already knew.
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 rest of the PR LGTM.
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.
Right, thanks for the catch!
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.
Could you check the change?
I'm still not too familiar with the internals of this mapping; I just found out (hopefully correctly) what the undocumented CO_FAST_HIDDEN
does.
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 change looks correct to me (with the search flag restored, the CO_FAST_HIDDEN
handling is unchanged from @gaogaotiantian's original PR).
I did suggest an explanatory comment on the early return, but that's definitely not a merge blocker. So even if @gaogaotiantian doesn't get time to take a look, I think this is good to go for rc1.
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.
Capturing my understanding of the CO_FAST_HIDDEN
handling, since it's genuinely not clear:
- in Python 3.12, the iteration variables for inlined comprehensions are visible for reads while the comprehension(s) is(/are) running, but not writable (even with LocalsToFast shenanigans), and not visible outside the comprehension(s)
- in Python 3.13, all of those properties remain true even though
sys._getframe().f_locals
produces fast locals proxy instances during iteration (writes to hidden variables go into the frame's "extra locals" storage rather than modifying the comprehension iteration variable)
Future versions could revisit that design if debugger developers ask for it to change, but live editing loop variables seems sufficiently fraught with peril that I'll be genuinely surprised if anyone ever notices this small discrepancy between module level comprehensions and function level ones.
Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
@gaogaotiantian and/or @markshannon, your review would be appreciated, so we can get this in 3.13. |
Looks good. There is one test I'd like to added, though.
but it doesn't seem like there is a test for this case (writing a fast local using a non-str key). |
Will do in ~6 hours, feel free to beat me to it. |
Test added. (There was one for writing a fast local using a non-str key, but not for writing a so far uninitialized local like your example, so I added that.) Thank you for the reviews! |
Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, @encukou, I could not cleanly backport this to
|
…sProxy (pythonGH-122309) Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com> (cherry picked from commit 5912487)
…sProxy (pythonGH-122309) Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com> (cherry picked from commit 5912487)
GH-122488 is a backport of this pull request to the 3.13 branch. |
…GH-122309) (#122488) [3.13] gh-120906: Support arbitrary hashable keys in FrameLocalsProxy (GH-122309) Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com> (cherry picked from commit 5912487)
…pythonGH-122309) Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
Make FrameLocalsProxy support arbitrary keys for lookup and update.
The given key needs to be hashable. It's a mapping, after all.
For the linear scan through the "real" locals:
PyObject_RichCompareBool
If there's no match, we know it matches none of the "real" locals,
and the read/write functions fall back to the "extras" dict.
Add tests, including tests for the mapping protocol (with selected exceptions).
FrameLocalsProxy
is stricter thandict
about what constitutes a match #120906