From 93276d2be56837bc197d0700027a65bfcc85d89c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Fri, 2 Jun 2023 17:39:22 -0700 Subject: [PATCH 1/7] gh-98963: Restore the ability to have a dict-less property. Ignore doc string assignment failures in `property` as has been the behavior of all past Python releases. One behavior change: The longstanding tested behavior of raising an `AttributeError` when using a slotted no-dict property subclass as a decorator on a getter sporting a docstring is now consistent with the subclassing behavior and does not produce an error. --- Lib/test/test_property.py | 73 ++++++++++++++++--- ...3-06-02-17-39-19.gh-issue-98963.J4wJgk.rst | 3 + Objects/descrobject.c | 33 +++++++-- 3 files changed, 91 insertions(+), 18 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst diff --git a/Lib/test/test_property.py b/Lib/test/test_property.py index d4bdf50c0192ae..e89665af7fb1dd 100644 --- a/Lib/test/test_property.py +++ b/Lib/test/test_property.py @@ -245,17 +245,68 @@ class PropertySubSlots(property): class PropertySubclassTests(unittest.TestCase): - def test_slots_docstring_copy_exception(self): - try: - class Foo(object): - @PropertySubSlots - def spam(self): - """Trying to copy this docstring will raise an exception""" - return 1 - except AttributeError: - pass - else: - raise Exception("AttributeError not raised") + @unittest.skipIf(sys.flags.optimize >= 2, + "Docstrings are omitted with -O2 and above") + def test_slots_getter_docstring_inherited_from_property_subclass(self): + # This raised an AttributeError prior to 3.12, now it matches the + # other long existing behavior of not reporting errors when a property + # docstring cannot be set due to being a class without a dict. + class Foo(object): + @PropertySubSlots + def spam(self): + """Trying to assign this docstring should silently fail.""" + return 1 + + self.assertEqual(Foo.spam.__doc__, PropertySubSlots.__doc__) + + def test_property_with_slots_no_docstring(self): + # https://github.com/python/cpython/issues/98963#issuecomment-1574413319 + class slotted_prop(property): + __slots__ = ("foo",) + + p = slotted_prop() # no AttributeError + self.assertIsNone(getattr(p, "__doc__", None)) + + def undocumented_getter(): + return 4 + + p = slotted_prop(undocumented_getter) # no AttributeError + self.assertIsNone(getattr(p, "__doc__", None)) + + @unittest.skipIf(sys.flags.optimize >= 2, + "Docstrings are omitted with -O2 and above") + def test_property_with_slots_docstring_silently_dropped(self): + # https://github.com/python/cpython/issues/98963#issuecomment-1574413319 + class slotted_prop(property): + __slots__ = ("foo",) + + p = slotted_prop(doc="what's up") # no AttributeError + self.assertIsNone(p.__doc__) + + def documented_getter(): + """getter doc.""" + return 4 + + # The getter doc, if any, is used if none is otherwise supplied. + p = slotted_prop(documented_getter) # no AttributeError + self.assertIsNone(p.__doc__) + + @unittest.skipIf(sys.flags.optimize >= 2, + "Docstrings are omitted with -O2 and above") + def test_property_with_slots_and_doc_slot_docstring_present(self): + # https://github.com/python/cpython/issues/98963#issuecomment-1574413319 + class slotted_prop(property): + __slots__ = ("foo", "__doc__") + + p = slotted_prop(doc="what's up") + self.assertEqual("what's up", p.__doc__) # meep meep! + + def documented_getter(): + """what's up getter doc?""" + return 4 + + p = slotted_prop(documented_getter) + self.assertEqual("what's up getter doc?", p.__doc__) # meep meep! @unittest.skipIf(sys.flags.optimize >= 2, "Docstrings are omitted with -O2 and above") diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst b/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst new file mode 100644 index 00000000000000..dcf694fa1c50cc --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst @@ -0,0 +1,3 @@ +Restore the ability for a subclass of :class:`property` to define +``__slots__`` or otherwise be dict-less by ignoring any attempt to set a +docstring on such a class. This behavior had regressed in 3.12beta1. diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 17c0c85a06c4b8..961f7e3ca066b5 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1485,7 +1485,10 @@ class property(object): self.__get = fget self.__set = fset self.__del = fdel - self.__doc__ = doc + try: + self.__doc__ = doc + except AttributeError: # read-only or dict-less class + pass def __get__(self, inst, type=None): if inst is None: @@ -1806,10 +1809,9 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, if (Py_IS_TYPE(self, &PyProperty_Type)) { Py_XSETREF(self->prop_doc, prop_doc); } else { - /* If this is a property subclass, put __doc__ - in dict of the subclass instance instead, - otherwise it gets shadowed by __doc__ in the - class's dict. */ + /* If this is a property subclass, put __doc__ in the dict + or designated slot of the subclass instance instead, otherwise + it gets shadowed by __doc__ in the class's dict. */ if (prop_doc == NULL) { prop_doc = Py_NewRef(Py_None); @@ -1817,8 +1819,25 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, int err = PyObject_SetAttr( (PyObject *)self, &_Py_ID(__doc__), prop_doc); Py_XDECREF(prop_doc); - if (err < 0) - return -1; + if (err < 0) { + if (PyErr_Occurred() == PyExc_AttributeError) { + PyErr_Clear(); + // https://github.com/python/cpython/issues/98963#issuecomment-1574413319 + // Python silently dropped this doc assignment through 3.11. + // We preserve that behavior for backwards compatibility. + if (prop_doc == Py_None) { + Py_DECREF(prop_doc); + return 0; + } + // If we ever want to deprecate this behavior, here is where to + // raise a DeprecationWarning; do not generate such noise when + // prop_doc is None. + Py_DECREF(prop_doc); + return 0; + } else { + return -1; + } + } } return 0; From c6cdc7f87bf6258c36a0ff0c9f91a89a5d428da7 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 4 Jun 2023 12:34:12 -0700 Subject: [PATCH 2/7] Fix ASAN issue, to many DECREFs :P --- Objects/descrobject.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 961f7e3ca066b5..2194e28f43f8a7 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1825,14 +1825,10 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, // https://github.com/python/cpython/issues/98963#issuecomment-1574413319 // Python silently dropped this doc assignment through 3.11. // We preserve that behavior for backwards compatibility. - if (prop_doc == Py_None) { - Py_DECREF(prop_doc); - return 0; - } - // If we ever want to deprecate this behavior, here is where to - // raise a DeprecationWarning; do not generate such noise when - // prop_doc is None. - Py_DECREF(prop_doc); + // + // If we ever want to deprecate this behavior, only raise a + // warning or error when proc_doc is not None so that + // property without a specific doc= still works. return 0; } else { return -1; From 3ceab53965fd4daf422d489b29ad9d6b7425baef Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 4 Jun 2023 12:35:27 -0700 Subject: [PATCH 3/7] s/tabs/spaces/ (ugh editor configs) --- Objects/descrobject.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 2194e28f43f8a7..d66e37c1e6716f 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1825,10 +1825,10 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, // https://github.com/python/cpython/issues/98963#issuecomment-1574413319 // Python silently dropped this doc assignment through 3.11. // We preserve that behavior for backwards compatibility. - // + // // If we ever want to deprecate this behavior, only raise a - // warning or error when proc_doc is not None so that - // property without a specific doc= still works. + // warning or error when proc_doc is not None so that + // property without a specific doc= still works. return 0; } else { return -1; From 95727f576902ad4f6f373ac182a4cc9e96d96592 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 4 Jun 2023 13:23:24 -0700 Subject: [PATCH 4/7] Restore all historical behaviors. --- Lib/test/test_property.py | 36 ++++++++++++++++++++---------------- Objects/descrobject.c | 19 +++++++++++++++++-- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/Lib/test/test_property.py b/Lib/test/test_property.py index e89665af7fb1dd..c61ea477adbbb5 100644 --- a/Lib/test/test_property.py +++ b/Lib/test/test_property.py @@ -245,19 +245,22 @@ class PropertySubSlots(property): class PropertySubclassTests(unittest.TestCase): - @unittest.skipIf(sys.flags.optimize >= 2, - "Docstrings are omitted with -O2 and above") - def test_slots_getter_docstring_inherited_from_property_subclass(self): - # This raised an AttributeError prior to 3.12, now it matches the - # other long existing behavior of not reporting errors when a property - # docstring cannot be set due to being a class without a dict. - class Foo(object): - @PropertySubSlots - def spam(self): - """Trying to assign this docstring should silently fail.""" - return 1 - - self.assertEqual(Foo.spam.__doc__, PropertySubSlots.__doc__) + def test_slots_docstring_copy_exception(self): + # A special case error that we preserve despite the GH-98963 behavior + # that would otherwise silence this error and not set a docstr. + # This came from https://github.com/python/cpython/commit/b18500d39d791c879e9904ebac293402b4a7cd34 + # as part of https://bugs.python.org/issue5890 which allowed docs to + # be set using property subclasses in the first place. + try: + class Foo(object): + @PropertySubSlots + def spam(self): + """Trying to copy this docstring will raise an exception""" + return 1 + except AttributeError: + pass + else: + raise Exception("AttributeError not raised") def test_property_with_slots_no_docstring(self): # https://github.com/python/cpython/issues/98963#issuecomment-1574413319 @@ -287,9 +290,10 @@ def documented_getter(): """getter doc.""" return 4 - # The getter doc, if any, is used if none is otherwise supplied. - p = slotted_prop(documented_getter) # no AttributeError - self.assertIsNone(p.__doc__) + # Historical behavior: A docstring from a getter always raises. + # (matches test_slots_docstring_copy_exception above). + with self.assertRaises(AttributeError): + p = slotted_prop(documented_getter) @unittest.skipIf(sys.flags.optimize >= 2, "Docstrings are omitted with -O2 and above") diff --git a/Objects/descrobject.c b/Objects/descrobject.c index d66e37c1e6716f..f3db1c9df9cce6 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1794,6 +1794,19 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, if (rc <= 0) { return rc; } + if (!Py_IS_TYPE(self, &PyProperty_Type) && + prop_doc != NULL && prop_doc != Py_None) { + // This oddity preserves the long existing behavior of surfacing + // an AttributeError when using a dict-less (__slots__) property + // subclass as a decorator on a getter method with a docstring. + // See PropertySubclassTest.test_slots_docstring_copy_exception. + int err = PyObject_SetAttr( + (PyObject *)self, &_Py_ID(__doc__), prop_doc); + Py_DECREF(prop_doc); + if (err < 0) { + return -1; + } + } if (prop_doc == Py_None) { prop_doc = NULL; Py_DECREF(Py_None); @@ -1818,9 +1831,9 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, } int err = PyObject_SetAttr( (PyObject *)self, &_Py_ID(__doc__), prop_doc); - Py_XDECREF(prop_doc); if (err < 0) { - if (PyErr_Occurred() == PyExc_AttributeError) { + assert(PyErr_Occurred()); + if (PyErr_ExceptionMatches(PyExc_AttributeError)) { PyErr_Clear(); // https://github.com/python/cpython/issues/98963#issuecomment-1574413319 // Python silently dropped this doc assignment through 3.11. @@ -1829,8 +1842,10 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, // If we ever want to deprecate this behavior, only raise a // warning or error when proc_doc is not None so that // property without a specific doc= still works. + Py_DECREF(prop_doc); return 0; } else { + Py_DECREF(prop_doc); return -1; } } From 996e70b78d9d1aac14faa0672ed94a673d53ba85 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 4 Jun 2023 14:08:46 -0700 Subject: [PATCH 5/7] Reword comments and NEWS. --- Lib/test/test_property.py | 18 +++++++----------- ...23-06-02-17-39-19.gh-issue-98963.J4wJgk.rst | 7 ++++--- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/Lib/test/test_property.py b/Lib/test/test_property.py index c61ea477adbbb5..45aa9e51c06de0 100644 --- a/Lib/test/test_property.py +++ b/Lib/test/test_property.py @@ -247,20 +247,16 @@ class PropertySubclassTests(unittest.TestCase): def test_slots_docstring_copy_exception(self): # A special case error that we preserve despite the GH-98963 behavior - # that would otherwise silence this error and not set a docstr. - # This came from https://github.com/python/cpython/commit/b18500d39d791c879e9904ebac293402b4a7cd34 + # that would otherwise silently ignore this error. + # This came from commit b18500d39d791c879e9904ebac293402b4a7cd34 # as part of https://bugs.python.org/issue5890 which allowed docs to - # be set using property subclasses in the first place. - try: + # be set via property subclasses in the first place. + with self.assertRaises(AttributeError): class Foo(object): @PropertySubSlots def spam(self): """Trying to copy this docstring will raise an exception""" return 1 - except AttributeError: - pass - else: - raise Exception("AttributeError not raised") def test_property_with_slots_no_docstring(self): # https://github.com/python/cpython/issues/98963#issuecomment-1574413319 @@ -273,7 +269,7 @@ class slotted_prop(property): def undocumented_getter(): return 4 - p = slotted_prop(undocumented_getter) # no AttributeError + p = slotted_prop(undocumented_getter) # New in 3.12: no AttributeError self.assertIsNone(getattr(p, "__doc__", None)) @unittest.skipIf(sys.flags.optimize >= 2, @@ -303,14 +299,14 @@ class slotted_prop(property): __slots__ = ("foo", "__doc__") p = slotted_prop(doc="what's up") - self.assertEqual("what's up", p.__doc__) # meep meep! + self.assertEqual("what's up", p.__doc__) # new in 3.12: This gets set. def documented_getter(): """what's up getter doc?""" return 4 p = slotted_prop(documented_getter) - self.assertEqual("what's up getter doc?", p.__doc__) # meep meep! + self.assertEqual("what's up getter doc?", p.__doc__) @unittest.skipIf(sys.flags.optimize >= 2, "Docstrings are omitted with -O2 and above") diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst b/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst index dcf694fa1c50cc..4caadb0875a188 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2023-06-02-17-39-19.gh-issue-98963.J4wJgk.rst @@ -1,3 +1,4 @@ -Restore the ability for a subclass of :class:`property` to define -``__slots__`` or otherwise be dict-less by ignoring any attempt to set a -docstring on such a class. This behavior had regressed in 3.12beta1. +Restore the ability for a subclass of :class:`property` to define ``__slots__`` +or otherwise be dict-less by ignoring failures to set a docstring on such a +class. This behavior had regressed in 3.12beta1. An :exc:`AttributeError` +where there had not previously been one was disruptive to existing code. From 517bd0dec879c86f4914f6e5778cc53cae778a60 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 4 Jun 2023 14:17:13 -0700 Subject: [PATCH 6/7] restore the singular decref. (correctness) --- Objects/descrobject.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Objects/descrobject.c b/Objects/descrobject.c index f3db1c9df9cce6..e26f90224f68b7 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1831,6 +1831,7 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, } int err = PyObject_SetAttr( (PyObject *)self, &_Py_ID(__doc__), prop_doc); + Py_DECREF(prop_doc); if (err < 0) { assert(PyErr_Occurred()); if (PyErr_ExceptionMatches(PyExc_AttributeError)) { @@ -1842,10 +1843,8 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, // If we ever want to deprecate this behavior, only raise a // warning or error when proc_doc is not None so that // property without a specific doc= still works. - Py_DECREF(prop_doc); return 0; } else { - Py_DECREF(prop_doc); return -1; } } From 0307832ee0cd470c9d0092f7df027348381d02be Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 4 Jun 2023 19:52:28 -0700 Subject: [PATCH 7/7] DECREF before return, not always. --- Objects/descrobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/descrobject.c b/Objects/descrobject.c index e26f90224f68b7..72ac4703949262 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1802,8 +1802,8 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset, // See PropertySubclassTest.test_slots_docstring_copy_exception. int err = PyObject_SetAttr( (PyObject *)self, &_Py_ID(__doc__), prop_doc); - Py_DECREF(prop_doc); if (err < 0) { + Py_DECREF(prop_doc); // release our new reference. return -1; } }