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 }})

gaogaotiantian

@gaogaotiantian

@blurb-it

@gaogaotiantian

@gaogaotiantian

@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.

ZeroIntensity

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.

@gaogaotiantian

@terryjreedy

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

@markshannon

I think this needs backporting to 3.13, as that's the version mentioned in the issue

markshannon

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

@bedevere-app

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@markshannon

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")

@gaogaotiantian

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.

@markshannon

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.

@gaogaotiantian

@gaogaotiantian

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.

@ncoghlan

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 with del statements or the pop()
method.

Using del, or the pop() method, to remove keys that correspond to local
variables on the underlying frame is NOT supported, and attempting to do so
will raise ValueError.

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.

ncoghlan

@markshannon

Ok, let's go with ValueError.

markshannon

Choose a reason for hiding this comment

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

Thanks

@miss-islington-app

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app

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

Oct 21, 2024

@gaogaotiantian @miss-islington

…xtra locals (pythonGH-125616)

(cherry picked from commit 5b7a872)

Co-authored-by: Tian Gao gaogaotiantian@hotmail.com

@bedevere-app

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request

Jan 12, 2025

@gaogaotiantian @ebonnal