Skip to content

Conversation

11happy
Copy link
Contributor

@11happy 11happy commented Sep 24, 2025

Summary

This PR implements https://docs.astral.sh/ruff/rules/break-outside-loop/ (F701) as a semantic syntax error.

Test Plan

inside_orelse: false,
});
for stmt in body {
self.visit_stmt(stmt, ctx);
Copy link
Contributor

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:

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!

Copy link
Contributor Author

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 : )

Copy link
Contributor Author

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>
Copy link
Contributor

github-actions bot commented Oct 5, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

Copy link
Contributor

github-actions bot commented Oct 5, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

Signed-off-by: 11happy <soni5happy@gmail.com>
@11happy 11happy marked this pull request as ready for review October 5, 2025 13:21
Copy link
Contributor

github-actions bot commented Oct 5, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Contributor

@ntBre ntBre left a 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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn in_loop_context(&self) -> bool {
fn in_loop_context(&self) -> bool {

Copy link
Contributor Author

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;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Contributor Author

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();
Copy link
Contributor

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.

Suggested change
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());
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

11happy and others added 8 commits October 11, 2025 21:13
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>
Copy link
Contributor

@ntBre ntBre left a 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.

@ntBre ntBre enabled auto-merge (squash) October 13, 2025 19:57
@ntBre ntBre added the internal An internal refactor or improvement label Oct 13, 2025
@ntBre ntBre changed the title [syntax-errors]: break outside loop F701 [syntax-errors]: Reimplement break-outside-loop (F701) as a semantic syntax error Oct 13, 2025
@ntBre ntBre changed the title [syntax-errors]: Reimplement break-outside-loop (F701) as a semantic syntax error [syntax-errors] Reimplement break-outside-loop (F701) as a semantic syntax error Oct 13, 2025
@ntBre ntBre merged commit 2b729b4 into astral-sh:main Oct 13, 2025
41 checks passed
dcreager added a commit that referenced this pull request Oct 13, 2025
…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)
  ...
dcreager added a commit that referenced this pull request Oct 13, 2025
* 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)
  ...
dcreager added a commit that referenced this pull request Oct 14, 2025
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants