Conversation
This case _is_ an error (async comprehension outside async function), but it's not an async comprehension in a sync comprehension, which is all this rule is supposed to catch. The error message stating that this is allowed after Python 3.11 is particularly nonsensical
|
|
I think the error message is a bit misleading. It suggests that an asynchronous comprehension can be used in a synchronous comprehension from Python 3.11 but that's not actually true. [[x async for x in foo(n)] for n in range(3)]The following raises error for both <3.11 and 3.11+ The user actually need to include it in an async function for it to be considered valid: async def _():
[[x async for x in foo(n)] for n in range(3)]I'm running this via I might be misunderstanding what's being changed here so feel free to correct me. |
|
I see what you mean, but that case will lead to a separate If it needs to be extended, do you have any suggestions? I was afraid the message was quite long already. |
Is this the error message that's removed in this PR? |
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Testing this out locally for a few snippets, it seems good but I think we can still improve this. I'm not sure how but multi-span diagnostics is one way to go to provide additional context.
I don't have a good suggestion for improving the error message, maybe something like:
SyntaxError: cannot use an asynchronous comprehension inside of a synchronous scope created by the outer comprehension on Python 3.9 (syntax was added in 3.11)
Maybe @AlexWaygood has a better recommendation?
|
No, the top one ( I don't think I explained this very well in the PR summary, looking back at it now. I'm doing two unrelated things in this PR:
I only added a new test for the first of these, which probably made it even less clear what was changing in the code. Sorry! |
ntBre
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
|
I'm resolving the merge conflicts now. Just as one additional data point, the CPython error message for this is also quite poor: >>> async def f(): [[x async for x in []] for x in []]
...
File "<stdin>", line 1
SyntaxError: asynchronous comprehension outside of an asynchronous functionThat's not an excuse for ours to be bad too, but I do think is already a pretty good improvement over the current message in ruff and over Python's message. I personally prefer this to the suggestion above too, but I don't feel strongly about it. If nobody else feels strongly, I'll just merge this for now, and we can tweak the message later. |
|
* main: (24 commits) [`airflow`] Extend `AIR301` rule (#17598) [`airflow`] update existing `AIR302` rules with better suggestions (#17542) red_knot_project: sort diagnostics from checking files [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621) [red-knot] fix inheritance-cycle detection for generic classes (#17620) [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411) Add Semantic Error Test for LateFutureImport (#17612) [red-knot] change TypeVarInstance to be interned, not tracked (#17616) [red-knot] Special case `@final`, `@override` (#17608) [red-knot] add TODO comment in specialization code (#17615) [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592) [syntax-errors] `nonlocal` declaration at module level (#17559) [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571) [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460) Bump 0.11.7 (#17613) [red-knot] Use iterative approach to collect overloads (#17607) red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest red_knot_python_semantic: improve diagnostics for unsupported boolean conversions red_knot_python_semantic: add "return type span" helper method red_knot_python_semantic: move parameter span helper method ...
* main: (27 commits) [red-knot] Add new property tests for subtyping with "bottom" callable (#17635) [red-knot] Create generic context for generic classes lazily (#17617) ruff_db: add tests for annotations with no ranges [`airflow`] Extend `AIR301` rule (#17598) [`airflow`] update existing `AIR302` rules with better suggestions (#17542) red_knot_project: sort diagnostics from checking files [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621) [red-knot] fix inheritance-cycle detection for generic classes (#17620) [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411) Add Semantic Error Test for LateFutureImport (#17612) [red-knot] change TypeVarInstance to be interned, not tracked (#17616) [red-knot] Special case `@final`, `@override` (#17608) [red-knot] add TODO comment in specialization code (#17615) [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592) [syntax-errors] `nonlocal` declaration at module level (#17559) [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571) [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460) Bump 0.11.7 (#17613) [red-knot] Use iterative approach to collect overloads (#17607) red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest ...
2 participants
Summary
While adding semantic error support to red-knot, I noticed duplicate diagnostics for code like this:
Beyond the duplication, the first error message doesn't make much sense because this syntax is not allowed on Python 3.11 either.
To fix this, this PR renames the
async-comprehension-outside-async-functionsemantic syntax error toasync-comprehension-in-sync-comprehensionand fixes the rule to avoid applying outside of sync comprehensions at all.Test Plan
New linter test demonstrating the false positive. The mdtests I'm updating in my red-knot PR will also reflect this change eventually.