Track fix isolation in unnecessary-pass (#7715)

## Summary This wasn't necessary in the past, since we _only_ applied this rule to bodies that contained two statements, one of which was a `pass`. Now that it applies to any `pass` in a block with multiple statements, we can run into situations in which we remove both passes, and so need to apply the fixes in isolation. See: #7455 (comment).
astral-sh · Sep 29, 2023 · 253fbb6 · 253fbb6
1 parent 974262a
commit 253fbb6
Show file tree

Hide file tree

Showing 8 changed files with 140 additions and 14 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py
@@ -134,3 +134,17 @@ def foo():
     """A docstring."""
     print("foo")
     pass
+
+
+for i in range(10):
+    pass
+    pass
+
+for i in range(10):
+    pass
+
+    pass
+
+for i in range(10):
+    pass  # comment
+    pass
diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs
@@ -82,9 +82,9 @@ pub(crate) fn duplicate_class_field_definition(checker: &mut Checker, body: &[St
             if checker.patch(diagnostic.kind.rule()) {
                 let edit =
                     fix::edits::delete_stmt(stmt, Some(stmt), checker.locator(), checker.indexer());
-                diagnostic.set_fix(Fix::suggested(edit).isolate(Checker::isolation(Some(
+                diagnostic.set_fix(Fix::suggested(edit).isolate(Checker::isolation(
                     checker.semantic().current_statement_id(),
-                ))));
+                )));
             }
             checker.diagnostics.push(diagnostic);
         }

diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_pass.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_pass.rs
@@ -70,7 +70,9 @@ pub(crate) fn no_unnecessary_pass(checker: &mut Checker, body: &[Stmt]) {
                     } else {
                         fix::edits::delete_stmt(stmt, None, checker.locator(), checker.indexer())
                     };
-                diagnostic.set_fix(Fix::automatic(edit));
+                diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(
+                    checker.semantic().current_statement_id(),
+                )));
             }
             checker.diagnostics.push(diagnostic);
         });

diff --git a/...c/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap b/...c/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap
@@ -349,5 +349,117 @@ PIE790.py:136:5: PIE790 [*] Unnecessary `pass` statement
 134 134 |     """A docstring."""
 135 135 |     print("foo")
 136     |-    pass
+137 136 | 
+138 137 | 
+139 138 | for i in range(10):
+
+PIE790.py:140:5: PIE790 [*] Unnecessary `pass` statement
+    |
+139 | for i in range(10):
+140 |     pass
+    |     ^^^^ PIE790
+141 |     pass
+    |
+    = help: Remove unnecessary `pass`
+
+ℹ Fix
+138 138 | 
+139 139 | for i in range(10):
+140 140 |     pass
+141     |-    pass
+142 141 | 
+143 142 | for i in range(10):
+144 143 |     pass
+
+PIE790.py:141:5: PIE790 [*] Unnecessary `pass` statement
+    |
+139 | for i in range(10):
+140 |     pass
+141 |     pass
+    |     ^^^^ PIE790
+142 | 
+143 | for i in range(10):
+    |
+    = help: Remove unnecessary `pass`
+
+ℹ Fix
+138 138 | 
+139 139 | for i in range(10):
+140 140 |     pass
+141     |-    pass
+142 141 | 
+143 142 | for i in range(10):
+144 143 |     pass
+
+PIE790.py:144:5: PIE790 [*] Unnecessary `pass` statement
+    |
+143 | for i in range(10):
+144 |     pass
+    |     ^^^^ PIE790
+145 | 
+146 |     pass
+    |
+    = help: Remove unnecessary `pass`
+
+ℹ Fix
+141 141 |     pass
+142 142 | 
+143 143 | for i in range(10):
+144     |-    pass
+145 144 | 
+146 145 |     pass
+147 146 | 
+
+PIE790.py:146:5: PIE790 [*] Unnecessary `pass` statement
+    |
+144 |     pass
+145 | 
+146 |     pass
+    |     ^^^^ PIE790
+147 | 
+148 | for i in range(10):
+    |
+    = help: Remove unnecessary `pass`
+
+ℹ Fix
+143 143 | for i in range(10):
+144 144 |     pass
+145 145 | 
+146     |-    pass
+147 146 | 
+148 147 | for i in range(10):
+149 148 |     pass  # comment
+
+PIE790.py:149:5: PIE790 [*] Unnecessary `pass` statement
+    |
+148 | for i in range(10):
+149 |     pass  # comment
+    |     ^^^^ PIE790
+150 |     pass
+    |
+    = help: Remove unnecessary `pass`
+
+ℹ Fix
+146 146 |     pass
+147 147 | 
+148 148 | for i in range(10):
+149     |-    pass  # comment
+    149 |+    # comment
+150 150 |     pass
+
+PIE790.py:150:5: PIE790 [*] Unnecessary `pass` statement
+    |
+148 | for i in range(10):
+149 |     pass  # comment
+150 |     pass
+    |     ^^^^ PIE790
+    |
+    = help: Remove unnecessary `pass`
+
+ℹ Fix
+147 147 | 
+148 148 | for i in range(10):
+149 149 |     pass  # comment
+150     |-    pass
 
 
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs
@@ -66,9 +66,9 @@ pub(crate) fn ellipsis_in_non_empty_class_body(checker: &mut Checker, body: &[St
             if checker.patch(diagnostic.kind.rule()) {
                 let edit =
                     fix::edits::delete_stmt(stmt, Some(stmt), checker.locator(), checker.indexer());
-                diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(Some(
+                diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(
                     checker.semantic().current_statement_id(),
-                ))));
+                )));
             }
             checker.diagnostics.push(diagnostic);
         }

diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/pass_in_class_body.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/pass_in_class_body.rs
@@ -37,9 +37,9 @@ pub(crate) fn pass_in_class_body(checker: &mut Checker, class_def: &ast::StmtCla
         if checker.patch(diagnostic.kind.rule()) {
             let edit =
                 fix::edits::delete_stmt(stmt, Some(stmt), checker.locator(), checker.indexer());
-            diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(Some(
+            diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(
                 checker.semantic().current_statement_id(),
-            ))));
+            )));
         }
         checker.diagnostics.push(diagnostic);
     }

diff --git a/crates/ruff_linter/src/rules/pylint/rules/useless_return.rs b/crates/ruff_linter/src/rules/pylint/rules/useless_return.rs
@@ -106,9 +106,9 @@ pub(crate) fn useless_return(
     if checker.patch(diagnostic.kind.rule()) {
         let edit =
             fix::edits::delete_stmt(last_stmt, Some(stmt), checker.locator(), checker.indexer());
-        diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(Some(
+        diagnostic.set_fix(Fix::automatic(edit).isolate(Checker::isolation(
             checker.semantic().current_statement_id(),
-        ))));
+        )));
     }
     checker.diagnostics.push(diagnostic);
 }
diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs
@@ -887,11 +887,9 @@ impl<'a> SemanticModel<'a> {
             .filter(|id| self.nodes[*id].is_statement())
     }
 
-    /// Return the [`NodeId`] of the current [`Stmt`].
-    pub fn current_statement_id(&self) -> NodeId {
-        self.current_statement_ids()
-            .next()
-            .expect("No current statement")
+    /// Return the [`NodeId`] of the current [`Stmt`], if any.
+    pub fn current_statement_id(&self) -> Option<NodeId> {
+        self.current_statement_ids().next()
     }
 
     /// Return the [`NodeId`] of the current [`Stmt`] parent, if any.