gh-94912: Added marker for non-standard coroutine function detection

[carltongibson]

This is ref the discussion on #94912 to add an official way of marking callables as coroutine functions where they would not otherwise be detected.

cc. @gvanrossum @andrewgodwin I've just had time this afternoon to block off some first tests, and docs. I wanted to break ground so we keep it moving. It's very consciously drafts: please make suggestions of how you think it should go.

@gvanrossum Can I ask for a pointer on how I'm meant to set co_flags for the _has_code_flag() check? 🤔

cpython/Lib/inspect.py

Lines 376 to 385 in c43714f

	def _has_code_flag(f, flag):
	"""Return true if ``f`` is a function (or a method or functools.partial
	wrapper wrapping a function) whose code object has the given ``flag``
	set in its flags."""
	while ismethod(f):
	f = f.__func__
	f = functools._unwrap_partial(f)
	if not (isfunction(f) or _signature_is_functionlike(f)):
	return False
	return bool(f.__code__.co_flags & flag)

Thanks!

First PR on CPython, so likely it's wrong 😊

• Clone and Build as per https://devguide.python.org.
• Run new just tests with % ./python.exe -m unittest test.test_inspect.TestPredicates
• Do we need to create a new ticket, or is make asyncio.iscoroutinefunction a deprecated alias of inspect.iscoroutinefunction and remove asyncio.coroutines._is_coroutine #94912 OK?

• Issue: make asyncio.iscoroutinefunction a deprecated alias of inspect.iscoroutinefunction and remove asyncio.coroutines._is_coroutine #94912

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[carltongibson]

CLA bot is giving an Heroku application error. (Seems to have worked anyway 🕺)

I can look into the News item.

[sobolevn]

@carltongibson happy to see you here! Feel free to ask any questions you have: I am happy to help with what I can :)

1. The simplest way of adding news is by using https://blurb-it.herokuapp.com/
2. Running tests is better with ./python.exe -m test -v test_inspect

co_flags for the _has_code_flag() check?

Python API has __code__.replace:

>>> def some(): ...
... 
>>> some.__code__.replace.__doc__
'Return a copy of the code object with new values for the specified fields.'
>>> some.__code__.replace.__text_signature__
'($self, /, *, co_argcount=-1, co_posonlyargcount=-1,\n        co_kwonlyargcount=-1, co_nlocals=-1, co_stacksize=-1,\n        co_flags=-1, co_firstlineno=-1, co_code=None, co_consts=None,\n        co_names=None, co_varnames=None, co_freevars=None,\n        co_cellvars=None, co_filename=None, co_name=None,\n        co_linetable=None)'

But, I don't know anything about its performance, sorry.

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[carltongibson]

Hi @sobolevn — thanks for your pointer very helpful.

I've pushed two commits, making the tests pass:

• ab422f7 uses the __code__.replace() technique, and looks good. (I need to move this to a decorator — I'll work on this next.)
• 8c6429e adjusts the iscoroutinefunction() function to check the extra case of a class with an async def __call__(). I went this way because I wasn't able to get the __code__ approach working. (AttributeError: type object 'Cl' has no attribute '__code__'. ...)

I wanted to push that to see if I could get feedback. Thanks.

I don't know anything about its performance

Certainly in our usage, this marker would be applied once at startup I think. If one were doing this in a hot-loop, I'd think the just use async def response would become dominant. 🤔

[sobolevn]

In the future don't forget to test objects with .__func__ set, because of how _has_code_flag works :)

[carltongibson]

I haven't followed exactly what you mean here. Sorry. 🤔

[sobolevn]

Oh, sorry. I've meant that objects like staticmethod and classmethod have __func__ property set, where the original function is stored. inspect generally supports this pattern and it should be tested in this case as well :)

[carltongibson]

@sobolevn — OK, let me have a play and see. Thanks!

[carltongibson]

OK, I'm marking Ready for Review, since this would resolve our need from the discussion in #99247 to be able to continue to identify sync functions returning awaitables, without calling them to see that. The behaviour is as I'd expect.

• I'm quite sure that it's not perfect, but I need guidance on how you want it to be. Quite happy to make adjustments.
• There were a couple of comments from @sobolevn that I didn't follow. Hopefully that additional tests clarify what's needed. (Just seen gh-94912: Added marker for non-standard coroutine function detection #99247 (comment) — will experiment.)

Thanks!

[sobolevn]

Sidenote: asyncio.iscoroutinefunction will also be affected by this change.

[gvanrossum]

Let me know when you both feel this is ready to go in.

[carltongibson]

Thanks @gvanrossum.

I'm going to add the extra test cases, and then (from my POV) it's a Any changes? question.

Sidenote: asyncio.iscoroutinefunction will also be affected by this change.

That's being made an alias to the inspect version as part of gh-94912 IIUC — so are additional changes (doc maybe) needed as part of this PR? 🤔

Thanks for the guidance both.

[carltongibson]

Hi @sobolevn and @gvanrossum — thanks for your patience. I've added the additional tests, and adjusted the docs as suggested. I can rebase/squash/etc as you need, but I think it's ready for you to look at. (Happy to adjust as you think!)

For my POV this would allow removal of the asyncio.iscoroutinefunction (and the _is_coroutine marker attribute): we'd introduce a compatibility shim until 3.12 was the lowest supported version, but that's easily enough done.
Thanks again.

//cc @andrewgodwin.

[sobolevn]

Thank you! I hope your first PR to CPython was a pleasant experience 😉

I have just a single main concern: that inspect.iscoroutinefunction no longer does what is documented.

Citing:

Return True if the object is a coroutine function (a function defined with an async def syntax).

Now it can return True for instances with async __call__ methods.
This might backfire for users that trust this check right now.

I suggest to (but, this is just my opinion, I don't know what others think of it):

• markcoroutinefunction is a great thing to have 👍
• But, maybe we can delay making changes to iscoroutinefunction to a separate PR? I see other options like creating new isdefiningcoroutine (or any similar name) function.

Or we can change the docs and list all special cases.

[carltongibson]

Thanks @sobolevn, yes it's been fine. 😊

Good point on the docs. Whichever way we go, a small tweak there. 👀

Some thoughts to your concern:

If I followed correctly, the issue is (only) with the Cl2 example, with the async def __call__.

Given there's no existing test covering this, I think it's similar to how support for partial instances was added here in Python 3.8. It's a case we do want True for, and should add (and doc).

I note the Starlette framework have the same detection for such a class instance in place. (On phone so don't have link) So it's not just Django that's needing this behaviour. Update: here's where they use it, defining an is_async_callable helper, which is a good name for what we're about here. 🤔

The name iscoroutinefunction gets stretched a little... I'm not sure a second function would carry its weight.

🤔

[bedevere-bot]

Thanks for making the requested changes!

@gvanrossum, @kumaraditya303: please review the changes made to this pull request.

[carltongibson]

For the check, we need to check plain functions and the __func__ cases, and the __call__ implementation for class instances. (Tests cover that: given that functions have a __call__ we need to be careful not to just check the one case.)

I looked at various ways of combining this into a single pass, but none that I found were particularly clear. Happy to take a suggestion, but I don't see this as being performance sensitive.

The implementation here is more sophisticated that the older asyncio one…

https://github.com/python/cpython/blob/3.11/Lib/asyncio/coroutines.py#L17-L24

… but (as per the added test cases) various extra (seemingly legitimate) cases are now covered. (So 👍 I guess)

[gvanrossum]

I'm thinking about this, but being distracted by other stuff. I'll try to get to it later this week.

[carltongibson]

Hey @gvanrossum — thanks.

I thought one maybe neater re-write looks like this:

diff --git a/Lib/inspect.py b/Lib/inspect.py
index 883d0b02bf..3b9bd14a38 100644
--- a/Lib/inspect.py
+++ b/Lib/inspect.py
@@ -410,12 +410,13 @@ def iscoroutinefunction(obj):
     Coroutine functions are normally defined with "async def" syntax, but may
     be marked via markcoroutinefunction.
     """
-    func = getattr(obj, "__func__", obj)
-    if getattr(func, "_is_coroutine", None) is _is_coroutine:
-        return True
-
-    if not isclass(obj) and callable(obj) and getattr(obj.__call__, "_is_coroutine", None) is _is_coroutine:
-        return True
+    if not isclass(obj) and callable(obj):
+        # Test both the function and the __call__ implementation for the
+        # _is_coroutine marker.
+        f = getattr(getattr(obj, "__func__", obj), "_is_coroutine", None)
+        c = getattr(obj.__call__, "_is_coroutine", None)
+        if f is _is_coroutine or c is _is_coroutine:
+            return True
 
     return _has_code_flag(obj, CO_COROUTINE) or (
         not isclass(obj) and callable(obj) and _has_code_flag(obj.__call__, CO_COROUTINE)

[gvanrossum]

So perhaps a confusing aspect is that everything that's callable has a __call__ method. Also the presence of __func__ indicates it's a class or instance method.

At least, those things seem to be obfuscating what's going on a bit.

Also, looking at the code for _has_code_flag(), I wonder if we don't need the same logic for checking the _is_coroutine flag.

What would happen if we wrote a helper function like this?

def _has_coroutine_mark(f):
    while ismethod(f):
        f = f.__func__
    f = functools._unwrap_partial(f)
    if not (isfunction(f) or _signature_is_functionlike(f)):
        return False
    return getattr(f, "_is_coroutine", None) is _is_coroutine

(FWIW I wonder if the variable _is_coroutine shouldn't be renamed _is_coroutine_marker.)

Now, I'm not entirely sure why you're testing for __call__, when none of the other is<something>function() predicates test for it. Maybe things would become simpler if we removed that feature?

We could then define our function like this:

def iscoroutinefunction(obj):
    return _has_code_flag(obj, CO_COROUTINE) or _has_coroutine_mark(obj)

[carltongibson]

Now, I'm not entirely sure why you're testing for call

OK, so the issue is we have classes like this:

        class Cl:
            async def __call__(self):
                pass

... where we need instances to be identified as async, and iscoroutinefunction doesn't (currently) pick it up.

Note that this is an actual, already-out-there, need. e.g. the Starlette framework has this exact check to work around this limitation. asgiref works around this limitation in its usage by marking the instance itself with the _is_coroutine marker.

Either way, I think it is desirable to return True for class instance of classes such as Cl with an async def __call__()

So that's the reason for adding the additional ... _has_code_flag(obj.__call__, ...) check.

In this PR, given that we were adding @staticmethod and @classmethod and so on, it seemed that we also should cover this kind of case:

        class Cl2:
            @inspect.markcoroutinefunction
            def __call__(self):
                pass

Which leads to needing similar in the _is_coroutine_marker block.

The latter case is somewhat hypothetical... — it comes from trying to cover all the cases here, rather than from real-use™. To that extent I'd be happy to drop it, but I guess if we do there's an issue in X months time, saying "I need to do this". 🤔

I don't know the correct thing to say. (?)

(FWIW I wonder if the variable _is_coroutine shouldn't be renamed _is_coroutine_marker.)

Yep OK. +1

What would happen if we wrote a helper function like this?

Let me have a read.

Thanks so much for your thought and input on this.

[gvanrossum]

Hum, I still feel that you're sneaking in a new feature under the radar. :-) What this PR is supposed to fix is just to add the ability to mark a sync function as "morally async". IOW the behavior of

class C:
    @inspect.markcoroutinefunction
    def __call__(self): ...
print(inspect.iscoroutinefunction(C())

should be the same as

class C:
    async def __call__(self): ...
print(inspect.iscoroutinefunction(C())

and since the latter prints False, I don't think this PR has any business changing the output to True.

If you think the functionality used by Starlette deserves to be included in inspect, you should probably open a new issue for that first so it can be discussed properly. But I personally think this is overreaching (however, I am repeating myself).

Have you considered adding def _has_coroutine_mark(f): as I suggested?

(Sorry to be such a pain. But once we let this in there's no way back, so I prefer to have the minimal necessary functionality only. Other core devs may differ though.)

news entry added

[netlify]

✅ Deploy Preview for python-cpython-preview ready!

Name	Link
🔨 Latest commit	5ffba32
🔍 Latest deploy log	https://app.netlify.com/sites/python-cpython-preview/deploys/6398396366b7310008247965
😎 Deploy Preview	https://deploy-preview-99247--python-cpython-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[carltongibson]

Hi @gvanrossum — I've updated as per your advice. Please let me know what you think.

Sorry to be such a pain. But once we let this in there's no way back, so I prefer to have the minimal necessary functionality only...

Absolutely, I understand that.

...so it can be discussed properly...

That's slightly frustrating (🙂), as I thought that was exactly the conversation we'd had leading to your #99247 (comment) above:

I think it's better not to add a new function ...

Repass over why I think `__call__` support is needed…

The issue is that inspect.iscoroutinefunction does not capture all the cases it needs to. The asyncio version doesn't either, but this surfaces because the proposal to remove that version makes the deficiency (breakingly) worse.

The two missing cases are:

• Sync functions that return coroutine objects.
• Class instances with async def __call__.

If we proceed here without handling the __call__ case we leave that broken (forcing clients to implement their own different shims for each framework) I can put that in a separate issue or PR or commit but as I'm reading you, you wouldn't accept that anyway?

Beyond those added here, there are no tests for the classes defining async def __call__ methods with iscoroutinefunction. But as a consumer of iscoroutinefunction I (clearly) want True in such cases (and the hand-marked equivalents.) Fixing this seems no different that fixing functools.partial support in Python 3.8.

…however, I am repeating myself…

So, yes, we're going round in circles. 🙂

Ultimately, I have to go with your decision (no problem). If you're not convinced then there's little left to say. Please advise if you think I should re-add __call__ support here, or open a new ticket, or leave it. (If you think leave it, that's totally fine. We can cope.)

Thanks for your patience and guidance on this PR, and all round work beyond that. 🎁

[carltongibson]

@gvanrossum I think this is now as you want it. Please let me know if there's further to do here. Thanks.

[gvanrossum]

Perfect!

[gvanrossum]

Thanks a lot for this improvement. Sorry it took us all a while to settle on the right way to do it.

[carltongibson]

Thanks @gvanrossum. No worries! Better to be sure. Thanks again for your help!