bpo-41287: Handle doc argument of property.__init__ in subclasses (python#23205)

sizmailov · JelleZijlstra · web-flow · commit 66f5add1f0ac · 2022-05-28T20:25:51.000-07:00
Fixes python#85459 Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
diff --git a/Lib/test/test_property.py b/Lib/test/test_property.py
@@ -219,6 +219,9 @@ def test_property_set_name_incorrect_args(self):
 class PropertySub(property):
     """This is a subclass of property"""
 
+class PropertySubWoDoc(property):
+    pass
+
 class PropertySubSlots(property):
     """This is a subclass of property that defines __slots__"""
     __slots__ = ()
@@ -237,6 +240,38 @@ def spam(self):
         else:
             raise Exception("AttributeError not raised")
 
+    @unittest.skipIf(sys.flags.optimize >= 2,
+                     "Docstrings are omitted with -O2 and above")
+    def test_issue41287(self):
+
+        self.assertEqual(PropertySub.__doc__, "This is a subclass of property",
+                         "Docstring of `property` subclass is ignored")
+
+        doc = PropertySub(None, None, None, "issue 41287 is fixed").__doc__
+        self.assertEqual(doc, "issue 41287 is fixed",
+                         "Subclasses of `property` ignores `doc` constructor argument")
+
+        def getter(x):
+            """Getter docstring"""
+
+        def getter_wo_doc(x):
+            pass
+
+        for ps in property, PropertySub, PropertySubWoDoc:
+            doc = ps(getter, None, None, "issue 41287 is fixed").__doc__
+            self.assertEqual(doc, "issue 41287 is fixed",
+                             "Getter overrides explicit property docstring (%s)" % ps.__name__)
+
+            doc = ps(getter, None, None, None).__doc__
+            self.assertEqual(doc, "Getter docstring", "Getter docstring is not picked-up (%s)" % ps.__name__)
+
+            doc = ps(getter_wo_doc, None, None, "issue 41287 is fixed").__doc__
+            self.assertEqual(doc, "issue 41287 is fixed",
+                             "Getter overrides explicit property docstring (%s)" % ps.__name__)
+
+            doc = ps(getter_wo_doc, None, None, None).__doc__
+            self.assertIsNone(doc, "Property class doc appears in instance __doc__ (%s)" % ps.__name__)
+
     @unittest.skipIf(sys.flags.optimize >= 2,
                      "Docstrings are omitted with -O2 and above")
     def test_docstring_copy(self):
@@ -249,6 +284,66 @@ def spam(self):
             Foo.spam.__doc__,
             "spam wrapped in property subclass")
 
+    @unittest.skipIf(sys.flags.optimize >= 2,
+                     "Docstrings are omitted with -O2 and above")
+    def test_docstring_copy2(self):
+        """
+        Property tries to provide the best docstring it finds for its instances.
+        If a user-provided docstring is available, it is preserved on copies.
+        If no docstring is available during property creation, the property
+        will utilize the docstring from the getter if available.
+        """
+        def getter1(self):
+            return 1
+        def getter2(self):
+            """doc 2"""
+            return 2
+        def getter3(self):
+            """doc 3"""
+            return 3
+
+        # Case-1: user-provided doc is preserved in copies
+        #         of property with undocumented getter
+        p = property(getter1, None, None, "doc-A")
+
+        p2 = p.getter(getter2)
+        self.assertEqual(p.__doc__, "doc-A")
+        self.assertEqual(p2.__doc__, "doc-A")
+
+        # Case-2: user-provided doc is preserved in copies
+        #         of property with documented getter
+        p = property(getter2, None, None, "doc-A")
+
+        p2 = p.getter(getter3)
+        self.assertEqual(p.__doc__, "doc-A")
+        self.assertEqual(p2.__doc__, "doc-A")
+
+        # Case-3: with no user-provided doc new getter doc
+        #         takes precendence
+        p = property(getter2, None, None, None)
+
+        p2 = p.getter(getter3)
+        self.assertEqual(p.__doc__, "doc 2")
+        self.assertEqual(p2.__doc__, "doc 3")
+
+        # Case-4: A user-provided doc is assigned after property construction
+        #         with documented getter. The doc IS NOT preserved.
+        #         It's an odd behaviour, but it's a strange enough
+        #         use case with no easy solution.
+        p = property(getter2, None, None, None)
+        p.__doc__ = "user"
+        p2 = p.getter(getter3)
+        self.assertEqual(p.__doc__, "user")
+        self.assertEqual(p2.__doc__, "doc 3")
+
+        # Case-5: A user-provided doc is assigned after property construction
+        #         with UNdocumented getter. The doc IS preserved.
+        p = property(getter1, None, None, None)
+        p.__doc__ = "user"
+        p2 = p.getter(getter2)
+        self.assertEqual(p.__doc__, "user")
+        self.assertEqual(p2.__doc__, "user")
+
     @unittest.skipIf(sys.flags.optimize >= 2,
                      "Docstrings are omitted with -O2 and above")
     def test_property_setter_copies_getter_docstring(self):
diff --git a/Misc/NEWS.d/next/Library/2022-04-03-11-25-02.bpo-41287.8CTdwf.rst b/Misc/NEWS.d/next/Library/2022-04-03-11-25-02.bpo-41287.8CTdwf.rst
@@ -0,0 +1 @@
+Fix handling of the ``doc`` argument in subclasses of :func:`property`.
diff --git a/Objects/descrobject.c b/Objects/descrobject.c
@@ -1782,38 +1782,55 @@ property_init_impl(propertyobject *self, PyObject *fget, PyObject *fset,
     Py_XINCREF(fget);
     Py_XINCREF(fset);
     Py_XINCREF(fdel);
-    Py_XINCREF(doc);
 
     Py_XSETREF(self->prop_get, fget);
     Py_XSETREF(self->prop_set, fset);
     Py_XSETREF(self->prop_del, fdel);
-    Py_XSETREF(self->prop_doc, doc);
+    Py_XSETREF(self->prop_doc, NULL);
     Py_XSETREF(self->prop_name, NULL);
 
     self->getter_doc = 0;
+    PyObject *prop_doc = NULL;
 
+    if (doc != NULL && doc != Py_None) {
+        prop_doc = doc;
+        Py_XINCREF(prop_doc);
+    }
     /* if no docstring given and the getter has one, use that one */
-    if ((doc == NULL || doc == Py_None) && fget != NULL) {
-        PyObject *get_doc;
-        int rc = _PyObject_LookupAttr(fget, &_Py_ID(__doc__), &get_doc);
+    else if (fget != NULL) {
+        int rc = _PyObject_LookupAttr(fget, &_Py_ID(__doc__), &prop_doc);
         if (rc <= 0) {
             return rc;
         }
-        if (Py_IS_TYPE(self, &PyProperty_Type)) {
-            Py_XSETREF(self->prop_doc, get_doc);
+        if (prop_doc == Py_None) {
+            prop_doc = NULL;
+            Py_DECREF(Py_None);
         }
-        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. */
-            int err = PyObject_SetAttr(
-                    (PyObject *)self, &_Py_ID(__doc__), get_doc);
-            Py_DECREF(get_doc);
-            if (err < 0)
-                return -1;
+        if (prop_doc != NULL){
+            self->getter_doc = 1;
+        }
+    }
+
+    /* At this point `prop_doc` is either NULL or
+       a non-None object with incremented ref counter */
+
+    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 (prop_doc == NULL) {
+            prop_doc = Py_None;
+            Py_INCREF(prop_doc);
         }
-        self->getter_doc = 1;
+        int err = PyObject_SetAttr(
+                    (PyObject *)self, &_Py_ID(__doc__), prop_doc);
+        Py_XDECREF(prop_doc);
+        if (err < 0)
+            return -1;
     }
 
     return 0;

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Fix handling of the ``doc`` argument in subclasses of :func:`property`.