diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_not_assigned.py b/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_not_assigned.py index e6514b35d0cb4..2109c29ad3f97 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_not_assigned.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_not_assigned.py @@ -11,6 +11,13 @@ def f(): print(X) +def f(): + global X + + if X > 0: + del X + + ### # Non-errors. ### diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index c35cf53a77a3a..df2598b33ba39 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Fix}; use ruff_python_semantic::analyze::visibility; -use ruff_python_semantic::{Binding, BindingKind, Imported, ScopeKind}; +use ruff_python_semantic::{Binding, BindingKind, Imported, ResolvedReference, ScopeKind}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -91,13 +91,29 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if checker.enabled(Rule::GlobalVariableNotAssigned) { for (name, binding_id) in scope.bindings() { let binding = checker.semantic.binding(binding_id); + // If the binding is a `global`, then it's a top-level `global` that was never + // assigned in the current scope. If it were assigned, the `global` would be + // shadowed by the assignment. if binding.kind.is_global() { - diagnostics.push(Diagnostic::new( - pylint::rules::GlobalVariableNotAssigned { - name: (*name).to_string(), - }, - binding.range(), - )); + // If the binding was conditionally deleted, it will include a reference within + // a `Del` context, but won't be shadowed by a `BindingKind::Deletion`, as in: + // ```python + // if condition: + // del var + // ``` + if binding + .references + .iter() + .map(|id| checker.semantic.reference(*id)) + .all(ResolvedReference::is_load) + { + diagnostics.push(Diagnostic::new( + pylint::rules::GlobalVariableNotAssigned { + name: (*name).to_string(), + }, + binding.range(), + )); + } } } } diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 43aaf86ccf3ea..562cb4e37c7d2 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -540,7 +540,11 @@ impl<'a> Visitor<'a> for Checker<'a> { for name in names { if let Some((scope_id, binding_id)) = self.semantic.nonlocal(name) { // Mark the binding as "used". - self.semantic.add_local_reference(binding_id, name.range()); + self.semantic.add_local_reference( + binding_id, + ExprContext::Load, + name.range(), + ); // Mark the binding in the enclosing scope as "rebound" in the current // scope. @@ -2113,7 +2117,8 @@ impl<'a> Checker<'a> { // Mark anything referenced in `__all__` as used. // TODO(charlie): `range` here should be the range of the name in `__all__`, not // the range of `__all__` itself. - self.semantic.add_global_reference(binding_id, range); + self.semantic + .add_global_reference(binding_id, ExprContext::Load, range); } else { if self.semantic.global_scope().uses_star_imports() { if self.enabled(Rule::UndefinedLocalWithImportStarUsage) { diff --git a/crates/ruff_linter/src/renamer.rs b/crates/ruff_linter/src/renamer.rs index 8f4d560afe6ed..8571d7f53b067 100644 --- a/crates/ruff_linter/src/renamer.rs +++ b/crates/ruff_linter/src/renamer.rs @@ -255,6 +255,7 @@ impl Renamer { | BindingKind::ClassDefinition(_) | BindingKind::FunctionDefinition(_) | BindingKind::Deletion + | BindingKind::ConditionalDeletion(_) | BindingKind::UnboundException(_) => { Some(Edit::range_replacement(target.to_string(), binding.range())) } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index a30f8feaffe3a..d9d7b45e2e5c6 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -121,8 +121,7 @@ pub(crate) fn runtime_import_in_type_checking_block( checker .semantic() .reference(reference_id) - .context() - .is_runtime() + .in_runtime_context() }) { let Some(node_id) = binding.source else { @@ -155,8 +154,7 @@ pub(crate) fn runtime_import_in_type_checking_block( if checker.settings.flake8_type_checking.quote_annotations && binding.references().all(|reference_id| { let reference = checker.semantic().reference(reference_id); - reference.context().is_typing() - || reference.in_runtime_evaluated_annotation() + reference.in_typing_context() || reference.in_runtime_evaluated_annotation() }) { actions @@ -268,7 +266,7 @@ fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) .flat_map(|ImportBinding { binding, .. }| { binding.references.iter().filter_map(|reference_id| { let reference = checker.semantic().reference(*reference_id); - if reference.context().is_runtime() { + if reference.in_runtime_context() { Some(quote_annotation( reference.expression_id()?, checker.semantic(), diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 13c2ba673ae05..75d8f54450438 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -499,7 +499,7 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> .flat_map(|ImportBinding { binding, .. }| { binding.references.iter().filter_map(|reference_id| { let reference = checker.semantic().reference(*reference_id); - if reference.context().is_runtime() { + if reference.in_runtime_context() { Some(quote_annotation( reference.expression_id()?, checker.semantic(), diff --git a/crates/ruff_linter/src/rules/pylint/rules/non_ascii_name.rs b/crates/ruff_linter/src/rules/pylint/rules/non_ascii_name.rs index 92bcd79907827..2023c58ad14a8 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/non_ascii_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/non_ascii_name.rs @@ -67,6 +67,7 @@ pub(crate) fn non_ascii_name(binding: &Binding, locator: &Locator) -> Option { return None; } diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index cff3c7ce75d1c..fff0d852bbe38 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -75,6 +75,11 @@ impl<'a> Binding<'a> { self.flags.intersects(BindingFlags::GLOBAL) } + /// Return `true` if this [`Binding`] was deleted. + pub const fn is_deleted(&self) -> bool { + self.flags.intersects(BindingFlags::DELETED) + } + /// Return `true` if this [`Binding`] represents an assignment to `__all__` with an invalid /// value (e.g., `__all__ = "Foo"`). pub const fn is_invalid_all_format(&self) -> bool { @@ -165,6 +170,7 @@ impl<'a> Binding<'a> { // Deletions, annotations, `__future__` imports, and builtins are never considered // redefinitions. BindingKind::Deletion + | BindingKind::ConditionalDeletion(_) | BindingKind::Annotation | BindingKind::FutureImport | BindingKind::Builtin => { @@ -265,6 +271,19 @@ bitflags! { /// ``` const GLOBAL = 1 << 4; + /// The binding was deleted (i.e., the target of a `del` statement). + /// + /// For example, the binding could be `x` in: + /// ```python + /// del x + /// ``` + /// + /// The semantic model will typically shadow a deleted binding via an additional binding + /// with [`BindingKind::Deletion`]; however, conditional deletions (e.g., + /// `if condition: del x`) do _not_ generate a shadow binding. This flag is thus used to + /// detect whether a binding was _ever_ deleted, even conditionally. + const DELETED = 1 << 5; + /// The binding represents an export via `__all__`, but the assigned value uses an invalid /// expression (i.e., a non-container type). /// @@ -272,7 +291,7 @@ bitflags! { /// ```python /// __all__ = 1 /// ``` - const INVALID_ALL_FORMAT = 1 << 5; + const INVALID_ALL_FORMAT = 1 << 6; /// The binding represents an export via `__all__`, but the assigned value contains an /// invalid member (i.e., a non-string). @@ -281,7 +300,7 @@ bitflags! { /// ```python /// __all__ = [1] /// ``` - const INVALID_ALL_OBJECT = 1 << 6; + const INVALID_ALL_OBJECT = 1 << 7; /// The binding represents a private declaration. /// @@ -289,7 +308,7 @@ bitflags! { /// ```python /// _T = "This is a private variable" /// ``` - const PRIVATE_DECLARATION = 1 << 7; + const PRIVATE_DECLARATION = 1 << 8; /// The binding represents an unpacked assignment. /// @@ -297,7 +316,7 @@ bitflags! { /// ```python /// (x, y) = 1, 2 /// ``` - const UNPACKED_ASSIGNMENT = 1 << 8; + const UNPACKED_ASSIGNMENT = 1 << 9; } } @@ -512,6 +531,13 @@ pub enum BindingKind<'a> { /// ``` Deletion, + /// A binding for a deletion, like `x` in: + /// ```python + /// if x > 0: + /// del x + /// ``` + ConditionalDeletion(BindingId), + /// A binding to bind an exception to a local variable, like `x` in: /// ```python /// try: diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index f6575656fdf57..f9f4fdc471602 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -5,7 +5,7 @@ use rustc_hash::FxHashMap; use ruff_python_ast::helpers::from_relative_import; use ruff_python_ast::name::{QualifiedName, UnqualifiedName}; -use ruff_python_ast::{self as ast, Expr, Operator, Stmt}; +use ruff_python_ast::{self as ast, Expr, ExprContext, Operator, Stmt}; use ruff_python_stdlib::path::is_python_stub_file; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -271,7 +271,7 @@ impl<'a> SemanticModel<'a> { .get(symbol) .map_or(true, |binding_id| { // Treat the deletion of a name as a reference to that name. - self.add_local_reference(binding_id, range); + self.add_local_reference(binding_id, ExprContext::Del, range); self.bindings[binding_id].is_unbound() }); @@ -296,8 +296,9 @@ impl<'a> SemanticModel<'a> { let reference_id = self.resolved_references.push( ScopeId::global(), self.node_id, - name.range, + ExprContext::Load, self.flags, + name.range, ); self.bindings[binding_id].references.push(reference_id); @@ -308,8 +309,9 @@ impl<'a> SemanticModel<'a> { let reference_id = self.resolved_references.push( ScopeId::global(), self.node_id, - name.range, + ExprContext::Load, self.flags, + name.range, ); self.bindings[binding_id].references.push(reference_id); } @@ -365,8 +367,9 @@ impl<'a> SemanticModel<'a> { let reference_id = self.resolved_references.push( self.scope_id, self.node_id, - name.range, + ExprContext::Load, self.flags, + name.range, ); self.bindings[binding_id].references.push(reference_id); @@ -377,8 +380,9 @@ impl<'a> SemanticModel<'a> { let reference_id = self.resolved_references.push( self.scope_id, self.node_id, - name.range, + ExprContext::Load, self.flags, + name.range, ); self.bindings[binding_id].references.push(reference_id); } @@ -426,6 +430,15 @@ impl<'a> SemanticModel<'a> { return ReadResult::UnboundLocal(binding_id); } + BindingKind::ConditionalDeletion(binding_id) => { + self.unresolved_references.push( + name.range, + self.exceptions(), + UnresolvedReferenceFlags::empty(), + ); + return ReadResult::UnboundLocal(binding_id); + } + // If we hit an unbound exception that shadowed a bound name, resole to the // bound name. For example, given: // @@ -446,8 +459,9 @@ impl<'a> SemanticModel<'a> { let reference_id = self.resolved_references.push( self.scope_id, self.node_id, - name.range, + ExprContext::Load, self.flags, + name.range, ); self.bindings[binding_id].references.push(reference_id); @@ -458,8 +472,9 @@ impl<'a> SemanticModel<'a> { let reference_id = self.resolved_references.push( self.scope_id, self.node_id, - name.range, + ExprContext::Load, self.flags, + name.range, ); self.bindings[binding_id].references.push(reference_id); } @@ -548,6 +563,7 @@ impl<'a> SemanticModel<'a> { match self.bindings[binding_id].kind { BindingKind::Annotation => continue, BindingKind::Deletion | BindingKind::UnboundException(None) => return None, + BindingKind::ConditionalDeletion(binding_id) => return Some(binding_id), BindingKind::UnboundException(Some(binding_id)) => return Some(binding_id), _ => return Some(binding_id), } @@ -1315,18 +1331,28 @@ impl<'a> SemanticModel<'a> { } /// Add a reference to the given [`BindingId`] in the local scope. - pub fn add_local_reference(&mut self, binding_id: BindingId, range: TextRange) { + pub fn add_local_reference( + &mut self, + binding_id: BindingId, + ctx: ExprContext, + range: TextRange, + ) { let reference_id = self.resolved_references - .push(self.scope_id, self.node_id, range, self.flags); + .push(self.scope_id, self.node_id, ctx, self.flags, range); self.bindings[binding_id].references.push(reference_id); } /// Add a reference to the given [`BindingId`] in the global scope. - pub fn add_global_reference(&mut self, binding_id: BindingId, range: TextRange) { + pub fn add_global_reference( + &mut self, + binding_id: BindingId, + ctx: ExprContext, + range: TextRange, + ) { let reference_id = self.resolved_references - .push(ScopeId::global(), self.node_id, range, self.flags); + .push(ScopeId::global(), self.node_id, ctx, self.flags, range); self.bindings[binding_id].references.push(reference_id); } @@ -1700,7 +1726,6 @@ bitflags! { /// only required by the Python interpreter, but by runtime type checkers too. const RUNTIME_REQUIRED_ANNOTATION = 1 << 2; - /// The model is in a type definition. /// /// For example, the model could be visiting `int` in: @@ -1886,7 +1911,6 @@ bitflags! { /// ``` const COMPREHENSION_ASSIGNMENT = 1 << 19; - /// The model is in a module / class / function docstring. /// /// For example, the model could be visiting either the module, class, diff --git a/crates/ruff_python_semantic/src/reference.rs b/crates/ruff_python_semantic/src/reference.rs index 6bb807e1c8523..d6735f4232dc8 100644 --- a/crates/ruff_python_semantic/src/reference.rs +++ b/crates/ruff_python_semantic/src/reference.rs @@ -3,10 +3,10 @@ use std::ops::Deref; use bitflags::bitflags; use ruff_index::{newtype_index, IndexSlice, IndexVec}; +use ruff_python_ast::ExprContext; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; -use crate::context::ExecutionContext; use crate::scope::ScopeId; use crate::{Exceptions, NodeId, SemanticModelFlags}; @@ -18,10 +18,12 @@ pub struct ResolvedReference { node_id: Option, /// The scope in which the reference is defined. scope_id: ScopeId, - /// The range of the reference in the source code. - range: TextRange, + /// The expression context in which the reference occurs (e.g., `Load`, `Store`, `Del`). + ctx: ExprContext, /// The model state in which the reference occurs. flags: SemanticModelFlags, + /// The range of the reference in the source code. + range: TextRange, } impl ResolvedReference { @@ -35,13 +37,19 @@ impl ResolvedReference { self.scope_id } - /// The [`ExecutionContext`] of the reference. - pub const fn context(&self) -> ExecutionContext { - if self.flags.intersects(SemanticModelFlags::TYPING_CONTEXT) { - ExecutionContext::Typing - } else { - ExecutionContext::Runtime - } + /// Return `true` if the reference occurred in a `Load` operation. + pub const fn is_load(&self) -> bool { + self.ctx.is_load() + } + + /// Return `true` if the context is in a typing context. + pub const fn in_typing_context(&self) -> bool { + self.flags.intersects(SemanticModelFlags::TYPING_CONTEXT) + } + + /// Return `true` if the context is in a runtime context. + pub const fn in_runtime_context(&self) -> bool { + !self.flags.intersects(SemanticModelFlags::TYPING_CONTEXT) } /// Return `true` if the context is in a typing-only type annotation. @@ -108,14 +116,16 @@ impl ResolvedReferences { &mut self, scope_id: ScopeId, node_id: Option, - range: TextRange, + ctx: ExprContext, flags: SemanticModelFlags, + range: TextRange, ) -> ResolvedReferenceId { self.0.push(ResolvedReference { node_id, scope_id, - range, + ctx, flags, + range, }) } }