mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-09-27 04:19:43 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Avoid propagating BindingKind::Global and BindingKind::Nonlocal (#5136)

Browse source 
## 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!
This commit is contained in:
Charlie Marsh 2023-06-16 11:06:59 -04:00 • committed by GitHub
parent fd1dfc3bfa
commit b3240dbfa2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 152 additions and 41 deletions
Download patch file Download diff file Expand all files Collapse all files

8
crates/ruff/resources/test/fixtures/flake8_type_checking/TCH002.py vendored
View file

@ -164,3 +164,11 @@ def f():
)
x: DataFrame = 2
def f():
global Member
from module import Member
x: Member = 1

9
crates/ruff/resources/test/fixtures/pyflakes/F841_3.py vendored
View file

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

7
crates/ruff/resources/test/fixtures/pylint/global_statement.py vendored
View file

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

40
crates/ruff/src/checkers/ast/mod.rs
View file

@ -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.
}
kind @ (BindingKind::Global | BindingKind::Nonlocal(_)) => {
// If the original binding was a global or nonlocal, then the new binding is
// too.
if !matches!(
shadowed.kind,
BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException,
) {
let references = shadowed.references.clone();
self.semantic.bindings[binding_id].kind = kind.clone();
self.semantic.bindings[binding_id].references = references;
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;
}
_ => {
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,9 +4844,6 @@ 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(),
@ -4851,8 +4853,6 @@ impl<'a> Checker<'a> {
}
}
}
}
}
// Imports in classes are public members.
if scope.kind.is_class() {

28
crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-third-party-import_TCH002.py.snap
View file

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

6
crates/ruff/src/rules/pyflakes/rules/unused_import.rs
View file

@ -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;
}

2
crates/ruff/src/rules/pyflakes/rules/unused_variable.rs
View file

@ -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__"

13
crates/ruff/src/rules/pylint/rules/global_statement.rs
View file

@ -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(
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(),
);
checker.diagnostics.push(diagnostic);
));
}
}
}
}

19
crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0603_global_statement.py.snap
View file

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

35
crates/ruff_python_semantic/src/binding.rs
View file

@ -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;
}
}