From 177280432b2e4846176382986cfac03beac199b8 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 16 Oct 2024 15:59:14 -0400 Subject: [PATCH 1/5] Allow FrameLocalsProxy to delete and pop keys from extra locals --- Lib/test/test_frame.py | 30 +++++++++++++++-- Objects/frameobject.c | 76 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 98 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 32de8ed9a13f80..8ff1004721514d 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -397,15 +397,41 @@ def test_repr(self): def test_delete(self): x = 1 d = sys._getframe().f_locals - with self.assertRaises(TypeError): + + # This needs to be tested before f_extra_locals is created + with self.assertRaises(KeyError): + del d['non_exist'] + + with self.assertRaises(KeyError): + d.pop('non_exist') + + with self.assertRaises(KeyError): del d['x'] with self.assertRaises(AttributeError): d.clear() - with self.assertRaises(AttributeError): + with self.assertRaises(KeyError): d.pop('x') + with self.assertRaises(KeyError): + d.pop('x', None) + + # 'm', 'n' is stored in f_extra_locals + d['m'] = 1 + d['n'] = 1 + + with self.assertRaises(KeyError): + d.pop('non_exist') + + del d['m'] + self.assertEqual(d.pop('n'), 1) + + self.assertNotIn('m', d) + self.assertNotIn('n', d) + + self.assertEqual(d.pop('n', 2), 2) + @support.cpython_only def test_sizeof(self): proxy = sys._getframe().f_locals diff --git a/Objects/frameobject.c b/Objects/frameobject.c index f3a66ffc9aac8f..b754db439774b5 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -158,16 +158,16 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) _PyStackRef *fast = _PyFrame_GetLocalsArray(frame->f_frame); PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); - if (value == NULL) { - PyErr_SetString(PyExc_TypeError, "cannot remove variables from FrameLocalsProxy"); - return -1; - } - int i = framelocalsproxy_getkeyindex(frame, key, false); if (i == -2) { return -1; } if (i >= 0) { + if (value == NULL) { + PyErr_SetString(PyExc_KeyError, "cannot remove local variables from FrameLocalsProxy"); + return -1; + } + _Py_Executors_InvalidateDependency(PyInterpreterState_Get(), co, 1); _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); @@ -202,6 +202,10 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) PyObject *extra = frame->f_extra_locals; if (extra == NULL) { + if (value == NULL) { + PyErr_Format(PyExc_KeyError, "'%R'", key); + return -1; + } extra = PyDict_New(); if (extra == NULL) { return -1; @@ -211,7 +215,11 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) assert(PyDict_Check(extra)); - return PyDict_SetItem(extra, key, value); + if (value == NULL) { + return PyDict_DelItem(extra, key); + } else { + return PyDict_SetItem(extra, key, value); + } } static int @@ -676,6 +684,60 @@ framelocalsproxy_setdefault(PyObject* self, PyObject *const *args, Py_ssize_t na return result; } +static PyObject* +framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) +{ + if (nargs < 1 || nargs > 2) { + PyErr_SetString(PyExc_TypeError, "pop expected 1 or 2 arguments"); + return NULL; + } + + PyObject *key = args[0]; + PyObject *default_value = NULL; + + if (nargs == 2) { + default_value = args[1]; + } + + PyFrameObject *frame = ((PyFrameLocalsProxyObject*)self)->frame; + + int i = framelocalsproxy_getkeyindex(frame, key, false); + if (i == -2) { + return NULL; + } + + if (i >= 0) { + PyErr_SetString(PyExc_KeyError, "cannot remove local variables from FrameLocalsProxy"); + return NULL; + } + + PyObject* result = NULL; + + if (frame->f_extra_locals == NULL) { + if (default_value != NULL) { + return Py_XNewRef(default_value); + } else { + PyErr_Format(PyExc_KeyError, "'%R'", key); + return NULL; + } + } + + if (PyDict_Pop(frame->f_extra_locals, key, &result) < 0) { + return NULL; + } + + if (result == NULL) { + if (default_value != NULL) { + return Py_XNewRef(default_value); + } else { + PyErr_Format(PyExc_KeyError, "'%R'", key); + return NULL; + } + } + + return result; +} + static PyObject* framelocalsproxy_copy(PyObject *self, PyObject *Py_UNUSED(ignored)) { @@ -743,6 +805,8 @@ static PyMethodDef framelocalsproxy_methods[] = { NULL}, {"get", _PyCFunction_CAST(framelocalsproxy_get), METH_FASTCALL, NULL}, + {"pop", _PyCFunction_CAST(framelocalsproxy_pop), METH_FASTCALL, + NULL}, {"setdefault", _PyCFunction_CAST(framelocalsproxy_setdefault), METH_FASTCALL, NULL}, {NULL, NULL} /* sentinel */ From 0130a66439c6de0c5ef1e95a94bbb0f76b515a03 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 16 Oct 2024 20:32:44 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2024-10-16-20-32-40.gh-issue-125590.stHzOP.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-16-20-32-40.gh-issue-125590.stHzOP.rst diff --git a/Misc/NEWS.d/next/Library/2024-10-16-20-32-40.gh-issue-125590.stHzOP.rst b/Misc/NEWS.d/next/Library/2024-10-16-20-32-40.gh-issue-125590.stHzOP.rst new file mode 100644 index 00000000000000..dc6765ada641a9 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-16-20-32-40.gh-issue-125590.stHzOP.rst @@ -0,0 +1 @@ +Allow ``FrameLocalsProxy`` to delete and pop if the key is not a fast variable. From 124aaa01181faa5cca7a7c7c2be703467ca2a2f1 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 16 Oct 2024 16:34:41 -0400 Subject: [PATCH 3/5] Remove blank line --- Lib/test/test_frame.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 8ff1004721514d..5775b523d47a90 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -423,7 +423,7 @@ def test_delete(self): with self.assertRaises(KeyError): d.pop('non_exist') - + del d['m'] self.assertEqual(d.pop('n'), 1) From c8a8eb48c6452aba68f102708a939ec4236d99e8 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 16 Oct 2024 21:29:23 -0400 Subject: [PATCH 4/5] Use RuntimeError and some internal functions --- Lib/test/test_frame.py | 8 ++++---- Objects/frameobject.c | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 5775b523d47a90..1e4ae6058afd24 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -399,22 +399,22 @@ def test_delete(self): d = sys._getframe().f_locals # This needs to be tested before f_extra_locals is created - with self.assertRaises(KeyError): + with self.assertRaisesRegex(KeyError, 'non_exist'): del d['non_exist'] with self.assertRaises(KeyError): d.pop('non_exist') - with self.assertRaises(KeyError): + with self.assertRaisesRegex(RuntimeError, 'local variables'): del d['x'] with self.assertRaises(AttributeError): d.clear() - with self.assertRaises(KeyError): + with self.assertRaises(RuntimeError): d.pop('x') - with self.assertRaises(KeyError): + with self.assertRaises(RuntimeError): d.pop('x', None) # 'm', 'n' is stored in f_extra_locals diff --git a/Objects/frameobject.c b/Objects/frameobject.c index b754db439774b5..05dee04abfb2a0 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -5,6 +5,7 @@ #include "pycore_code.h" // CO_FAST_LOCAL, etc. #include "pycore_function.h" // _PyFunction_FromConstructor() #include "pycore_moduleobject.h" // _PyModule_GetDict() +#include "pycore_modsupport.h" // _PyArg_CheckPositional() #include "pycore_object.h" // _PyObject_GC_UNTRACK() #include "pycore_opcode_metadata.h" // _PyOpcode_Deopt, _PyOpcode_Caches @@ -164,7 +165,7 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) } if (i >= 0) { if (value == NULL) { - PyErr_SetString(PyExc_KeyError, "cannot remove local variables from FrameLocalsProxy"); + PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy"); return -1; } @@ -203,7 +204,7 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) if (extra == NULL) { if (value == NULL) { - PyErr_Format(PyExc_KeyError, "'%R'", key); + _PyErr_SetKeyError(key); return -1; } extra = PyDict_New(); @@ -687,8 +688,7 @@ framelocalsproxy_setdefault(PyObject* self, PyObject *const *args, Py_ssize_t na static PyObject* framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) { - if (nargs < 1 || nargs > 2) { - PyErr_SetString(PyExc_TypeError, "pop expected 1 or 2 arguments"); + if (!_PyArg_CheckPositional("pop", nargs, 1, 2)) { return NULL; } @@ -707,17 +707,17 @@ framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) } if (i >= 0) { - PyErr_SetString(PyExc_KeyError, "cannot remove local variables from FrameLocalsProxy"); + PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy"); return NULL; } - PyObject* result = NULL; + PyObject *result = NULL; if (frame->f_extra_locals == NULL) { if (default_value != NULL) { return Py_XNewRef(default_value); } else { - PyErr_Format(PyExc_KeyError, "'%R'", key); + _PyErr_SetKeyError(key); return NULL; } } @@ -730,7 +730,7 @@ framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) if (default_value != NULL) { return Py_XNewRef(default_value); } else { - PyErr_Format(PyExc_KeyError, "'%R'", key); + _PyErr_SetKeyError(key); return NULL; } } From d16cfadffd97edcfbbbe8bd76d2ee5c7937fcfcb Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Fri, 18 Oct 2024 17:38:35 -0400 Subject: [PATCH 5/5] Use ValueError instead of RuntimeError --- Lib/test/test_frame.py | 6 +++--- Objects/frameobject.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 1e4ae6058afd24..11f191700ccef0 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -405,16 +405,16 @@ def test_delete(self): with self.assertRaises(KeyError): d.pop('non_exist') - with self.assertRaisesRegex(RuntimeError, 'local variables'): + with self.assertRaisesRegex(ValueError, 'local variables'): del d['x'] with self.assertRaises(AttributeError): d.clear() - with self.assertRaises(RuntimeError): + with self.assertRaises(ValueError): d.pop('x') - with self.assertRaises(RuntimeError): + with self.assertRaises(ValueError): d.pop('x', None) # 'm', 'n' is stored in f_extra_locals diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 05dee04abfb2a0..5ef48919a081be 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -165,7 +165,7 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) } if (i >= 0) { if (value == NULL) { - PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy"); + PyErr_SetString(PyExc_ValueError, "cannot remove local variables from FrameLocalsProxy"); return -1; } @@ -707,7 +707,7 @@ framelocalsproxy_pop(PyObject* self, PyObject *const *args, Py_ssize_t nargs) } if (i >= 0) { - PyErr_SetString(PyExc_RuntimeError, "cannot remove local variables from FrameLocalsProxy"); + PyErr_SetString(PyExc_ValueError, "cannot remove local variables from FrameLocalsProxy"); return NULL; }