[ty] make del x force local resolution of x in the current scope (astral-sh#19389)

Fixes astral-sh/ty#769.

**Updated:** The preferred approach here is to keep the SemanticIndex
simple (`del` of any name marks that name "bound" in the current scope)
and to move complexity to type inference (free variable resolution stops
when it finds a binding, unless that binding is declared `nonlocal`). As
part of this change, free variable resolution will now union the types
it finds as it walks in enclosing scopes. This approach is still
incomplete, because it doesn't consider inner scopes or sibling scopes,
but it improves the common case.

---------

Co-authored-by: Carl Meyer <[email protected]>

Author: oconnor663

crates/ty_python_semantic/resources/mdtest/del.md

@@ -40,22 +40,92 @@ else:
 # error: [possibly-unresolved-reference]
 reveal_type(c)  # revealed: Literal[2]
 
-d = 1
+d = [1, 2, 3]
 
 def delete():
-    # TODO: this results in `UnboundLocalError`; we should emit `unresolved-reference`
-    del d
+    del d  # error: [unresolved-reference] "Name `d` used when not defined"
 
 delete()
-reveal_type(d)  # revealed: Literal[1]
+reveal_type(d)  # revealed: list[Unknown]
+
+def delete_element():
+    # When the `del` target isn't a name, it doesn't force local resolution.
+    del d[0]
+    print(d)
 
 def delete_global():
     global d
     del d
+    # We could lint that `d` is unbound in this trivial case, but because it's global we'd need to
+    # be careful about false positives if `d` got reinitialized somehow in between the two `del`s.
+    del d
 
 delete_global()
-# The variable should have been removed, but we won't check it for now.
-reveal_type(d)  # revealed: Literal[1]
+# Again, the variable should have been removed, but we don't check it.
+reveal_type(d)  # revealed: list[Unknown]
+
+def delete_nonlocal():
+    e = 2
+
+    def delete_nonlocal_bad():
+        del e  # error: [unresolved-reference] "Name `e` used when not defined"
+
+    def delete_nonlocal_ok():
+        nonlocal e
+        del e
+        # As with `global` above, we don't track that the nonlocal `e` is unbound.
+        del e
+```
+
+## `del` forces local resolution even if it's unreachable
+
+Without a `global x` or `nonlocal x` declaration in `foo`, `del x` in `foo` causes `print(x)` in an
+inner function `bar` to resolve to `foo`'s binding, in this case an unresolved reference / unbound
+local error:
+
+```py
+x = 1
+
+def foo():
+    print(x)  # error: [unresolved-reference] "Name `x` used when not defined"
+    if False:
+        # Assigning to `x` would have the same effect here.
+        del x
+
+    def bar():
+        print(x)  # error: [unresolved-reference] "Name `x` used when not defined"
+```
+
+## But `del` doesn't force local resolution of `global` or `nonlocal` variables
+
+However, with `global x` in `foo`, `print(x)` in `bar` resolves in the global scope, despite the
+`del` in `foo`:
+
+```py
+x = 1
+
+def foo():
+    global x
+    def bar():
+        # allowed, refers to `x` in the global scope
+        reveal_type(x)  # revealed: Unknown | Literal[1]
+    bar()
+    del x  # allowed, deletes `x` in the global scope (though we don't track that)
+```
+
+`nonlocal x` has a similar effect, if we add an extra `enclosing` scope to give it something to
+refer to:
+
+```py
+def enclosing():
+    x = 2
+    def foo():
+        nonlocal x
+        def bar():
+            # allowed, refers to `x` in `enclosing`
+            reveal_type(x)  # revealed: Unknown | Literal[2]
+        bar()
+        del x  # allowed, deletes `x` in `enclosing` (though we don't track that)
 ```
 
 ## Delete attributes

crates/ty_python_semantic/resources/mdtest/scopes/nonlocal.md

@@ -84,6 +84,52 @@ def f():
         x = "hello"  # error: [invalid-assignment] "Object of type `Literal["hello"]` is not assignable to `int`"
 ```
 
+## The types of `nonlocal` binding get unioned
+
+Without a type declaration, we union the bindings in enclosing scopes to infer a type. But name
+resolution stops at the closest binding that isn't declared `nonlocal`, and we ignore bindings
+outside of that one:
+
+```py
+def a():
+    # This binding is shadowed in `b`, so we ignore it in inner scopes.
+    x = 1
+
+    def b():
+        x = 2
+
+        def c():
+            nonlocal x
+            x = 3
+
+            def d():
+                nonlocal x
+                reveal_type(x)  # revealed: Unknown | Literal[3, 2]
+                x = 4
+                reveal_type(x)  # revealed: Literal[4]
+
+                def e():
+                    reveal_type(x)  # revealed: Unknown | Literal[4, 3, 2]
+```
+
+However, currently the union of types that we build is incomplete. We walk parent scopes, but not
+sibling scopes, child scopes, second-cousin-once-removed scopes, etc:
+
+```py
+def a():
+    x = 1
+    def b():
+        nonlocal x
+        x = 2
+
+    def c():
+        def d():
+            nonlocal x
+            x = 3
+        # TODO: This should include 2 and 3.
+        reveal_type(x)  # revealed: Unknown | Literal[1]
+```
+
 ## Local variable bindings "look ahead" to any assignment in the current scope
 
 The binding `x = 2` in `g` causes the earlier read of `x` to refer to `g`'s not-yet-initialized
@@ -390,3 +436,13 @@ def f():
         nonlocal x
     x = 1
 ```
+
+## Narrowing nonlocal types to `Never` doesn't make them unbound
+
+```py
+def foo():
+    x: int = 1
+    def bar():
+        if isinstance(x, str):
+            reveal_type(x)  # revealed: Never
+```

crates/ty_python_semantic/src/semantic_index/builder.rs

@@ -1994,8 +1994,26 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> {
                 walk_stmt(self, stmt);
                 for target in targets {
                     if let Ok(target) = PlaceExpr::try_from(target) {
+                        let is_name = target.is_name();
                         let place_id = self.add_place(PlaceExprWithFlags::new(target));
-                        self.current_place_table_mut().mark_place_used(place_id);
+                        let place_table = self.current_place_table_mut();
+                        if is_name {
+                            // `del x` behaves like an assignment in that it forces all references
+                            // to `x` in the current scope (including *prior* references) to refer
+                            // to the current scope's binding (unless `x` is declared `global` or
+                            // `nonlocal`). For example, this is an UnboundLocalError at runtime:
+                            //
+                            // ```py
+                            // x = 1
+                            // def foo():
+                            //     print(x)  # can't refer to global `x`
+                            //     if False:
+                            //         del x
+                            // foo()
+                            // ```
+                            place_table.mark_place_bound(place_id);
+                        }
+                        place_table.mark_place_used(place_id);
                         self.delete_binding(place_id);
                     }
                 }

crates/ty_python_semantic/src/types/infer.rs

@@ -5978,6 +5978,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
             // definition of this name visible to us (would be `LOAD_DEREF` at runtime.)
             // Note that we skip the scope containing the use that we are resolving, since we
             // already looked for the place there up above.
+            let mut nonlocal_union_builder = UnionBuilder::new(db);
+            let mut found_some_definition = false;
             for (enclosing_scope_file_id, _) in self.index.ancestor_scopes(file_scope_id).skip(1) {
                 // Class scopes are not visible to nested scopes, and we need to handle global
                 // scope differently (because an unbound name there falls back to builtins), so
@@ -6063,21 +6065,25 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                 let Some(enclosing_place) = enclosing_place_table.place_by_expr(expr) else {
                     continue;
                 };
+
+                // Reads of "free" variables terminate at any enclosing scope that marks the
+                // variable `global`, whether or not that scope actually binds the variable. If we
+                // see a `global` declaration, stop walking scopes and proceed to the global
+                // handling below. (If we're walking from a prior/inner scope where this variable
+                // is `nonlocal`, then this is a semantic syntax error, but we don't enforce that
+                // here. See `infer_nonlocal_statement`.)
                 if enclosing_place.is_marked_global() {
-                    // Reads of "free" variables can terminate at an enclosing scope that marks the
-                    // variable `global` but doesn't actually bind it. In that case, stop walking
-                    // scopes and proceed to the global handling below. (But note that it's a
-                    // semantic syntax error for the `nonlocal` keyword to do this. See
-                    // `infer_nonlocal_statement`.)
                     break;
                 }
+
+                // If the name is declared or bound in this scope, figure out its type. This might
+                // resolve the name and end the walk. But if the name is declared `nonlocal` in
+                // this scope, we'll keep walking enclosing scopes and union this type with the
+                // other types we find. (It's a semantic syntax error to declare a type for a
+                // `nonlocal` variable, but we don't enforce that here. See the
+                // `ast::Stmt::AnnAssign` handling in `SemanticIndexBuilder::visit_stmt`.)
                 if enclosing_place.is_bound() || enclosing_place.is_declared() {
-                    // We can return early here, because the nearest function-like scope that
-                    // defines a name must be the only source for the nonlocal reference (at
-                    // runtime, it is the scope that creates the cell for our closure.) If the name
-                    // isn't bound in that scope, we should get an unbound name, not continue
-                    // falling back to other scopes / globals / builtins.
-                    return place(
+                    let local_place_and_qualifiers = place(
                         db,
                         enclosing_scope_id,
                         expr,
@@ -6086,6 +6092,25 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                     .map_type(|ty| {
                         self.narrow_place_with_applicable_constraints(expr, ty, &constraint_keys)
                     });
+                    // We could have Place::Unbound here, despite the checks above, for example if
+                    // this scope contains a `del` statement but no binding or declaration.
+                    if let Place::Type(type_, boundness) = local_place_and_qualifiers.place {
+                        nonlocal_union_builder.add_in_place(type_);
+                        // `ConsideredDefinitions::AllReachable` never returns PossiblyUnbound
+                        debug_assert_eq!(boundness, Boundness::Bound);
+                        found_some_definition = true;
+                    }
+
+                    if !enclosing_place.is_marked_nonlocal() {
+                        // We've reached a function-like scope that marks this name bound or
+                        // declared but doesn't mark it `nonlocal`. The name is therefore resolved,
+                        // and we won't consider any scopes outside of this one.
+                        return if found_some_definition {
+                            Place::Type(nonlocal_union_builder.build(), Boundness::Bound).into()
+                        } else {
+                            Place::Unbound.into()
+                        };
+                    }
                 }
             }