Make coroutine function return type more specific #5052
Conversation
Previously, the return type was `Awaitable`, which is correct, but not specific enough for some use cases. For example, if you have a function parameter that should be a coroutine object (but not a `Future`, `Task` or other awaitable), then `mypy` is unable to detect incorrect invocations of the function. This change is (deliberately) imcomplete as is. It seems like this breaks quite a few tests in `mypy`. The first symptom is that coroutine objects are now (incorrectly) detected as generators..
This change removes the undesired "return type of a generator function should be `Generator` or one of its subtypes for coroutine function definitions. However, it introduces a new error where the return type of coroutine functions is expected to be `Coroutine[T, Any, Any]` instead of the desired `T`. It looks like we were hijacking a generator-specific path for checking the return type of coroutine functinos. I added an explicit path for coroutine functions, allowing our test to pass. However, lots of tests now fail. A few of them were simply places that were incidentally relying on coroutine functions to have type `Awaitable`. I fix them. The remaining failures all seem to be about coroutine functions with return type `None` without an explicit return statement. Seems like this is also something for which we were relying on implicit classification as generators.
Most of the tests are fixed, but two tests still fail. One about not detecting invalid `yield from` on `AwaitableGenerator`. The other about types being erased in call to `asyncio.gather()`.
Changing the return type of coroutine functions to `Coroutine` introduced a regression in the expression checks.
AndreLouisCaron
changed the title
This fixes the type inference logic that was causing the last failing test to fail. Build should now be green :-)
mypy/semanal.py
Outdated
| # has external return type `Awaitable[T]`. | ||
| ret_type = self.named_type_or_none('typing.Awaitable', [defn.type.ret_type]) | ||
| assert ret_type is not None, "Internal error: typing.Awaitable not found" | ||
| # has external return type `Coroutine[T, Any, Any]`. |
There was a problem hiding this comment.
Please update this comment to Coroutine[Any, Any, T].
There was a problem hiding this comment.
Fixed in latest commit :-)
mypy/checker.py
Outdated
| @@ -878,7 +890,8 @@ def is_unannotated_any(t: Type) -> bool: | |||
| if is_unannotated_any(ret_type): | |||
| self.fail(messages.RETURN_TYPE_EXPECTED, fdef) | |||
| elif (fdef.is_coroutine and isinstance(ret_type, Instance) and | |||
| is_unannotated_any(ret_type.args[0])): | |||
| is_unannotated_any(ret_type.args[2])): | |||
There was a problem hiding this comment.
I would use self.get_coroutine_return_type here, then the below note is not needed.
There was a problem hiding this comment.
Fixed in latest commit :-)
mypy/checker.py
Outdated
| if isinstance(return_type, AnyType): | ||
| return AnyType(TypeOfAny.from_another_any, source_any=return_type) | ||
| assert isinstance(return_type, Instance), "Should only be called on coroutine functions!" | ||
| # return type is 3rd type specification in Coroutine! |
There was a problem hiding this comment.
I would format this comment as # Note: return type is the 3rd type parameter of Coroutine.
There was a problem hiding this comment.
Fixed in latest commit :-)
mypy/checker.py
Outdated
| def get_coroutine_return_type(self, return_type: Type) -> Type: | ||
| if isinstance(return_type, AnyType): | ||
| return AnyType(TypeOfAny.from_another_any, source_any=return_type) | ||
| assert isinstance(return_type, Instance), "Should only be called on coroutine functions!" |
There was a problem hiding this comment.
We don't use exclamation signs in assertion messages.
There was a problem hiding this comment.
Fixed in latest commit :-)
| if c: | ||
| tr = self.get_coroutine_return_type(t) | ||
| else: | ||
| tr = self.get_generator_return_type(t, c) |
There was a problem hiding this comment.
I like the symmetry between the branches here. 👍
| @@ -398,8 +398,7 @@ def do_func_def(self, n: Union[ast3.FunctionDef, ast3.AsyncFunctionDef], | |||
| self.as_required_block(n.body, n.lineno), | |||
| func_type) | |||
| if is_coroutine: | |||
| # A coroutine is also a generator, mostly for internal reasons. | |||
| func_def.is_generator = func_def.is_coroutine = True | |||
| func_def.is_coroutine = True | |||
There was a problem hiding this comment.
It is good you also cleaned this up.
mypy/semanal.py
Outdated
| assert ret_type is not None, "Internal error: typing.Awaitable not found" | ||
| # has external return type `Coroutine[T, Any, Any]`. | ||
| any_type = AnyType(TypeOfAny.special_form) | ||
| ret_type = self.named_type_or_none('typing.Coroutine', [ |
There was a problem hiding this comment.
I would move the opening bracket to the next line.
|
Thanks @ilevkivskyi for the quick feedback. I pushed a new commit with all the requested changes :-) |
| assert isinstance(return_type, Instance), "Should only be called on coroutine functions." | ||
| # Note: return type is the 3rd type parameter of Coroutine. | ||
| return return_type.args[2] | ||
|
|
||
| def get_generator_return_type(self, return_type: Type, is_coroutine: bool) -> Type: |
There was a problem hiding this comment.
Is the is_coroutine argument here still useful, or can we remove it now?
There was a problem hiding this comment.
I'm pretty sure we still need it to handle asynchronous generators. In that case, they have both is_generator=True and is_coroutine=True.
|
Thank you for fixing such a sophisticated problem! Vive les sprint. :-) |
See the commit messages for details and rationale. I tried to "tell a story" with them :-)
Fixes #3569.
Fixes #4460.
Special thanks to @ilevkivskyi and @gvanrossum for their time and their infinite patience with all my questions :-)