diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 1217b6c9c8be69..0190de443528c2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -29,9 +29,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pycodestyle::rules::ambiguous_variable_name(name, name.range()) })); } - if checker.enabled(Rule::RepeatedGlobal) { - refurb::rules::repeated_global(checker, stmt); - } } Stmt::Nonlocal(nonlocal @ ast::StmtNonlocal { names, range: _ }) => { if checker.enabled(Rule::AmbiguousVariableName) { @@ -56,9 +53,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::NonlocalAndGlobal) { pylint::rules::nonlocal_and_global(checker, nonlocal); } - if checker.enabled(Rule::RepeatedGlobal) { - refurb::rules::repeated_global(checker, stmt); - } } Stmt::Break(_) => { if checker.enabled(Rule::BreakOutsideLoop) { diff --git a/crates/ruff_linter/src/checkers/ast/analyze/suite.rs b/crates/ruff_linter/src/checkers/ast/analyze/suite.rs index 3cad1b3a1e730e..c004ce85ca7f1d 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/suite.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/suite.rs @@ -3,10 +3,14 @@ use ruff_python_ast::Stmt; use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::rules::flake8_pie; +use crate::rules::refurb; /// Run lint rules over a suite of [`Stmt`] syntax nodes. pub(crate) fn suite(suite: &[Stmt], checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryPlaceholder) { flake8_pie::rules::unnecessary_placeholder(checker, suite); } + if checker.enabled(Rule::RepeatedGlobal) { + refurb::rules::repeated_global(checker, suite); + } } diff --git a/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs b/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs index 3c33d091754195..44c1f4dd444192 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs @@ -2,22 +2,23 @@ use itertools::Itertools; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{traversal, Stmt}; -use ruff_python_semantic::SemanticModel; +use ruff_python_ast::Stmt; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for consecutive `global` (`nonlocal`) statements. +/// Checks for consecutive `global` (or `nonlocal`) statements. /// /// ## Why is this bad? -/// The `global` and `nonlocal` keywords can take multiple comma-separated names, removing the need -/// for multiple lines. +/// The `global` and `nonlocal` keywords accepts multiple comma-separated names. +/// Instead of using multiple `global` (or `nonlocal`) statements for separate +/// variables, you can use a single statement to declare multiple variables at +/// once. /// /// ## Example /// ```python -/// def some_func(): +/// def func(): /// global x /// global y /// @@ -26,7 +27,7 @@ use crate::checkers::ast::Checker; /// /// Use instead: /// ```python -/// def some_func(): +/// def func(): /// global x, y /// /// print(x, y) @@ -47,7 +48,54 @@ impl AlwaysFixableViolation for RepeatedGlobal { } fn fix_title(&self) -> String { - format!("Merge to one `{}`", self.global_kind) + format!("Merge `{}` statements", self.global_kind) + } +} + +/// FURB154 +pub(crate) fn repeated_global(checker: &mut Checker, mut suite: &[Stmt]) { + while let Some(idx) = suite + .iter() + .find(|stmt| GlobalKind::from_stmt(stmt).is_some()) + { + let global_kind = GlobalKind::from_stmt(&suite[idx]).unwrap(); + + suite = &suite[idx..]; + + // Collect until we see a non-`global` (or non-`nonlocal`) statement. + let (globals_sequence, next_suite) = suite.split_at( + suite + .iter() + .position(|stmt| GlobalKind::from_stmt(stmt) != Some(global_kind)) + .unwrap_or(suite.len()), + ); + + // If there are at least two consecutive `global` (or `nonlocal`) statements, raise a + // diagnostic. + if let [first, .., last] = globals_sequence { + let range = first.range().cover(last.range()); + checker.diagnostics.push( + Diagnostic::new(RepeatedGlobal { global_kind }, range).with_fix(Fix::safe_edit( + Edit::range_replacement( + format!( + "{global_kind} {}", + globals_sequence + .iter() + .flat_map(|stmt| match stmt { + Stmt::Global(stmt) => &stmt.names, + Stmt::Nonlocal(stmt) => &stmt.names, + _ => unreachable!(), + }) + .map(|identifier| &identifier.id) + .format(", ") + ), + range, + ), + )), + ); + } + + suite = next_suite; } } @@ -57,6 +105,16 @@ enum GlobalKind { NonLocal, } +impl GlobalKind { + fn from_stmt(stmt: &Stmt) -> Option { + match stmt { + Stmt::Global(_) => Some(GlobalKind::Global), + Stmt::Nonlocal(_) => Some(GlobalKind::NonLocal), + _ => None, + } + } +} + impl std::fmt::Display for GlobalKind { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -65,69 +123,3 @@ impl std::fmt::Display for GlobalKind { } } } - -fn get_global_kind(stmt: &Stmt) -> Option { - match stmt { - Stmt::Global(_) => Some(GlobalKind::Global), - Stmt::Nonlocal(_) => Some(GlobalKind::NonLocal), - _ => None, - } -} - -fn match_globals_sequence<'a>( - semantic: &'a SemanticModel<'a>, - stmt: &'a Stmt, -) -> Option<(GlobalKind, &'a [Stmt])> { - let global_kind = get_global_kind(stmt)?; - - let siblings = if semantic.at_top_level() { - semantic.definitions.python_ast()? - } else { - semantic - .current_statement_parent() - .and_then(|parent| traversal::suite(stmt, parent))? - }; - let stmt_idx = siblings.iter().position(|sibling| sibling == stmt)?; - if stmt_idx != 0 && get_global_kind(&siblings[stmt_idx - 1]) == Some(global_kind) { - return None; - } - let siblings = &siblings[stmt_idx..]; - Some(( - global_kind, - siblings - .iter() - .position(|sibling| get_global_kind(sibling) != Some(global_kind)) - .map_or(siblings, |size| &siblings[..size]), - )) -} - -/// FURB154 -pub(crate) fn repeated_global(checker: &mut Checker, stmt: &Stmt) { - let Some((global_kind, globals_sequence)) = match_globals_sequence(checker.semantic(), stmt) - else { - return; - }; - // if there are at least 2 consecutive `global` (`nonlocal`) statements - if let [first, .., last] = globals_sequence { - let range = first.range().cover(last.range()); - checker.diagnostics.push( - Diagnostic::new(RepeatedGlobal { global_kind }, range).with_fix(Fix::safe_edit( - Edit::range_replacement( - format!( - "{global_kind} {}", - globals_sequence - .iter() - .flat_map(|stmt| match stmt { - Stmt::Global(stmt) => &stmt.names, - Stmt::Nonlocal(stmt) => &stmt.names, - _ => unreachable!(), - }) - .map(|identifier| &identifier.id) - .format(", ") - ), - range, - ), - )), - ); - } -} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap index fc6c906d8f86b1..a935e1ae3c4fe9 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap @@ -9,7 +9,7 @@ FURB154.py:4:5: FURB154 [*] Use of repeated consecutive `global` 5 | | global y | |____________^ FURB154 | - = help: Merge to one `global` + = help: Merge `global` statements ℹ Safe fix 1 1 | # Errors @@ -31,7 +31,7 @@ FURB154.py:9:5: FURB154 [*] Use of repeated consecutive `global` 11 | | global z | |____________^ FURB154 | - = help: Merge to one `global` + = help: Merge `global` statements ℹ Safe fix 6 6 | @@ -55,7 +55,7 @@ FURB154.py:15:5: FURB154 [*] Use of repeated consecutive `global` 17 | pass 18 | global x | - = help: Merge to one `global` + = help: Merge `global` statements ℹ Safe fix 12 12 | @@ -77,7 +77,7 @@ FURB154.py:18:5: FURB154 [*] Use of repeated consecutive `global` 19 | | global y | |____________^ FURB154 | - = help: Merge to one `global` + = help: Merge `global` statements ℹ Safe fix 15 15 | global x @@ -100,7 +100,7 @@ FURB154.py:26:9: FURB154 [*] Use of repeated consecutive `nonlocal` 28 | 29 | def inner2(): | - = help: Merge to one `nonlocal` + = help: Merge `nonlocal` statements ℹ Safe fix 23 23 | x = y = z = 1 @@ -124,7 +124,7 @@ FURB154.py:30:9: FURB154 [*] Use of repeated consecutive `nonlocal` 33 | 34 | def inner3(): | - = help: Merge to one `nonlocal` + = help: Merge `nonlocal` statements ℹ Safe fix 27 27 | nonlocal y @@ -148,7 +148,7 @@ FURB154.py:35:9: FURB154 [*] Use of repeated consecutive `nonlocal` 37 | pass 38 | nonlocal x | - = help: Merge to one `nonlocal` + = help: Merge `nonlocal` statements ℹ Safe fix 32 32 | nonlocal z @@ -170,7 +170,7 @@ FURB154.py:38:9: FURB154 [*] Use of repeated consecutive `nonlocal` 39 | | nonlocal y | |__________________^ FURB154 | - = help: Merge to one `nonlocal` + = help: Merge `nonlocal` statements ℹ Safe fix 35 35 | nonlocal x @@ -193,7 +193,7 @@ FURB154.py:46:9: FURB154 [*] Use of repeated consecutive `global` 48 | nonlocal y 49 | nonlocal z | - = help: Merge to one `global` + = help: Merge `global` statements ℹ Safe fix 43 43 | w = x = y = z = 1 @@ -217,7 +217,7 @@ FURB154.py:48:9: FURB154 [*] Use of repeated consecutive `nonlocal` 50 | 51 | def inner2(): | - = help: Merge to one `nonlocal` + = help: Merge `nonlocal` statements ℹ Safe fix 45 45 | def inner(): @@ -239,7 +239,7 @@ FURB154.py:53:9: FURB154 [*] Use of repeated consecutive `nonlocal` 54 | | nonlocal z | |__________________^ FURB154 | - = help: Merge to one `nonlocal` + = help: Merge `nonlocal` statements ℹ Safe fix 50 50 | @@ -261,7 +261,7 @@ FURB154.py:58:5: FURB154 [*] Use of repeated consecutive `global` 60 | | global d, e, f | |__________________^ FURB154 | - = help: Merge to one `global` + = help: Merge `global` statements ℹ Safe fix 55 55 |