-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[syntax-errors] Async comprehension in sync comprehension #17177
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
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
b15f6bb
[syntax-errors] Async comprehension in sync comprehension
ntBre 336e6a3
in-line `TestContext` into SemanticSyntaxCheckerVisitor
ntBre 0835fa2
refactor some `Default` impls to make clippy happier
ntBre 3785e4b
add test_ok all-async case on 3.10 and track in_async_context
ntBre 76dd5d5
add failing linter test, this should be an error
ntBre 30a56a8
track async status for generator scopes and pass test
ntBre 7462cd6
add 3.11 test, don't reuse command, can't pass target-version twice
ntBre 04c57f0
track async context on SemanticSyntaxChecker, with checkpoint
ntBre 35f9c9c
remove apparently unnecessary checkpointing code
ntBre 50e0bc8
revert other unused context changes
ntBre f811c1a
revert unnecessary linter tests
ntBre ffcd250
tidy up and add docs
ntBre d2127e8
add more test cases, finally find a test_ok that needs checkpoints
ntBre 5b440de
restore checkpoints, convert to stack to pass test
ntBre a96ae02
restore linter tests
ntBre d49c50c
add a test case for exit_stmt and some commentary
ntBre 624f671
wire up exit methods and add test, but earlier test is failing
ntBre a21be7e
isolate the inline test that fails in the linter
ntBre bf69ef9
defer visits to function bodies
ntBre cc42a38
rename visit_{stmt,expr} to enter_{stmt,expr} and add docs
ntBre d1e1cc4
fix doc links and mention `exit_*` methods
ntBre 7f99f5f
Merge branch 'main' into brent/syn-async-comprehensions
ntBre 0f95773
return early on recent python version
ntBre 48f1e52
use the call stack instead of Vec<Checkpoint> stack
ntBre 970fe3d
Merge branch 'main' into brent/syn-async-comprehensions
ntBre 7d3b4f6
add `must_use` and some docs
ntBre 2dd610f
pass source_type to SemanticSyntaxChecker
ntBre 6db55c6
add notebook CLI test
ntBre f730e85
move non-notebook tests to ruff_linter
ntBre 83d3a70
refactor check_syntax_errors to handle notebooks, move notebook test
ntBre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
57 changes: 57 additions & 0 deletions
57
crates/ruff_linter/resources/test/fixtures/syntax_errors/async_comprehension.ipynb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| { | ||
| "cells": [ | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "6a70904e-dbfe-441c-99ec-12e6cf57f8ba", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "async def elements(n): yield n" | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "5412fc2f-76eb-42c0-8db1-b5af6fdc46aa", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "[x async for x in elements(5)] # okay, async at top level" | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "dc3c94a7-2e64-42de-9351-260b3f41c3fd", | ||
| "metadata": { | ||
| "scrolled": true | ||
| }, | ||
| "outputs": [], | ||
| "source": [ | ||
| "[[x async for x in elements(5)] for i in range(5)] # error on 3.10, okay after" | ||
| ] | ||
| } | ||
| ], | ||
| "metadata": { | ||
| "kernelspec": { | ||
| "display_name": "Python 3 (ipykernel)", | ||
| "language": "python", | ||
| "name": "python3" | ||
| }, | ||
| "language_info": { | ||
| "codemirror_mode": { | ||
| "name": "ipython", | ||
| "version": 3 | ||
| }, | ||
| "file_extension": ".py", | ||
| "mimetype": "text/x-python", | ||
| "name": "python", | ||
| "nbconvert_exporter": "python", | ||
| "pygments_lexer": "ipython3", | ||
| "version": "3.10.16" | ||
| } | ||
| }, | ||
| "nbformat": 4, | ||
| "nbformat_minor": 5 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
...linter__tests__async_comprehension_in_sync_comprehension_deferred_function_body_3.10.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/linter.rs | ||
| --- | ||
|
|
8 changes: 8 additions & 0 deletions
8
...f_linter__linter__tests__async_comprehension_in_sync_comprehension_error_on_310_3.10.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/linter.rs | ||
| --- | ||
| <filename>:1:27: SyntaxError: cannot use an asynchronous comprehension outside of an asynchronous function on Python 3.10 (syntax was added in 3.11) | ||
| | | ||
| 1 | async def f(): return [[x async for x in foo(n)] for n in range(3)] | ||
| | ^^^^^^^^^^^^^^^^^^^^^ | ||
| | |
10 changes: 10 additions & 0 deletions
10
.../ruff_linter__linter__tests__async_comprehension_in_sync_comprehension_notebook_3.10.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/linter.rs | ||
| --- | ||
| resources/test/fixtures/syntax_errors/async_comprehension.ipynb:3:5: SyntaxError: cannot use an asynchronous comprehension outside of an asynchronous function on Python 3.10 (syntax was added in 3.11) | ||
| | | ||
| 1 | async def elements(n): yield n | ||
| 2 | [x async for x in elements(5)] # okay, async at top level | ||
| 3 | [[x async for x in elements(5)] for i in range(5)] # error on 3.10, okay after | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | |
4 changes: 4 additions & 0 deletions
4
.../ruff_linter__linter__tests__async_comprehension_in_sync_comprehension_notebook_3.11.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/linter.rs | ||
| --- | ||
|
|
4 changes: 4 additions & 0 deletions
4
...ff_linter__linter__tests__async_comprehension_in_sync_comprehension_okay_on_310_3.10.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/linter.rs | ||
| --- | ||
|
|
4 changes: 4 additions & 0 deletions
4
...ff_linter__linter__tests__async_comprehension_in_sync_comprehension_okay_on_311_3.11.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/linter.rs | ||
| --- | ||
|
|
6 changes: 6 additions & 0 deletions
6
crates/ruff_python_parser/resources/inline/err/nested_async_comprehension_py310.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # parse_options: {"target-version": "3.10"} | ||
| async def f(): return [[x async for x in foo(n)] for n in range(3)] # list | ||
| async def g(): return [{x: 1 async for x in foo(n)} for n in range(3)] # dict | ||
| async def h(): return [{x async for x in foo(n)} for n in range(3)] # set | ||
| async def i(): return [([y async for y in range(1)], [z for z in range(2)]) for x in range(5)] | ||
| async def j(): return [([y for y in range(1)], [z async for z in range(2)]) for x in range(5)] |
2 changes: 2 additions & 0 deletions
2
crates/ruff_python_parser/resources/inline/ok/all_async_comprehension_py310.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # parse_options: {"target-version": "3.10"} | ||
| async def test(): return [[x async for x in elements(n)] async for n in range(3)] |
9 changes: 9 additions & 0 deletions
9
crates/ruff_python_parser/resources/inline/ok/nested_async_comprehension_py310.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # parse_options: {"target-version": "3.10"} | ||
| # this case fails if exit_expr doesn't run | ||
| async def f(): | ||
| [_ for n in range(3)] | ||
| [_ async for n in range(3)] | ||
| # and this fails without exit_stmt | ||
| async def f(): | ||
| def g(): ... | ||
| [_ async for n in range(3)] |
4 changes: 4 additions & 0 deletions
4
crates/ruff_python_parser/resources/inline/ok/nested_async_comprehension_py311.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # parse_options: {"target-version": "3.11"} | ||
| async def f(): return [[x async for x in foo(n)] for n in range(3)] # list | ||
| async def g(): return [{x: 1 async for x in foo(n)} for n in range(3)] # dict | ||
| async def h(): return [{x async for x in foo(n)} for n in range(3)] # set |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't understand the reasoning for skipping function definitions here. Won't this result in stale
in_async_contextflags because we don't update the state until after we visited the entire body?If there's a need for deferred visiting, then I'd prefer to have a
enter_deferred_stmtor, better, move the necessary checks intoexit_stmtand also pass the statement and context becauseexitis exactly the hook called after visiting the node's childrenThere 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 think I might be missing something here, but my reasoning for this was that the
ast::Checkerdoesn't actually visit the body of the function untilvisit_deferred_function:ruff/crates/ruff_linter/src/checkers/ast/mod.rs
Lines 2588 to 2591 in 5cee346
Before I added this logic, the function body was being visited twice by the
SemanticSyntaxCheckerbut only once by theast::Checkerand this integration test was failing even though the inline version passed:which pointed to an issue in the visit order because the test visitor is much simpler:
Again I might be misunderstanding, but I don't think an
enter_deferred_stmthelps here because we still need this logic invisit_stmtitself to avoid duplicating the visit. Unless you mean hiding this logic inside ofSemanticSyntaxChecker::enter_stmt. I guess that could work if I also update the test visitor to defer function bodies like the realast::Checker.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.
Oh I see. This seems fragile. I wouldn't be aware of this out-of-order visiting when working on the
SemanticSyntaxChecker. We should at least document the constraints in which theentermethods are called. I assumed it would be in semantic visiting order but it seems its in semantic visiting order except for functions which may be deferred (or not, depending on the caller)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.
Yes, very fragile. I was so confused when the integration test was failing but the inline test was fine, until I realized the function bodies were deferred. I'll work on expanding the docs for this.