gh-142418: Fix inspect.iscoroutinefunction for marked functools.partial and partialmethod objects #142503
Conversation
….partial and partialmethod objects
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Corrected the NEWS entry
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Updated to follow format
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
…ne_mark Partial and method objects cannot form reference cycles in this context, so the cycle-detection logic in _has_coroutine_mark introduced unnecessary overhead. This change removes the visited-set check and keeps the function as a simple unwrapping loop while still correctly detecting explicitly marked coroutine wrappers.
… cycle-detection logic
| # Functions created by partialmethod descriptors keep a __partialmethod__ reference | ||
| pm = getattr(f, "__partialmethod__", None) | ||
| if isinstance(pm, functools.partialmethod): | ||
| f = pm | ||
| continue |
There was a problem hiding this comment.
Well, this can also be moved forward by one block to avoid the time spent on obtaining the attribute when it is not necessary. I hope I have not bored you with these micro-optimizations.
| continue | ||
|
|
||
| # partial and partialmethod share .func | ||
| if isinstance(f, (functools.partial, functools.partialmethod)): |
There was a problem hiding this comment.
I will also add, "for the record", why I do not handle partialmethod objects in this way in the code attached to the original issue. They are not callable, and therefore applying markcoroutinefunction() to them is incorrect, which means they should not be checked. Being defined as a class member, accessing the corresponding attribute will return a regular function object created by partialmethod (or a method object for such a function, if via an instance). Therefore, there is no point in unnecessary iteration, and you can go straight to pm.func (see the block above).
|
|
||
| Interactive mode will start even when :data:`sys.stdin` does not appear to be a terminal. The | ||
| :envvar:`PYTHONSTARTUP` file is not read. | ||
| In these "execute then interact" cases, Python runs the script or command |
There was a problem hiding this comment.
This seems unrelated changes
| f = f.__func__ | ||
| f = functools._unwrap_partial(f) | ||
| return getattr(f, "_is_coroutine_marker", None) is _is_coroutine_mark | ||
| while True: |
There was a problem hiding this comment.
Please:
- Wrap all lines under 80 characters.
- Remove "obvious" comments. "Methods: unwrap first" is clear from the way you're doing it.
- Avoid blank lines. The standard library usually tries to avoid expanding the code vertically.
| return getattr(f, "_is_coroutine_marker", None) is _is_coroutine_mark | ||
| while True: | ||
| # Methods: unwrap first (methods cannot be coroutine-marked) | ||
| if ismethod(f): |
There was a problem hiding this comment.
Why not doing while ismethod(f): f = f.__func__ here?
There was a problem hiding this comment.
>>> from functools import partialmethod
>>> class MyClass:
... def a(self): ...
... b = partialmethod(a)
>>> obj = MyClass()
>>> obj.b
functools.partial(<bound method MyClass.a of <__main__.MyClass object at 0x7fd343f4cc20>>)
>>> obj.b.func.__func__
<function MyClass.a at 0x7fd343f42cf0>When unwrapping the reference to obj.b, the functools.partial object comes before the method object, so the assumption that method objects always precede other objects is incorrect.
>>> from functools import partialmethod
>>> class MyFirstClass:
... def f(self, other): ...
>>> first = MyFirstClass()
>>> class MySecondClass:
... g = partialmethod(first.f)
>>> second = MySecondClass()
>>> second.g
<bound method partialmethod._make_unbound_method.<locals>._method of <__main__.MySecondClass object at 0x7fd343f4d400>>
>>> second.g.__func__.__partialmethod__.func
<bound method MyFirstClass.f of <__main__.MyFirstClass object at 0x7fd343f4d2b0>>Well, it seems to me that the current implementation (in the main branch) is not very well thought out. Personally, I would not rely on it.
There was a problem hiding this comment.
If your comment is only about why there is if instead of while... why do we need a nested loop when we already have an outer one? Moreover, how often are methods that refer to other methods used? Does it even make sense to try to optimize this case? I think it is sufficient and simpler to rely on the outer loop.
As you can easily see, the code in this PR is very similar to the one I attached to the original issue, except for the order of the blocks and some points borrowed from the original code (AI-generated or just copy-paste?). I use something similar in the implementation of similar functions in my library, although they are more general in nature.
| # Functions created by partialmethod descriptors keep a __partialmethod__ reference | ||
| pm = getattr(f, "__partialmethod__", None) | ||
| if isinstance(pm, functools.partialmethod): | ||
| f = pm | ||
| continue |
There was a problem hiding this comment.
I think you should use functools._unwrap_partialmethod which handles both partial methods and partial functions (first it unwraps partial methods then unwraps partial functions)
There was a problem hiding this comment.
Please read the discussion at #142505.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
4 participants
Fixes gh-142418.
This PR updates
inspect.iscoroutinefunction()so that coroutine markersapplied via
inspect.markcoroutinefunction()are correctly detected onwrapped callables, including
functools.partialandfunctools.partialmethodobjects.Previously,
iscoroutinefunction()only checked the marker after unwrappingthe callable, which caused false negatives for marked
partialandpartialmethodobjects. The new logic checks for the marker at each unwrapstage, with cycle protection, ensuring that any explicitly marked wrapper is
recognized as a coroutine function.
This change also adds regression tests verifying correct behavior for marked
and unmarked
functools.partialandfunctools.partialmethodobjects, andincludes a NEWS entry documenting the fix.
📚 Documentation preview 📚: https://cpython-previews--142503.org.readthedocs.build/