Use a separate fix-isolation group for every parent node (#4774)

Cargo.lock

@@ -1908,7 +1908,6 @@ name = "ruff_diagnostics"
 version = "0.0.0"
 dependencies = [
  "anyhow",
- "is-macro",
  "log",
  "ruff_text_size",
  "serde",

crates/ruff/src/autofix/mod.rs

@@ -1,10 +1,11 @@
 use std::collections::BTreeSet;
 
 use itertools::Itertools;
+use nohash_hasher::IntSet;
 use ruff_text_size::{TextRange, TextSize};
 use rustc_hash::FxHashMap;
 
-use ruff_diagnostics::{Diagnostic, Edit, Fix};
+use ruff_diagnostics::{Diagnostic, Edit, Fix, IsolationLevel};
 use ruff_python_ast::source_code::Locator;
 
 use crate::linter::FixTable;
@@ -37,8 +38,8 @@ fn apply_fixes<'a>(
 ) -> (String, FixTable) {
     let mut output = String::with_capacity(locator.len());
     let mut last_pos: Option<TextSize> = None;
-    let mut isolation = false;
     let mut applied: BTreeSet<&Edit> = BTreeSet::default();
+    let mut isolated: IntSet<u32> = IntSet::default();
     let mut fixed = FxHashMap::default();
 
     for (rule, fix) in diagnostics
@@ -66,13 +67,12 @@ fn apply_fixes<'a>(
             continue;
         }
 
-        // If this fix requires isolation, and we've already applied another fix that
+        // If this fix requires isolation, and we've already applied another fix in the
-        // requires isolation, skip it. We apply at most one isolated fix per run.
+        // same isolation group, skip it.
-        if fix.isolation().is_isolated() {
+        if let IsolationLevel::Group(id) = fix.isolation() {
-            if isolation {
+            if !isolated.insert(id) {
                 continue;
             }
-            isolation = true;
         }
 
         for edit in fix

crates/ruff/src/checkers/ast/mod.rs

@@ -169,6 +169,20 @@ impl<'a> Checker<'a> {
         )
     }
 
+    /// Returns the [`IsolationLevel`] for fixes in the current context.
+    ///
+    /// The primary use-case for fix isolation is to ensure that we don't delete all statements
+    /// in a given indented block, which would cause a syntax error. We therefore need to ensure
+    /// that we delete at most one statement per indented block per fixer pass. Fix isolation should
+    /// thus be applied whenever we delete a statement, but can otherwise be omitted.
+    pub(crate) fn isolation(&self, parent: Option<&Stmt>) -> IsolationLevel {
+        parent
+            .and_then(|stmt| self.semantic_model.stmts.node_id(stmt))
+            .map_or(IsolationLevel::default(), |node_id| {
+                IsolationLevel::Group(node_id.into())
+            })
+    }
+
     pub(crate) const fn semantic_model(&self) -> &SemanticModel<'a> {
         &self.semantic_model
     }
@@ -5318,11 +5332,9 @@ impl<'a> Checker<'a> {
                         }
                         if let Some(edit) = fix.as_ref() {
                             diagnostic.set_fix(Fix::automatic(edit.clone()).isolate(
-                                if parent.is_some() {
+                                parent_id.map_or(IsolationLevel::default(), |node_id| {
-                                    IsolationLevel::Isolated
+                                    IsolationLevel::Group(node_id.into())
-                                } else {
+                                }),
-                                    IsolationLevel::NonOverlapping
-                                },
                             ));
                         }
                         diagnostics.push(diagnostic);

crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs

@@ -2,7 +2,7 @@ use rustc_hash::FxHashSet;
 use rustpython_parser::ast::{self, Expr, Ranged, Stmt};
 
 use ruff_diagnostics::Diagnostic;
-use ruff_diagnostics::{AlwaysAutofixableViolation, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Fix};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -95,7 +95,7 @@ pub(crate) fn duplicate_class_field_definition<'a, 'b>(
                     checker.indexer,
                     checker.stylist,
                 );
-                diagnostic.set_fix(Fix::suggested(edit).isolate(IsolationLevel::Isolated));
+                diagnostic.set_fix(Fix::suggested(edit).isolate(checker.isolation(Some(parent))));
             }
             checker.diagnostics.push(diagnostic);
         }

crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs

@@ -1,6 +1,6 @@
 use rustpython_parser::ast::{Expr, ExprConstant, Ranged, Stmt, StmtExpr};
 
-use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, IsolationLevel, Violation};
+use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -76,7 +76,7 @@ pub(crate) fn ellipsis_in_non_empty_class_body<'a>(
                 checker.indexer,
                 checker.stylist,
             );
-            diagnostic.set_fix(Fix::automatic(edit).isolate(IsolationLevel::Isolated));
+            diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent))));
         }
         checker.diagnostics.push(diagnostic);
     }

crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs

@@ -1,6 +1,6 @@
 use rustpython_parser::ast::{Ranged, Stmt};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -46,7 +46,7 @@ pub(crate) fn pass_in_class_body<'a>(
                 checker.indexer,
                 checker.stylist,
             );
-            diagnostic.set_fix(Fix::automatic(edit).isolate(IsolationLevel::Isolated));
+            diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent))));
         }
         checker.diagnostics.push(diagnostic);
     }

crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs

@@ -1,7 +1,7 @@
 use rustpython_parser::ast;
 use rustpython_parser::ast::Ranged;
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -69,11 +69,7 @@ pub(crate) fn empty_type_checking_block(checker: &mut Checker, stmt: &ast::StmtI
             checker.indexer,
             checker.stylist,
         );
-        diagnostic.set_fix(Fix::automatic(edit).isolate(if parent.is_some() {
+        diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent)));
-            IsolationLevel::Isolated
-        } else {
-            IsolationLevel::NonOverlapping
-        }));
     }
     checker.diagnostics.push(diagnostic);
 }

crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs

@@ -1,4 +1,4 @@
-use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, IsolationLevel, Violation};
+use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_semantic::binding::{
     Binding, BindingKind, FromImportation, Importation, SubmoduleImportation,
@@ -118,13 +118,8 @@ pub(crate) fn runtime_import_in_type_checking_block(
                 )?;
 
                 Ok(
-                    Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()).isolate(
+                    Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits())
-                        if parent.is_some() {
+                        .isolate(checker.isolation(parent)),
-                            IsolationLevel::Isolated
-                        } else {
-                            IsolationLevel::NonOverlapping
-                        },
-                    ),
                 )
             });
         }

crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs

@@ -1,4 +1,4 @@
-use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, IsolationLevel, Violation};
+use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_semantic::binding::{
     Binding, BindingKind, FromImportation, Importation, SubmoduleImportation,
@@ -391,13 +391,8 @@ pub(crate) fn typing_only_runtime_import(
                 )?;
 
                 Ok(
-                    Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()).isolate(
+                    Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits())
-                        if parent.is_some() {
+                        .isolate(checker.isolation(parent)),
-                            IsolationLevel::Isolated
-                        } else {
-                            IsolationLevel::NonOverlapping
-                        },
-                    ),
                 )
             });
         }

crates/ruff/src/rules/pyflakes/rules/unused_variable.rs

@@ -3,7 +3,7 @@ use ruff_text_size::TextRange;
 use rustpython_parser::ast::{self, Ranged, Stmt};
 use rustpython_parser::{lexer, Mode, Tok};
 
-use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, IsolationLevel, Violation};
+use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::contains_effect;
 use ruff_python_ast::source_code::Locator;
@@ -215,11 +215,7 @@ fn remove_unused_variable(
                         checker.indexer,
                         checker.stylist,
                     );
-                    Some(Fix::suggested(edit).isolate(if parent.is_some() {
+                    Some(Fix::suggested(edit).isolate(checker.isolation(parent)))
-                        IsolationLevel::Isolated
-                    } else {
-                        IsolationLevel::NonOverlapping
-                    }))
                 };
             }
         }
@@ -250,11 +246,7 @@ fn remove_unused_variable(
                     checker.indexer,
                     checker.stylist,
                 );
-                Some(Fix::suggested(edit).isolate(if parent.is_some() {
+                Some(Fix::suggested(edit).isolate(checker.isolation(parent)))
-                    IsolationLevel::Isolated
-                } else {
-                    IsolationLevel::NonOverlapping
-                }))
             };
         }
     }

crates/ruff/src/rules/pylint/rules/useless_return.rs

@@ -1,6 +1,6 @@
 use rustpython_parser::ast::{self, Constant, Expr, Ranged, Stmt};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::{is_const_none, ReturnStatementVisitor};
 use ruff_python_ast::statement_visitor::StatementVisitor;
@@ -110,7 +110,7 @@ pub(crate) fn useless_return<'a>(
             checker.indexer,
             checker.stylist,
         );
-        diagnostic.set_fix(Fix::automatic(edit).isolate(IsolationLevel::Isolated));
+        diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(stmt))));
     }
     checker.diagnostics.push(diagnostic);
 }

crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs

@@ -1,7 +1,7 @@
 use itertools::Itertools;
 use rustpython_parser::ast::{Alias, Ranged, Stmt};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -119,11 +119,7 @@ pub(crate) fn unnecessary_builtin_import(
                 checker.indexer,
                 checker.stylist,
             )?;
-            Ok(Fix::suggested(edit).isolate(if parent.is_some() {
+            Ok(Fix::suggested(edit).isolate(checker.isolation(parent)))
-                IsolationLevel::Isolated
-            } else {
-                IsolationLevel::NonOverlapping
-            }))
         });
     }
     checker.diagnostics.push(diagnostic);

crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs

@@ -1,7 +1,7 @@
 use itertools::Itertools;
 use rustpython_parser::ast::{Alias, Ranged, Stmt};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -98,11 +98,7 @@ pub(crate) fn unnecessary_future_import(checker: &mut Checker, stmt: &Stmt, name
                 checker.indexer,
                 checker.stylist,
             )?;
-            Ok(Fix::suggested(edit).isolate(if parent.is_some() {
+            Ok(Fix::suggested(edit).isolate(checker.isolation(parent)))
-                IsolationLevel::Isolated
-            } else {
-                IsolationLevel::NonOverlapping
-            }))
         });
     }
     checker.diagnostics.push(diagnostic);

crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs

@@ -1,6 +1,6 @@
 use rustpython_parser::ast::{self, Expr, Ranged, Stmt};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, IsolationLevel};
+use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::autofix;
@@ -55,7 +55,7 @@ pub(crate) fn useless_metaclass_type(
             checker.indexer,
             checker.stylist,
         );
-        diagnostic.set_fix(Fix::automatic(edit).isolate(IsolationLevel::Isolated));
+        diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent)));
     }
     checker.diagnostics.push(diagnostic);
 }

crates/ruff_diagnostics/Cargo.toml

@@ -11,6 +11,5 @@ rust-version = { workspace = true }
 ruff_text_size = { workspace = true }
 
 anyhow = { workspace = true }
-is-macro = { workspace = true }
 log = { workspace = true }
 serde = { workspace = true, optional = true, features = [] }

crates/ruff_diagnostics/src/fix.rs

@@ -28,11 +28,11 @@ pub enum Applicability {
 }
 
 /// Indicates the level of isolation required to apply a fix.
-#[derive(Default, Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, is_macro::Is)]
+#[derive(Default, Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub enum IsolationLevel {
-    /// The fix should be applied in isolation.
+    /// The fix should be applied as long as no other fixes in the same group have been applied.
-    Isolated,
+    Group(u32),
     /// The fix should be applied as long as it does not overlap with any other fixes.
     #[default]
     NonOverlapping,