Implement 'async def' and friends ('await', 'async for', 'async with')#1808
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! |
| 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.
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.
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.
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.
I thought they were both None?
There was a problem hiding this comment.
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.
Do you need this check? Doesn't an Awaitable always have a type parameter?
There was a problem hiding this comment.
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.
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.
Do you still use this now that you have get_awaitable_return_type?
There was a problem hiding this comment.
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.
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.
Can we not make this change as part of this diff?
|
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 beT` in both cases.