diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py b/crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py new file mode 100644 index 0000000000000..dd146e17c00c5 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py @@ -0,0 +1,67 @@ +# Positive cases + +counter = 0 + + +def count(): + global counter + nonlocal counter + counter += 1 + + +def count(): + counter = 0 + + def count(counter_type): + if counter_type == "nonlocal": + nonlocal counter + counter += 1 + else: + global counter + counter += 1 + + +def count(): + counter = 0 + + def count_twice(): + for i in range(2): + nonlocal counter + counter += 1 + global counter + + +def count(): + nonlocal counter + global counter + counter += 1 + + +# Negative cases + +counter = 0 + + +def count(): + global counter + counter += 1 + + +def count(): + counter = 0 + + def count_local(): + nonlocal counter + counter += 1 + + +def count(): + counter = 0 + + def count_local(): + nonlocal counter + counter += 1 + + def count_global(): + global counter + counter += 1 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index d4f6287bf95e9..3c1dc40867b8d 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -30,7 +30,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { })); } } - Stmt::Nonlocal(ast::StmtNonlocal { names, range: _ }) => { + Stmt::Nonlocal(nonlocal @ ast::StmtNonlocal { names, range: _ }) => { if checker.enabled(Rule::AmbiguousVariableName) { checker.diagnostics.extend(names.iter().filter_map(|name| { pycodestyle::rules::ambiguous_variable_name(name, name.range()) @@ -50,6 +50,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } } + if checker.enabled(Rule::NonlocalAndGlobal) { + pylint::rules::nonlocal_and_global(checker, nonlocal); + } } Stmt::Break(_) => { if checker.enabled(Rule::BreakOutsideLoop) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index e6900d54d24c5..98879a0fa6152 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -234,6 +234,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C3002") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryDirectLambdaCall), (Pylint, "E0100") => (RuleGroup::Stable, rules::pylint::rules::YieldInInit), (Pylint, "E0101") => (RuleGroup::Stable, rules::pylint::rules::ReturnInInit), + (Pylint, "E0115") => (RuleGroup::Preview, rules::pylint::rules::NonlocalAndGlobal), (Pylint, "E0116") => (RuleGroup::Stable, rules::pylint::rules::ContinueInFinally), (Pylint, "E0117") => (RuleGroup::Stable, rules::pylint::rules::NonlocalWithoutBinding), (Pylint, "E0118") => (RuleGroup::Stable, rules::pylint::rules::LoadBeforeGlobalDeclaration), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index ebd20c35ff90c..cdae4961410e2 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -92,6 +92,7 @@ mod tests { Rule::NamedExprWithoutContext, Path::new("named_expr_without_context.py") )] + #[test_case(Rule::NonlocalAndGlobal, Path::new("nonlocal_and_global.py"))] #[test_case(Rule::NonlocalWithoutBinding, Path::new("nonlocal_without_binding.py"))] #[test_case(Rule::NonSlotAssignment, Path::new("non_slot_assignment.py"))] #[test_case(Rule::PropertyWithParameters, Path::new("property_with_parameters.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 0f7c0e9e983ea..93b7bf466ac9c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -44,6 +44,7 @@ pub(crate) use no_self_use::*; pub(crate) use non_ascii_module_import::*; pub(crate) use non_ascii_name::*; pub(crate) use non_slot_assignment::*; +pub(crate) use nonlocal_and_global::*; pub(crate) use nonlocal_without_binding::*; pub(crate) use potential_index_error::*; pub(crate) use property_with_parameters::*; @@ -134,6 +135,7 @@ mod no_self_use; mod non_ascii_module_import; mod non_ascii_name; mod non_slot_assignment; +mod nonlocal_and_global; mod nonlocal_without_binding; mod potential_index_error; mod property_with_parameters; diff --git a/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs new file mode 100644 index 0000000000000..6d7d25736a025 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs @@ -0,0 +1,70 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for variables which are both declared as both `nonlocal` and +/// `global`. +/// +/// ## Why is this bad? +/// A `nonlocal` variable is a variable that is defined in the nearest +/// enclosing scope, but not in the global scope, while a `global` variable is +/// a variable that is defined in the global scope. +/// +/// Declaring a variable as both `nonlocal` and `global` is contradictory and +/// will raise a `SyntaxError`. +/// +/// ## Example +/// ```python +/// counter = 0 +/// +/// +/// def increment(): +/// global counter +/// nonlocal counter +/// counter += 1 +/// ``` +/// +/// Use instead: +/// ```python +/// counter = 0 +/// +/// +/// def increment(): +/// global counter +/// counter += 1 +/// ``` +/// +/// ## 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#nonlocal) +#[violation] +pub struct NonlocalAndGlobal { + pub(crate) name: String, +} + +impl Violation for NonlocalAndGlobal { + #[derive_message_formats] + fn message(&self) -> String { + let NonlocalAndGlobal { name } = self; + format!("Name `{name}` is both `nonlocal` and `global`") + } +} + +/// E115 +pub(crate) fn nonlocal_and_global(checker: &mut Checker, nonlocal: &ast::StmtNonlocal) { + // Determine whether any of the newly declared `nonlocal` variables are already declared as + // `global`. + for name in &nonlocal.names { + if let Some(global) = checker.semantic().global(name) { + checker.diagnostics.push(Diagnostic::new( + NonlocalAndGlobal { + name: name.to_string(), + }, + global, + )); + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0115_nonlocal_and_global.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0115_nonlocal_and_global.py.snap new file mode 100644 index 0000000000000..9ea0e9ace3249 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0115_nonlocal_and_global.py.snap @@ -0,0 +1,37 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +nonlocal_and_global.py:7:12: PLE0115 Name `counter` is both `nonlocal` and `global` + | +6 | def count(): +7 | global counter + | ^^^^^^^ PLE0115 +8 | nonlocal counter +9 | counter += 1 + | + +nonlocal_and_global.py:20:20: PLE0115 Name `counter` is both `nonlocal` and `global` + | +18 | counter += 1 +19 | else: +20 | global counter + | ^^^^^^^ PLE0115 +21 | counter += 1 + | + +nonlocal_and_global.py:31:16: PLE0115 Name `counter` is both `nonlocal` and `global` + | +29 | nonlocal counter +30 | counter += 1 +31 | global counter + | ^^^^^^^ PLE0115 + | + +nonlocal_and_global.py:36:12: PLE0115 Name `counter` is both `nonlocal` and `global` + | +34 | def count(): +35 | nonlocal counter +36 | global counter + | ^^^^^^^ PLE0115 +37 | counter += 1 + | diff --git a/ruff.schema.json b/ruff.schema.json index 9f06af7bc68b0..ae9ec00f86728 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3208,6 +3208,7 @@ "PLE0100", "PLE0101", "PLE011", + "PLE0115", "PLE0116", "PLE0117", "PLE0118",