gh-120381: Fix inspect.ismethoddescriptor() (#120383)

The `inspect.ismethoddescriptor()` function did not check for the lack of
`__delete__()` and, consequently, erroneously returned True when applied
to *data* descriptors with only `__get__()` and `__delete__()` defined.

Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Alyssa Coghlan <[email protected]>

Author: 3 people

Doc/library/inspect.rst

@@ -504,9 +504,9 @@ attributes (see :ref:`import-mod-attrs` for module attributes):
    are true.
 
    This, for example, is true of ``int.__add__``.  An object passing this test
-   has a :meth:`~object.__get__` method but not a :meth:`~object.__set__`
-   method, but beyond that the set of attributes varies.  A
-   :attr:`~definition.__name__` attribute is usually
+   has a :meth:`~object.__get__` method, but not a :meth:`~object.__set__`
+   method or a :meth:`~object.__delete__` method.  Beyond that, the set of
+   attributes varies.  A :attr:`~definition.__name__` attribute is usually
    sensible, and :attr:`!__doc__` often is.
 
    Methods implemented via descriptors that also pass one of the other tests
@@ -515,6 +515,11 @@ attributes (see :ref:`import-mod-attrs` for module attributes):
    :attr:`~method.__func__` attribute (etc) when an object passes
    :func:`ismethod`.
 
+   .. versionchanged:: 3.13
+      This function no longer incorrectly reports objects with :meth:`~object.__get__`
+      and :meth:`~object.__delete__`, but not :meth:`~object.__set__`, as being method
+      descriptors (such objects are data descriptors, not method descriptors).
+
 
 .. function:: isdatadescriptor(object)
 

Lib/inspect.py

@@ -307,9 +307,10 @@ def ismethoddescriptor(object):
     But not if ismethod() or isclass() or isfunction() are true.
 
     This is new in Python 2.2, and, for example, is true of int.__add__.
-    An object passing this test has a __get__ attribute but not a __set__
-    attribute, but beyond that the set of attributes varies.  __name__ is
-    usually sensible, and __doc__ often is.
+    An object passing this test has a __get__ attribute, but not a
+    __set__ attribute or a __delete__ attribute. Beyond that, the set
+    of attributes varies; __name__ is usually sensible, and __doc__
+    often is.
 
     Methods implemented via descriptors that also pass one of the other
     tests return false from the ismethoddescriptor() test, simply because
@@ -319,7 +320,9 @@ def ismethoddescriptor(object):
         # mutual exclusion
         return False
     tp = type(object)
-    return hasattr(tp, "__get__") and not hasattr(tp, "__set__")
+    return (hasattr(tp, "__get__")
+            and not hasattr(tp, "__set__")
+            and not hasattr(tp, "__delete__"))
 
 def isdatadescriptor(object):
     """Return true if the object is a data descriptor.

Lib/test/test_inspect/test_inspect.py

@@ -55,9 +55,8 @@
 # ismodule, isclass, ismethod, isfunction, istraceback, isframe, iscode,
 # isbuiltin, isroutine, isgenerator, isgeneratorfunction, getmembers,
 # getdoc, getfile, getmodule, getsourcefile, getcomments, getsource,
-# getclasstree, getargvalues, formatargvalues,
-# currentframe, stack, trace, isdatadescriptor,
-# ismethodwrapper
+# getclasstree, getargvalues, formatargvalues, currentframe,
+# stack, trace, ismethoddescriptor, isdatadescriptor, ismethodwrapper
 
 # NOTE: There are some additional tests relating to interaction with
 #       zipimport in the test_zipimport_support test module.
@@ -179,6 +178,7 @@ def test_excluding_predicates(self):
         self.istest(inspect.ismethod, 'git.argue')
         self.istest(inspect.ismethod, 'mod.custom_method')
         self.istest(inspect.ismodule, 'mod')
+        self.istest(inspect.ismethoddescriptor, 'int.__add__')
         self.istest(inspect.isdatadescriptor, 'collections.defaultdict.default_factory')
         self.istest(inspect.isgenerator, '(x for x in range(2))')
         self.istest(inspect.isgeneratorfunction, 'generator_function_example')
@@ -1813,6 +1813,121 @@ def test_typing_replacement(self):
         self.assertEqual(inspect.formatannotation(ann1), 'Union[List[testModule.typing.A], int]')
 
 
+class TestIsMethodDescriptor(unittest.TestCase):
+
+    def test_custom_descriptors(self):
+        class MethodDescriptor:
+            def __get__(self, *_): pass
+        class MethodDescriptorSub(MethodDescriptor):
+            pass
+        class DataDescriptorWithNoGet:
+            def __set__(self, *_): pass
+        class DataDescriptorWithGetSet:
+            def __get__(self, *_): pass
+            def __set__(self, *_): pass
+        class DataDescriptorWithGetDelete:
+            def __get__(self, *_): pass
+            def __delete__(self, *_): pass
+        class DataDescriptorSub(DataDescriptorWithNoGet,
+                                DataDescriptorWithGetDelete):
+            pass
+
+        # Custom method descriptors:
+        self.assertTrue(
+            inspect.ismethoddescriptor(MethodDescriptor()),
+            '__get__ and no __set__/__delete__ => method descriptor')
+        self.assertTrue(
+            inspect.ismethoddescriptor(MethodDescriptorSub()),
+            '__get__ (inherited) and no __set__/__delete__'
+            ' => method descriptor')
+
+        # Custom data descriptors:
+        self.assertFalse(
+            inspect.ismethoddescriptor(DataDescriptorWithNoGet()),
+            '__set__ (and no __get__) => not a method descriptor')
+        self.assertFalse(
+            inspect.ismethoddescriptor(DataDescriptorWithGetSet()),
+            '__get__ and __set__ => not a method descriptor')
+        self.assertFalse(
+            inspect.ismethoddescriptor(DataDescriptorWithGetDelete()),
+            '__get__ and __delete__ => not a method descriptor')
+        self.assertFalse(
+            inspect.ismethoddescriptor(DataDescriptorSub()),
+            '__get__, __set__ and __delete__ => not a method descriptor')
+
+        # Classes of descriptors (are *not* descriptors themselves):
+        self.assertFalse(inspect.ismethoddescriptor(MethodDescriptor))
+        self.assertFalse(inspect.ismethoddescriptor(MethodDescriptorSub))
+        self.assertFalse(inspect.ismethoddescriptor(DataDescriptorSub))
+
+    def test_builtin_descriptors(self):
+        builtin_slot_wrapper = int.__add__  # This one is mentioned in docs.
+        class Owner:
+            def instance_method(self): pass
+            @classmethod
+            def class_method(cls): pass
+            @staticmethod
+            def static_method(): pass
+            @property
+            def a_property(self): pass
+        class Slotermeyer:
+            __slots__ = 'a_slot',
+        def function():
+            pass
+        a_lambda = lambda: None
+
+        # Example builtin method descriptors:
+        self.assertTrue(
+            inspect.ismethoddescriptor(builtin_slot_wrapper),
+            'a builtin slot wrapper is a method descriptor')
+        self.assertTrue(
+            inspect.ismethoddescriptor(Owner.__dict__['class_method']),
+            'a classmethod object is a method descriptor')
+        self.assertTrue(
+            inspect.ismethoddescriptor(Owner.__dict__['static_method']),
+            'a staticmethod object is a method descriptor')
+
+        # Example builtin data descriptors:
+        self.assertFalse(
+            inspect.ismethoddescriptor(Owner.__dict__['a_property']),
+            'a property is not a method descriptor')
+        self.assertFalse(
+            inspect.ismethoddescriptor(Slotermeyer.__dict__['a_slot']),
+            'a slot is not a method descriptor')
+
+        # `types.MethodType`/`types.FunctionType` instances (they *are*
+        # method descriptors, but `ismethoddescriptor()` explicitly
+        # excludes them):
+        self.assertFalse(inspect.ismethoddescriptor(Owner().instance_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner().class_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner().static_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner.instance_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner.class_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner.static_method))
+        self.assertFalse(inspect.ismethoddescriptor(function))
+        self.assertFalse(inspect.ismethoddescriptor(a_lambda))
+
+    def test_descriptor_being_a_class(self):
+        class MethodDescriptorMeta(type):
+            def __get__(self, *_): pass
+        class ClassBeingMethodDescriptor(metaclass=MethodDescriptorMeta):
+            pass
+        # `ClassBeingMethodDescriptor` itself *is* a method descriptor,
+        # but it is *also* a class, and `ismethoddescriptor()` explicitly
+        # excludes classes.
+        self.assertFalse(
+            inspect.ismethoddescriptor(ClassBeingMethodDescriptor),
+            'classes (instances of type) are explicitly excluded')
+
+    def test_non_descriptors(self):
+        class Test:
+            pass
+        self.assertFalse(inspect.ismethoddescriptor(Test()))
+        self.assertFalse(inspect.ismethoddescriptor(Test))
+        self.assertFalse(inspect.ismethoddescriptor([42]))
+        self.assertFalse(inspect.ismethoddescriptor(42))
+
+
 class TestIsDataDescriptor(unittest.TestCase):
 
     def test_custom_descriptors(self):

Misc/NEWS.d/next/Library/2024-06-12-11-54-05.gh-issue-120381.O-BNLs.rst

@@ -0,0 +1,2 @@
+Correct :func:`inspect.ismethoddescriptor` to check also for the lack of
+:meth:`~object.__delete__`.  Patch by Jan Kaliszewski.