-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Implement 'async def' and friends ('await', 'async for', 'async with') #1808
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
I suspect that the reason is the If the type context is |
952551d
to
0e6634d
Compare
Ensure 'async def' is a syntax error in Python 2 (at least with the fast parser).
Taking a look at this now! |
@@ -762,6 +765,39 @@ def visit_ExtSlice(self, n: ast35.ExtSlice) -> Node: | |||
def visit_Index(self, n: ast35.Index) -> Node: | |||
return self.visit(n.value) | |||
|
|||
# PEP 492 nodes: 'async def', 'await', 'async for', 'async with'. |
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 a nit, but it'd be better if these were left in their original locations rather than separated out. Currently, the visit_*
functions in this file are in the same order as the nodes in the Python.asdl
, which is a nice property to maintain. Also, most of them are virtually identical to the non-async versions -- having them right next to each other will make it more likely they stay in sync.
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.
is_generator_return_type() now takes an extra is_coroutine flag.
self.fail("'yield' in async function", expr, True, blocker=True) | ||
else: | ||
self.function_stack[-1].is_generator = True | ||
if expr.expr: |
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.
Await is required to have an expr, right?
This revealed a spurious error "Function does not return a value", fixed that.
# | ||
# A classic generator must define a return type that's either | ||
# `Generator[ty, ts, tr]`, Iterator[ty], or Iterable[ty] (or | ||
# object or Any). If ts/tr are not given, both are Any. |
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 thought they were both 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.
Actually in the code below ts is Void, tr is Any. But I wonder if it isn't better to make both None -- forcing developers to declare the return type as Generator[ty, ts, tr]
if they want to either receive or return values. There's a unit test that currently checks that return value
is allowed in a generator declared as Iterator
but I think I'll just change that too.
elif return_type.type.fullname() == 'typing.Awaitable': | ||
# Awaitable is the other type which specifies the type of values it returns into | ||
# `yield from` expressions (using `return`). | ||
if len(return_type.args) == 1: |
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.
Do you need this check? Doesn't an Awaitable always have a type parameter?
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.
IIUC when you use a generic class without parameters args is empty. I also worry about crashes when someone writes e.g. Generator[int]
-- it may well produce an error message but also an Instance with len(args) == 1
, so I'm doing this defensively.
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.
That seems reasonable. We should probably consider filling in the parameters at some earlier stage so we can later rely on the length...
… regular. Also Use get_generator_return_type() instead of manually unpacking the value.
Pushed responses to most of the review. I actually still haven't figured out how to extract the type parameter from a subclass of Awaitable[T](if it's even possible). I also found that the error message you get when the return type for |
@ddfisher that's all I planned to do in this PR -- another review please? |
return return_type.args[2] | ||
else: | ||
return AnyType() | ||
elif return_type.type.fullname() == 'typing.Awaitable': |
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.
Do you still use this now that you have get_awaitable_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.
get_awaitable_return_type()
is for getting the type of the expression in await
, async for
and async def
. get_generator_return_type()
is for the return
statement inside a generator or async def.
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.
Ah, I see. get_awaitable_return_type
shouldn't have a parallel name, then. It'd also be nice to mention that in the docstrings -- those are also virtually identical at the moment.
The Awaitable parts of |
else: | ||
# `return_type` is supertype of Generator, so callers won't be able to see the return | ||
# type when used in a `yield from` expression. | ||
return Void() |
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.
Can we not make this change as part of this diff?
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.
OK.
Finished with review! |
Just one remaining nit around the naming/documentation of |
OK, done.
|
Thanks! Now I'll start with the follow-on PR...
|
This is not ready for merge, but I have a question. [UPDATE: it is ready!]
@JukkaL Why would these two functions not be equivalent?
In f(), the revealed type is
Any
, while in g() it isT
(spelled as'T
-1'). I would expect it to be
T` in both cases.