gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals by gaogaotiantian · Pull Request #125616 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
@terryjreedy the only tests failed were Idle on MacOS-13, which is a bit weird. I can imagine changing f_locals
impacts Idle, but only on a specific platform? Do you have any idea why the test failed? I looked at the error messages and they do not quite make sense to me.
if (default_value != NULL) { |
---|
return Py_XNewRef(default_value); |
} else { |
PyErr_Format(PyExc_KeyError, "'%R'", key); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny optimization to consider (though, it's more complex--feel free to reject this):
PyErr_Format(PyExc_KeyError, "'%R'", key); |
---|
PyObject *repr = PyObject_Repr(key); |
if (repr == NULL) { |
return NULL; |
} |
PyErr_SetObject(PyExc_KeyError, repr); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized I could've used _PyErr_SetKeyError(key)
like dictobject.c
. I'll switch to that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that sounds better. I wasn't aware of that, TIL!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I switched to _PyErr_SetKeyError(key)
when the key does not exist. However, for local variables cases, I think RuntimeError
would fit better. One important reason was KeyError
actually expects the "key" to be the explanation string, and giving a reason is a bit weird. Also in this case we have the key, we just can't remove it, so RuntimeError
might be better.
MacOS versions after 10 have various odd interactions with tkinter and python. Without a saved reference to the now-overwritten failing test, I cannot comment other than to note the following:
Searching 'locals()' in C:\Programs\Python314\Lib\idlelib\*.py ...
C:\Programs\Python314\Lib\idlelib\debugger.py: 248: self.show_locals()
C:\Programs\Python314\Lib\idlelib\debugger_r.py: 214: return self._get_f_locals()
The first call must result in the second, which I have no memory of. There is no direct test of either function. I'm glad you found better code that worked on major systems
I think this needs backporting to 3.13, as that's the version mentioned in the issue
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception types need changing from RuntimeError
to KeyError
.
Otherwise, looks good.
int i = framelocalsproxy_getkeyindex(frame, key, false); |
---|
if (i == -2) { |
return -1; |
} |
if (i >= 0) { |
if (value == NULL) { |
PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} |
---|
if (i >= 0) { |
PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again
.
FTR, from this PR, the potential additions to PEP 667 would be:
def __delitem__(self, name):
if name in co._name_to_offset_mapping:
raise KeyError(f"Cannot delete local variable '{name}'")
del self._extra_locals[name]
def pop(self, name, default):
if name in co._name_to_offset_mapping:
raise KeyError(f"Cannot delete local variable '{name}'")
return self._extra_locals.pop(name, default)
I think this is what we want.
Although maybe instead of k = KeyError(f"Cannot delete local variable '{name}'")
, we could do this:
k = KeyError(name)
k.add_note("Cannot delete local variable")
From the documentation, it explains KeyError
as
Raised when a mapping (dictionary) key is not found in the set of existing keys.
I think there's a distinctive difference here. The key is in the mapping, we can even read it, key in mapping == True
. We fail the operation because we don't allow deleting this key, not the key does not exist. I think a RuntimeError
would fit better in this case.
I appreciate KeyError
is not ideal.
I guess the question is do we want:
try: del f.f_locals["local_name"] except KeyError: ...
to handle the exception or not?
If not, then I think ValueError
is better than RuntimeError
.
exception ValueError
Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.
Other than that, the PR looks ready to merge.
I guess the question is do we want:
try: del f.f_locals["local_name"] except KeyError: ...
to handle the exception or not?
I think we need two separate exceptions because I don't know what they would do in the except
block here. Trying to remove a real local variable is very different than trying to remove an artificial variable. Even for a try ... except block, I think those should be separate cases.
ValueError
is okay to me.
I also agree ValueError
makes sense.
For python/peps#3845, I've described this with prose rather than the equivalent Python code:
Extra keys (which do not correspond to local variables on the underlying
frame) may be removed as usual withdel
statements or thepop()
method.Using
del
, or thepop()
method, to remove keys that correspond to local
variables on the underlying frame is NOT supported, and attempting to do so
will raiseValueError
.Local variables can only be set to
None
(or some other value) via the proxy,
they cannot be unbound completely.
(and then linked back to this PR and the associated bug report from the "Implementation Notes" section)
For the main docs, looking at https://docs.python.org/dev/reference/datamodel.html#frame-objects there's nothing that currently goes into that level of detail. I've filed a separate docs issue (#125731) suggesting adding a 4th subsection there that talks more about the write-through proxy behaviour.
Ok, let's go with ValueError.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
…xtra locals (pythonGH-125616)
(cherry picked from commit 5b7a872)
Co-authored-by: Tian Gao gaogaotiantian@hotmail.com
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request