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

sterliakov · 2024-11-14T02:08:36Z

mypy/message_registry.py

@@ -80,6 +80,8 @@ def with_additional_msg(self, info: str) -> ErrorMessage:
 ASYNC_FOR_OUTSIDE_COROUTINE: Final = '"async for" outside async function'
 ASYNC_WITH_OUTSIDE_COROUTINE: Final = '"async with" outside async function'

+AWAIT_WITH_OUTSIDE_COROUTINE: Final = '"await" outside coroutine ("async def")'


I'd prefer this message to be consistent with previous two: "await" outside async function, but probably that isn't worth changing an existing message.

Yes, I moved it here and kept the same message that was defined before. Took the liberty to now also provide another key in here for the hardcoded message for "await outside functions" that was as well present as a string when we visit Await Expressions specifically.

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

mypy/message_registry.py

@@ -80,6 +80,8 @@ def with_additional_msg(self, info: str) -> ErrorMessage:
 ASYNC_FOR_OUTSIDE_COROUTINE: Final = '"async for" outside async function'
 ASYNC_WITH_OUTSIDE_COROUTINE: Final = '"async with" outside async function'

+AWAIT_WITH_OUTSIDE_COROUTINE: Final = '"await" outside coroutine ("async def")'


Did you really intend to call it AWAIT_WITH_OUTSIDE_COROUTINE? This looks like a copying issue to me, why WITH? And please remove the blank line before this defn, those three messages are related to each other

indeed was a yanking error. thank you for pointing that out.

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

github-actions · 2024-11-14T12:29:25Z

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

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.

sterliakov · 2024-11-14T21:00:53Z

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


 def bad() -> None:
    # These are always critical / syntax issues:
    y = [await foo(x) for x in [1, 2, 3]]  # E: "await" outside coroutine ("async def")  [await-not-async]
 async def good() -> None:
    y = [await foo(x) for x in [1, 2, 3]]  # OK
+


This is an artefact of --update-data - please remove

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

sterliakov reviewed Nov 14, 2024

View reviewed changes

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

josetapadas Nov 14, 2024

sterliakov Nov 14, 2024

josetapadas Nov 14, 2024

sterliakov Nov 14, 2024

sterliakov Nov 14, 2024

josetapadas Nov 14, 2024

sterliakov Nov 14, 2024

josetapadas commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

sterliakov Nov 14, 2024

sterliakov Nov 14, 2024

sterliakov Nov 14, 2024 •

edited

Loading

sterliakov Nov 14, 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.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josetapadas commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sterliakov Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sterliakov Nov 14, 2024 •

edited

Loading