From c7ff743d30a834d6d8f42ad8818cf499bc126749 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 16 Jul 2023 00:34:42 -0400 Subject: [PATCH] Use `semantic().global()` to power `global-statement` rule (#5795) ## Summary The intent of this rule is to always flag the `global` declaration, not the usage. The current implementation does the wrong thing if a global is assigned multiple times. Using `semantic().global()` is also more efficient. --- .../test/fixtures/pylint/global_statement.py | 5 ++++ .../rules/pylint/rules/global_statement.rs | 27 +++++++------------ ...t__tests__PLW0603_global_statement.py.snap | 5 ++-- 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pylint/global_statement.py b/crates/ruff/resources/test/fixtures/pylint/global_statement.py index 1d28c61955..01b5c9e6b6 100644 --- a/crates/ruff/resources/test/fixtures/pylint/global_statement.py +++ b/crates/ruff/resources/test/fixtures/pylint/global_statement.py @@ -80,3 +80,8 @@ def multiple_assignment(): global CONSTANT # [global-statement] CONSTANT = 1 CONSTANT = 2 + + +def no_assignment(): + """Shouldn't warn""" + global CONSTANT diff --git a/crates/ruff/src/rules/pylint/rules/global_statement.rs b/crates/ruff/src/rules/pylint/rules/global_statement.rs index 3eef3e4123..c359cf4511 100644 --- a/crates/ruff/src/rules/pylint/rules/global_statement.rs +++ b/crates/ruff/src/rules/pylint/rules/global_statement.rs @@ -1,5 +1,3 @@ -use rustpython_parser::ast::Ranged; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -55,21 +53,14 @@ impl Violation for GlobalStatement { /// PLW0603 pub(crate) fn global_statement(checker: &mut Checker, name: &str) { - let scope = checker.semantic().scope(); - if let Some(binding_id) = scope.get(name) { - let binding = checker.semantic().binding(binding_id); - if binding.is_global() { - if let Some(source) = binding.source { - let source = checker.semantic().stmts[source]; - checker.diagnostics.push(Diagnostic::new( - GlobalStatement { - name: name.to_string(), - }, - // Match Pylint's behavior by reporting on the `global` statement`, rather - // than the variable usage. - source.range(), - )); - } - } + if let Some(range) = checker.semantic().global(name) { + checker.diagnostics.push(Diagnostic::new( + GlobalStatement { + name: name.to_string(), + }, + // Match Pylint's behavior by reporting on the `global` statement`, rather + // than the variable usage. + range, + )); } } 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 index 3d858431c5..910f5d31fc 100644 --- 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 @@ -89,12 +89,13 @@ global_statement.py:80:5: PLW0603 Using the global statement to update `CONSTANT 82 | CONSTANT = 2 | -global_statement.py:81:5: PLW0603 Using the global statement to update `CONSTANT` is discouraged +global_statement.py:80:5: PLW0603 Using the global statement to update `CONSTANT` is discouraged | +78 | def multiple_assignment(): 79 | """Should warn on every assignment.""" 80 | global CONSTANT # [global-statement] + | ^^^^^^^^^^^^^^^ PLW0603 81 | CONSTANT = 1 - | ^^^^^^^^^^^^ PLW0603 82 | CONSTANT = 2 |