Skip to content

Commit

Permalink
Revert back
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Jun 8, 2024
1 parent 333b7f4 commit aec5dc0
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 92 deletions.
6 changes: 0 additions & 6 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/suite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
140 changes: 66 additions & 74 deletions crates/ruff_linter/src/rules/refurb/rules/repeated_global.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand All @@ -26,7 +27,7 @@ use crate::checkers::ast::Checker;
///
/// Use instead:
/// ```python
/// def some_func():
/// def func():
/// global x, y
///
/// print(x, y)
Expand All @@ -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;
}
}

Expand All @@ -57,6 +105,16 @@ enum GlobalKind {
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 {
Expand All @@ -65,69 +123,3 @@ impl std::fmt::Display for GlobalKind {
}
}
}

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,
),
)),
);
}
}
24 changes: 12 additions & 12 deletions ...ter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB154_FURB154.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 |
Expand All @@ -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 |
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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():
Expand All @@ -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 |
Expand All @@ -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 |
Expand Down

0 comments on commit aec5dc0

Please sign in to comment.