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 00000000000000..db3248502d507d --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/global_statement.py @@ -0,0 +1,72 @@ +# # Adapted from: https://github.com/PyCQA/pylint/blob/b70d2abd7fabe9bfd735a30b593b9cd5eaa36194/tests/functional/g/globals.py +CONSTANT = 1 + +def FUNC(): + pass + + +class CLASS: + pass + + +def fix_contant(value): + """all this is ok, but try not using 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 0d25adf49b7193..156b7f2e92db89 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 @@ -819,6 +823,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); } @@ -922,6 +929,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); } @@ -1179,6 +1197,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 { @@ -1561,6 +1589,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) { @@ -1824,6 +1858,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); } @@ -1874,7 +1916,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 3ffe6f83e1c323..8ba55c196040ec 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -146,6 +146,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 ca6dbdbe7bca8c..2849c6482c9c38 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -146,6 +146,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 6a9b341552d9ef..02b0406be8d385 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 00000000000000..5853e089bf0229 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/global_statement.rs @@ -0,0 +1,82 @@ +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::types::BindingKind; +use crate::checkers::ast::Checker; +use crate::registry::Diagnostic; +use crate::violation::Violation; +use crate::Range; + +define_violation!( + /// ## What it does + /// Checks for usage of the `global` statement to update a global identifier. + /// + /// ## Why is this bad? + /// Global variables should be avoided unless very necessary because of these non-exhaustive + /// list of reasons: it breaks encapsulation and makes tracing program flow very difficult. + /// + /// ## 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.scopes[*checker.scope_stack.last().expect("No current scope found")]; + + // println!("{:#?}", name); + if let Some(&bidx) = scope.bindings.get(name) { + let binding = &checker.bindings[bidx]; + // println!("{:#?}", binding); + if BindingKind::is_global(&binding.kind) { + let diag = Diagnostic::new( + GlobalStatement { + name: name.to_string(), + }, + Range::from_located( + // NOTE: This could've been `binding.range` except pylint wants to report the + // location of the `global` keyword instead of the identifier. + binding + .source + .as_ref() + .expect("Global statements should always have `source`"), + ), + ); + checker.diagnostics.push(diag); + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 3c150358faef81..9f54897f5ebe6a 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -4,6 +4,7 @@ pub use bad_string_format_type::{bad_string_format_type, BadStringFormatType}; pub use bidirectional_unicode::{bidirectional_unicode, BidirectionalUnicode}; 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}; @@ -35,6 +36,7 @@ mod bad_string_format_type; mod bidirectional_unicode; 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 00000000000000..0ca080c35125b0 --- /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: 14 + column: 4 + end_location: + row: 14 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: sys + location: + row: 21 + column: 4 + end_location: + row: 21 + column: 14 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: namedtuple + location: + row: 27 + column: 4 + end_location: + row: 27 + column: 21 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 33 + column: 4 + end_location: + row: 33 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 40 + column: 4 + end_location: + row: 40 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CONSTANT + location: + row: 47 + column: 4 + end_location: + row: 47 + column: 19 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: FUNC + location: + row: 57 + column: 4 + end_location: + row: 57 + column: 15 + fix: ~ + parent: ~ +- kind: + GlobalStatement: + name: CLASS + location: + row: 67 + column: 4 + end_location: + row: 67 + column: 16 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index 2ca3be755dc27b..d8403875f4a79b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1842,6 +1842,7 @@ "PLW06", "PLW060", "PLW0602", + "PLW0603", "PLW2", "PLW29", "PLW290",