diff --git a/Cargo.lock b/Cargo.lock index ab4e9ad7ee..3df63e6e02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1908,7 +1908,6 @@ name = "ruff_diagnostics" version = "0.0.0" dependencies = [ "anyhow", - "is-macro", "log", "ruff_text_size", "serde", diff --git a/crates/ruff/src/autofix/mod.rs b/crates/ruff/src/autofix/mod.rs index 749daabf5e..2bc6e1b12b 100644 --- a/crates/ruff/src/autofix/mod.rs +++ b/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 = None; - let mut isolation = false; let mut applied: BTreeSet<&Edit> = BTreeSet::default(); + let mut isolated: IntSet = 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 - // requires isolation, skip it. We apply at most one isolated fix per run. - if fix.isolation().is_isolated() { - if isolation { + // If this fix requires isolation, and we've already applied another fix in the + // same isolation group, skip it. + if let IsolationLevel::Group(id) = fix.isolation() { + if !isolated.insert(id) { continue; } - isolation = true; } for edit in fix diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index f6edf17bca..8c63f245c8 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/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() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - }, + parent_id.map_or(IsolationLevel::default(), |node_id| { + IsolationLevel::Group(node_id.into()) + }), )); } diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs b/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs index 152448729b..ce0991ac98 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs +++ b/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); } diff --git a/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs b/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs index 73ec4b2f56..3648d27345 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs +++ b/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); } diff --git a/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs b/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs index 442f8f5239..43f1829e27 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs +++ b/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); } diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs b/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs index d7474c0caa..2e90e50284 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs +++ b/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() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - })); + diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent))); } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index 39d2678ba2..82f839664b 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/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( - if parent.is_some() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - }, - ), + Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()) + .isolate(checker.isolation(parent)), ) }); } diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 167025e136..3b26aadd0b 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/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( - if parent.is_some() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - }, - ), + Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()) + .isolate(checker.isolation(parent)), ) }); } diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index ac286623df..afb8500de0 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/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() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - })) + Some(Fix::suggested(edit).isolate(checker.isolation(parent))) }; } } @@ -250,11 +246,7 @@ fn remove_unused_variable( checker.indexer, checker.stylist, ); - Some(Fix::suggested(edit).isolate(if parent.is_some() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - })) + Some(Fix::suggested(edit).isolate(checker.isolation(parent))) }; } } diff --git a/crates/ruff/src/rules/pylint/rules/useless_return.rs b/crates/ruff/src/rules/pylint/rules/useless_return.rs index 488f439d0e..82436e976e 100644 --- a/crates/ruff/src/rules/pylint/rules/useless_return.rs +++ b/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); } diff --git a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs index f4970dcd26..b604684c1c 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs +++ b/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() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - })) + Ok(Fix::suggested(edit).isolate(checker.isolation(parent))) }); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs index e57fdd53f5..34cd3e5ca5 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs +++ b/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() { - IsolationLevel::Isolated - } else { - IsolationLevel::NonOverlapping - })) + Ok(Fix::suggested(edit).isolate(checker.isolation(parent))) }); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs b/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs index a575aeaaa9..783bc520c8 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs +++ b/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); } diff --git a/crates/ruff_diagnostics/Cargo.toml b/crates/ruff_diagnostics/Cargo.toml index 8041e75ada..980abbbc51 100644 --- a/crates/ruff_diagnostics/Cargo.toml +++ b/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 = [] } diff --git a/crates/ruff_diagnostics/src/fix.rs b/crates/ruff_diagnostics/src/fix.rs index fc57a856fa..ae54282d04 100644 --- a/crates/ruff_diagnostics/src/fix.rs +++ b/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. - Isolated, + /// The fix should be applied as long as no other fixes in the same group have been applied. + Group(u32), /// The fix should be applied as long as it does not overlap with any other fixes. #[default] NonOverlapping,