[pylint] W0603: global-statement

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()

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);

crates/ruff/src/codes.rs

@@ -146,6 +146,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (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,

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,

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")]

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);
+        }
+    }
+}

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;

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: ~
+