[refurb] Implement repeated-global (FURB154)

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/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);
+    }
 }

crates/ruff_linter/src/codes.rs

@@ -1073,6 +1073,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,125 @@
+use itertools::Itertools;
+
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::Stmt;
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for consecutive `global` (or `nonlocal`) statements.
+///
+/// ## Why is this bad?
+/// 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 func():
+///     global x
+///     global y
+///
+///     print(x, y)
+/// ```
+///
+/// Use instead:
+/// ```python
+/// def 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 `{}` statements", self.global_kind)
+    }
+}
+
+/// FURB154
+pub(crate) fn repeated_global(checker: &mut Checker, mut suite: &[Stmt]) {
+    while let Some(idx) = suite
+        .iter()
+        .position(|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;
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+enum GlobalKind {
+    Global,
+    NonLocal,
+}
+
+impl GlobalKind {
+    fn from_stmt(stmt: &Stmt) -> Option<Self> {
+        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 {
+            GlobalKind::Global => write!(f, "global"),
+            GlobalKind::NonLocal => write!(f, "nonlocal"),
+        }
+    }
+}