[refurb] implement repeated_global (FURB154) lint

crates/ruff_linter/resources/test/fixtures/refurb/FURB154.py

@@ -0,0 +1,86 @@
+# Errors
+
+def f1():
+    global x
+    global y
+
+
+def f3():
+    global x
+    global y
+    global z
+
+
+def f4():
+    global x
+    global y
+    pass
+    global x
+    global y
+
+
+def f2():
+    x = y = z = 1
+
+    def inner():
+        nonlocal x
+        nonlocal y
+
+    def inner2():
+        nonlocal x
+        nonlocal y
+        nonlocal z
+
+    def inner3():
+        nonlocal x
+        nonlocal y
+        pass
+        nonlocal x
+        nonlocal y
+
+
+def f5():
+    w = x = y = z = 1
+
+    def inner():
+        global w
+        global x
+        nonlocal y
+        nonlocal z
+
+    def inner2():
+        global x
+        nonlocal y
+        nonlocal z
+
+
+def f6():
+    global x, y, z
+    global a, b, c
+    global d, e, f
+
+
+# Ok
+
+def fx():
+    x = y = 1
+
+    def inner():
+        global x
+        nonlocal y
+
+    def inner2():
+        nonlocal x
+        pass
+        nonlocal y
+
+
+def fy():
+    global x
+    pass
+    global y
+
+
+def fz():
+    pass
+    global x

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -29,6 +29,9 @@ 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) {
@@ -53,6 +56,9 @@ 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) {

crates/ruff_linter/src/codes.rs

@@ -1064,6 +1064,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
         (Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),
         (Refurb, "152") => (RuleGroup::Preview, rules::refurb::rules::MathConstant),
+        (Refurb, "154") => (RuleGroup::Preview, rules::refurb::rules::RepeatedGlobal),
         (Refurb, "157") => (RuleGroup::Preview, rules::refurb::rules::VerboseDecimalConstructor),
         (Refurb, "161") => (RuleGroup::Preview, rules::refurb::rules::BitCount),
         (Refurb, "163") => (RuleGroup::Preview, rules::refurb::rules::RedundantLogBase),

crates/ruff_linter/src/rules/refurb/mod.rs

@@ -27,6 +27,7 @@ mod tests {
     #[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
     #[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))]
     #[test_case(Rule::MathConstant, Path::new("FURB152.py"))]
+    #[test_case(Rule::RepeatedGlobal, Path::new("FURB154.py"))]
     #[test_case(Rule::VerboseDecimalConstructor, Path::new("FURB157.py"))]
     #[test_case(Rule::UnnecessaryFromFloat, Path::new("FURB164.py"))]
     #[test_case(Rule::PrintEmptyString, Path::new("FURB105.py"))]

crates/ruff_linter/src/rules/refurb/rules/mod.rs

@@ -20,6 +20,7 @@ pub(crate) use regex_flag_alias::*;
 pub(crate) use reimplemented_operator::*;
 pub(crate) use reimplemented_starmap::*;
 pub(crate) use repeated_append::*;
+pub(crate) use repeated_global::*;
 pub(crate) use single_item_membership_test::*;
 pub(crate) use slice_copy::*;
 pub(crate) use sorted_min_max::*;
@@ -51,6 +52,7 @@ mod regex_flag_alias;
 mod reimplemented_operator;
 mod reimplemented_starmap;
 mod repeated_append;
+mod repeated_global;
 mod single_item_membership_test;
 mod slice_copy;
 mod sorted_min_max;

crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs

@@ -0,0 +1,133 @@
+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_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for consecutive `global` (`nonlocal`) statements.
+///
+/// ## Why is this bad?
+/// The `global` and `nonlocal` keywords can take multiple comma-separated names, removing the need
+/// for multiple lines.
+///
+/// ## Example
+/// ```python
+/// def some_func():
+///     global x
+///     global y
+///
+///     print(x, y)
+/// ```
+///
+/// Use instead:
+/// ```python
+/// def some_func():
+///     global x, y
+///
+///     print(x, y)
+/// ```
+///
+/// ## References
+/// - [Python documentation: the `global` statement](https://docs.python.org/3/reference/simple_stmts.html#the-global-statement)
+/// - [Python documentation: the `nonlocal` statement](https://docs.python.org/3/reference/simple_stmts.html#the-nonlocal-statement)
+#[violation]
+pub struct RepeatedGlobal {
+    global_kind: GlobalKind,
+}
+
+impl AlwaysFixableViolation for RepeatedGlobal {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Use of repeated consecutive `{}`", self.global_kind)
+    }
+
+    fn fix_title(&self) -> String {
+        format!("Merge to one `{}`", self.global_kind)
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+enum GlobalKind {
+    Global,
+    NonLocal,
+}
+
+impl std::fmt::Display for GlobalKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            GlobalKind::Global => write!(f, "global"),
+            GlobalKind::NonLocal => write!(f, "nonlocal"),
+        }
+    }
+}
+
+fn get_global_kind(stmt: &Stmt) -> Option<GlobalKind> {
+    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,
+                ),
+            )),
+        );
+    }
+}