New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make coroutine function return type more specific #5052

Merged
merged 8 commits into from May 17, 2018

Conversation

Projects
None yet
4 participants
@AndreLouisCaron
Contributor

AndreLouisCaron commented May 15, 2018

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 :-)

AndreLouisCaron added some commits May 15, 2018

Change the return type of coroutine functions to coroutine object
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..
Prevent coroutine functions from being classified 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.
Allow implicit return for coroutine functions that return `None`
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()`.
Fix detection of await expression on direct coroutine function call
Changing the return type of coroutine functions to  `Coroutine`
introduced a regression in the expression checks.

@AndreLouisCaron AndreLouisCaron changed the title from Fix coroutine function return type to Make coroutine function return type more specific May 15, 2018

Fix position of return type in `Coroutine`
This fixes the type inference logic that was causing the last
failing test to fail.  Build should now be green :-)
@ilevkivskyi

Thanks for the PR! Mostly looks good, just few comments.

# 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]`.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

Please update this comment to Coroutine[Any, Any, T].

This comment has been minimized.

@AndreLouisCaron

AndreLouisCaron May 15, 2018

Contributor

Fixed in latest commit :-)

@@ -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])):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

I would use self.get_coroutine_return_type here, then the below note is not needed.

This comment has been minimized.

@AndreLouisCaron

AndreLouisCaron May 15, 2018

Contributor

Fixed in latest commit :-)

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!

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

I would format this comment as # Note: return type is the 3rd type parameter of Coroutine.

This comment has been minimized.

@AndreLouisCaron

AndreLouisCaron May 15, 2018

Contributor

Fixed in latest commit :-)

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!"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

We don't use exclamation signs in assertion messages.

This comment has been minimized.

@AndreLouisCaron

AndreLouisCaron May 15, 2018

Contributor

Fixed in latest commit :-)

if c:
tr = self.get_coroutine_return_type(t)
else:
tr = self.get_generator_return_type(t, c)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

It is good you also cleaned this up.

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', [

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 15, 2018

Collaborator

I would move the opening bracket to the next line.

@AndreLouisCaron

This comment has been minimized.

Contributor

AndreLouisCaron commented May 15, 2018

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:

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 16, 2018

Collaborator

Is the is_coroutine argument here still useful, or can we remove it now?

This comment has been minimized.

@AndreLouisCaron

AndreLouisCaron May 16, 2018

Contributor

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.

@ilevkivskyi

OK, thanks, LGTM now!

@ilevkivskyi ilevkivskyi merged commit 6519eb6 into python:master May 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gvanrossum

This comment has been minimized.

Member

gvanrossum commented May 17, 2018

Thank you for fixing such a sophisticated problem! Vive les sprint. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment