[ruff] Add fallible-context-manager (RUF075)#22844
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF075 | 87 | 87 | 0 | 0 | 0 |
1c0c5dd to
349e9dd
Compare
|
I looked through the ecosystem results, and there are a number of cases where either |
|
Thanks for the detailed feedback, @amyreese! I've updated the rule to no longer flag yields in terminal position (where there's no cleanup code to protect) and to accept try/except as sufficient exception handling. The ecosystem check shows 127 fewer false positives across 20 projects with no lost true positives. Would love your review whenever you get a chance! |
ntBre
left a comment
There was a problem hiding this comment.
Thanks for working on this! I agree with Amy, I don't think we can make this a flake8-bugbear rule and especially not B036, which has a different meaning in the upstream linter. I would probably just make this a new RUF rule.
The Visitor feels a little complicated to me, but I'm not sure if it's actually over-complicated yet. I'll have to take a deeper look to be sure I understand it. I think it would help to add a couple of documentation comments on the Visitor struct, its fields, and maybe the methods in its impl block (like on visit_with_body and on visit_body_with_terminal).
The general idea looks reasonable to me, I'm just wishing we could reuse more of the default Visitor behavior instead of having to inline so many parts of walk_stmt.
|
Hi @ntBre , @amyreese
Would appreciate another look when you get a chance. Thanks! |
B036 contextmanager without try/finallyruff] Add RUF069 contextmanager without try/finally
ntBre
left a comment
There was a problem hiding this comment.
Thanks again for working on this. I took a deeper look at this today, playing with the code myself locally. I added a few inline comments as I went, but the very brief summary is that we need a lot more test coverage, or much of this code is unused. I'll put the patch I ended up with locally down below, but I was able to delete a ton of code without failing any existing tests. Some of this I believe was genuinely redundant, like the in_with_last_statement flag, but others like the in_protected_try flag were just casualties of too little test coverage.
Patch
diff --git a/crates/ruff_linter/src/rules/ruff/rules/fallible_context_manager.rs b/crates/ruff_linter/src/rules/ruff/rules/fallible_context_manager.rs
index 9024b8f738..0a46de3048 100644
--- a/crates/ruff_linter/src/rules/ruff/rules/fallible_context_manager.rs
+++ b/crates/ruff_linter/src/rules/ruff/rules/fallible_context_manager.rs
@@ -66,7 +66,7 @@ pub(crate) fn fallible_context_manager(checker: &Checker, function_def: &StmtFun
}
let mut visitor = YieldFinallyVisitor::new(checker);
- visitor.visit_body_with_terminal(&function_def.body, true);
+ visitor.visit_body_with_terminal(&function_def.body);
}
fn has_contextmanager_decorator(checker: &Checker, function_def: &StmtFunctionDef) -> bool {
@@ -94,12 +94,6 @@ fn has_contextmanager_decorator(checker: &Checker, function_def: &StmtFunctionDe
struct YieldFinallyVisitor<'a, 'b> {
/// The checker used to emit diagnostics.
checker: &'a Checker<'b>,
- /// Whether the visitor is currently inside a `try` block that has
- /// `finally` or `except` handlers.
- in_protected_try: bool,
- /// Whether the visitor is at the last statement in a `with` block body,
- /// where the `with` statement's `__exit__` provides exception handling.
- in_with_last_statement: bool,
/// Whether the visitor is at a terminal position: the last statement in
/// the function body, or a `yield` immediately before a `return`.
in_terminal_position: bool,
@@ -110,9 +104,7 @@ impl<'a, 'b> YieldFinallyVisitor<'a, 'b> {
fn new(checker: &'a Checker<'b>) -> Self {
Self {
checker,
- in_protected_try: false,
- in_with_last_statement: false,
- in_terminal_position: false,
+ in_terminal_position: true,
}
}
}
@@ -122,79 +114,10 @@ impl Visitor<'_> for YieldFinallyVisitor<'_, '_> {
match stmt {
Stmt::FunctionDef(_) | Stmt::ClassDef(_) => {}
- Stmt::Try(ast::StmtTry {
- body,
- handlers,
- orelse,
- finalbody,
- ..
- }) if !finalbody.is_empty() || !handlers.is_empty() => {
- let prev = self.in_protected_try;
- self.in_protected_try = true;
- self.visit_body(body);
- for handler in handlers {
- self.visit_except_handler(handler);
- }
- self.visit_body(orelse);
- self.in_protected_try = prev;
- self.visit_body(finalbody);
- }
-
Stmt::With(ast::StmtWith { body, .. }) => {
self.visit_with_body(body);
}
- Stmt::If(ast::StmtIf {
- test,
- body,
- elif_else_clauses,
- ..
- }) => {
- self.visit_expr(test);
- let terminal = self.in_terminal_position;
- self.visit_body_with_terminal(body, terminal);
- for clause in elif_else_clauses {
- if let Some(test) = &clause.test {
- self.visit_expr(test);
- }
- self.visit_body_with_terminal(&clause.body, terminal);
- }
- }
-
- Stmt::For(ast::StmtFor {
- target,
- iter,
- body,
- orelse,
- ..
- }) => {
- self.visit_expr(iter);
- self.visit_expr(target);
- let terminal = self.in_terminal_position;
- self.visit_body_with_terminal(body, terminal);
- self.visit_body_with_terminal(orelse, terminal);
- }
-
- Stmt::While(ast::StmtWhile {
- test, body, orelse, ..
- }) => {
- self.visit_expr(test);
- let terminal = self.in_terminal_position;
- self.visit_body_with_terminal(body, terminal);
- self.visit_body_with_terminal(orelse, terminal);
- }
-
- Stmt::Match(ast::StmtMatch { subject, cases, .. }) => {
- self.visit_expr(subject);
- let terminal = self.in_terminal_position;
- for case in cases {
- if let Some(guard) = &case.guard {
- self.visit_expr(guard);
- }
- self.visit_body_with_terminal(&case.body, terminal);
- }
- }
-
_ => walk_stmt(self, stmt),
}
}
@@ -202,10 +125,7 @@ impl Visitor<'_> for YieldFinallyVisitor<'_, '_> {
fn visit_expr(&mut self, expr: &Expr) {
match expr {
Expr::Yield(_) | Expr::YieldFrom(_) => {
- if !self.in_protected_try
- && !self.in_with_last_statement
- && !self.in_terminal_position
- {
+ if !self.in_terminal_position {
self.checker
.report_diagnostic(FallibleContextManager, expr.range());
}
@@ -221,7 +141,7 @@ impl YieldFinallyVisitor<'_, '_> {
///
/// A statement is considered terminal if it is the last in the body (when `terminal` is true)
/// or if it is a yield statement immediately followed by a `return`.
- fn visit_body_with_terminal(&mut self, body: &[Stmt], terminal: bool) {
+ fn visit_body_with_terminal(&mut self, body: &[Stmt]) {
for (i, stmt) in body.iter().enumerate() {
let is_last = i == body.len() - 1;
let is_yield_before_return = Self::is_yield_statement(stmt)
@@ -230,7 +150,7 @@ impl YieldFinallyVisitor<'_, '_> {
.is_some_and(|next| matches!(next, Stmt::Return(_)));
let prev = self.in_terminal_position;
- self.in_terminal_position = (is_last && terminal) || is_yield_before_return;
+ self.in_terminal_position &= is_last || is_yield_before_return;
self.visit_stmt(stmt);
self.in_terminal_position = prev;
}
@@ -241,27 +161,26 @@ impl YieldFinallyVisitor<'_, '_> {
/// The last statement in a `with` block inherits terminal status from the parent context
/// and is additionally marked as a "with last statement" if it is a yield.
fn visit_with_body(&mut self, body: &[Stmt]) {
- if body.is_empty() {
+ let [rest @ .., last] = body else {
return;
- }
+ };
let parent_terminal = self.in_terminal_position;
// Non-last statements: delegate terminal tracking to visit_body_with_terminal
- if body.len() > 1 {
- self.visit_body_with_terminal(&body[..body.len() - 1], false);
- }
+ self.in_terminal_position = false;
+ self.visit_body_with_terminal(rest);
+ self.in_terminal_position = parent_terminal;
// Last statement: inherit terminal from parent, set with-last-statement if yield
- let last = &body[body.len() - 1];
let prev_terminal = self.in_terminal_position;
self.in_terminal_position = parent_terminal;
if Self::is_yield_statement(last) {
- let prev_with = self.in_with_last_statement;
- self.in_with_last_statement = true;
+ let prev_with = self.in_terminal_position;
+ self.in_terminal_position = true;
self.visit_stmt(last);
- self.in_with_last_statement = prev_with;
+ self.in_terminal_position = prev_with;
} else {
self.visit_stmt(last);
}f913bf7 to
675459c
Compare
|
Hi @ntBre, thank you so much for the detailed review! I really appreciate the thoughtful feedback. I've addressed all the comments:
For the code I kept, I added test coverage to demonstrate why it's needed:
Please let me know if there's anything else I can improve. Thank you again for your time! |
|
Thanks for the update, I'll try to take another look soon. Apologies in advance, I don't usually like to meddle with contributors' workflows, but in the future it would be preferable to push additional commits instead of rebasing. GitHub's review interface doesn't work very well with rebases (the "commits since your last review" button shows an error), and we squash-merge PRs at the end anyway, so we don't need to keep a clean history in the PR. For a big PR like this, that would have been particularly helpful. |
Thank you so much for letting me know! I'll definitely stick to pushing additional commits going forward. Thanks for your patience, and I look forward to your next review whenever you get the chance. |
ruff] Add RUF071 contextmanager without try/finallyruff] Add RUF072 contextmanager without try/finally
# Conflicts: # crates/ruff_linter/resources/test/fixtures/ruff/RUF072.py # crates/ruff_linter/src/codes.rs # ruff.schema.json
ruff] Add RUF072 contextmanager without try/finallyruff] Add RUF074 contextmanager without try/finally
ntBre
left a comment
There was a problem hiding this comment.
Thank you! This looks very close to ready to go. I only had a few minor comments, mostly around testing some edge cases. This branch will also need a merge with main now that your other RUF074 PR landed first.
Resolve conflicts caused by upstream landing RUF074 (`incorrect-decorator-order`, astral-sh#23461). Renumber the in-progress `FallibleContextManager` rule from RUF074 to RUF075: - codes.rs: keep `IncorrectDecoratorOrder` at 074 (landed), add `FallibleContextManager` at 075 - mod.rs: keep both `test_case` entries; FallibleContextManager now references `RUF075.py` - Rename fixture: HEAD's `RUF074.py` content moves to new `RUF075.py` with `# RUF074` annotations updated to `# RUF075` - Rename snapshot: HEAD's snapshot becomes `RUF075_RUF075.py.snap` with `RUF074` references rewritten to `RUF075` - fallible_context_manager.rs: rule-id comment updated to RUF075 - ruff.schema.json regenerated via `cargo dev generate-all`
ruff] Add RUF074 contextmanager without try/finallyruff] Add RUF075 contextmanager without try/finally
…rate tests to mdtest
|
@ntBre thank you so much for the re-review. I have addressed all the feedbacks and updated the pr. Would like to request you for your rer-review, whenever you get a chance. thank you |
ntBre
left a comment
There was a problem hiding this comment.
Thanks again for all of your work here! I managed to find a few more suggestions, but again this is very close.
…e mdtest, drop stray RUF069 artifacts
|
@ntBre thank you so much for the re-review. I have addressed all the feedbacks in the latest push. Would like to request you for your another look, whenever you get a chance. thank you |
ntBre
left a comment
There was a problem hiding this comment.
Excellent work here, thank you!
ruff] Add RUF075 contextmanager without try/finallyruff] Add fallible-context-manager (RUF075)
## Summary Adds a new rule `RUF075` (preview) that detects `@contextlib.contextmanager` and `@contextlib.asynccontextmanager` decorated functions where `yield` is not protected against exceptions. Closes astral-sh#15629. ## Details When a context manager yields without exception protection, cleanup code after `yield` won't run if an exception is raised inside the `with` block. ```python # Bad - cleanup won't run on exception @contextmanager def bad(): print("setup") yield print("cleanup") # never runs if exception raised # Good @contextmanager def good(): print("setup") try: yield finally: print("cleanup") # always runs ``` The rule does **not** flag: - Yields inside `try` blocks that have a `finally` or `except` handler - Yields as the last statement of a `with` block (cleanup is delegated to `__exit__`) - Yields in terminal position (last statement in the function body, or immediately before a `return`), where there is no trailing cleanup that could be skipped Terminal position is inherited through `if` / `for` / `while` / `match` branches: a yield that is the last statement of a branch is terminal only when the enclosing statement is itself terminal. The rule is in preview mode. ## Test Plan Tests live in `crates/ruff_linter/resources/mdtest/ruff.md` and are run via: ```sh cargo nextest run -p ruff_mdtest -- mdtest::ruff ```
## Summary Adds a new rule `RUF075` (preview) that detects `@contextlib.contextmanager` and `@contextlib.asynccontextmanager` decorated functions where `yield` is not protected against exceptions. Closes astral-sh#15629. ## Details When a context manager yields without exception protection, cleanup code after `yield` won't run if an exception is raised inside the `with` block. ```python # Bad - cleanup won't run on exception @contextmanager def bad(): print("setup") yield print("cleanup") # never runs if exception raised # Good @contextmanager def good(): print("setup") try: yield finally: print("cleanup") # always runs ``` The rule does **not** flag: - Yields inside `try` blocks that have a `finally` or `except` handler - Yields as the last statement of a `with` block (cleanup is delegated to `__exit__`) - Yields in terminal position (last statement in the function body, or immediately before a `return`), where there is no trailing cleanup that could be skipped Terminal position is inherited through `if` / `for` / `while` / `match` branches: a yield that is the last statement of a branch is terminal only when the enclosing statement is itself terminal. The rule is in preview mode. ## Test Plan Tests live in `crates/ruff_linter/resources/mdtest/ruff.md` and are run via: ```sh cargo nextest run -p ruff_mdtest -- mdtest::ruff ```
Summary
Adds a new rule
RUF075(preview) that detects@contextlib.contextmanagerand@contextlib.asynccontextmanagerdecorated functions whereyieldis not protected against exceptions.Closes #15629.
Details
When a context manager yields without exception protection, cleanup code after
yieldwon't run if an exception is raised inside thewithblock.The rule does not flag:
tryblocks that have afinallyorexcepthandlerwithblock (cleanup is delegated to__exit__)return), where there is no trailing cleanup that could be skippedTerminal position is inherited through
if/for/while/matchbranches: a yield that is the last statement of a branch is terminal only when the enclosing statement is itself terminal.The rule is in preview mode.
Test Plan
Tests live in
crates/ruff_linter/resources/mdtest/ruff.mdand are run via: