[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](b70d2abd7f/tests/functional/g/globals.py)):

* Assign
* AugAssign
* ClassDef
* FunctionDef | AsyncFunctionDef
* Import
* ImportFrom
* Delete

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

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

crates/ruff/src/codes.rs

@@ -149,6 +149,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

@@ -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,

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

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;

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

ruff.schema.json

@@ -1859,6 +1859,7 @@
         "PLW06",
         "PLW060",
         "PLW0602",
+        "PLW0603",
         "PLW2",
         "PLW29",
         "PLW290",