Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions Doc/library/inspect.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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__`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this part of the text requires a .. versionadded:: 3.14, but I am not sure on that

Copy link
Contributor Author

@zuo zuo Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe .. versionchanged:: ..... would be more approproate, as here we change the behavior of an existing entity, rather than add a new entity.

Also – here we come back to the question I asked at the beginning – shouldn't it be applied also to Python 3.13? (considering that Python 3.13 is still in the beta phase, and here we have a bugfix, rather than a feature, even if it seems too disruptive to be backported to the 3.12 and earlier Python versions...)

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, versionchanged would be the right docs notation.

Copy link
Contributor

@ncoghlan ncoghlan Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note in the __set__ documentation (along with the interpreter's actual behaviour) makes it clear that this is a bugfix (i.e. adding __delete__ is enough to make a descriptor a data descriptor), so backporting to 3.13 makes sense to me.

I also agree that the risk/reward for backporting further isn't there, so 3.13 is as far back as the change should go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added. :)

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
Expand All @@ -515,6 +515,12 @@ attributes (see :ref:`import-mod-attrs` for module attributes):
:attr:`~method.__func__` attribute (etc) when an object passes
:func:`ismethod`.

.. versionchanged:: 3.13
Objects with :meth:`~object.__get__` and :meth:`~object.__delete__`
but without :meth:`~object.__set__` are no more considered by this
function as method descriptors (in previous versions, they were
incorrectly deemed as such).
zuo marked this conversation as resolved.
Show resolved Hide resolved


.. function:: isdatadescriptor(object)

Expand Down
11 changes: 7 additions & 4 deletions Lib/inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
121 changes: 118 additions & 3 deletions Lib/test/test_inspect/test_inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -1803,6 +1803,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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.assertFalse(inspect.ismethoddescriptor(Owner().static_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))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, I'd rather prefer to reduce the number of these cases...

I'd keep:

  • Owner().instance_method (method object – bound to an instance),
  • Owner().class_method (method object – bound to a class),
  • Owner().static_method (just a function object)

I'd remove: function and a_lambda completely – each of them is also just a function object, so they are redundant.

When it comes to the ones you proposed, please note that Owner.instance_method and Owner.static_method are also just function objects, and Owner.class_method is a method object bound to a class (identical to the Owner().class_method one).

This test class should focus on the behavior of inspect.ismethoddescriptor(). Various ways of obtaining objects of the same type are out of scope of it.

(In the first version, I wanted to mimic some elements of the already existing test class TestIsDataDescriptor. But now I believe that that one should be improved instead – though rather not as a part of this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree with my reasoning, or would you prefer these new cases to be added anyway? Could you please decide, I have no strong feelings about this particular detail. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer having all possible cases. One reason is that it shows how robust the inspect is and for me (at Sphinx), it's quite important to have a robust inspect module. So, if there are some possibilities in the future where something could go wrong (hello Murphy), it's better to have a wide coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! (it also implicitly helps in asserting that a function and a lambda function have the same type)

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):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Correct :func:`inspect.ismethoddescriptor` to check also for the lack of
:meth:`~object.__delete__`. Patch by Jan Kaliszewski.
Loading