gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals #125616

gaogaotiantian · 2024-10-16T20:31:20Z

It's okay for us to pop/delete the keys that were added to f_locals manually, just ban the real fast variables.
@ncoghlan not sure if there's any documentation that needs to be changed.

There are two types of exceptions that could be raised:

RuntimeError when users trying to remove a local variable
KeyError when users trying to remove a key that does not exist

Issue: it's no longer possible to delete items from f_locals (FrameLocalsProxy) in 3.13+ #125590

gaogaotiantian · 2024-10-16T21:37:18Z

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

Objects/frameobject.c

ZeroIntensity · 2024-10-17T00:55:41Z

Objects/frameobject.c

+        if (default_value != NULL) {
+            return Py_XNewRef(default_value);
+        } else {
+            PyErr_Format(PyExc_KeyError, "'%R'", key);


A tiny optimization to consider (though, it's more complex--feel free to reject this):

Suggested change

PyErr_Format(PyExc_KeyError, "'%R'", key);

PyObject *repr = PyObject_Repr(key);

if (repr == NULL) {

return NULL;

}

PyErr_SetObject(PyExc_KeyError, repr);

I just realized I could've used _PyErr_SetKeyError(key) like dictobject.c. I'll switch to that.

Oh, that sounds better. I wasn't aware of that, TIL!

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.

Lib/test/test_frame.py

terryjreedy · 2024-10-17T06:05:17Z

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 · 2024-10-17T08:12:12Z

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

markshannon

The exception types need changing from RuntimeError to KeyError.
Otherwise, looks good.

markshannon · 2024-10-17T08:19:41Z

Objects/frameobject.c

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


I think this should be a KeyError.
Users will expect mapping types to raise a KeyError. https://docs.python.org/3/library/stdtypes.html#mapping-types-dict

markshannon · 2024-10-17T08:19:46Z

Objects/frameobject.c

+    }
+
+    if (i >= 0) {
+        PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy");


bedevere-app · 2024-10-17T08:21:04Z

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

markshannon · 2024-10-17T08:29:25Z

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 · 2024-10-17T15:45:39Z

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 · 2024-10-18T09:15:20Z

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 · 2024-10-18T21:40:40Z

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 · 2024-10-19T14:42:48Z

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.

markshannon · 2024-10-21T10:37:02Z

Ok, let's go with ValueError.

markshannon

Thanks

miss-islington-app · 2024-10-21T15:43:12Z

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

miss-islington-app · 2024-10-21T16:42:15Z

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!

…xtra locals (pythonGH-125616) (cherry picked from commit 5b7a872) Co-authored-by: Tian Gao <[email protected]>

bedevere-app · 2024-10-21T16:42:36Z

GH-125797 is a backport of this pull request to the 3.13 branch.

…extra locals (GH-125616) (#125797) gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals (GH-125616) (cherry picked from commit 5b7a872) Co-authored-by: Tian Gao <[email protected]>

Allow FrameLocalsProxy to delete and pop keys from extra locals

1772804

gaogaotiantian requested a review from ncoghlan October 16, 2024 20:31

gaogaotiantian requested a review from markshannon as a code owner October 16, 2024 20:31

bedevere-app bot added the awaiting core review label Oct 16, 2024

bedevere-app bot mentioned this pull request Oct 16, 2024

it's no longer possible to delete items from f_locals (FrameLocalsProxy) in 3.13+ #125590

Closed

blurb-it bot and others added 2 commits October 16, 2024 20:32

📜🤖 Added by blurb_it.

0130a66

Remove blank line

124aaa0

ZeroIntensity reviewed Oct 17, 2024

View reviewed changes

Use RuntimeError and some internal functions

c8a8eb4

markshannon added the needs backport to 3.13 bugs and security fixes label Oct 17, 2024

markshannon requested changes Oct 17, 2024

View reviewed changes

bedevere-app bot removed the awaiting core review label Oct 17, 2024

bedevere-app bot added the awaiting changes label Oct 17, 2024

markshannon mentioned this pull request Oct 18, 2024

PEP 667: Clarify specification ambiguities and the expected impact python/peps#3845

Merged

3 tasks

Use ValueError instead of RuntimeError

d16cfad

ncoghlan approved these changes Oct 19, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Oct 19, 2024

markshannon self-requested a review October 21, 2024 10:39

markshannon approved these changes Oct 21, 2024

View reviewed changes

gaogaotiantian merged commit 5b7a872 into python:main Oct 21, 2024
38 of 39 checks passed

bedevere-app bot removed the awaiting merge label Oct 21, 2024

gaogaotiantian deleted the allow-framelocalsproxy-pop-extra branch October 21, 2024 15:43

zware added needs backport to 3.13 bugs and security fixes and removed needs backport to 3.13 bugs and security fixes labels Oct 21, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 21, 2024

tyralla mentioned this pull request Oct 24, 2024

Support Python 3.13 and drop Python 3.9 hydpy-dev/hydpy#157

Open

2 tasks

fabioz mentioned this pull request Oct 26, 2024

Support 3.13 in PyDev.Debugger fabioz/PyDev.Debugger#293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals #125616

gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals #125616

gaogaotiantian commented Oct 16, 2024 •

edited

Loading

gaogaotiantian commented Oct 16, 2024

ZeroIntensity Oct 17, 2024

gaogaotiantian Oct 17, 2024

ZeroIntensity Oct 17, 2024

gaogaotiantian Oct 17, 2024

terryjreedy commented Oct 17, 2024

markshannon commented Oct 17, 2024

markshannon left a comment

markshannon Oct 17, 2024

markshannon Oct 17, 2024

bedevere-app bot commented Oct 17, 2024

markshannon commented Oct 17, 2024 •

edited

Loading

gaogaotiantian commented Oct 17, 2024

markshannon commented Oct 18, 2024

gaogaotiantian commented Oct 18, 2024 •

edited

Loading

ncoghlan commented Oct 19, 2024 •

edited

Loading

markshannon commented Oct 21, 2024

markshannon left a comment

miss-islington-app bot commented Oct 21, 2024

miss-islington-app bot commented Oct 21, 2024

bedevere-app bot commented Oct 21, 2024

-            PyErr_Format(PyExc_KeyError, "'%R'", key);
+            PyObject *repr = PyObject_Repr(key);
+            if (repr == NULL) {
+                return NULL;
+            }
+            PyErr_SetObject(PyExc_KeyError, repr);

gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals #125616

gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals #125616

Conversation

gaogaotiantian commented Oct 16, 2024 • edited Loading

gaogaotiantian commented Oct 16, 2024

ZeroIntensity Oct 17, 2024

Choose a reason for hiding this comment

gaogaotiantian Oct 17, 2024

Choose a reason for hiding this comment

ZeroIntensity Oct 17, 2024

Choose a reason for hiding this comment

gaogaotiantian Oct 17, 2024

Choose a reason for hiding this comment

terryjreedy commented Oct 17, 2024

markshannon commented Oct 17, 2024

markshannon left a comment

Choose a reason for hiding this comment

markshannon Oct 17, 2024

Choose a reason for hiding this comment

markshannon Oct 17, 2024

Choose a reason for hiding this comment

bedevere-app bot commented Oct 17, 2024

markshannon commented Oct 17, 2024 • edited Loading

gaogaotiantian commented Oct 17, 2024

markshannon commented Oct 18, 2024

gaogaotiantian commented Oct 18, 2024 • edited Loading

ncoghlan commented Oct 19, 2024 • edited Loading

markshannon commented Oct 21, 2024

markshannon left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Oct 21, 2024

miss-islington-app bot commented Oct 21, 2024

bedevere-app bot commented Oct 21, 2024

gaogaotiantian commented Oct 16, 2024 •

edited

Loading

markshannon commented Oct 17, 2024 •

edited

Loading

gaogaotiantian commented Oct 18, 2024 •

edited

Loading

ncoghlan commented Oct 19, 2024 •

edited

Loading