Track conditional deletions in the semantic model (#10415)

## Summary Given `del X`, we'll typically add a `BindingKind::Deletion` to `X` to shadow the current binding. However, if the deletion is inside of a conditional operation, we _won't_, as in: ```python def f(): global X if X > 0: del X ``` We will, however, track it as a reference to the binding. This PR adds the expression context to those resolved references, so that we can detect that the `X` in `global X` was "assigned to". Closes #10397.
astral-sh · Mar 15, 2024 · 10ace88 · 10ace88
1 parent a8e50a7
commit 10ace88
Show file tree

Hide file tree

Showing 10 changed files with 133 additions and 45 deletions.
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
@@ -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
@@ -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
@@ -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
@@ -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/...ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/...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
@@ -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
@@ -67,6 +67,7 @@ pub(crate) fn non_ascii_name(binding: &Binding, locator: &Locator) -> Option<Dia
         | BindingKind::FromImport(_)
         | BindingKind::SubmoduleImport(_)
         | BindingKind::Deletion
+        | BindingKind::ConditionalDeletion(_)
         | BindingKind::UnboundException(_) => {
             return None;
         }

diff --git 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,14 +271,27 @@ 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).
         ///
         /// For example:
         /// ```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,23 +300,23 @@ bitflags! {
         /// ```python
         /// __all__ = [1]
         /// ```
-        const INVALID_ALL_OBJECT = 1 << 6;
+        const INVALID_ALL_OBJECT = 1 << 7;
 
         /// The binding represents a private declaration.
         ///
         /// For example, the binding could be `_T` in:
         /// ```python
         /// _T = "This is a private variable"
         /// ```
-        const PRIVATE_DECLARATION = 1 << 7;
+        const PRIVATE_DECLARATION = 1 << 8;
 
         /// The binding represents an unpacked assignment.
         ///
         /// For example, the binding could be `x` in:
         /// ```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
@@ -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,