From dbc624cfa9db28cdbf66b90ca313fad96a75c74b Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 13 Jul 2022 16:37:40 -0700 Subject: [PATCH 1/7] Add failing regression tests --- Lib/test/test_opcache.py | 85 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 61f337d70ea787..2412a14ccf3fc2 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -21,3 +21,88 @@ def f(o): Descriptor.__set__ = lambda *args: None self.assertEqual(f(o), 2) + + def test_metaclass_descriptor_added_after_optimization(self): + class Descriptor: + pass + + class Metaclass(type): + attribute = Descriptor() + + class Class(metaclass=Metaclass): + attribute = True + + def f(): + return Class.attribute + + for _ in range(1025): + self.assertTrue(f()) + + Descriptor.__get__ = lambda self, instance, value: False + Descriptor.__set__ = lambda *args: None + + self.assertFalse(f()) + + def test_metaclass_descriptor_shadows_class_attribute(self): + class Metaclass(type): + @property + def attribute(self): + return True + + class Class(metaclass=Metaclass): + attribute = False + + def f(): + return Class.attribute + + for _ in range(1025): + self.assertTrue(f()) + + def test_metaclass_set_descriptor_after_optimization(self): + class Metaclass(type): + pass + + class Class(metaclass=Metaclass): + attribute = True + + @property + def attribute(self): + return False + + def f(): + return Class.attribute + + for _ in range(1025): + self.assertTrue(f()) + + Metaclass.attribute = attribute + self.assertFalse(f()) + + def test_metaclass_del_descriptor_after_optimization(self): + class Metaclass(type): + @property + def attribute(self): + return True + + class Class(metaclass=Metaclass): + attribute = False + + def f(): + return Class.attribute + + for _ in range(1025): + self.assertTrue(f()) + + del Metaclass.attribute + self.assertFalse(f()) + + def test_type_descriptor_shadows_attribute(self): + class Class: + __name__ = "Spam" + + for _ in range(1025): + self.assertEqual(Class.__name__, "Class") + +if __name__ == "__main__": + import unittest + unittest.main() \ No newline at end of file From 28d109391f8372ca1da7ab87e0ec1d8b4aca5ea4 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 13 Jul 2022 16:38:24 -0700 Subject: [PATCH 2/7] Check metaclass versions in LOAD_ATTR_CLASS --- Python/ceval.c | 4 +++- Python/specialize.c | 12 ++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 2a0ce63d435d7c..8091bf43c2b76f 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -3672,7 +3672,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int DEOPT_IF(((PyTypeObject *)cls)->tp_version_tag != type_version, LOAD_ATTR); assert(type_version != 0); - + uint32_t meta_version = read_u32(cache->keys_version); + DEOPT_IF(Py_TYPE(cls)->tp_version_tag != meta_version, LOAD_ATTR); + assert(meta_version != 0); STAT_INC(LOAD_ATTR, hit); PyObject *res = read_obj(cache->descr); assert(res != NULL); diff --git a/Python/specialize.c b/Python/specialize.c index 66cae44fa8f3d7..5a6bb9c1909986 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -947,22 +947,22 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, _PyLoadMethodCache *cache = (_PyLoadMethodCache *)(instr + 1); PyObject *descr = NULL; DescriptorClassification kind = 0; + if (_PyType_Lookup(Py_TYPE(owner), name)) { + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_ATTRIBUTE); + return -1; + } kind = analyze_descriptor((PyTypeObject *)owner, name, &descr, 0); switch (kind) { case METHOD: case NON_DESCRIPTOR: write_u32(cache->type_version, ((PyTypeObject *)owner)->tp_version_tag); + write_u32(cache->keys_version, Py_TYPE(owner)->tp_version_tag); write_obj(cache->descr, descr); _Py_SET_OPCODE(*instr, LOAD_ATTR_CLASS); return 0; #ifdef Py_STATS case ABSENT: - if (_PyType_Lookup(Py_TYPE(owner), name) != NULL) { - SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_ATTRIBUTE); - } - else { - SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); - } + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR); return -1; #endif default: From 3f1e5ce26a261f1b942cdafec4421d3ea4e2addd Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 13 Jul 2022 17:32:40 -0700 Subject: [PATCH 3/7] blurb add --- .../2022-07-13-16-42-57.gh-issue-94822.zRRzBN.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-07-13-16-42-57.gh-issue-94822.zRRzBN.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-07-13-16-42-57.gh-issue-94822.zRRzBN.rst b/Misc/NEWS.d/next/Core and Builtins/2022-07-13-16-42-57.gh-issue-94822.zRRzBN.rst new file mode 100644 index 00000000000000..5b24918e497706 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-07-13-16-42-57.gh-issue-94822.zRRzBN.rst @@ -0,0 +1,2 @@ +Fix an issue where lookups of metaclass descriptors may be ignored when an +identically-named attribute also exists on the class itself. From 4f84a5ce63fb4809079ed8841f63d32c2a8a3a4d Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 13 Jul 2022 17:32:50 -0700 Subject: [PATCH 4/7] Add more tests --- Lib/test/test_opcache.py | 183 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 181 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 2412a14ccf3fc2..f3afb24e39d94b 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -100,9 +100,188 @@ def test_type_descriptor_shadows_attribute(self): class Class: __name__ = "Spam" + def f(): + return Class.__name__ + + for _ in range(1025): + self.assertEqual(f(), "Class") + + def test_type_descriptor_shadows_attribute_method(self): + class Class: + mro = None + + def f(): + return Class.mro + + for _ in range(1025): + self.assertIsNone(f()) + + def test_type_descriptor_shadows_attribute_member(self): + class Class: + __base__ = None + + def f(): + return Class.__base__ + + for _ in range(1025): + self.assertIs(f(), object) + + def test_type_descriptor_shadows_attribute_getset(self): + class Class: + __name__ = "Spam" + + def f(): + return Class.__name__ + + for _ in range(1025): + self.assertEqual(f(), "Class") + + @unittest.skip("Not fixed yet!") + def test_metaclass_getattribute(self): + class Metaclass(type): + def __getattribute__(self, name): + return True + + class Class(metaclass=Metaclass): + attribute = False + + def f(): + return Class.attribute + + for _ in range(1025): + self.assertTrue(f()) + +class TestLoadMethodCache(unittest.TestCase): + def test_descriptor_added_after_optimization(self): + class Descriptor: + pass + + class C: + def __init__(self): + self.x = lambda: 1 + x = Descriptor() + + def f(o): + return o.x() + + o = C() + for i in range(1025): + assert f(o) == 1 + + Descriptor.__get__ = lambda self, instance, value: lambda: 2 + Descriptor.__set__ = lambda *args: None + + self.assertEqual(f(o), 2) + + def test_metaclass_descriptor_added_after_optimization(self): + class Descriptor: + pass + + class Metaclass(type): + attribute = Descriptor() + + class Class(metaclass=Metaclass): + attribute = lambda: True + + def f(): + return Class.attribute() + + for _ in range(1025): + self.assertTrue(f()) + + Descriptor.__get__ = lambda self, instance, value: lambda: False + Descriptor.__set__ = lambda *args: None + + self.assertFalse(f()) + + def test_metaclass_descriptor_shadows_class_attribute(self): + class Metaclass(type): + @property + def attribute(self): + return lambda: True + + class Class(metaclass=Metaclass): + attribute = lambda: False + + def f(): + return Class.attribute() + + for _ in range(1025): + self.assertTrue(f()) + + def test_metaclass_set_descriptor_after_optimization(self): + class Metaclass(type): + pass + + class Class(metaclass=Metaclass): + attribute = lambda: True + + @property + def attribute(self): + return lambda: False + + def f(): + return Class.attribute() + for _ in range(1025): - self.assertEqual(Class.__name__, "Class") + self.assertTrue(f()) + + Metaclass.attribute = attribute + self.assertFalse(f()) + + def test_metaclass_del_descriptor_after_optimization(self): + class Metaclass(type): + @property + def attribute(self): + return lambda: True + + class Class(metaclass=Metaclass): + attribute = lambda: False + + def f(): + return Class.attribute() + + for _ in range(1025): + self.assertTrue(f()) + + del Metaclass.attribute + self.assertFalse(f()) + + def test_type_descriptor_shadows_attribute_method(self): + class Class: + mro = lambda: ["Spam", "eggs"] + + def f(): + return Class.mro() + + for _ in range(1025): + self.assertEqual(f(), ["Spam", "eggs"]) + + def test_type_descriptor_shadows_attribute_member(self): + class Class: + __base__ = lambda: "Spam" + + def f(): + return Class.__base__() + + for _ in range(1025): + self.assertNotEqual(f(), "Spam") + + @unittest.skip("Not fixed yet!") + def test_metaclass_getattribute(self): + class Metaclass(type): + def __getattribute__(self, name): + return lambda: True + + class Class(metaclass=Metaclass): + attribute = lambda: False + + def f(): + return Class.attribute() + + for _ in range(1025): + self.assertTrue(f()) if __name__ == "__main__": import unittest - unittest.main() \ No newline at end of file + unittest.main() From 606d4605a2c5ebc24fb0417c2fe56ef080f3e3fd Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 14 Jul 2022 11:26:43 -0700 Subject: [PATCH 5/7] Refactor tests --- Lib/test/test_opcache.py | 110 +++++++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 40 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index f3afb24e39d94b..03fc8f7a2ebe39 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -1,5 +1,6 @@ import unittest + class TestLoadAttrCache(unittest.TestCase): def test_descriptor_added_after_optimization(self): class Descriptor: @@ -32,16 +33,23 @@ class Metaclass(type): class Class(metaclass=Metaclass): attribute = True + def __get__(self, instance, owner): + return False + + def __set__(self, instance, value): + return None + def f(): return Class.attribute for _ in range(1025): self.assertTrue(f()) - Descriptor.__get__ = lambda self, instance, value: False - Descriptor.__set__ = lambda *args: None + Descriptor.__get__ = __get__ + Descriptor.__set__ = __set__ - self.assertFalse(f()) + for _ in range(1025): + self.assertFalse(f()) def test_metaclass_descriptor_shadows_class_attribute(self): class Metaclass(type): @@ -76,7 +84,9 @@ def f(): self.assertTrue(f()) Metaclass.attribute = attribute - self.assertFalse(f()) + + for _ in range(1025): + self.assertFalse(f()) def test_metaclass_del_descriptor_after_optimization(self): class Metaclass(type): @@ -94,17 +104,9 @@ def f(): self.assertTrue(f()) del Metaclass.attribute - self.assertFalse(f()) - - def test_type_descriptor_shadows_attribute(self): - class Class: - __name__ = "Spam" - - def f(): - return Class.__name__ - + for _ in range(1025): - self.assertEqual(f(), "Class") + self.assertFalse(f()) def test_type_descriptor_shadows_attribute_method(self): class Class: @@ -136,7 +138,6 @@ def f(): for _ in range(1025): self.assertEqual(f(), "Class") - @unittest.skip("Not fixed yet!") def test_metaclass_getattribute(self): class Metaclass(type): def __getattribute__(self, name): @@ -151,27 +152,38 @@ def f(): for _ in range(1025): self.assertTrue(f()) + class TestLoadMethodCache(unittest.TestCase): def test_descriptor_added_after_optimization(self): class Descriptor: pass - class C: - def __init__(self): - self.x = lambda: 1 - x = Descriptor() + class Class: + attribute = Descriptor() - def f(o): - return o.x() + def __get__(self, instance, owner): + return lambda: False + + def __set__(self, instance, value): + return None - o = C() - for i in range(1025): - assert f(o) == 1 + def attribute(): + return True - Descriptor.__get__ = lambda self, instance, value: lambda: 2 - Descriptor.__set__ = lambda *args: None + instance = Class() + instance.attribute = attribute - self.assertEqual(f(o), 2) + def f(): + return instance.attribute() + + for _ in range(1025): + self.assertTrue(f()) + + Descriptor.__get__ = __get__ + Descriptor.__set__ = __set__ + + for _ in range(1025): + self.assertFalse(f()) def test_metaclass_descriptor_added_after_optimization(self): class Descriptor: @@ -181,7 +193,14 @@ class Metaclass(type): attribute = Descriptor() class Class(metaclass=Metaclass): - attribute = lambda: True + def attribute(): + return True + + def __get__(self, instance, owner): + return lambda: False + + def __set__(self, instance, value): + return None def f(): return Class.attribute() @@ -189,10 +208,11 @@ def f(): for _ in range(1025): self.assertTrue(f()) - Descriptor.__get__ = lambda self, instance, value: lambda: False - Descriptor.__set__ = lambda *args: None + Descriptor.__get__ = __get__ + Descriptor.__set__ = __set__ - self.assertFalse(f()) + for _ in range(1025): + self.assertFalse(f()) def test_metaclass_descriptor_shadows_class_attribute(self): class Metaclass(type): @@ -201,7 +221,8 @@ def attribute(self): return lambda: True class Class(metaclass=Metaclass): - attribute = lambda: False + def attribute(): + return False def f(): return Class.attribute() @@ -214,7 +235,8 @@ class Metaclass(type): pass class Class(metaclass=Metaclass): - attribute = lambda: True + def attribute(): + return True @property def attribute(self): @@ -227,7 +249,9 @@ def f(): self.assertTrue(f()) Metaclass.attribute = attribute - self.assertFalse(f()) + + for _ in range(1025): + self.assertFalse(f()) def test_metaclass_del_descriptor_after_optimization(self): class Metaclass(type): @@ -236,7 +260,8 @@ def attribute(self): return lambda: True class Class(metaclass=Metaclass): - attribute = lambda: False + def attribute(): + return False def f(): return Class.attribute() @@ -245,11 +270,14 @@ def f(): self.assertTrue(f()) del Metaclass.attribute - self.assertFalse(f()) + + for _ in range(1025): + self.assertFalse(f()) def test_type_descriptor_shadows_attribute_method(self): class Class: - mro = lambda: ["Spam", "eggs"] + def mro(): + return ["Spam", "eggs"] def f(): return Class.mro() @@ -259,7 +287,8 @@ def f(): def test_type_descriptor_shadows_attribute_member(self): class Class: - __base__ = lambda: "Spam" + def __base__(): + return "Spam" def f(): return Class.__base__() @@ -267,14 +296,14 @@ def f(): for _ in range(1025): self.assertNotEqual(f(), "Spam") - @unittest.skip("Not fixed yet!") def test_metaclass_getattribute(self): class Metaclass(type): def __getattribute__(self, name): return lambda: True class Class(metaclass=Metaclass): - attribute = lambda: False + def attribute(): + return False def f(): return Class.attribute() @@ -282,6 +311,7 @@ def f(): for _ in range(1025): self.assertTrue(f()) + if __name__ == "__main__": import unittest unittest.main() From 6864e951ef9aa41d053f3c70194e4d9c725c1275 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 14 Jul 2022 11:27:17 -0700 Subject: [PATCH 6/7] Don't specialize with metaclass __getattribute__ --- Python/specialize.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/Python/specialize.c b/Python/specialize.c index 5a6bb9c1909986..63fcda99dbb97b 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -945,18 +945,28 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name) { _PyLoadMethodCache *cache = (_PyLoadMethodCache *)(instr + 1); - PyObject *descr = NULL; - DescriptorClassification kind = 0; - if (_PyType_Lookup(Py_TYPE(owner), name)) { + PyTypeObject *metaclass = Py_TYPE(owner); + if (_PyType_Lookup(metaclass, name)) { + // The metaclass has the named attribute. This breaks LOAD_ATTR_CLASS + // when the attribute *also* exists on the class... but it also means we + // can't specialize for things like Class.__doc__, Class.__name__, etc. + // Perhaps LOAD_ATTR_METACLASS may be worth adding in the future? SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_ATTRIBUTE); return -1; } + if (metaclass->tp_getattro != PyType_Type.tp_getattro) { + // The metaclass defines __getattribute__. All bets are off: + SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OVERRIDDEN); + return -1; + } + PyObject *descr = NULL; + DescriptorClassification kind = 0; kind = analyze_descriptor((PyTypeObject *)owner, name, &descr, 0); switch (kind) { case METHOD: case NON_DESCRIPTOR: write_u32(cache->type_version, ((PyTypeObject *)owner)->tp_version_tag); - write_u32(cache->keys_version, Py_TYPE(owner)->tp_version_tag); + write_u32(cache->keys_version, metaclass->tp_version_tag); write_obj(cache->descr, descr); _Py_SET_OPCODE(*instr, LOAD_ATTR_CLASS); return 0; From 03aa529c9c9c6fc9d214b59fda7a0e72e39a9b97 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 14 Jul 2022 12:46:39 -0700 Subject: [PATCH 7/7] make patchcheck --- Lib/test/test_opcache.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_opcache.py b/Lib/test/test_opcache.py index 03fc8f7a2ebe39..550c430b24e17b 100644 --- a/Lib/test/test_opcache.py +++ b/Lib/test/test_opcache.py @@ -35,7 +35,7 @@ class Class(metaclass=Metaclass): def __get__(self, instance, owner): return False - + def __set__(self, instance, value): return None @@ -102,9 +102,9 @@ def f(): for _ in range(1025): self.assertTrue(f()) - + del Metaclass.attribute - + for _ in range(1025): self.assertFalse(f()) @@ -163,7 +163,7 @@ class Class: def __get__(self, instance, owner): return lambda: False - + def __set__(self, instance, value): return None @@ -198,7 +198,7 @@ def attribute(): def __get__(self, instance, owner): return lambda: False - + def __set__(self, instance, value): return None @@ -249,7 +249,7 @@ def f(): self.assertTrue(f()) Metaclass.attribute = attribute - + for _ in range(1025): self.assertFalse(f()) @@ -268,9 +268,9 @@ def f(): for _ in range(1025): self.assertTrue(f()) - + del Metaclass.attribute - + for _ in range(1025): self.assertFalse(f())