From 13355c1184b3d23398bca5ff4800e33b05a8ff3a Mon Sep 17 00:00:00 2001 From: boolean-light <61526956+boolean-light@users.noreply.github.com> Date: Thu, 14 Mar 2024 14:32:15 +0900 Subject: [PATCH 1/3] Implement E115 for pylint nonlocal-and-global --- .../fixtures/pylint/nonlocal_and_global.py | 48 ++++++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../rules/pylint/rules/nonlocal_and_global.rs | 62 +++++++++++++++++++ ...tests__PLE0115_nonlocal_and_global.py.snap | 31 ++++++++++ ruff.schema.json | 1 + 8 files changed, 149 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0115_nonlocal_and_global.py.snap 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..34f2807164996 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py @@ -0,0 +1,48 @@ +# These should throw error + +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 + +# These should be OK + +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 191456bf0766b..6761917762b49 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -50,6 +50,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } } + if checker.enabled(Rule::NonlocalAndGlobal) { + pylint::rules::nonlocal_and_global(checker, names); + } } Stmt::Break(_) => { if checker.enabled(Rule::BreakOutsideLoop) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index c471c890a0f1a..b040df1069309 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 0cbd284ddbc8f..1003b78c1b564 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 8f6d40554d2ad..9c53502d11e6c 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::*; @@ -133,6 +134,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..1195911d26318 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs @@ -0,0 +1,62 @@ +use ruff_python_ast::Identifier; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for variables which are both declared as `nonlocal` and `global`. +/// +/// ## Why is this bad? +/// `nonlocal` is included in the scope of `global`, thus this should be replaced with single `global`. +/// +/// ## Example +/// ```python +/// counter = 0 +/// def increment(): +/// global counter +/// nonlocal counter +/// counter += 1 +/// ``` +/// +/// Use instead: +/// ```python +/// counter = 0 +/// def increment(): +/// global counter +/// counter += 1 +/// ``` +/// +/// ## References +/// - [Pylint documentation: `nonlocal-and-global`](https://pylint.readthedocs.io/en/stable/user_guide/messages/error/nonlocal-and-global.html) +/// - [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 nonlocal and global") + } +} + +/// E115 +pub(crate) fn nonlocal_and_global(checker: &mut Checker, names: &Vec) { + // when this function is called, names are all nonlocals. + // thus we check only if the names are global. + for name in names { + if let Some(global_range) = checker.semantic().global(name) { + checker.diagnostics.push(Diagnostic::new( + NonlocalAndGlobal { + name: name.to_string(), + }, + global_range, + )); + } + } +} 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..664abd65f85ae --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0115_nonlocal_and_global.py.snap @@ -0,0 +1,31 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +nonlocal_and_global.py:5:12: PLE0115 Name `counter` is nonlocal and global + | +3 | counter = 0 +4 | def count(): +5 | global counter + | ^^^^^^^ PLE0115 +6 | nonlocal counter +7 | counter += 1 + | + +nonlocal_and_global.py:16:20: PLE0115 Name `counter` is nonlocal and global + | +14 | counter += 1 +15 | else: +16 | global counter + | ^^^^^^^ PLE0115 +17 | counter += 1 + | + +nonlocal_and_global.py:25:16: PLE0115 Name `counter` is nonlocal and global + | +23 | nonlocal counter +24 | counter += 1 +25 | global counter + | ^^^^^^^ PLE0115 +26 | +27 | # These should be OK + | diff --git a/ruff.schema.json b/ruff.schema.json index 7b65b8f158596..e1a227c20a0c7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3208,6 +3208,7 @@ "PLE0100", "PLE0101", "PLE011", + "PLE0115", "PLE0116", "PLE0117", "PLE0118", From 08798e45085f56256d1f4284eb34f81f9e1fa726 Mon Sep 17 00:00:00 2001 From: boolean-light <61526956+boolean-light@users.noreply.github.com> Date: Thu, 14 Mar 2024 15:02:10 +0900 Subject: [PATCH 2/3] Fixed docs --- .../ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs | 4 ++++ 1 file changed, 4 insertions(+) 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 index 1195911d26318..2cd2b5e1b43b8 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs @@ -14,6 +14,8 @@ use crate::checkers::ast::Checker; /// ## Example /// ```python /// counter = 0 +/// +/// /// def increment(): /// global counter /// nonlocal counter @@ -23,6 +25,8 @@ use crate::checkers::ast::Checker; /// Use instead: /// ```python /// counter = 0 +/// +/// /// def increment(): /// global counter /// counter += 1 From 2c142d8d1e8014f6f3e9e60416d7125b1887f9d4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 17 Mar 2024 20:36:01 -0400 Subject: [PATCH 3/3] Tweaks --- .../fixtures/pylint/nonlocal_and_global.py | 23 ++++++++++- .../src/checkers/ast/analyze/statement.rs | 4 +- .../rules/pylint/rules/nonlocal_and_global.rs | 28 +++++++------ ...tests__PLE0115_nonlocal_and_global.py.snap | 40 +++++++++++-------- 4 files changed, 62 insertions(+), 33 deletions(-) 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 index 34f2807164996..dd146e17c00c5 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/nonlocal_and_global.py @@ -1,13 +1,17 @@ -# These should throw error +# 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 @@ -16,29 +20,44 @@ def count(counter_type): global counter counter += 1 + def count(): counter = 0 + def count_twice(): for i in range(2): nonlocal counter counter += 1 global counter -# These should be OK + +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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 5e9bbb8d8d7b3..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()) @@ -51,7 +51,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } if checker.enabled(Rule::NonlocalAndGlobal) { - pylint::rules::nonlocal_and_global(checker, names); + pylint::rules::nonlocal_and_global(checker, nonlocal); } } Stmt::Break(_) => { 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 index 2cd2b5e1b43b8..6d7d25736a025 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_and_global.rs @@ -1,15 +1,20 @@ -use ruff_python_ast::Identifier; - 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 `nonlocal` and `global`. +/// Checks for variables which are both declared as both `nonlocal` and +/// `global`. /// /// ## Why is this bad? -/// `nonlocal` is included in the scope of `global`, thus this should be replaced with single `global`. +/// 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 @@ -33,7 +38,6 @@ use crate::checkers::ast::Checker; /// ``` /// /// ## References -/// - [Pylint documentation: `nonlocal-and-global`](https://pylint.readthedocs.io/en/stable/user_guide/messages/error/nonlocal-and-global.html) /// - [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] @@ -45,21 +49,21 @@ impl Violation for NonlocalAndGlobal { #[derive_message_formats] fn message(&self) -> String { let NonlocalAndGlobal { name } = self; - format!("Name `{name}` is nonlocal and global") + format!("Name `{name}` is both `nonlocal` and `global`") } } /// E115 -pub(crate) fn nonlocal_and_global(checker: &mut Checker, names: &Vec) { - // when this function is called, names are all nonlocals. - // thus we check only if the names are global. - for name in names { - if let Some(global_range) = checker.semantic().global(name) { +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_range, + 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 index 664abd65f85ae..9ea0e9ace3249 100644 --- 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 @@ -1,31 +1,37 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -nonlocal_and_global.py:5:12: PLE0115 Name `counter` is nonlocal and global +nonlocal_and_global.py:7:12: PLE0115 Name `counter` is both `nonlocal` and `global` | -3 | counter = 0 -4 | def count(): -5 | global counter +6 | def count(): +7 | global counter | ^^^^^^^ PLE0115 -6 | nonlocal counter -7 | counter += 1 +8 | nonlocal counter +9 | counter += 1 | -nonlocal_and_global.py:16:20: PLE0115 Name `counter` is nonlocal and global +nonlocal_and_global.py:20:20: PLE0115 Name `counter` is both `nonlocal` and `global` | -14 | counter += 1 -15 | else: -16 | global counter +18 | counter += 1 +19 | else: +20 | global counter | ^^^^^^^ PLE0115 -17 | counter += 1 +21 | counter += 1 | -nonlocal_and_global.py:25:16: PLE0115 Name `counter` is nonlocal and global +nonlocal_and_global.py:31:16: PLE0115 Name `counter` is both `nonlocal` and `global` | -23 | nonlocal counter -24 | counter += 1 -25 | global counter +29 | nonlocal counter +30 | counter += 1 +31 | global counter | ^^^^^^^ PLE0115 -26 | -27 | # These should be OK + | + +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 |