-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fine-grained: Support async def/for/with and await #4969
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
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.
Thanks! I left few comments.
echk = self.expr_checker | ||
iterable = echk.accept(expr) | ||
method = echk.analyze_external_member_access('__iter__', iterable, expr) | ||
iterator = echk.check_call(method, [], [], expr)[0] | ||
|
||
if isinstance(iterable, TupleType): | ||
joined = UninhabitedType() # type: Type | ||
for item in iterable.items: | ||
joined = join_types(joined, item) |
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.
Ideally, this would be already available in the tuple fallback instance, but it doesn't work yet because of problems with forward refs (I will fix this later).
main:20: error: "Iterable[int]" has no attribute "__aiter__"; maybe "__iter__"? | ||
main:21: error: Iterable expected | ||
main:21: error: "asyncify[int]" has no attribute "__iter__"; maybe "__aiter__"? | ||
main:18: error: "Iterable[int]" has no attribute "__aiter__"; maybe "__iter__"? (not async iterable) |
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.
If it is easy to change, then I would skip alternative name suggestion for these particular names. If it is not easy then ignore this comment.
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.
Will tweak this a a bit. The __aiter__
suggestion seems potentially useful though.
test-data/unit/fine-grained.test
Outdated
@@ -5800,3 +5800,123 @@ class M(type): | |||
[out] | |||
== | |||
a.py:2: error: Argument 1 to "f" of "M" has incompatible type "int"; expected "str" | |||
|
|||
[case testAwaitAndAsyncDef-skip-cache] |
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.
Why skip cache here and in some tests below?
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.
To avoid running the test cases twice -- the second run doesn't seem very useful.
main:5: error: Incompatible return value type (got "str", expected "int") | ||
== | ||
== | ||
main:4: error: "C" has no attribute "__aexit__" |
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 few tests where iterators and context managers change kind (i.e. from normal to async and back).
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 wrote these tests and they passed but they don't seem to increase test coverage in a meaningful way so I'd like to leave them out. (Not sure if you've noticed but I'm getting tired of the slow local test runs... I suspect that the tests are over 2x slower than not too long ago.)
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 didn't notice they are so much slower. Last run took 155 sec. on my laptop, which is not much. Maybe we can think about making tests more lightweight, thinking about which tests to include may impose more mental burden than actually waiting, but this may be subjective.
from a import C | ||
|
||
async def f() -> int: | ||
async with C() as x: |
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 a test without assignment target (x
in this case).
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.
Will add to this same test.
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.
Note there are merge conflicts now, otherwise looks good.
Some of the changes motivated cleaning up some duplicate error
messages.
Also noticed missing
__next__
/next
dependency in ordinary forstatements and fixed it as well.
Work towards #4951.