From 4b5538f74e62a799727f56d25b44b4f35b70f80d Mon Sep 17 00:00:00 2001 From: Ivan Gozali Date: Sun, 26 Feb 2023 15:40:24 -0800 Subject: [PATCH] [`pylint`] W0603: `global-statement` (#3227) Implements pylint rule [W0603: global-statement](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/global-statement.html). Currently checks for global statement usage in a few StmtKinds (as tested in the `pylint` `global-statement` test case [here](https://github.com/PyCQA/pylint/blob/b70d2abd7fabe9bfd735a30b593b9cd5eaa36194/tests/functional/g/globals.py)): * Assign * AugAssign * ClassDef * FunctionDef | AsyncFunctionDef * Import * ImportFrom * Delete --- .../test/fixtures/pylint/global_statement.py | 75 +++++++++++++++ crates/ruff/src/checkers/ast.rs | 52 ++++++++++- crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/global_statement.rs | 74 +++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ...t__tests__PLW0603_global_statement.py.snap | 93 +++++++++++++++++++ ruff.schema.json | 1 + 9 files changed, 299 insertions(+), 1 deletion(-) create mode 100644 crates/ruff/resources/test/fixtures/pylint/global_statement.py create mode 100644 crates/ruff/src/rules/pylint/rules/global_statement.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0603_global_statement.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/global_statement.py b/crates/ruff/resources/test/fixtures/pylint/global_statement.py new file mode 100644 index 0000000000000..69bcc36775208 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/global_statement.py @@ -0,0 +1,75 @@ +# Adapted from: +# https://github.com/PyCQA/pylint/blob/b70d2abd7fabe9bfd735a30b593b9cd5eaa36194/tests/functional/g/globals.py + +CONSTANT = 1 + + +def FUNC(): + pass + + +class CLASS: + pass + + +def fix_constant(value): + """All this is ok, but try not to use `global` ;)""" + global CONSTANT # [global-statement] + print(CONSTANT) + CONSTANT = value + + +def global_with_import(): + """Should only warn for global-statement when using `Import` node""" + global sys # [global-statement] + import sys + + +def global_with_import_from(): + """Should only warn for global-statement when using `ImportFrom` node""" + global namedtuple # [global-statement] + from collections import namedtuple + + +def global_del(): + """Deleting the global name prevents `global-variable-not-assigned`""" + global CONSTANT # [global-statement] + print(CONSTANT) + del CONSTANT + + +def global_operator_assign(): + """Operator assigns should only throw a global statement error""" + global CONSTANT # [global-statement] + print(CONSTANT) + CONSTANT += 1 + + +def global_function_assign(): + """Function assigns should only throw a global statement error""" + global CONSTANT # [global-statement] + + def CONSTANT(): + pass + + CONSTANT() + + +def override_func(): + """Overriding a function should only throw a global statement error""" + global FUNC # [global-statement] + + def FUNC(): + pass + + FUNC() + + +def override_class(): + """Overriding a class should only throw a global statement error""" + global CLASS # [global-statement] + + class CLASS: + pass + + CLASS() diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 4141839b5e4bf..3e329dabab992 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -556,6 +556,10 @@ where } } + if self.settings.rules.enabled(&Rule::GlobalStatement) { + pylint::rules::global_statement(self, name); + } + if self .settings .rules @@ -834,6 +838,9 @@ where self.diagnostics.push(diagnostic); } } + if self.settings.rules.enabled(&Rule::GlobalStatement) { + pylint::rules::global_statement(self, name); + } if self.settings.rules.enabled(&Rule::UselessObjectInheritance) { pyupgrade::rules::useless_object_inheritance(self, stmt, name, bases, keywords); } @@ -937,6 +944,17 @@ where { pycodestyle::rules::module_import_not_at_top_of_file(self, stmt); } + + if self.settings.rules.enabled(&Rule::GlobalStatement) { + for name in names.iter() { + if let Some(asname) = name.node.asname.as_ref() { + pylint::rules::global_statement(self, asname); + } else { + pylint::rules::global_statement(self, &name.node.name); + } + } + } + if self.settings.rules.enabled(&Rule::RewriteCElementTree) { pyupgrade::rules::replace_c_element_tree(self, stmt); } @@ -1194,6 +1212,16 @@ where pycodestyle::rules::module_import_not_at_top_of_file(self, stmt); } + if self.settings.rules.enabled(&Rule::GlobalStatement) { + for name in names.iter() { + if let Some(asname) = name.node.asname.as_ref() { + pylint::rules::global_statement(self, asname); + } else { + pylint::rules::global_statement(self, &name.node.name); + } + } + } + if self.settings.rules.enabled(&Rule::UnnecessaryFutureImport) && self.settings.target_version >= PythonVersion::Py37 { @@ -1576,6 +1604,12 @@ where } StmtKind::AugAssign { target, .. } => { self.handle_node_load(target); + + if self.settings.rules.enabled(&Rule::GlobalStatement) { + if let ExprKind::Name { id, .. } = &target.node { + pylint::rules::global_statement(self, id); + } + } } StmtKind::If { test, body, orelse } => { if self.settings.rules.enabled(&Rule::IfTuple) { @@ -1846,6 +1880,14 @@ where } } + if self.settings.rules.enabled(&Rule::GlobalStatement) { + for target in targets.iter() { + if let ExprKind::Name { id, .. } = &target.node { + pylint::rules::global_statement(self, id); + } + } + } + if self.settings.rules.enabled(&Rule::UselessMetaclassType) { pyupgrade::rules::useless_metaclass_type(self, stmt, value, targets); } @@ -1896,7 +1938,15 @@ where ); } } - StmtKind::Delete { .. } => {} + StmtKind::Delete { targets } => { + if self.settings.rules.enabled(&Rule::GlobalStatement) { + for target in targets.iter() { + if let ExprKind::Name { id, .. } = &target.node { + pylint::rules::global_statement(self, id); + } + } + } + } StmtKind::Expr { value, .. } => { if self.settings.rules.enabled(&Rule::UselessComparison) { flake8_bugbear::rules::useless_comparison(self, value); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 3cbfd78cfb385..76174eab4d1b6 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -149,6 +149,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "R2004") => Rule::MagicValueComparison, (Pylint, "W0120") => Rule::UselessElseOnLoop, (Pylint, "W0602") => Rule::GlobalVariableNotAssigned, + (Pylint, "W0603") => Rule::GlobalStatement, (Pylint, "R0911") => Rule::TooManyReturnStatements, (Pylint, "R0913") => Rule::TooManyArguments, (Pylint, "R0912") => Rule::TooManyBranches, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index ea1202f4d2769..f9016e36ba073 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -149,6 +149,7 @@ ruff_macros::register_rules!( rules::pylint::rules::ConsiderUsingSysExit, rules::pylint::rules::MagicValueComparison, rules::pylint::rules::UselessElseOnLoop, + rules::pylint::rules::GlobalStatement, rules::pylint::rules::GlobalVariableNotAssigned, rules::pylint::rules::TooManyReturnStatements, rules::pylint::rules::TooManyArguments, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index f7cf12679dac4..631312f717bb7 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -39,6 +39,7 @@ mod tests { #[test_case(Rule::MagicValueComparison, Path::new("magic_value_comparison.py"); "PLR2004")] #[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"); "PLW0120")] #[test_case(Rule::GlobalVariableNotAssigned, Path::new("global_variable_not_assigned.py"); "PLW0602")] + #[test_case(Rule::GlobalStatement, Path::new("global_statement.py"); "PLW0603")] #[test_case(Rule::InvalidAllFormat, Path::new("invalid_all_format.py"); "PLE0605")] #[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"); "PLE0604")] #[test_case(Rule::TooManyReturnStatements, Path::new("too_many_return_statements.py"); "PLR0911")] diff --git a/crates/ruff/src/rules/pylint/rules/global_statement.rs b/crates/ruff/src/rules/pylint/rules/global_statement.rs new file mode 100644 index 0000000000000..39c2e5e312855 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/global_statement.rs @@ -0,0 +1,74 @@ +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::checkers::ast::Checker; +use crate::registry::Diagnostic; +use crate::violation::Violation; +use crate::Range; + +define_violation!( + /// ## What it does + /// Checks for the use of `global` statements to update identifiers. + /// + /// ## Why is this bad? + /// Pylint discourages the use of `global` variables as global mutable + /// state is a common source of bugs and confusing behavior. + /// + /// ## Example + /// ```python + /// var = 1 + /// + /// def foo(): + /// global var # [global-statement] + /// var = 10 + /// print(var) + /// + /// foo() + /// print(var) + /// ``` + /// + /// Use instead: + /// ```python + /// var = 1 + /// + /// def foo(): + /// print(var) + /// return 10 + /// + /// var = foo() + /// print(var) + /// ``` + pub struct GlobalStatement { + pub name: String, + } +); +impl Violation for GlobalStatement { + #[derive_message_formats] + fn message(&self) -> String { + let GlobalStatement { name } = self; + format!("Using the global statement to update `{name}` is discouraged") + } +} + +/// PLW0603 +pub fn global_statement(checker: &mut Checker, name: &str) { + let scope = checker.current_scope(); + if let Some(index) = scope.bindings.get(name) { + let binding = &checker.bindings[*index]; + if binding.kind.is_global() { + let diagnostic = Diagnostic::new( + GlobalStatement { + name: name.to_string(), + }, + // Match Pylint's behavior by reporting on the `global` statement`, rather + // than the variable usage. + Range::from_located( + binding + .source + .as_ref() + .expect("`global` bindings should always have a `source`"), + ), + ); + checker.diagnostics.push(diagnostic); + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index f8996485da95b..9eb81a4395294 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -5,6 +5,7 @@ pub use bidirectional_unicode::{bidirectional_unicode, BidirectionalUnicode}; pub use collapsible_else_if::{collapsible_else_if, CollapsibleElseIf}; pub use comparison_of_constant::{comparison_of_constant, ComparisonOfConstant}; pub use consider_using_sys_exit::{consider_using_sys_exit, ConsiderUsingSysExit}; +pub use global_statement::{global_statement, GlobalStatement}; pub use global_variable_not_assigned::GlobalVariableNotAssigned; pub use invalid_all_format::{invalid_all_format, InvalidAllFormat}; pub use invalid_all_object::{invalid_all_object, InvalidAllObject}; @@ -37,6 +38,7 @@ mod bidirectional_unicode; mod collapsible_else_if; mod comparison_of_constant; mod consider_using_sys_exit; +mod global_statement; mod global_variable_not_assigned; mod invalid_all_format; mod invalid_all_object; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0603_global_statement.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0603_global_statement.py.snap new file mode 100644 index 0000000000000..1a630a78433c6 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0603_global_statement.py.snap @@ -0,0 +1,93 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 17 + column: 4 + end_location: + row: 17 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: sys + location: + row: 24 + column: 4 + end_location: + row: 24 + column: 14 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: namedtuple + location: + row: 30 + column: 4 + end_location: + row: 30 + column: 21 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 36 + column: 4 + end_location: + row: 36 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 43 + column: 4 + end_location: + row: 43 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 50 + column: 4 + end_location: + row: 50 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: FUNC + location: + row: 60 + column: 4 + end_location: + row: 60 + column: 15 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CLASS + location: + row: 70 + column: 4 + end_location: + row: 70 + column: 16 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index 22319d61471d8..a36dd9ce44b8d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1859,6 +1859,7 @@ "PLW06", "PLW060", "PLW0602", + "PLW0603", "PLW2", "PLW29", "PLW290",