Track symbol deletions separately from bindings (#4888)

crates/ruff/resources/test/fixtures/pylint/nonlocal_without_binding.py

@@ -17,3 +17,30 @@ def f():
 
     def f():
         nonlocal y
+
+
+def f():
+    x = 1
+
+    def g():
+        nonlocal x
+
+    del x
+
+
+def f():
+    def g():
+        nonlocal x
+
+    del x
+
+
+def f():
+    try:
+        pass
+    except Exception as x:
+        pass
+
+    def g():
+        nonlocal x
+        x = 2

crates/ruff/src/checkers/ast/mod.rs

@@ -307,9 +307,18 @@ where
                                 stmt.range(),
                                 ExecutionContext::Runtime,
                             );
-                        } else {
-                            // Ensure that every nonlocal has an existing binding from a parent scope.
-                            if self.enabled(Rule::NonlocalWithoutBinding) {
+                        }
+
+                        // Ensure that every nonlocal has an existing binding from a parent scope.
+                        if self.enabled(Rule::NonlocalWithoutBinding) {
+                            if self
+                                .semantic_model
+                                .scopes
+                                .ancestors(self.semantic_model.scope_id)
+                                .skip(1)
+                                .take_while(|scope| !scope.kind.is_module())
+                                .all(|scope| !scope.declares(name.as_str()))
+                            {
                                 self.diagnostics.push(Diagnostic::new(
                                     pylint::rules::NonlocalWithoutBinding {
                                         name: name.to_string(),
@@ -4039,7 +4048,7 @@ where
                         let name_range =
                             helpers::excepthandler_name_range(excepthandler, self.locator).unwrap();
 
-                        if self.semantic_model.scope().defines(name) {
+                        if self.semantic_model.scope().has(name) {
                             self.handle_node_store(
                                 name,
                                 &Expr::Name(ast::ExprName {
@@ -4064,7 +4073,7 @@ where
 
                         if let Some(binding_id) = {
                             let scope = self.semantic_model.scope_mut();
-                            scope.remove(name)
+                            scope.delete(name)
                         } {
                             if !self.semantic_model.is_used(binding_id) {
                                 if self.enabled(Rule::UnusedVariable) {
@@ -4694,19 +4703,16 @@ impl<'a> Checker<'a> {
         }
 
         let scope = self.semantic_model.scope_mut();
-        if scope.remove(id.as_str()).is_some() {
-            return;
-        }
-        if !self.enabled(Rule::UndefinedName) {
-            return;
+        if scope.delete(id.as_str()).is_none() {
+            if self.enabled(Rule::UndefinedName) {
+                self.diagnostics.push(Diagnostic::new(
+                    pyflakes::rules::UndefinedName {
+                        name: id.to_string(),
+                    },
+                    expr.range(),
+                ));
+            }
         }
-
-        self.diagnostics.push(Diagnostic::new(
-            pyflakes::rules::UndefinedName {
-                name: id.to_string(),
-            },
-            expr.range(),
-        ));
     }
 
     fn check_deferred_future_type_definitions(&mut self) {
@@ -4974,7 +4980,7 @@ impl<'a> Checker<'a> {
                         .collect();
                     if !sources.is_empty() {
                         for (name, range) in &exports {
-                            if !scope.defines(name) {
+                            if !scope.has(name) {
                                 diagnostics.push(Diagnostic::new(
                                     pyflakes::rules::UndefinedLocalWithImportStarUsage {
                                         name: (*name).to_string(),

crates/ruff/src/rules/pyflakes/rules/undefined_export.rs

@@ -51,7 +51,7 @@ impl Violation for UndefinedExport {
 pub(crate) fn undefined_export(name: &str, range: TextRange, scope: &Scope) -> Vec<Diagnostic> {
     let mut diagnostics = Vec::new();
     if !scope.uses_star_imports() {
-        if !scope.defines(name) {
+        if !scope.has(name) {
             diagnostics.push(Diagnostic::new(
                 UndefinedExport {
                     name: (*name).to_string(),

crates/ruff/src/rules/pyflakes/rules/undefined_local.rs

@@ -47,7 +47,7 @@ impl Violation for UndefinedLocal {
 pub(crate) fn undefined_local(checker: &mut Checker, name: &str) {
     // If the name hasn't already been defined in the current scope...
     let current = checker.semantic_model().scope();
-    if !current.kind.is_any_function() || current.defines(name) {
+    if !current.kind.is_any_function() || current.has(name) {
         return;
     }
 

crates/ruff_python_semantic/src/scope.rs

@@ -23,6 +23,8 @@ pub struct Scope<'a> {
     bindings: FxHashMap<&'a str, BindingId>,
     /// A map from binding ID to binding ID that it shadows.
     shadowed_bindings: HashMap<BindingId, BindingId, BuildNoHashHasher<BindingId>>,
+    /// A list of all names that have been deleted in this scope.
+    deleted_symbols: Vec<&'a str>,
     /// Index into the globals arena, if the scope contains any globally-declared symbols.
     globals_id: Option<GlobalsId>,
 }
@@ -36,6 +38,7 @@ impl<'a> Scope<'a> {
             star_imports: Vec::default(),
             bindings: FxHashMap::default(),
             shadowed_bindings: IntMap::default(),
+            deleted_symbols: Vec::default(),
             globals_id: None,
         }
     }
@@ -48,6 +51,7 @@ impl<'a> Scope<'a> {
             star_imports: Vec::default(),
             bindings: FxHashMap::default(),
             shadowed_bindings: IntMap::default(),
+            deleted_symbols: Vec::default(),
             globals_id: None,
         }
     }
@@ -67,14 +71,23 @@ impl<'a> Scope<'a> {
         }
     }
 
-    /// Returns `true` if this scope defines a binding with the given name.
-    pub fn defines(&self, name: &str) -> bool {
+    /// Removes the binding with the given name.
+    pub fn delete(&mut self, name: &'a str) -> Option<BindingId> {
+        self.deleted_symbols.push(name);
+        self.bindings.remove(name)
+    }
+
+    /// Returns `true` if this scope has a binding with the given name.
+    pub fn has(&self, name: &str) -> bool {
         self.bindings.contains_key(name)
     }
 
-    /// Removes the binding with the given name
-    pub fn remove(&mut self, name: &str) -> Option<BindingId> {
-        self.bindings.remove(name)
+    /// Returns `true` if the scope declares a symbol with the given name.
+    ///
+    /// Unlike [`Scope::has`], the name may no longer be bound to a value (e.g., it could be
+    /// deleted).
+    pub fn declares(&self, name: &str) -> bool {
+        self.has(name) || self.deleted_symbols.contains(&name)
     }
 
     /// Returns the ids of all bindings defined in this scope.