[3.11] gh-84753: Make inspect.iscoroutinefunction() work with AsyncMock (GH-94050)

[miss-islington]

The inspect version was not working with unittest.mock.AsyncMock.

The fix introduces special-casing of AsyncMock in
inspect.iscoroutinefunction equivalent to the one
performed in asyncio.iscoroutinefunction.

Co-authored-by: Łukasz Langa lukasz@langa.pl
(cherry picked from commit 4261b6b)

Co-authored-by: Mehdi ABAAKOUK sileht@sileht.net

• Issue: inspect.iscorutinefunction() returns False for unittest.mock.AsyncMock instances #84753

[miss-islington]

Status check is done, and it's a success ✅ .

[miss-islington]

Status check is done, and it's a success ✅ .

[graingert]

Isn't 3.11 and 3.10 in feature freeze? Cc @pablogsal

[pablogsal]

Isn't 3.11 and 3.10 in feature freeze? Cc @pablogsal

This is a bugfix, isn't it? The bug being they is was incorrectly identifying AsyncMock as not a coroutine. If we were raising or doing something different then that would have been an incompatible change or a new feature, but the current behaviour is wrong.

Maybe I am missing something, thought.

[graingert]

I read this as a feature change to allow duck typed Function-like objects to pass iscoroutinefunction/isgeneratorfunction/isasyncgeneratorfunction

[pablogsal]

All the descriptions of the issue and the tests are about AsyncMock and the way the original issue is written and the code is as if the current behaviour is a bug. Are you concerns around that this affects more than just AsyncMock?

[pablogsal]

@ambv Thoughts?

[graingert]

Are you concerns around that this affects more than just AsyncMock?

Yeah this affects both more than AsyncMock and more than iscoroutinefunction

[graingert]

Also the notes in the PR reflect the old approach and not the approach used in the code:

The fix introduces special-casing of AsyncMock in
inspect.iscoroutinefunction equivalent to the one
performed in asyncio.iscoroutinefunction.

[pablogsal]

Hummm, @ambv I think this should then not have been backported. We should revert both backports

[cjw296]

I dunno, this feels like a bug that is being fixed.
@graingert - what situations are you concerned about where this wouldn't be a bug fix?

[ambv]

I agree with @cjw296 that this is a bug fix. Instead of hard-coding AsyncMock as a type in inspect, we use a more generic version, which catches AsyncMock fine. The duck type needs to be very thorough to pass the test. This method was introduced for Cython functions.

I agree that the description of the PR could better indicate what the change is vs what problem it fixes.

[pablogsal]

we use a more generic version, which catches AsyncMock fine. The duck type needs to be very thorough to pass the test. This method was introduced for Cython functions.

Well, but wouldn't this affect other code that currently is behaving in a (arguably incorrect) given way? Technically that prevents the backport unless all possible affected code is surely doing it wrong.

[pablogsal]

I agree that the description of the PR could better indicate what the change is vs what problem it fixes.

At the very least, lest improve the NEWS entry, please.

[cjw296]

Well, but wouldn't this affect other code that currently is behaving in a (arguably incorrect) given way?

If it's behaving in an incorrect way before this fix, isn't that just another indication this is a bugfix?

[pablogsal]

If it's behaving in an incorrect way before this fix, isn't that just another indication this is a bugfix?

Yes, as long as every affected case is behaving incorrectly.