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

zuo · 2024-06-12T03:53:27Z

The inspect.ismethoddescriptor() function did not check for the lack of __delete__() and, consequently, erroneously returned True when applied to data descriptors that implemented just __get__() and __delete__().

Also, unit tests for inspect.ismethoddescriptor() (which were missing) have been added.

Issue: inspect.ismethoddescriptor(): lack of __delete__() is not checked #120381

📚 Documentation preview 📚: https://cpython-previews--120383.org.readthedocs.build/

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

zuo · 2024-06-12T03:55:58Z

Seems important:

As I noted in gh-120381, a care is needed when deciding to which Python versions this fix should be applied.

IMHO, the fix can be safely applied to the branches 3.14 (main) and 3.13 (still in the beta phase), yet backporting it to any of the earlier versions might be too disruptive.

Obviously, the decision will be made by a core developer, not me. :)

eendebakpt · 2024-06-12T05:56:59Z

Misc/NEWS.d/next/Library/2024-06-12-04-44-13.gh-issue-120381.GwTGOV.rst

@@ -0,0 +1,2 @@
+Correct ``inspect.ismethoddescriptor`` to check also for the lack of


Suggested change

Correct ``inspect.ismethoddescriptor`` to check also for the lack of

Correct :meth:`inspect.ismethoddescriptor` to check also for the lack of

OK, fixed both references (actually, the first one is a :func: one, and the second is a :meth: one).

eendebakpt · 2024-06-13T19:30:58Z

Lib/test/test_inspect/test_inspect.py

+        # excludes them):
+        self.assertFalse(inspect.ismethoddescriptor(Owner().instance_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner().class_method))
+        self.assertFalse(inspect.ismethoddescriptor(Owner().static_method))


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))

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).

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. :)

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.

OK, added :)

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

eendebakpt

Thanks for the extensive tests in the PR. I left two minor comments, but otherwise this looks good.

eendebakpt · 2024-06-13T19:37:40Z

Doc/library/inspect.rst

-   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__`


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

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?

Yes, versionchanged would be the right docs notation.

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.

OK, added. :)

serhiy-storchaka · 2024-06-15T09:12:50Z

Lib/inspect.py

+    if hasattr(tp, "__set__") or hasattr(tp, "__delete__"):
+        return False
+    return hasattr(tp, "__get__")


I would check __get__ first. It looks more natural.

Suggested change

if hasattr(tp, "__set__") or hasattr(tp, "__delete__"):

return False

return hasattr(tp, "__get__")

return hasattr(tp, "__get__") and not (hasattr(tp, "__set__") or hasattr(tp, "__delete__"))

OK, changed. :)

picnixz · 2024-06-15T10:30:00Z

Doc/library/inspect.rst

+      For objects with :meth:`~object.__get__` and :meth:`~object.__delete__`
+      but without :meth:`~object.__set__`, ``False`` is now returned
+      (previously, ``True`` was returned for such objects, which was
+      incorrect).


Suggested change

For objects with :meth:`~object.__get__` and :meth:`~object.__delete__`

but without :meth:`~object.__set__`, ``False`` is now returned

(previously, ``True`` was returned for such objects, which was

incorrect).

Objects with :meth:`~object.__get__` and :meth:`~object.__delete__`

but without :meth:`~object.__set__` are no more considered as method

descriptors. In previous versions, they were incorrectly deemed as such.

@picnixz OK, I committed your suggestion. However, doesn't the fragment are no more considered as method descriptors sound too general? I mean, someone could misunderstand it – that before Python 3.13 such objects were generally considered as method descriptors; whereas our intent is to inform that just this particular function incorrectly considered them as such.

What do you think about such wording:

Objects with __get__() and __delete__() but without __set__() are no more considered by this function as method descriptors. In previous versions, they incorrectly were.

(Or some other wording that would emphasize the focus on the behavior of this particular function...)

I just committed as slightly different variant:

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).

Could you please decide whether it is OK. :)

I've added a suggestion to tighten this up and make it clear it is describing a bug fix in this function rather than a change to the language itself.

Thank you! :) I applied it.

Sure thanks! my suggestions are in the end only... suggestions. I appreciate when someone likes it verbatim but I don't mind when it is reformulated!

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

Doc/library/inspect.rst

Co-authored-by: Alyssa Coghlan <[email protected]>

serhiy-storchaka

LGTM.

ncoghlan · 2024-06-18T11:56:24Z

Hypothesis test failure looked like an unrelated intermittent failure or false alarm (in test_glob), so I merged in main to trigger a CI rerun.

miss-islington-app · 2024-06-18T12:19:47Z

Thanks @zuo for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

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. (cherry picked from commit dacc5ac) Co-authored-by: Jan Kaliszewski <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Alyssa Coghlan <[email protected]>

bedevere-app · 2024-06-18T12:19:59Z

GH-120684 is a backport of this pull request to the 3.13 branch.

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]>

pythongh-120381: Fix inspect.ismethoddescriptor()

dcc561b

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

bedevere-app bot mentioned this pull request Jun 12, 2024

inspect.ismethoddescriptor(): lack of __delete__() is not checked #120381

Closed

bedevere-app bot added the awaiting review label Jun 12, 2024

Merge branch 'main' into inspect-ismethoddescriptor-fix

7740b0e

eendebakpt reviewed Jun 12, 2024

View reviewed changes

Fix the blurb

2dc2168

zuo requested a review from eendebakpt June 12, 2024 10:01

Merge branch 'main' into inspect-ismethoddescriptor-fix

4f13243

eendebakpt reviewed Jun 13, 2024

View reviewed changes

eendebakpt approved these changes Jun 13, 2024

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Jun 13, 2024

serhiy-storchaka self-requested a review June 14, 2024 07:47

zuo requested a review from eendebakpt June 15, 2024 00:34

serhiy-storchaka reviewed Jun 15, 2024

View reviewed changes

zuo and others added 4 commits June 15, 2024 11:34

Merge branch 'main' into inspect-ismethoddescriptor-fix

7fdc95a

Clean up/improve/refactor tests

52f579f

Refactor

57875a7

Doc: add versionchanged...

01178fa

zuo requested review from ncoghlan and serhiy-storchaka June 15, 2024 10:17

Test: added more method/function cases (requested by @eendebakpt)

71c3735

zuo requested a review from picnixz June 15, 2024 10:22

Test: added one more method/function case...

2daa16f

picnixz reviewed Jun 15, 2024

View reviewed changes

Update Doc/library/inspect.rst

0c7bb3f

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

zuo requested a review from picnixz June 15, 2024 17:02

zuo added 2 commits June 16, 2024 14:57

A couple of renames

84aed74

Improve the versionchanged description

0390f85

ncoghlan reviewed Jun 16, 2024

View reviewed changes

Doc/library/inspect.rst Outdated Show resolved Hide resolved

ncoghlan added needs backport to 3.13 bugs and security fixes docs Documentation in the Doc dir labels Jun 16, 2024

zuo and others added 2 commits June 17, 2024 01:41

Update Doc/library/inspect.rst

bcfeeb6

Co-authored-by: Alyssa Coghlan <[email protected]>

minor reST syntax fix

eb4d852

zuo requested a review from ncoghlan June 16, 2024 23:47

serhiy-storchaka approved these changes Jun 17, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Jun 17, 2024

picnixz approved these changes Jun 17, 2024

View reviewed changes

Merge branch 'main' into inspect-ismethoddescriptor-fix

8ccb761

ncoghlan approved these changes Jun 18, 2024

View reviewed changes

ncoghlan enabled auto-merge (squash) June 18, 2024 11:59

ncoghlan merged commit dacc5ac into python:main Jun 18, 2024
33 checks passed

bedevere-app bot removed the awaiting merge label Jun 18, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zuo commented Jun 12, 2024 •

edited

Loading

zuo commented Jun 12, 2024 •

edited

Loading

eendebakpt Jun 12, 2024

zuo Jun 12, 2024 •

edited

Loading

eendebakpt Jun 13, 2024

zuo Jun 15, 2024

zuo Jun 15, 2024

picnixz Jun 15, 2024

zuo Jun 15, 2024

picnixz Jun 15, 2024

eendebakpt left a comment

eendebakpt Jun 13, 2024

zuo Jun 15, 2024 •

edited

Loading

ncoghlan Jun 15, 2024

ncoghlan Jun 15, 2024 •

edited

Loading

zuo Jun 15, 2024

serhiy-storchaka Jun 15, 2024

zuo Jun 15, 2024

picnixz Jun 15, 2024

zuo Jun 15, 2024 •

edited

Loading

zuo Jun 15, 2024

zuo Jun 16, 2024

ncoghlan Jun 16, 2024

zuo Jun 16, 2024

picnixz Jun 17, 2024

serhiy-storchaka left a comment

ncoghlan commented Jun 18, 2024 •

edited

Loading

miss-islington-app bot commented Jun 18, 2024

bedevere-app bot commented Jun 18, 2024

		@@ -0,0 +1,2 @@
		Correct ``inspect.ismethoddescriptor`` to check also for the lack of

	Correct ``inspect.ismethoddescriptor`` to check also for the lack of
	Correct :meth:`inspect.ismethoddescriptor` to check also for the lack of

-        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))

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

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

Conversation

zuo commented Jun 12, 2024 • edited Loading

zuo commented Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

zuo Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eendebakpt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuo Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuo Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

ncoghlan commented Jun 18, 2024 • edited Loading

miss-islington-app bot commented Jun 18, 2024

bedevere-app bot commented Jun 18, 2024

zuo commented Jun 12, 2024 •

edited

Loading

zuo commented Jun 12, 2024 •

edited

Loading

zuo Jun 12, 2024 •

edited

Loading

zuo Jun 15, 2024 •

edited

Loading

ncoghlan Jun 15, 2024 •

edited

Loading

zuo Jun 15, 2024 •

edited

Loading

ncoghlan commented Jun 18, 2024 •

edited

Loading