From 56b7a2d985a358ff498a57cf87901e1b2ad69861 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 26 Jul 2024 13:56:43 +0200 Subject: [PATCH 1/9] gh-120906: Support arbitrary hashable keys in FrameLocalsProxy --- Lib/test/test_frame.py | 47 +++++++ ...-07-26-13-56-32.gh-issue-120906.qBh2I9.rst | 1 + Objects/frameobject.c | 131 +++++++++--------- 3 files changed, 112 insertions(+), 67 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-07-26-13-56-32.gh-issue-120906.qBh2I9.rst diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index b7ef6cefaabbc0..d4cbcf75f7e79e 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -431,6 +431,53 @@ def test_is_mapping(self): kind = "other" self.assertEqual(kind, "mapping") + def test_proxy_key_stringlikes(self): + class StringSubclass(str): + pass + + class ImpostorX: + def __hash__(self): + return hash('x') + + def __eq__(self, other): + return other == 'x' + + def f(obj): + x = 1 + proxy = sys._getframe().f_locals + proxy[obj] = 2 + return ( + proxy.keys() == ['obj', 'x', 'proxy'], + proxy == {'obj': 'x', 'x': 2, 'proxy': proxy}, + proxy + ) + + for obj in StringSubclass('x'), ImpostorX(): + with self.subTest(cls=type(obj).__name__): + + assertion1, assertion2, proxy = f(obj) + self.assertEqual(proxy.keys(), ['obj', 'x', 'proxy']) + self.assertEqual(proxy, {'obj': 'x', 'x': 2, 'proxy': proxy}) + self.assertTrue(assertion1) + self.assertTrue(assertion2) + + def test_proxy_key_unhashables(self): + class StringSubclass(str): + __hash__ = None + + class ObjectSubclass: + __hash__ = None + + proxy = sys._getframe().f_locals + + for obj in StringSubclass('x'), ObjectSubclass(): + with self.subTest(cls=type(obj).__name__): + with self.assertRaises(TypeError): + proxy[obj] + with self.assertRaises(TypeError): + proxy[obj] = 0 + + class TestFrameCApi(unittest.TestCase): def test_basic(self): x = 1 diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-07-26-13-56-32.gh-issue-120906.qBh2I9.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-07-26-13-56-32.gh-issue-120906.qBh2I9.rst new file mode 100644 index 00000000000000..2b753bc37d4a39 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-07-26-13-56-32.gh-issue-120906.qBh2I9.rst @@ -0,0 +1 @@ +:attr:`frame.f_locals` now supports arbitrary hashable objects as keys. diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 88093eb9071ae4..7bf7a77ca595a4 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -53,22 +53,35 @@ static int framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) { /* - * Returns the fast locals index of the key + * Returns -2 (!) if an error occurred; exception will be set. + * Returns the fast locals index of the key on success: * - if read == true, returns the index if the value is not NULL * - if read == false, returns the index if the value is not hidden + * Otherwise returns -1. */ - assert(PyUnicode_CheckExact(key)); - PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); - int found_key = false; - // We do 2 loops here because it's highly possible the key is interned - // and we can do a pointer comparison. + // If the key is interned (highly possible), we can do a pointer comparison. + int interned = PyUnicode_CheckExact(key) && PyUnicode_CHECK_INTERNED(key); + if (!interned) { + // Ensure that the key is hashable. + Py_hash_t hash = PyObject_Hash(key); + if (hash == -1) { + return -2; + } + } + for (int i = 0; i < co->co_nlocalsplus; i++) { PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - if (name == key) { - found_key = true; + int same = (name == key); + if (!same && !interned) { + same = PyObject_RichCompareBool(name, key, Py_EQ); + if (same < 0) { + return -2; + } + } + if (same) { if (read) { if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) { return i; @@ -81,25 +94,6 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) } } - if (!found_key) { - // This is unlikely, but we need to make sure. This means the key - // is not interned. - for (int i = 0; i < co->co_nlocalsplus; i++) { - PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - if (_PyUnicode_EQ(name, key)) { - if (read) { - if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) { - return i; - } - } else { - if (!(_PyLocals_GetKind(co->co_localspluskinds, i) & CO_FAST_HIDDEN)) { - return i; - } - } - } - } - } - return -1; } @@ -109,13 +103,14 @@ framelocalsproxy_getitem(PyObject *self, PyObject *key) PyFrameObject* frame = ((PyFrameLocalsProxyObject*)self)->frame; PyCodeObject* co = _PyFrame_GetCode(frame->f_frame); - if (PyUnicode_CheckExact(key)) { - int i = framelocalsproxy_getkeyindex(frame, key, true); - if (i >= 0) { - PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); - assert(value != NULL); - return Py_NewRef(value); - } + int i = framelocalsproxy_getkeyindex(frame, key, true); + if (i == -2) { + return NULL; + } + if (i >= 0) { + PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); + assert(value != NULL); + return Py_NewRef(value); } // Okay not in the fast locals, try extra locals @@ -145,37 +140,38 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) return -1; } - if (PyUnicode_CheckExact(key)) { - int i = framelocalsproxy_getkeyindex(frame, key, false); - if (i >= 0) { - _Py_Executors_InvalidateDependency(PyInterpreterState_Get(), co, 1); - - _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); - _PyStackRef oldvalue = fast[i]; - PyObject *cell = NULL; - if (kind == CO_FAST_FREE) { - // The cell was set when the frame was created from - // the function's closure. - assert(oldvalue.bits != 0 && PyCell_Check(PyStackRef_AsPyObjectBorrow(oldvalue))); - cell = PyStackRef_AsPyObjectBorrow(oldvalue); - } else if (kind & CO_FAST_CELL && oldvalue.bits != 0) { - PyObject *as_obj = PyStackRef_AsPyObjectBorrow(oldvalue); - if (PyCell_Check(as_obj)) { - cell = as_obj; - } + int i = framelocalsproxy_getkeyindex(frame, key, false); + if (i == -2) { + return -1; + } + if (i >= 0) { + _Py_Executors_InvalidateDependency(PyInterpreterState_Get(), co, 1); + + _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); + _PyStackRef oldvalue = fast[i]; + PyObject *cell = NULL; + if (kind == CO_FAST_FREE) { + // The cell was set when the frame was created from + // the function's closure. + assert(oldvalue.bits != 0 && PyCell_Check(PyStackRef_AsPyObjectBorrow(oldvalue))); + cell = PyStackRef_AsPyObjectBorrow(oldvalue); + } else if (kind & CO_FAST_CELL && oldvalue.bits != 0) { + PyObject *as_obj = PyStackRef_AsPyObjectBorrow(oldvalue); + if (PyCell_Check(as_obj)) { + cell = as_obj; } - if (cell != NULL) { - PyObject *oldvalue_o = PyCell_GET(cell); - if (value != oldvalue_o) { - PyCell_SET(cell, Py_XNewRef(value)); - Py_XDECREF(oldvalue_o); - } - } else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) { - PyStackRef_XCLOSE(fast[i]); - fast[i] = PyStackRef_FromPyObjectNew(value); + } + if (cell != NULL) { + PyObject *oldvalue_o = PyCell_GET(cell); + if (value != oldvalue_o) { + PyCell_SET(cell, Py_XNewRef(value)); + Py_XDECREF(oldvalue_o); } - return 0; + } else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) { + PyStackRef_XCLOSE(fast[i]); + fast[i] = PyStackRef_FromPyObjectNew(value); } + return 0; } // Okay not in the fast locals, try extra locals @@ -545,11 +541,12 @@ framelocalsproxy_contains(PyObject *self, PyObject *key) { PyFrameObject *frame = ((PyFrameLocalsProxyObject*)self)->frame; - if (PyUnicode_CheckExact(key)) { - int i = framelocalsproxy_getkeyindex(frame, key, true); - if (i >= 0) { - return 1; - } + int i = framelocalsproxy_getkeyindex(frame, key, true); + if (i == -2) { + return -1; + } + if (i >= 0) { + return 1; } PyObject *extra = ((PyFrameObject*)frame)->f_extra_locals; From 791e4fccca1f835a3734c02d0b8a2f0ec97f5802 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 26 Jul 2024 15:16:41 +0200 Subject: [PATCH 2/9] Run mapping protocol tests on FrameLocalsProxy --- Lib/test/test_frame.py | 64 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index d4cbcf75f7e79e..776a531b2a9af9 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -15,6 +15,7 @@ from test import support from test.support import import_helper, threading_helper from test.support.script_helper import assert_python_ok +from test import mapping_tests class ClearTest(unittest.TestCase): @@ -478,6 +479,69 @@ class ObjectSubclass: proxy[obj] = 0 +class FrameLocalsProxyMappingTests(mapping_tests.TestHashMappingProtocol): + """Test that FrameLocalsProxy behaves like a Mapping (with exceptions)""" + + def _f(*args, **kwargs): + def _f(): + return sys._getframe().f_locals + return _f() + type2test = _f + + @unittest.skipIf(True, 'Unlike a mapping: empty proxy != empty proxy') + def test_constructor(self): + pass + + @unittest.skipIf(True, 'Unlike a mapping: del proxy[key] fails') + def test_write(self): + pass + + @unittest.skipIf(True, 'Unlike a mapping: no proxy.popitem') + def test_popitem(self): + pass + + @unittest.skipIf(True, 'Unlike a mapping: no proxy.pop') + def test_pop(self): + pass + + @unittest.skipIf(True, 'Unlike a mapping: no proxy.clear') + def test_clear(self): + pass + + @unittest.skipIf(True, 'Unlike a mapping: no proxy.fromkeys') + def test_fromkeys(self): + pass + + # no del + def test_getitem(self): + mapping_tests.BasicTestMappingProtocol.test_getitem(self) + d = self._full_mapping({'a': 1, 'b': 2}) + self.assertEqual(d['a'], 1) + self.assertEqual(d['b'], 2) + d['c'] = 3 + d['a'] = 4 + self.assertEqual(d['c'], 3) + self.assertEqual(d['a'], 4) + + @unittest.skipIf(True, 'Unlike a mapping: no proxy.update') + def test_update(self): + pass + + # proxy.copy rerourns a regular dict + def test_copy(self): + d = self._full_mapping({1:1, 2:2, 3:3}) + self.assertEqual(d.copy(), {1:1, 2:2, 3:3}) + d = self._empty_mapping() + self.assertEqual(d.copy(), d) + self.assertRaises(TypeError, d.copy, None) + + self.assertIsInstance(d.copy(), dict) + + @unittest.skipIf(True, 'Unlike a mapping: empty proxy != empty proxy') + def test_eq(self): + pass + + class TestFrameCApi(unittest.TestCase): def test_basic(self): x = 1 From 03dc148667c1a782049c0c6337c1fee4e0aa535d Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Sat, 27 Jul 2024 08:33:32 +0200 Subject: [PATCH 3/9] Revert to two loops --- Objects/frameobject.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 7bf7a77ca595a4..47bbcf5fe5cfa8 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -62,25 +62,38 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); - // If the key is interned (highly possible), we can do a pointer comparison. - int interned = PyUnicode_CheckExact(key) && PyUnicode_CHECK_INTERNED(key); - if (!interned) { - // Ensure that the key is hashable. + // Ensure that the key is hashable. + if (!PyUnicode_CheckExact(key)) { Py_hash_t hash = PyObject_Hash(key); if (hash == -1) { return -2; } } + // 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); - int same = (name == key); - if (!same && !interned) { - same = PyObject_RichCompareBool(name, key, Py_EQ); - if (same < 0) { - return -2; + if (name == key) { + if (read) { + if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) { + return i; + } + } else { + if (!(_PyLocals_GetKind(co->co_localspluskinds, i) & CO_FAST_HIDDEN)) { + return i; + } } } + } + // This is unlikely, but we need to make sure. This means the key + // is not interned. + for (int i = 0; i < co->co_nlocalsplus; i++) { + PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); + int same = PyObject_RichCompareBool(name, key, Py_EQ); + if (same < 0) { + return -2; + } if (same) { if (read) { if (framelocalsproxy_getval(frame->f_frame, co, i) != NULL) { From ddc4384588fa217198069e2771977e9fb4ab2d31 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Sat, 27 Jul 2024 08:37:15 +0200 Subject: [PATCH 4/9] Check that the hash matches --- Objects/frameobject.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 47bbcf5fe5cfa8..2e81709f053689 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -63,11 +63,9 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) PyCodeObject *co = _PyFrame_GetCode(frame->f_frame); // Ensure that the key is hashable. - if (!PyUnicode_CheckExact(key)) { - Py_hash_t hash = PyObject_Hash(key); - if (hash == -1) { - return -2; - } + Py_hash_t key_hash = PyObject_Hash(key); + if (key_hash == -1) { + return -2; } // We do 2 loops here because it's highly possible the key is interned @@ -90,6 +88,11 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) // is not interned. for (int i = 0; i < co->co_nlocalsplus; i++) { PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); + Py_hash_t name_hash = PyObject_Hash(name); + assert(name_hash != -1); // keys are exact unicode + if (name_hash != key_hash) { + continue; + } int same = PyObject_RichCompareBool(name, key, Py_EQ); if (same < 0) { return -2; From 4bba239465c4ca27f0c062aca20b00a1434ca4b5 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Sat, 27 Jul 2024 17:54:19 +0200 Subject: [PATCH 5/9] Apply test suggestions from code review Co-authored-by: Alyssa Coghlan --- Lib/test/test_frame.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 776a531b2a9af9..3f76191489319c 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -448,19 +448,21 @@ def f(obj): proxy = sys._getframe().f_locals proxy[obj] = 2 return ( - proxy.keys() == ['obj', 'x', 'proxy'], - proxy == {'obj': 'x', 'x': 2, 'proxy': proxy}, + list(proxy.keys()), + dict(proxy), proxy ) for obj in StringSubclass('x'), ImpostorX(): with self.subTest(cls=type(obj).__name__): - assertion1, assertion2, proxy = f(obj) - self.assertEqual(proxy.keys(), ['obj', 'x', 'proxy']) - self.assertEqual(proxy, {'obj': 'x', 'x': 2, 'proxy': proxy}) - self.assertTrue(assertion1) - self.assertTrue(assertion2) + keys_snapshot, proxy_snapshot, proxy = f(obj) + expected_keys = ['obj', 'x', 'proxy'] + expected_dict = {'obj': 'x', 'x': 2, 'proxy': proxy} + self.assertEqual(proxy.keys(), expected_keys) + self.assertEqual(proxy, expected_dict) + self.assertEqual(keys_snapshot, expected_keys) + self.assertEqual(proxy_snapshot, expected_dict) def test_proxy_key_unhashables(self): class StringSubclass(str): @@ -488,7 +490,7 @@ def _f(): return _f() type2test = _f - @unittest.skipIf(True, 'Unlike a mapping: empty proxy != empty proxy') + @unittest.skipIf(True, 'Locals proxies for different frames never compare as equal') def test_constructor(self): pass @@ -527,7 +529,7 @@ def test_getitem(self): def test_update(self): pass - # proxy.copy rerourns a regular dict + # proxy.copy returns a regular dict def test_copy(self): d = self._full_mapping({1:1, 2:2, 3:3}) self.assertEqual(d.copy(), {1:1, 2:2, 3:3}) @@ -537,7 +539,7 @@ def test_copy(self): self.assertIsInstance(d.copy(), dict) - @unittest.skipIf(True, 'Unlike a mapping: empty proxy != empty proxy') + @unittest.skipIf(True, 'Locals proxies for different frames never compare as equal') def test_eq(self): pass From d37d8f2246552e5816140a8eea0ddb6f45d00c13 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 29 Jul 2024 15:12:16 +0200 Subject: [PATCH 6/9] Early return for NULL/hidden values --- Objects/frameobject.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 2e81709f053689..b1489b3884a5b1 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -82,6 +82,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) return i; } } + return -1; } } // This is unlikely, but we need to make sure. This means the key @@ -107,6 +108,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) return i; } } + return -1; } } From f5df751f1088678050b471e908969b1e746a09ff Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 29 Jul 2024 16:03:12 +0200 Subject: [PATCH 7/9] Handle hidden values in the short-circuit --- Objects/frameobject.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index b1489b3884a5b1..564fa8e4c5a5e4 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -67,6 +67,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) if (key_hash == -1) { return -2; } + bool found = false; // We do 2 loops here because it's highly possible the key is interned // and we can do a pointer comparison. @@ -82,9 +83,12 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) return i; } } - return -1; + found = true; } } + if (found) { + return -1; + } // This is unlikely, but we need to make sure. This means the key // is not interned. for (int i = 0; i < co->co_nlocalsplus; i++) { @@ -108,7 +112,6 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) return i; } } - return -1; } } From 34c3d24aa143872e9805ac9f7f7316c83dba17f6 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 30 Jul 2024 14:12:18 +0200 Subject: [PATCH 8/9] Add comment in Objects/frameobject.c Co-authored-by: Alyssa Coghlan --- Objects/frameobject.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 564fa8e4c5a5e4..a8be7d75371c16 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -87,6 +87,8 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject* key, bool read) } } if (found) { + // This is an attempt to read an unset local variable or + // write to a variable that is hidden from regular write operations return -1; } // This is unlikely, but we need to make sure. This means the key From 7d3e8b5d7f64e9b2aeceaec68432469ad827415c Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 30 Jul 2024 23:28:28 +0200 Subject: [PATCH 9/9] Test setting an unintialized local through the proxy --- Lib/test/test_frame.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 3f76191489319c..ca88e657367d9a 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -432,7 +432,7 @@ def test_is_mapping(self): kind = "other" self.assertEqual(kind, "mapping") - def test_proxy_key_stringlikes(self): + def _x_stringlikes(self): class StringSubclass(str): pass @@ -443,6 +443,9 @@ def __hash__(self): def __eq__(self, other): return other == 'x' + return StringSubclass('x'), ImpostorX(), 'x' + + def test_proxy_key_stringlikes_overwrite(self): def f(obj): x = 1 proxy = sys._getframe().f_locals @@ -453,7 +456,7 @@ def f(obj): proxy ) - for obj in StringSubclass('x'), ImpostorX(): + for obj in self._x_stringlikes(): with self.subTest(cls=type(obj).__name__): keys_snapshot, proxy_snapshot, proxy = f(obj) @@ -464,6 +467,17 @@ def f(obj): self.assertEqual(keys_snapshot, expected_keys) self.assertEqual(proxy_snapshot, expected_dict) + def test_proxy_key_stringlikes_ftrst_write(self): + def f(obj): + proxy = sys._getframe().f_locals + proxy[obj] = 2 + self.assertEqual(x, 2) + x = 1 + + for obj in self._x_stringlikes(): + with self.subTest(cls=type(obj).__name__): + f(obj) + def test_proxy_key_unhashables(self): class StringSubclass(str): __hash__ = None