Skip to content

Commit

Permalink
Propagate reads on global variables (#11584)
Browse files Browse the repository at this point in the history
## Summary

This PR ensures that if a variable is bound via `global`, and then the
`global` is read, the originating variable is also marked as read. It's
not perfect, in that it won't detect _rebindings_, like:

```python
from app import redis_connection

def func():
    global redis_connection

    redis_connection = 1
    redis_connection()
```

So, above, `redis_connection` is still marked as unused.

But it does avoid flagging `redis_connection` as unused in:

```python
from app import redis_connection

def func():
    global redis_connection

    redis_connection()
```

Closes #11518.
  • Loading branch information
charliermarsh authored May 28, 2024
1 parent 4a30558 commit 69d9212
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 15 deletions.
13 changes: 8 additions & 5 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,16 +588,18 @@ impl<'a> Visitor<'a> for Checker<'a> {
Stmt::Global(ast::StmtGlobal { names, range: _ }) => {
if !self.semantic.scope_id.is_global() {
for name in names {
if let Some(binding_id) = self.semantic.global_scope().get(name) {
// Mark the binding in the global scope as "rebound" in the current scope.
let binding_id = self.semantic.global_scope().get(name);

// Mark the binding in the global scope as "rebound" in the current scope.
if let Some(binding_id) = binding_id {
self.semantic
.add_rebinding_scope(binding_id, self.semantic.scope_id);
}

// Add a binding to the current scope.
let binding_id = self.semantic.push_binding(
name.range(),
BindingKind::Global,
BindingKind::Global(binding_id),
BindingFlags::GLOBAL,
);
let scope = self.semantic.current_scope_mut();
Expand All @@ -609,7 +611,8 @@ impl<'a> Visitor<'a> for Checker<'a> {
if !self.semantic.scope_id.is_global() {
for name in names {
if let Some((scope_id, binding_id)) = self.semantic.nonlocal(name) {
// Mark the binding as "used".
// Mark the binding as "used", since the `nonlocal` requires an existing
// binding.
self.semantic.add_local_reference(
binding_id,
ExprContext::Load,
Expand All @@ -624,7 +627,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
// Add a binding to the current scope.
let binding_id = self.semantic.push_binding(
name.range(),
BindingKind::Nonlocal(scope_id),
BindingKind::Nonlocal(binding_id, scope_id),
BindingFlags::NONLOCAL,
);
let scope = self.semantic.current_scope_mut();
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_linter/src/renamer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ impl Renamer {
let scope_id = scope.get_all(name).find_map(|binding_id| {
let binding = semantic.binding(binding_id);
match binding.kind {
BindingKind::Global => Some(ScopeId::global()),
BindingKind::Nonlocal(symbol_id) => Some(symbol_id),
BindingKind::Global(_) => Some(ScopeId::global()),
BindingKind::Nonlocal(_, scope_id) => Some(scope_id),
_ => None,
}
});
Expand Down Expand Up @@ -266,8 +266,8 @@ impl Renamer {
| BindingKind::LoopVar
| BindingKind::ComprehensionVar
| BindingKind::WithItemVar
| BindingKind::Global
| BindingKind::Nonlocal(_)
| BindingKind::Global(_)
| BindingKind::Nonlocal(_, _)
| BindingKind::ClassDefinition(_)
| BindingKind::FunctionDefinition(_)
| BindingKind::Deletion
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/pandas_vet/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ pub(super) fn test_expression(expr: &Expr, semantic: &SemanticModel) -> Resoluti
| BindingKind::NamedExprAssignment
| BindingKind::LoopVar
| BindingKind::ComprehensionVar
| BindingKind::Global
| BindingKind::Nonlocal(_) => Resolution::RelevantLocal,
| BindingKind::Global(_)
| BindingKind::Nonlocal(_, _) => Resolution::RelevantLocal,
BindingKind::Import(import)
if matches!(import.qualified_name().segments(), ["pandas"]) =>
{
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/pylint/rules/non_ascii_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ pub(crate) fn non_ascii_name(binding: &Binding, locator: &Locator) -> Option<Dia
BindingKind::LoopVar => Kind::LoopVar,
BindingKind::ComprehensionVar => Kind::ComprenhensionVar,
BindingKind::WithItemVar => Kind::WithItemVar,
BindingKind::Global => Kind::Global,
BindingKind::Nonlocal(_) => Kind::Nonlocal,
BindingKind::Global(_) => Kind::Global,
BindingKind::Nonlocal(_, _) => Kind::Nonlocal,
BindingKind::ClassDefinition(_) => Kind::ClassDefinition,
BindingKind::FunctionDefinition(_) => Kind::FunctionDefinition,
BindingKind::BoundException => Kind::BoundException,
Expand Down
15 changes: 13 additions & 2 deletions crates/ruff_python_semantic/src/binding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,14 +470,14 @@ pub enum BindingKind<'a> {
/// def foo():
/// global x
/// ```
Global,
Global(Option<BindingId>),

/// A binding for a nonlocal variable, like `x` in:
/// ```python
/// def foo():
/// nonlocal x
/// ```
Nonlocal(ScopeId),
Nonlocal(BindingId, ScopeId),

/// A binding for a builtin, like `print` or `bool`.
Builtin,
Expand Down Expand Up @@ -670,3 +670,14 @@ impl<'a, 'ast> Imported<'ast> for AnyImport<'a, 'ast> {
}
}
}

#[cfg(test)]
mod tests {
use crate::BindingKind;

#[test]
#[cfg(target_pointer_width = "64")]
fn size() {
assert!(std::mem::size_of::<BindingKind>() <= 24);
}
}
17 changes: 17 additions & 0 deletions crates/ruff_python_semantic/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,23 @@ impl<'a> SemanticModel<'a> {
return ReadResult::Resolved(binding_id);
}

BindingKind::Global(Some(binding_id))
| BindingKind::Nonlocal(binding_id, _) => {
// Mark the shadowed binding as used.
let reference_id = self.resolved_references.push(
self.scope_id,
self.node_id,
ExprContext::Load,
self.flags,
name.range,
);
self.bindings[binding_id].references.push(reference_id);

// Treat it as resolved.
self.resolved_names.insert(name.into(), binding_id);
return ReadResult::Resolved(binding_id);
}

_ => {
// Otherwise, treat it as resolved.
self.resolved_names.insert(name.into(), binding_id);
Expand Down

0 comments on commit 69d9212

Please sign in to comment.