don't do nested bindings inference if we have a declared type

Author: oconnor663

crates/ty_python_semantic/resources/mdtest/public_types.md

@@ -312,7 +312,7 @@ def outer() -> None:
     set_x()
 
     def inner() -> None:
-        reveal_type(x)  # revealed: None | Literal[1]
+        reveal_type(x)  # revealed: Literal[1] | None
     inner()
 ```
 

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

@@ -201,7 +201,7 @@ x = 42
 
 def f():
     global x
-    reveal_type(x)  # revealed: Unknown | Literal[42, "56"]
+    reveal_type(x)  # revealed: Unknown | Literal["56", 42]
     x = "56"
     reveal_type(x)  # revealed: Literal["56"]
 ```

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

@@ -106,12 +106,12 @@ def a():
                 nonlocal x
                 # It's counterintuitive that 4 gets included here, since we haven't reached the
                 # binding in this scope, but this function might get called more than once.
-                reveal_type(x)  # revealed: Literal[2, 3, 4]
+                reveal_type(x)  # revealed: Literal[3, 4, 2]
                 x = 4
                 reveal_type(x)  # revealed: Literal[4]
 
                 def e():
-                    reveal_type(x)  # revealed: Literal[2, 3, 4]
+                    reveal_type(x)  # revealed: Literal[3, 4, 2]
 ```
 
 In addition to parent scopes, we also consider sibling scopes, child scopes,
@@ -131,7 +131,7 @@ def a():
         def d():
             nonlocal x
             x = 3
-        reveal_type(x)  # revealed: Literal[1, 2, 3]
+        reveal_type(x)  # revealed: Literal[2, 3, 1]
     # `x` is local here, so we don't look at nested scopes.
     reveal_type(x)  # revealed: Literal[1]
 ```

crates/ty_python_semantic/src/place.rs

@@ -651,32 +651,6 @@ fn place_by_id<'db>(
 ) -> PlaceAndQualifiers<'db> {
     let use_def = use_def_map(db, scope);
 
-    // If there are any nested bindings (via `global` or `nonlocal` variables) for this symbol,
-    // infer them and union the results. Nested bindings aren't allowed to have declarations or
-    // qualifiers, and we can just union their inferred types.
-    let mut nested_bindings_union = UnionBuilder::new(db);
-    if let Some(symbol_id) = place_id.as_symbol() {
-        let current_place_table = place_table(db, scope);
-        let symbol = current_place_table.symbol(symbol_id);
-        for nested_file_scope_id in place_table(db, scope).nested_scopes_with_bindings(symbol_id) {
-            let nested_scope_id = nested_file_scope_id.to_scope_id(db, scope.file(db));
-            let nested_place_table = place_table(db, nested_scope_id);
-            let nested_symbol_id = nested_place_table
-                .symbol_id(symbol.name())
-                .expect("nested_scopes_with_bindings says this reference exists");
-            let nested_place = place_by_id(
-                db,
-                nested_scope_id,
-                ScopedPlaceId::Symbol(nested_symbol_id),
-                RequiresExplicitReExport::No,
-                ConsideredDefinitions::AllReachable,
-            );
-            if let Place::Type(nested_type, _) = nested_place.place {
-                nested_bindings_union.add_in_place(nested_type);
-            }
-        }
-    }
-
     // If the place is declared, the public type is based on declarations; otherwise, it's based
     // on inference from bindings.
 
@@ -692,6 +666,47 @@ fn place_by_id<'db>(
         ConsideredDefinitions::AllReachable => use_def.all_reachable_bindings(place_id),
     };
 
+    // If there are any nested bindings (via `global` or `nonlocal` variables) for this symbol,
+    // infer them and union the results. Note that this is potentially recursive, and we can
+    // trigger fixed-point iteration here in cases like this:
+    // ```
+    // def f():
+    //     x = 1
+    //     def g():
+    //         nonlocal x
+    //         x += 1
+    // ```
+    let nested_bindings = || {
+        // For performance reasons, avoid creating a union unless we have more one binding.
+        let mut union = UnionBuilder::new(db);
+        if let Some(symbol_id) = place_id.as_symbol() {
+            let current_place_table = place_table(db, scope);
+            let symbol = current_place_table.symbol(symbol_id);
+            for &nested_file_scope_id in
+                place_table(db, scope).nested_scopes_with_bindings(symbol_id)
+            {
+                let nested_scope_id = nested_file_scope_id.to_scope_id(db, scope.file(db));
+                let nested_place_table = place_table(db, nested_scope_id);
+                let nested_symbol_id = nested_place_table
+                    .symbol_id(symbol.name())
+                    .expect("nested_scopes_with_bindings says this reference exists");
+                let place = place_by_id(
+                    db,
+                    nested_scope_id,
+                    ScopedPlaceId::Symbol(nested_symbol_id),
+                    RequiresExplicitReExport::No,
+                    ConsideredDefinitions::AllReachable,
+                );
+                // Nested bindings aren't allowed to have declarations or qualifiers, so we can
+                // just extract their inferred types.
+                if let Place::Type(nested_type, _) = place.place {
+                    union.add_in_place(nested_type);
+                }
+            }
+        }
+        union
+    };
+
     // If a symbol is undeclared, but qualified with `typing.Final`, we use the right-hand side
     // inferred type, without unioning with `Unknown`, because it can not be modified.
     if let Some(qualifiers) = declared
@@ -751,17 +766,11 @@ fn place_by_id<'db>(
                     // TODO: We probably don't want to report `Bound` here. This requires a bit of
                     // design work though as we might want a different behavior for stubs and for
                     // normal modules.
-                    Place::Type(
-                        nested_bindings_union.add(declared_ty).build(),
-                        Boundness::Bound,
-                    )
+                    Place::Type(nested_bindings().add(declared_ty).build(), Boundness::Bound)
                 }
                 // Place is possibly undeclared and (possibly) bound
                 Place::Type(inferred_ty, boundness) => Place::Type(
-                    nested_bindings_union
-                        .add(inferred_ty)
-                        .add(declared_ty)
-                        .build(),
+                    nested_bindings().add(inferred_ty).add(declared_ty).build(),
                     if boundness_analysis == BoundnessAnalysis::AssumeBound {
                         Boundness::Bound
                     } else {
@@ -784,13 +793,12 @@ fn place_by_id<'db>(
 
             // If there are nested bindings, union whatever we inferred from those into what we've
             // inferred here.
-            if let Some(nested_bindings_type) = nested_bindings_union.try_build() {
-                match &mut inferred {
-                    Place::Type(inferred_type, _) => {
-                        *inferred_type =
-                            UnionType::from_elements(db, [*inferred_type, nested_bindings_type]);
-                    }
-                    Place::Unbound => {
+            match &mut inferred {
+                Place::Type(inferred_type, _) => {
+                    *inferred_type = nested_bindings().add(*inferred_type).build();
+                }
+                Place::Unbound => {
+                    if let Some(nested_bindings_type) = nested_bindings().try_build() {
                         inferred = Place::Type(nested_bindings_type, Boundness::PossiblyUnbound);
                     }
                 }