-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Implement async generators (PEP 525) #2711
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
Conversation
|
It's a |
IOW it's OK to have |
Thanks for clarifying the yield/finally issue! An earlier version of my patch did have an internal error when typing.AsyncGenerator didn't exist, but that is fixed now (I amended my message above yesterday to reflect that). People using 3.6.0 will just have to guard their In other words, both of the issues I flagged should be resolved. |
@JelleZijlstra What is the status of this PR? I am asking just to see whether it would be possible to have full Python 3.6 support in 0.480 (that should be released in around one week if I understand correctly the current schedule). |
@ilevkivskyi I think the code is ready, but it hasn't been reviewed yet. Also, there is a merge conflict now—I'll fix that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good, here are some minor comments.
def get_generator_yield_type(self, return_type: Type, is_coroutine: bool) -> Type: | ||
"""Given the declared return type of a generator (t), return the type it yields (ty).""" | ||
if isinstance(return_type, AnyType): | ||
return AnyType() | ||
elif not self.is_generator_return_type(return_type, is_coroutine): | ||
elif (not self.is_generator_return_type(return_type, is_coroutine) | ||
and not self.is_async_generator_return_type(return_type)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment there, but you might update the comment after elif return_type.args:
below (add AsyncGenerator
).
mypy/checker.py
Outdated
@@ -1705,7 +1733,7 @@ def check_return_stmt(self, s: ReturnStmt) -> None: | |||
self.warn(messages.RETURN_ANY.format(return_type), s) | |||
return | |||
|
|||
if self.is_unusable_type(return_type): | |||
if self.is_unusable_type(return_type) or defn.is_async_generator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a separate (more clear) error message for async generators, like: "return statement with value in async generator is prohibited". Maybe it could be even blocking (since formally speaking this is a SyntaxError
).
Also, I would probably move this check to semanal.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting this in semanal.py would be hard because we only discover that the function is a generator during semantic analysis. I suppose we could do it in ThirdPass, but there are some code comments suggesting that ThirdPass may be removed in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But I'll change the error message.)
|
||
async def gen() -> AsyncGenerator[int, None]: | ||
yield 1 | ||
return 42 # E: No return value expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add more tests here, both return None
and return f()
(for f
that returns None
) should fail (both with and without --strict-optional
), since these are syntax errors.
-- --------------------------------------------------------------------- | ||
|
||
[case testAsyncGenerator] | ||
# flags: --fast-parser --python-version 3.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--fast-parser
flag is not necessary anymore.
self.fail("'yield' in async function", expr, True, blocker=True) | ||
else: | ||
self.function_stack[-1].is_generator = True | ||
self.function_stack[-1].is_async_generator = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not important, but it looks a bit strange that you need to set both flags here.
async def f() -> AsyncGenerator[int, None]: | ||
pass | ||
|
||
async def gen() -> AsyncGenerator[int, None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add some tests with AsyncIterator[int]
?
|
||
async def genfunc() -> AsyncGenerator[int, None]: | ||
yield 1 | ||
yield 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also add a test where you yield
a wrong type?
Thanks for the comments! I will have to rebase the PR because it now relies on an obsolete version of typed_ast. |
…nc_generator_return_type
Probably not worth it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- only one idea for an additional test case.
@@ -34,6 +34,9 @@ | |||
INVALID_EXCEPTION_TYPE = 'Exception type must be derived from BaseException' | |||
INVALID_RETURN_TYPE_FOR_GENERATOR = \ | |||
'The return type of a generator function should be "Generator" or one of its supertypes' | |||
INVALID_RETURN_TYPE_FOR_ASYNC_GENERATOR = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be a test for this error message. Can you add one?
🎉 |
This turned out to be pretty straightforward.
Two issues that may need more discussion:
typing.AsyncGenerator
; it was added in add AsyncGenerator typing#346 and should be in 3.6.1. But this patch assumes that typing.AsyncGenerator exists, so if you run mypy in 3.6.0 on a file with an async generator, it says that typing.AsyncGenerator doesn't exist. I originally put AsyncGenerator in mypy_extensions to work around this, but maybe we need some other solution.This closes #2616