From b3240dbfa2c2b42dc88a1bc3bb14557ca41f9382 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 16 Jun 2023 11:06:59 -0400 Subject: [PATCH] Avoid propagating `BindingKind::Global` and `BindingKind::Nonlocal` (#5136) ## Summary This PR fixes a small quirk in the semantic model. Typically, when we see an import, like `import foo`, we create a `BindingKind::Importation` for it. However, if `foo` has been declared as a `global`, then we propagate the kind forward. So given: ```python global foo import foo ``` We'd create two bindings for `foo`, both with type `global`. This was originally borrowed from Pyflakes, and it exists to help avoid false-positives like: ```python def f(): global foo # Don't mark `foo` as "assigned but unused"! It's a global! foo = 1 ``` This PR removes that behavior, and instead tracks "Does this binding refer to a global?" as a flag. This is much cleaner, since it means we don't "lose" the identity of various bindings. As a very strange example of why this matters, consider: ```python def foo(): global Member from module import Member x: Member = 1 ``` `Member` is only used in a typing context, so we should flag it and say "move it to a `TYPE_CHECKING` block". However, when we go to analyze `from module import Member`, it has `BindingKind::Global`. So we don't even know that it's an import! --- .../fixtures/flake8_type_checking/TCH002.py | 8 +++ .../test/fixtures/pyflakes/F841_3.py | 9 ++++ .../test/fixtures/pylint/global_statement.py | 7 +++ crates/ruff/src/checkers/ast/mod.rs | 54 +++++++++---------- ...ing-only-third-party-import_TCH002.py.snap | 28 ++++++++++ .../src/rules/pyflakes/rules/unused_import.rs | 6 ++- .../rules/pyflakes/rules/unused_variable.rs | 2 + .../rules/pylint/rules/global_statement.rs | 25 +++++---- ...t__tests__PLW0603_global_statement.py.snap | 19 +++++++ crates/ruff_python_semantic/src/binding.rs | 35 ++++++++++++ 10 files changed, 152 insertions(+), 41 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_type_checking/TCH002.py b/crates/ruff/resources/test/fixtures/flake8_type_checking/TCH002.py index 82d6d2f10b..9248c10775 100644 --- a/crates/ruff/resources/test/fixtures/flake8_type_checking/TCH002.py +++ b/crates/ruff/resources/test/fixtures/flake8_type_checking/TCH002.py @@ -164,3 +164,11 @@ def f(): ) x: DataFrame = 2 + + +def f(): + global Member + + from module import Member + + x: Member = 1 diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py b/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py index bd2a3f2e02..526c999441 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F841_3.py @@ -138,3 +138,12 @@ def f(provided: int) -> int: match provided: case {**x}: pass + + +global CONSTANT + + +def f() -> None: + global CONSTANT + CONSTANT = 1 + CONSTANT = 2 diff --git a/crates/ruff/resources/test/fixtures/pylint/global_statement.py b/crates/ruff/resources/test/fixtures/pylint/global_statement.py index 69bcc36775..1d28c61955 100644 --- a/crates/ruff/resources/test/fixtures/pylint/global_statement.py +++ b/crates/ruff/resources/test/fixtures/pylint/global_statement.py @@ -73,3 +73,10 @@ def override_class(): pass CLASS() + + +def multiple_assignment(): + """Should warn on every assignment.""" + global CONSTANT # [global-statement] + CONSTANT = 1 + CONSTANT = 2 diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 4fe244633d..a784e6d743 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -264,7 +264,7 @@ where let binding_id = self.semantic.push_binding( *range, BindingKind::Global, - BindingFlags::empty(), + BindingFlags::GLOBAL, ); let scope = self.semantic.scope_mut(); scope.add(name, binding_id); @@ -299,7 +299,7 @@ where let binding_id = self.semantic.push_binding( *range, BindingKind::Nonlocal(scope_id), - BindingFlags::empty(), + BindingFlags::NONLOCAL, ); let scope = self.semantic.scope_mut(); scope.add(name, binding_id); @@ -4279,22 +4279,27 @@ impl<'a> Checker<'a> { return binding_id; } + // Avoid shadowing builtins. let shadowed = &self.semantic.bindings[shadowed_id]; - match &shadowed.kind { - BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException => { - // Avoid overriding builtins. + if !matches!( + shadowed.kind, + BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException, + ) { + let references = shadowed.references.clone(); + let is_global = shadowed.is_global(); + let is_nonlocal = shadowed.is_nonlocal(); + + // If the shadowed binding was global, then this one is too. + if is_global { + self.semantic.bindings[binding_id].flags |= BindingFlags::GLOBAL; } - kind @ (BindingKind::Global | BindingKind::Nonlocal(_)) => { - // If the original binding was a global or nonlocal, then the new binding is - // too. - let references = shadowed.references.clone(); - self.semantic.bindings[binding_id].kind = kind.clone(); - self.semantic.bindings[binding_id].references = references; - } - _ => { - let references = shadowed.references.clone(); - self.semantic.bindings[binding_id].references = references; + + // If the shadowed binding was non-local, then this one is too. + if is_nonlocal { + self.semantic.bindings[binding_id].flags |= BindingFlags::NONLOCAL; } + + self.semantic.bindings[binding_id].references = references; } } @@ -4389,7 +4394,7 @@ impl<'a> Checker<'a> { if self.semantic.scope().kind.is_any_function() { // Ignore globals. if !self.semantic.scope().get(id).map_or(false, |binding_id| { - self.semantic.binding(binding_id).kind.is_global() + self.semantic.binding(binding_id).is_global() }) { pep8_naming::rules::non_lowercase_variable_in_function(self, expr, parent, id); } @@ -4839,17 +4844,12 @@ impl<'a> Checker<'a> { for (name, binding_id) in scope.bindings() { let binding = self.semantic.binding(binding_id); if binding.kind.is_global() { - if let Some(source) = binding.source { - let stmt = &self.semantic.stmts[source]; - if stmt.is_global_stmt() { - diagnostics.push(Diagnostic::new( - pylint::rules::GlobalVariableNotAssigned { - name: (*name).to_string(), - }, - binding.range, - )); - } - } + diagnostics.push(Diagnostic::new( + pylint::rules::GlobalVariableNotAssigned { + name: (*name).to_string(), + }, + binding.range, + )); } } } diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-third-party-import_TCH002.py.snap b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-third-party-import_TCH002.py.snap index 7005762cc5..ab4ed38714 100644 --- a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-third-party-import_TCH002.py.snap +++ b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-third-party-import_TCH002.py.snap @@ -221,4 +221,32 @@ TCH002.py:47:22: TCH002 [*] Move third-party import `pandas` into a type-checkin 49 52 | x = dict["pd.DataFrame", "pd.DataFrame"] 50 53 | +TCH002.py:172:24: TCH002 [*] Move third-party import `module.Member` into a type-checking block + | +170 | global Member +171 | +172 | from module import Member + | ^^^^^^ TCH002 +173 | +174 | x: Member = 1 + | + = help: Move into type-checking block + +ℹ Suggested fix +1 1 | """Tests to determine accurate detection of typing-only imports.""" + 2 |+from typing import TYPE_CHECKING + 3 |+ + 4 |+if TYPE_CHECKING: + 5 |+ from module import Member +2 6 | +3 7 | +4 8 | def f(): +-------------------------------------------------------------------------------- +169 173 | def f(): +170 174 | global Member +171 175 | +172 |- from module import Member +173 176 | +174 177 | x: Member = 1 + diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index d004b6433c..ca76ccfda2 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -103,7 +103,11 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut for binding_id in scope.binding_ids() { let binding = checker.semantic().binding(binding_id); - if binding.is_used() || binding.is_explicit_export() { + if binding.is_used() + || binding.is_explicit_export() + || binding.is_nonlocal() + || binding.is_global() + { continue; } diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index e3edd38b15..de24c4d795 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs @@ -295,6 +295,8 @@ pub(crate) fn unused_variable(checker: &mut Checker, scope: ScopeId) { .map(|(name, binding_id)| (name, checker.semantic().binding(binding_id))) .filter_map(|(name, binding)| { if (binding.kind.is_assignment() || binding.kind.is_named_expr_assignment()) + && !binding.is_nonlocal() + && !binding.is_global() && !binding.is_used() && !checker.settings.dummy_variable_rgx.is_match(name) && name != "__tracebackhide__" diff --git a/crates/ruff/src/rules/pylint/rules/global_statement.rs b/crates/ruff/src/rules/pylint/rules/global_statement.rs index 6a8613b639..3eef3e4123 100644 --- a/crates/ruff/src/rules/pylint/rules/global_statement.rs +++ b/crates/ruff/src/rules/pylint/rules/global_statement.rs @@ -58,19 +58,18 @@ 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.kind.is_global() { - let source = checker.semantic().stmts[binding - .source - .expect("`global` bindings should always have a `source`")]; - let diagnostic = Diagnostic::new( - GlobalStatement { - name: name.to_string(), - }, - // Match Pylint's behavior by reporting on the `global` statement`, rather - // than the variable usage. - source.range(), - ); - checker.diagnostics.push(diagnostic); + 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(), + )); + } } } } 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 5228b0e568..3d858431c5 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 @@ -79,4 +79,23 @@ global_statement.py:70:5: PLW0603 Using the global statement to update `CLASS` i 72 | class CLASS: | +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 +82 | CONSTANT = 2 + | + +global_statement.py:81:5: PLW0603 Using the global statement to update `CONSTANT` is discouraged + | +79 | """Should warn on every assignment.""" +80 | global CONSTANT # [global-statement] +81 | CONSTANT = 1 + | ^^^^^^^^^^^^ PLW0603 +82 | CONSTANT = 2 + | + diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 7284de17f6..1eb256e089 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -57,6 +57,19 @@ impl<'a> Binding<'a> { self.flags.contains(BindingFlags::ALIAS) } + /// Return `true` if this [`Binding`] represents a `nonlocal`. A [`Binding`] is a `nonlocal` + /// if it's declared by a `nonlocal` statement, or shadows a [`Binding`] declared by a + /// `nonlocal` statement. + pub const fn is_nonlocal(&self) -> bool { + self.flags.contains(BindingFlags::NONLOCAL) + } + + /// Return `true` if this [`Binding`] represents a `global`. A [`Binding`] is a `global` if it's + /// declared by a `global` statement, or shadows a [`Binding`] declared by a `global` statement. + pub const fn is_global(&self) -> bool { + self.flags.contains(BindingFlags::GLOBAL) + } + /// Return `true` if this [`Binding`] represents an unbound variable /// (e.g., `x` in `x = 1; del x`). pub const fn is_unbound(&self) -> bool { @@ -193,6 +206,28 @@ bitflags! { /// from fastapi import FastAPI as app /// ``` const ALIAS = 1 << 2; + + /// The binding is `nonlocal` to the declaring scope. This could be a binding created by + /// a `nonlocal` statement, or a binding that shadows such a binding. + /// + /// For example, both of the bindings in the following function are `nonlocal`: + /// ```python + /// def f(): + /// nonlocal x + /// x = 1 + /// ``` + const NONLOCAL = 1 << 3; + + /// The binding is `global`. This could be a binding created by a `global` statement, or a + /// binding that shadows such a binding. + /// + /// For example, both of the bindings in the following function are `global`: + /// ```python + /// def f(): + /// global x + /// x = 1 + /// ``` + const GLOBAL = 1 << 4; } }