mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-26 20:10:09 +00:00
Propagate reads on global variables (#11584)
## 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 https://github.com/astral-sh/ruff/issues/11518.
This commit is contained in:
parent
4a305588e9
commit
69d9212817
6 changed files with 46 additions and 15 deletions
|
@ -588,8 +588,10 @@ impl<'a> Visitor<'a> for Checker<'a> {
|
||||||
Stmt::Global(ast::StmtGlobal { names, range: _ }) => {
|
Stmt::Global(ast::StmtGlobal { names, range: _ }) => {
|
||||||
if !self.semantic.scope_id.is_global() {
|
if !self.semantic.scope_id.is_global() {
|
||||||
for name in names {
|
for name in names {
|
||||||
if let Some(binding_id) = self.semantic.global_scope().get(name) {
|
let binding_id = self.semantic.global_scope().get(name);
|
||||||
|
|
||||||
// Mark the binding in the global scope as "rebound" in the current scope.
|
// Mark the binding in the global scope as "rebound" in the current scope.
|
||||||
|
if let Some(binding_id) = binding_id {
|
||||||
self.semantic
|
self.semantic
|
||||||
.add_rebinding_scope(binding_id, self.semantic.scope_id);
|
.add_rebinding_scope(binding_id, self.semantic.scope_id);
|
||||||
}
|
}
|
||||||
|
@ -597,7 +599,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
|
||||||
// Add a binding to the current scope.
|
// Add a binding to the current scope.
|
||||||
let binding_id = self.semantic.push_binding(
|
let binding_id = self.semantic.push_binding(
|
||||||
name.range(),
|
name.range(),
|
||||||
BindingKind::Global,
|
BindingKind::Global(binding_id),
|
||||||
BindingFlags::GLOBAL,
|
BindingFlags::GLOBAL,
|
||||||
);
|
);
|
||||||
let scope = self.semantic.current_scope_mut();
|
let scope = self.semantic.current_scope_mut();
|
||||||
|
@ -609,7 +611,8 @@ impl<'a> Visitor<'a> for Checker<'a> {
|
||||||
if !self.semantic.scope_id.is_global() {
|
if !self.semantic.scope_id.is_global() {
|
||||||
for name in names {
|
for name in names {
|
||||||
if let Some((scope_id, binding_id)) = self.semantic.nonlocal(name) {
|
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(
|
self.semantic.add_local_reference(
|
||||||
binding_id,
|
binding_id,
|
||||||
ExprContext::Load,
|
ExprContext::Load,
|
||||||
|
@ -624,7 +627,7 @@ impl<'a> Visitor<'a> for Checker<'a> {
|
||||||
// Add a binding to the current scope.
|
// Add a binding to the current scope.
|
||||||
let binding_id = self.semantic.push_binding(
|
let binding_id = self.semantic.push_binding(
|
||||||
name.range(),
|
name.range(),
|
||||||
BindingKind::Nonlocal(scope_id),
|
BindingKind::Nonlocal(binding_id, scope_id),
|
||||||
BindingFlags::NONLOCAL,
|
BindingFlags::NONLOCAL,
|
||||||
);
|
);
|
||||||
let scope = self.semantic.current_scope_mut();
|
let scope = self.semantic.current_scope_mut();
|
||||||
|
|
|
@ -125,8 +125,8 @@ impl Renamer {
|
||||||
let scope_id = scope.get_all(name).find_map(|binding_id| {
|
let scope_id = scope.get_all(name).find_map(|binding_id| {
|
||||||
let binding = semantic.binding(binding_id);
|
let binding = semantic.binding(binding_id);
|
||||||
match binding.kind {
|
match binding.kind {
|
||||||
BindingKind::Global => Some(ScopeId::global()),
|
BindingKind::Global(_) => Some(ScopeId::global()),
|
||||||
BindingKind::Nonlocal(symbol_id) => Some(symbol_id),
|
BindingKind::Nonlocal(_, scope_id) => Some(scope_id),
|
||||||
_ => None,
|
_ => None,
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
@ -266,8 +266,8 @@ impl Renamer {
|
||||||
| BindingKind::LoopVar
|
| BindingKind::LoopVar
|
||||||
| BindingKind::ComprehensionVar
|
| BindingKind::ComprehensionVar
|
||||||
| BindingKind::WithItemVar
|
| BindingKind::WithItemVar
|
||||||
| BindingKind::Global
|
| BindingKind::Global(_)
|
||||||
| BindingKind::Nonlocal(_)
|
| BindingKind::Nonlocal(_, _)
|
||||||
| BindingKind::ClassDefinition(_)
|
| BindingKind::ClassDefinition(_)
|
||||||
| BindingKind::FunctionDefinition(_)
|
| BindingKind::FunctionDefinition(_)
|
||||||
| BindingKind::Deletion
|
| BindingKind::Deletion
|
||||||
|
|
|
@ -48,8 +48,8 @@ pub(super) fn test_expression(expr: &Expr, semantic: &SemanticModel) -> Resoluti
|
||||||
| BindingKind::NamedExprAssignment
|
| BindingKind::NamedExprAssignment
|
||||||
| BindingKind::LoopVar
|
| BindingKind::LoopVar
|
||||||
| BindingKind::ComprehensionVar
|
| BindingKind::ComprehensionVar
|
||||||
| BindingKind::Global
|
| BindingKind::Global(_)
|
||||||
| BindingKind::Nonlocal(_) => Resolution::RelevantLocal,
|
| BindingKind::Nonlocal(_, _) => Resolution::RelevantLocal,
|
||||||
BindingKind::Import(import)
|
BindingKind::Import(import)
|
||||||
if matches!(import.qualified_name().segments(), ["pandas"]) =>
|
if matches!(import.qualified_name().segments(), ["pandas"]) =>
|
||||||
{
|
{
|
||||||
|
|
|
@ -54,8 +54,8 @@ pub(crate) fn non_ascii_name(binding: &Binding, locator: &Locator) -> Option<Dia
|
||||||
BindingKind::LoopVar => Kind::LoopVar,
|
BindingKind::LoopVar => Kind::LoopVar,
|
||||||
BindingKind::ComprehensionVar => Kind::ComprenhensionVar,
|
BindingKind::ComprehensionVar => Kind::ComprenhensionVar,
|
||||||
BindingKind::WithItemVar => Kind::WithItemVar,
|
BindingKind::WithItemVar => Kind::WithItemVar,
|
||||||
BindingKind::Global => Kind::Global,
|
BindingKind::Global(_) => Kind::Global,
|
||||||
BindingKind::Nonlocal(_) => Kind::Nonlocal,
|
BindingKind::Nonlocal(_, _) => Kind::Nonlocal,
|
||||||
BindingKind::ClassDefinition(_) => Kind::ClassDefinition,
|
BindingKind::ClassDefinition(_) => Kind::ClassDefinition,
|
||||||
BindingKind::FunctionDefinition(_) => Kind::FunctionDefinition,
|
BindingKind::FunctionDefinition(_) => Kind::FunctionDefinition,
|
||||||
BindingKind::BoundException => Kind::BoundException,
|
BindingKind::BoundException => Kind::BoundException,
|
||||||
|
|
|
@ -470,14 +470,14 @@ pub enum BindingKind<'a> {
|
||||||
/// def foo():
|
/// def foo():
|
||||||
/// global x
|
/// global x
|
||||||
/// ```
|
/// ```
|
||||||
Global,
|
Global(Option<BindingId>),
|
||||||
|
|
||||||
/// A binding for a nonlocal variable, like `x` in:
|
/// A binding for a nonlocal variable, like `x` in:
|
||||||
/// ```python
|
/// ```python
|
||||||
/// def foo():
|
/// def foo():
|
||||||
/// nonlocal x
|
/// nonlocal x
|
||||||
/// ```
|
/// ```
|
||||||
Nonlocal(ScopeId),
|
Nonlocal(BindingId, ScopeId),
|
||||||
|
|
||||||
/// A binding for a builtin, like `print` or `bool`.
|
/// A binding for a builtin, like `print` or `bool`.
|
||||||
Builtin,
|
Builtin,
|
||||||
|
@ -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);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
@ -540,6 +540,23 @@ impl<'a> SemanticModel<'a> {
|
||||||
return ReadResult::Resolved(binding_id);
|
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.
|
// Otherwise, treat it as resolved.
|
||||||
self.resolved_names.insert(name.into(), binding_id);
|
self.resolved_names.insert(name.into(), binding_id);
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue