-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[syntax-errors] Reimplement break-outside-loop
(F701
) as a semantic syntax error
#20556
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
Conversation
inside_orelse: false, | ||
}); | ||
for stmt in body { | ||
self.visit_stmt(stmt, ctx); |
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.
We shouldn't be recursing here. The SemanticSyntaxChecker
isn't a full Visitor
, it only checks the single Stmt
it's given and relies on the parent visitor (either Checker
in Ruff or SemanticIndexBuilder
in ty) to drive the AST traversal.
I think this also means that managing the loop state isn't really feasible in the SemanticSyntaxChecker
itself. Instead, we'll probably need to add in_loop_context
as a method on the SemanticSyntaxContext
trait and implement it for Ruff and ty (and the test visitor if we want to write inline tests).
You could also consider trying to add a SemanticSyntaxContext::current_statements()
method, which would be more general and match how Ruff currently implements this check:
ruff/crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Lines 54 to 61 in 83f80ef
if checker.is_rule_enabled(Rule::BreakOutsideLoop) { | |
pyflakes::rules::break_outside_loop( | |
checker, | |
stmt, | |
&mut checker.semantic.current_statements().skip(1), | |
); | |
} | |
} |
Those are just some ideas based on a quick look, hope that helps!
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.
Sure, Thank you for these pointers, I will try them out & keep you updated : )
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.
done, I have added a new current_statements
.
Thank you : )
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Signed-off-by: 11happy <soni5happy@gmail.com>
|
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.
Thanks! But I think we can make a few improvements here.
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 think we should delete this test case. We still need to emit F701, even if the main implementation is as a syntax error.
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.
done
self.semantic_syntax_errors.borrow_mut().push(error); | ||
} | ||
} | ||
fn in_loop_context(&self) -> bool { |
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.
fn in_loop_context(&self) -> bool { | |
fn in_loop_context(&self) -> bool { |
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.
done
self.seen_futures_boundary = true; | ||
} | ||
} | ||
|
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.
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.
done
} | ||
|
||
fn in_loop_context(&self) -> bool { | ||
let statements = self.current_statements(); |
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 think we need to track a separate Vec
of current statements on the Checker
. This should already be available on the SemanticModel
field, as in the code I linked earlier.
let statements = self.current_statements(); | |
let statements = self.semantic.current_statements(); |
My earlier suggestion about adding a current_statements
method was not for Checker
, I meant that instead of SemanticSyntaxContext::in_loop_context
, we could consider adding SemanticSyntaxContext::current_statements
, which would return an iterator over Stmt
nodes:
fn current_statements(&self) -> impl Iterator<Item = &Stmt> {
self.semantic.current_statements()
}
This should be easier to implement for both Ruff and ty, and then the actual in_loop_context
check could be shared between the two. But I think SemanticSyntaxContext::in_loop_context
is also okay, especially if you tried this already.
match *parent { | ||
Stmt::For(ast::StmtFor { orelse, .. }) | ||
| Stmt::While(ast::StmtWhile { orelse, .. }) => { | ||
let in_orelse = orelse.contains(cur_stmt.unwrap()); |
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 think we should just pass the current statement in, like in the original F701 implementation, to avoid needing to unwrap
here.
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.
done
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
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.
Thank you! I pushed a couple of very small cleanups, but this looks good to me now.
break-outside-loop
(F701
) as a semantic syntax error
break-outside-loop
(F701
) as a semantic syntax errorbreak-outside-loop
(F701
) as a semantic syntax error
…tity * origin/main: (24 commits) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) Update Rust crate memchr to v2.7.6 (#20834) ...
* main: (25 commits) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) ...
…rable * origin/main: (26 commits) [ty] Add separate type for typevar "identity" (#20813) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) ...
Summary
This PR implements https://docs.astral.sh/ruff/rules/break-outside-loop/ (F701) as a semantic syntax error.
Test Plan