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

[carltongibson]

trafficstars

This is ref the discussion on https://github.com/python/cpython/issues/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? 🤔

https://github.com/python/cpython/blob/c43714fbcdb9bb0927872d6ebf5697edd2e2a1e9/Lib/inspect.py#L376-L385

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 #94912 OK?

• Issue: gh-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[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:

• https://github.com/python/cpython/pull/99247/commits/ab422f7ec02a55c93b620670c415e76279247192 uses the __code__.replace() technique, and looks good. (I need to move this to a decorator — I'll work on this next.)
• https://github.com/python/cpython/pull/99247/commits/8c6429ec008521e9e8cb8110c8221e308442835d 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. 🤔

[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 https://github.com/python/cpython/pull/99247#discussion_r1024993071 — will experiment.)

Thanks!

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

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

🤔

[gvanrossum]

I think it's better not to add a new function, it would require lots of places to be updated to call the new function. Setting the marker is meant to avoid that need. But I'm okay with a slight doc update. I'll hold off approving (although it LGTM) for now.

[carltongibson]

Hi all. Thank you for your patience. I've adjusted the docs. Any input on phrasing welcome.

[bedevere-bot]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[carltongibson]

I have made the requested changes; please review again

(PS. Don't force-push, that makes the review process harder.)

Sorry, yes. It was an instant !fixup on the tip commit, so I hoped to correct that to reduce noise. I'll not do so next time. Thanks.

[bedevere-bot]

Thanks for making the requested changes!

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

[carltongibson]

I think the test failing is unrelated:

File "D:\a\cpython\cpython\Lib\test\support\__init__.py", line 214, in _force_run
[777](https://github.com/python/cpython/actions/runs/3581945841/jobs/6025596943#step:6:778)
    return func(*args)
[778](https://github.com/python/cpython/actions/runs/3581945841/jobs/6025596943#step:6:779)
           ^^^^^^^^^^^
[779](https://github.com/python/cpython/actions/runs/3581945841/jobs/6025596943#step:6:780)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\a\\cpython\\cpython\\build\\test_python_6040�\\test_python_worker_1444�'

[bedevere-bot]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[gvanrossum]

Agreed with Kumar, this needs a little "what's new in 3.12" bit.

Note: in the past we didn't require that, some release manager would write those that based on the change logs. But it's easier on the release manager if individual contributors write their own "what's new" bits. Let us know if you need help with this.

[TeamSpen210]

Is it safe to set CO_COROUTINE on a code object which isn't actually compiled as async def? That would cause even the interpreter loop to think that the function actually is a coroutine, and do things like raise unawaited coroutine warnings if the function was a generator, for example. There's other references in C code too.

[carltongibson]

this needs a little "what's new in 3.12" bit.

No problem. I'll add that.

... do things like raise unawaited coroutine warnings...

I think that's desired behaviour 🤔 If I mark a sync function with the new decorator, it would be a programming error to not await its return value, so reporting that would be desired.

Not sure what to make of if the function was a generator — I can't immediately see the problem case that you'd pass to the new decorator. I guess it's possible to add guards if there are likely ones? 🤔

[gvanrossum]

Whoa. I must've not paid attention when I approved this. :-(

I don't believe this should be implemented by setting the CO_COROUTINE flag in code.co_flags. That flag has meaning for the interpreter and I don't think the bytecode is a regular function is compatible with that flag.

I had understood when we first decided to do this that the mark decorator would set a simple flag attribute on the function object which iscoroutinefunction would then check. This is how asyncio.iscoroutinefunction works.

Am I missing something?

[carltongibson]

Ok, so I should rewrite it setting an attribute rather than using the code flags? (Can do)

[gvanrossum]

Ok, so I should rewrite it setting an attribute rather than using the code flags? (Can do)

Yeah, since you don't seem to recall a discussion leading to the conclusion that we could use the code flags, I think that would be safer.

[carltongibson]

I have made the requested changes; please review again

• Reworked to use a _is_coroutine marker.
• Added "What's New…" entry.

[bedevere-bot]

Thanks for making the requested changes!

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

[netlify[bot]]

✅ Deploy Preview for python-cpython-preview ready!

Name	Link
🔨 Latest commit	5ffba320051f474cad2796621bc648e0e66890dc
🔍 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 https://github.com/python/cpython/pull/99247#issuecomment-1325971890 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]

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