fix: refactor sem analysis of the await-not-async on generators and list comprehension #18152

josetapadas · 2024-11-13T09:59:04Z

This PR proposes the introduction of two extra checks in order to error or any await or async within list comprehensions but not when inside a GeneratorExpression.

To achieve this we propose:

when visiting a ListComprehension we should also check for the existing of an await expression and error outside a courotine
error on the same scenario when visiting a DictionaryComprehension
when visiting a AwaitExpression we add an extra check to verify if it is inside a GeneratorExpression to not fail

…ist comprehension

for more information, see https://pre-commit.ci

mypy/message_registry.py

sterliakov · 2024-11-14T02:12:29Z

mypy/semanal.py

+                    expr,
+                    code=codes.AWAIT_NOT_ASYNC,
+                    serious=True,
+                )

        expr.generator.accept(self)



Shouldn't visit_set_comprehension below use the same logic?

sterliakov · 2024-11-14T02:22:13Z

mypy/semanal.py

@@ -6090,7 +6104,12 @@ def visit_set_comprehension(self, expr: SetComprehension) -> None:
    def visit_dictionary_comprehension(self, expr: DictionaryComprehension) -> None:


And here as well? Probably worth extracting into a helper method

extracted it to a small method and apply the check on both dict and list comprehensions visits

sterliakov · 2024-11-14T02:33:53Z

test-data/unit/check-async-await.test

@@ -1013,13 +1013,17 @@ async def foo(x: int) -> int: ...

 # These are allowed in some cases:
 top_level = await foo(1)  # E: "await" outside function  [top-level-await]
-crasher = [await foo(x) for x in [1, 2, 3]]  # E: "await" outside function  [top-level-await]
+crasher = [await foo(x) for x in [1, 2, 3]]  # E: "await" outside coroutine ("async def")  [await-not-async] \


More explicit tests would be really helpful here. Consider at least checking all of the following in different scopes (module level, non-async function body and async function body at least; the former two should produce the same errors and async function should allow all of those):

(await x for x in xs) # OK [await x for x in xs] # E {await x for x in xs} # E {0: await x for x in xs} # E {await x: 0 for x in xs} # E (x async for x in xs) # OK [x async for x in xs] # E {x async for x in xs} # E {x: 0 async for x in xs} # E (x for x in await xs) # E [x for x in await xs] # E {x for x in await xs} # E {x: 0 for x in await xs} # E

There's already testAsyncForOutsideCoroutine for the 2nd block, the 3rd one is probably not interesting (follows regular rules), but the 1st one isn't exercised yet.

…ehensions

for more information, see https://pre-commit.ci

josetapadas · 2024-11-14T12:09:55Z

Thank you for the review @sterliakov 🙏 I have now pushed the suggested changes

sterliakov · 2024-11-14T20:48:58Z

mypy/semanal.py

@@ -6085,12 +6107,21 @@ def visit_set_comprehension(self, expr: SetComprehension) -> None:
            if not self.is_func_scope() or not self.function_stack[-1].is_coroutine:
                self.fail(message_registry.ASYNC_FOR_OUTSIDE_COROUTINE, expr, code=codes.SYNTAX)


There's now a minor inconsistency - serious=True is passed everywhere except this call.

sterliakov · 2024-11-14T20:57:48Z

mypy/semanal.py

+                self.fail(
+                    message_registry.ASYNC_FOR_OUTSIDE_COROUTINE,
+                    expr,
+                    code=codes.SYNTAX,


I don't really like the use of codes.SYNTAX vs codes.AWAIT_NOT_ASYNC here. Prior to this PR AWAIT_NOT_ASYNC was only used once.

As far as I understand, the purpose of using a special code was to support "specific" environments: e.g. IPython allows top-level awaits, so it should be easy to disable such a check when checking IPython snippets.

I see two possible resolutions here:

Declare that we only type-check true python code. Then codes.AWAIT_NOT_ASYNC can be safely removed altogether and replaced with codes.SYNTAX

Declare that we want to support such environments. Then all these checks become more complicated: when at module scope (note it isn't same as not self.is_func_scope()), produce codes.AWAIT_NOT_ASYNC, otherwise produce codes.SYNTAX. This can also be extracted into a helper like def require_async_scope(self, message). This logic is close to what visit_await_expr was doing before.

I somewhat prefer 2 as that isn't that much work to do, but can live with 1 either.

Disclaimer: please note I'm not a mypy maintainer and just try to help sometimes. You'd better ask for a maintainer's opinion here: @brianschubert do you have any preference?

OTOH 2 can also be misleading and deserves an explicit mention in the docs.

Thank you for the review and explanation, I understand your point so thank you for clarifying it. Will then wait for any guidance on how to proceed on any of these two options.

test-data/unit/check-async-await.test

for more information, see https://pre-commit.ci

github-actions · 2024-11-18T18:09:43Z

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

josetapadas and others added 4 commits November 13, 2024 10:24

fix: refactor sem analysis of the await-not-async on generators and l…

d3b2d9d

…ist comprehension

[pre-commit.ci] auto fixes from pre-commit.com hooks

ed2937a

for more information, see https://pre-commit.ci

chore: added unit test

c97bce7

chore: refactor to remove function for simple check

ada4720

josetapadas force-pushed the 18124-await-not-async-gen-list-fix branch from dacc60f to ada4720 Compare November 13, 2024 10:25

This comment has been minimized.

Sign in to view

josetapadas marked this pull request as ready for review November 13, 2024 10:47

josetapadas mentioned this pull request Nov 13, 2024

await-not-async missed & false alarms with genexp and list comprehensions #18124

Open

fix: fixed the await outside coroutine within list comprehensions error

43fc65a

josetapadas force-pushed the 18124-await-not-async-gen-list-fix branch from 2ffa91e to 43fc65a Compare November 13, 2024 11:18

chore: error type is not syntax but await-not-async

e3004a1

josetapadas force-pushed the 18124-await-not-async-gen-list-fix branch from 63bad66 to e3004a1 Compare November 13, 2024 11:22

[pre-commit.ci] auto fixes from pre-commit.com hooks

e7ef1f6

for more information, see https://pre-commit.ci

This comment has been minimized.

Sign in to view

sterliakov suggested changes Nov 14, 2024

View reviewed changes

josetapadas and others added 2 commits November 14, 2024 12:05

fix: added await-outside-coroutine checks also to list and dict compr…

0c82fb9

…ehensions

[pre-commit.ci] auto fixes from pre-commit.com hooks

8e8298e

for more information, see https://pre-commit.ci

josetapadas requested a review from sterliakov November 14, 2024 12:09

This comment has been minimized.

Sign in to view

sterliakov reviewed Nov 14, 2024

View reviewed changes

josetapadas and others added 3 commits November 18, 2024 17:47

chore: remove artefact from update data

0fee8d6

chore: fail consistently for the new case also on the set comprehension

95c5730

[pre-commit.ci] auto fixes from pre-commit.com hooks

48148ac

for more information, see https://pre-commit.ci

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refactor sem analysis of the await-not-async on generators and list comprehension #18152

fix: refactor sem analysis of the await-not-async on generators and list comprehension #18152

josetapadas commented Nov 13, 2024

This comment has been minimized.

This comment has been minimized.

sterliakov Nov 14, 2024

sterliakov Nov 14, 2024

josetapadas Nov 14, 2024

sterliakov Nov 14, 2024

josetapadas commented Nov 14, 2024

This comment has been minimized.

sterliakov Nov 14, 2024

sterliakov Nov 14, 2024

sterliakov Nov 14, 2024 •

edited

Loading

josetapadas Nov 18, 2024

github-actions bot commented Nov 18, 2024

		@@ -6090,7 +6104,12 @@ def visit_set_comprehension(self, expr: SetComprehension) -> None:
		def visit_dictionary_comprehension(self, expr: DictionaryComprehension) -> None:

		@@ -6085,12 +6107,21 @@ def visit_set_comprehension(self, expr: SetComprehension) -> None:
		if not self.is_func_scope() or not self.function_stack[-1].is_coroutine:
		self.fail(message_registry.ASYNC_FOR_OUTSIDE_COROUTINE, expr, code=codes.SYNTAX)

fix: refactor sem analysis of the await-not-async on generators and list comprehension #18152

Are you sure you want to change the base?

fix: refactor sem analysis of the await-not-async on generators and list comprehension #18152

Conversation

josetapadas commented Nov 13, 2024

This comment has been minimized.

This comment has been minimized.

sterliakov Nov 14, 2024

Choose a reason for hiding this comment

sterliakov Nov 14, 2024

Choose a reason for hiding this comment

josetapadas Nov 14, 2024

Choose a reason for hiding this comment

sterliakov Nov 14, 2024

Choose a reason for hiding this comment

josetapadas commented Nov 14, 2024

This comment has been minimized.

sterliakov Nov 14, 2024

Choose a reason for hiding this comment

sterliakov Nov 14, 2024

Choose a reason for hiding this comment

sterliakov Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

josetapadas Nov 18, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2024

sterliakov Nov 14, 2024 •

edited

Loading