From 884e7c92ba9485b1cf8aed9c98e2ba7b3c425ba8 Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Mon, 29 Apr 2024 00:20:54 +0200 Subject: [PATCH] [`refurb`] implement repeated_global (FURB154) lint --- .../resources/test/fixtures/refurb/FURB154.py | 86 ++++++ .../src/checkers/ast/analyze/suite.rs | 5 +- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + .../src/rules/refurb/rules/repeated_global.rs | 117 ++++++++ ...es__refurb__tests__FURB154_FURB154.py.snap | 276 ++++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 488 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB154.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB154.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB154.py new file mode 100644 index 00000000000000..a9d99deded90ee --- /dev/null +++ b/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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/suite.rs b/crates/ruff_linter/src/checkers/ast/analyze/suite.rs index 3cad1b3a1e730e..4407a7ef4f8bb0 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/suite.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/suite.rs @@ -2,11 +2,14 @@ use ruff_python_ast::Stmt; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::flake8_pie; +use crate::rules::{flake8_pie, 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/codes.rs b/crates/ruff_linter/src/codes.rs index 3022fdc79b02da..2503c1d303e554 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 19b17dfaa42a6f..02813d0ac4b20a 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/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"))] diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 9cfd2c9ff97663..acb57bbc55a74c 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs b/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs new file mode 100644 index 00000000000000..7cbc932af78286 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs @@ -0,0 +1,117 @@ +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` (`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 { + match stmt { + Stmt::Global(_) => Some(GlobalKind::Global), + Stmt::Nonlocal(_) => Some(GlobalKind::NonLocal), + _ => None, + } +} + +/// FURB154 +pub(crate) fn repeated_global(checker: &mut Checker, mut suite: &[Stmt]) { + while let Some(idx) = suite + .iter() + .position(|stmt| get_global_kind(stmt).is_some()) + { + let global_kind = get_global_kind(&suite[idx]).unwrap(); + + suite = &suite[idx..]; + let (globals_sequence, next_suite) = suite.split_at( + suite + .iter() + .position(|stmt| get_global_kind(stmt) != Some(global_kind)) + .unwrap_or(suite.len()), + ); + suite = next_suite; + + // 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 new file mode 100644 index 00000000000000..fc6c906d8f86b1 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap @@ -0,0 +1,276 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB154.py:4:5: FURB154 [*] Use of repeated consecutive `global` + | +3 | def f1(): +4 | global x + | _____^ +5 | | global y + | |____________^ FURB154 + | + = help: Merge to one `global` + +ℹ Safe fix +1 1 | # Errors +2 2 | +3 3 | def f1(): +4 |- global x +5 |- global y + 4 |+ global x, y +6 5 | +7 6 | +8 7 | def f3(): + +FURB154.py:9:5: FURB154 [*] Use of repeated consecutive `global` + | + 8 | def f3(): + 9 | global x + | _____^ +10 | | global y +11 | | global z + | |____________^ FURB154 + | + = help: Merge to one `global` + +ℹ Safe fix +6 6 | +7 7 | +8 8 | def f3(): +9 |- global x +10 |- global y +11 |- global z + 9 |+ global x, y, z +12 10 | +13 11 | +14 12 | def f4(): + +FURB154.py:15:5: FURB154 [*] Use of repeated consecutive `global` + | +14 | def f4(): +15 | global x + | _____^ +16 | | global y + | |____________^ FURB154 +17 | pass +18 | global x + | + = help: Merge to one `global` + +ℹ Safe fix +12 12 | +13 13 | +14 14 | def f4(): +15 |- global x +16 |- global y + 15 |+ global x, y +17 16 | pass +18 17 | global x +19 18 | global y + +FURB154.py:18:5: FURB154 [*] Use of repeated consecutive `global` + | +16 | global y +17 | pass +18 | global x + | _____^ +19 | | global y + | |____________^ FURB154 + | + = help: Merge to one `global` + +ℹ Safe fix +15 15 | global x +16 16 | global y +17 17 | pass +18 |- global x +19 |- global y + 18 |+ global x, y +20 19 | +21 20 | +22 21 | def f2(): + +FURB154.py:26:9: FURB154 [*] Use of repeated consecutive `nonlocal` + | +25 | def inner(): +26 | nonlocal x + | _________^ +27 | | nonlocal y + | |__________________^ FURB154 +28 | +29 | def inner2(): + | + = help: Merge to one `nonlocal` + +ℹ Safe fix +23 23 | x = y = z = 1 +24 24 | +25 25 | def inner(): +26 |- nonlocal x +27 |- nonlocal y + 26 |+ nonlocal x, y +28 27 | +29 28 | def inner2(): +30 29 | nonlocal x + +FURB154.py:30:9: FURB154 [*] Use of repeated consecutive `nonlocal` + | +29 | def inner2(): +30 | nonlocal x + | _________^ +31 | | nonlocal y +32 | | nonlocal z + | |__________________^ FURB154 +33 | +34 | def inner3(): + | + = help: Merge to one `nonlocal` + +ℹ Safe fix +27 27 | nonlocal y +28 28 | +29 29 | def inner2(): +30 |- nonlocal x +31 |- nonlocal y +32 |- nonlocal z + 30 |+ nonlocal x, y, z +33 31 | +34 32 | def inner3(): +35 33 | nonlocal x + +FURB154.py:35:9: FURB154 [*] Use of repeated consecutive `nonlocal` + | +34 | def inner3(): +35 | nonlocal x + | _________^ +36 | | nonlocal y + | |__________________^ FURB154 +37 | pass +38 | nonlocal x + | + = help: Merge to one `nonlocal` + +ℹ Safe fix +32 32 | nonlocal z +33 33 | +34 34 | def inner3(): +35 |- nonlocal x +36 |- nonlocal y + 35 |+ nonlocal x, y +37 36 | pass +38 37 | nonlocal x +39 38 | nonlocal y + +FURB154.py:38:9: FURB154 [*] Use of repeated consecutive `nonlocal` + | +36 | nonlocal y +37 | pass +38 | nonlocal x + | _________^ +39 | | nonlocal y + | |__________________^ FURB154 + | + = help: Merge to one `nonlocal` + +ℹ Safe fix +35 35 | nonlocal x +36 36 | nonlocal y +37 37 | pass +38 |- nonlocal x +39 |- nonlocal y + 38 |+ nonlocal x, y +40 39 | +41 40 | +42 41 | def f5(): + +FURB154.py:46:9: FURB154 [*] Use of repeated consecutive `global` + | +45 | def inner(): +46 | global w + | _________^ +47 | | global x + | |________________^ FURB154 +48 | nonlocal y +49 | nonlocal z + | + = help: Merge to one `global` + +ℹ Safe fix +43 43 | w = x = y = z = 1 +44 44 | +45 45 | def inner(): +46 |- global w +47 |- global x + 46 |+ global w, x +48 47 | nonlocal y +49 48 | nonlocal z +50 49 | + +FURB154.py:48:9: FURB154 [*] Use of repeated consecutive `nonlocal` + | +46 | global w +47 | global x +48 | nonlocal y + | _________^ +49 | | nonlocal z + | |__________________^ FURB154 +50 | +51 | def inner2(): + | + = help: Merge to one `nonlocal` + +ℹ Safe fix +45 45 | def inner(): +46 46 | global w +47 47 | global x +48 |- nonlocal y +49 |- nonlocal z + 48 |+ nonlocal y, z +50 49 | +51 50 | def inner2(): +52 51 | global x + +FURB154.py:53:9: FURB154 [*] Use of repeated consecutive `nonlocal` + | +51 | def inner2(): +52 | global x +53 | nonlocal y + | _________^ +54 | | nonlocal z + | |__________________^ FURB154 + | + = help: Merge to one `nonlocal` + +ℹ Safe fix +50 50 | +51 51 | def inner2(): +52 52 | global x +53 |- nonlocal y +54 |- nonlocal z + 53 |+ nonlocal y, z +55 54 | +56 55 | +57 56 | def f6(): + +FURB154.py:58:5: FURB154 [*] Use of repeated consecutive `global` + | +57 | def f6(): +58 | global x, y, z + | _____^ +59 | | global a, b, c +60 | | global d, e, f + | |__________________^ FURB154 + | + = help: Merge to one `global` + +ℹ Safe fix +55 55 | +56 56 | +57 57 | def f6(): +58 |- global x, y, z +59 |- global a, b, c +60 |- global d, e, f + 58 |+ global x, y, z, a, b, c, d, e, f +61 59 | +62 60 | +63 61 | # Ok diff --git a/ruff.schema.json b/ruff.schema.json index 3c0072df3ede29..55f747d37c2c01 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3062,6 +3062,7 @@ "FURB148", "FURB15", "FURB152", + "FURB154", "FURB157", "FURB16", "FURB161",