Skip to content

semanal: Check that async for/with is inside an async function #12418

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

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

shoracek
Copy link
Contributor

Description

Added a check that async for/with is in an async function.
The code and error message are similar to those checking await.

Fixes #10594

Test Plan

I used an expanded example from the issue as a unit test. The example was extended to also cover for loop, with expression and usage outside of a function.

Also fixed another test case using async with expression outside of an async function.

@jhance
Copy link
Collaborator

jhance commented Mar 21, 2022

We should probably have an error code. Also, the format string for the message should be in the message registry.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

This can probably just go into the misc code. It's a syntax error at runtime, so I don't see much reason for users to want to skip this error.

@jhance
Copy link
Collaborator

jhance commented Mar 21, 2022

Is there a reason the ast module itself doesn't raise for syntax?

@JelleZijlstra
Copy link
Member

Some SyntaxErrors are raised after parsing in the Python compiler, e.g. invalid nonlocal declarations, some things with async. So mypy has to replicate such checks in semanal.

@shoracek
Copy link
Contributor Author

shoracek commented Mar 22, 2022

I have kept the error code as misc, made the messages use terminology that the Python's syntax error uses, and added the message to the registry. Although, now that I am looking closer at the possible errors, there's a syntax code, so it probably could have been better to use that.

@github-actions

This comment has been minimized.

@jhance
Copy link
Collaborator

jhance commented Mar 22, 2022

Yeah, syntax seems better if it already exists.

@shoracek
Copy link
Contributor Author

Used syntax as the error code and also set it to be a blocking error, since Python will not run code with this error.

mypy/semanal.py Outdated
@@ -4173,6 +4183,11 @@ def visit_dictionary_comprehension(self, expr: DictionaryComprehension) -> None:
self.analyze_comp_for_2(expr)

def visit_generator_expr(self, expr: GeneratorExpr) -> None:
if True in expr.is_async:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An async genexp is actually legal in a non-async function: def f(): return (x async for x in y). Could you add a test for that?

@JelleZijlstra
Copy link
Member

Not sure we should use a blocking error. There's a risk that this check has a bug or that a future Python version changes the rules, and in that case it would be useful for users to be able to skip this error. I don't think we should be using blocking errors for anything we can recover from.

@github-actions

This comment has been minimized.

@shoracek
Copy link
Contributor Author

As suggested, I've switched it back to non-blocking error, removed the error for generator expression, and added the remaining expressions into the test.

@github-actions

This comment has been minimized.

@jhance
Copy link
Collaborator

jhance commented Mar 23, 2022

It looks good. Can we change True in ... to any(genr.is_async) for all of the cases where we have something like that? Afterwards I think we can merge this. IMO this is less odd and easy to read.

@shoracek
Copy link
Contributor Author

shoracek commented Mar 23, 2022

Done.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@jhance jhance merged commit fa9921a into python:master Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async for/async with are allowed in non-async functions
3 participants