[ty] Model reachability of star import definitions for nonlocal lookups (#19066)

## Summary

Temporarily modify `UseDefMapBuilder::reachability` for star imports in
order for new definitions to pick up the right reachability. This was
already working for `UseDefMapBuilder::place_states`, but not for
`UseDefMapBuilder::reachable_definitions`.

closes https://github.com/astral-sh/ty/issues/728

## Test Plan

Regression test

crates/ty_python_semantic/resources/mdtest/import/star.md

@@ -655,7 +655,7 @@ from b import *
 reveal_type(X)  # revealed: bool
 ```
 
-## Visibility constraints
+## Reachability constraints
 
 If an `importer` module contains a `from exporter import *` statement in its global namespace, the
 statement will *not* necessarily import *all* symbols that have definitions in `exporter.py`'s
@@ -664,13 +664,13 @@ imported by the `*` import if at least one definition for that symbol is visible
 `exporter.py`'s global scope.
 
 For example, say that `exporter.py` contains a symbol `X` in its global scope, and the definition
-for `X` in `exporter.py` has visibility constraints <code>vis<sub>1</sub></code>. The
+for `X` in `exporter.py` has reachability constraints <code>c<sub>1</sub></code>. The
 `from exporter import *` statement in `importer.py` creates a definition for `X` in `importer`, and
-there are visibility constraints <code>vis<sub>2</sub></code> on the import statement in
+there are reachability constraints <code>c<sub>2</sub></code> on the import statement in
-`importer.py`. This means that the overall visibility constraints on the `X` definnition created by
+`importer.py`. This means that the overall reachability constraints on the `X` definition created by
-the import statement in `importer.py` will be <code>vis<sub>1</sub> AND vis<sub>2</sub></code>.
+the import statement in `importer.py` will be <code>c<sub>1</sub> AND c<sub>2</sub></code>.
 
-A visibility constraint in the external module must be understood and evaluated whether or not its
+A reachability constraint in the external module must be understood and evaluated whether or not its
 truthiness can be statically determined.
 
 ### Statically known branches in the external module
@@ -712,11 +712,19 @@ reveal_type(Y)  # revealed: Unknown
 # to be dead code given the `python-version` configuration.
 # Thus this still reveals `Literal[True]`.
 reveal_type(Z)  # revealed: Literal[True]
+
+# Make sure that reachability constraints are also correctly applied
+# for nonlocal lookups:
+def _():
+    reveal_type(X)  # revealed: bool
+    # error: [unresolved-reference]
+    reveal_type(Y)  # revealed: Unknown
+    reveal_type(Z)  # revealed: bool
 ```
 
-### Multiple `*` imports with always-false visibility constraints
+### Multiple `*` imports with always-false reachability constraints
 
-Our understanding of visibility constraints in an external module remains accurate, even if there
+Our understanding of reachability constraints in an external module remains accurate, even if there
 are multiple `*` imports from that module.
 
 ```toml
@@ -745,7 +753,7 @@ from exporter import *
 reveal_type(Z)  # revealed: Literal[True]
 ```
 
-### Ambiguous visibility constraints
+### Ambiguous reachability constraints
 
 Some constraints in the external module may resolve to an "ambiguous truthiness". For these, we
 should emit `possibly-unresolved-reference` diagnostics when they are used in the module in which
@@ -775,7 +783,7 @@ reveal_type(A)  # revealed: Unknown | Literal[1]
 reveal_type(B)  # revealed: Unknown | Literal[2, 3]
 ```
 
-### Visibility constraints in the importing module
+### Reachability constraints in the importing module
 
 `exporter.py`:
 
@@ -796,7 +804,7 @@ if coinflip():
 reveal_type(A)  # revealed: Unknown | Literal[1]
 ```
 
-### Visibility constraints in the exporting module *and* the importing module
+### Reachability constraints in the exporting module *and* the importing module
 
 ```toml
 [environment]

crates/ty_python_semantic/resources/mdtest/terminal_statements.md

@@ -622,9 +622,9 @@ def return_from_nested_if(cond1: bool, cond2: bool):
 
 ## Statically known terminal statements
 
-We model reachability using the same visibility constraints that we use to model statically known
+We model reachability using the same constraints that we use to model statically known bounds. In
-bounds. In this example, we see that the `return` statement is always executed, and therefore that
+this example, we see that the `return` statement is always executed, and therefore that the `"b"`
-the `"b"` assignment is not visible to the `reveal_type`.
+assignment is not visible to the `reveal_type`.
 
 ```py
 def _(cond: bool):

crates/ty_python_semantic/src/semantic_index/builder.rs

@@ -1316,15 +1316,38 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> {
                                 referenced_module,
                             );
 
+                            let star_import_predicate = self.add_predicate(star_import.into());
+
                             let pre_definition =
                                 self.current_use_def_map().single_place_snapshot(symbol_id);
+                            let pre_definition_reachability =
+                                self.current_use_def_map().reachability;
+
+                            // Temporarily modify the reachability to include the star import predicate,
+                            // in order for the new definition to pick it up.
+                            let reachability_constraints =
+                                &mut self.current_use_def_map_mut().reachability_constraints;
+                            let star_import_reachability =
+                                reachability_constraints.add_atom(star_import_predicate);
+                            let definition_reachability = reachability_constraints
+                                .add_and_constraint(
+                                    pre_definition_reachability,
+                                    star_import_reachability,
+                                );
+                            self.current_use_def_map_mut().reachability = definition_reachability;
+
                             self.push_additional_definition(symbol_id, node_ref);
+
                             self.current_use_def_map_mut()
                                 .record_and_negate_star_import_reachability_constraint(
-                                    star_import,
+                                    star_import_reachability,
                                     symbol_id,
                                     pre_definition,
                                 );
+
+                            // Restore the reachability to its pre-definition state
+                            self.current_use_def_map_mut().reachability =
+                                pre_definition_reachability;
                         }
 
                         continue;

crates/ty_python_semantic/src/semantic_index/use_def.rs

@@ -248,7 +248,6 @@ use crate::semantic_index::place::{
 };
 use crate::semantic_index::predicate::{
     Predicate, PredicateOrLiteral, Predicates, PredicatesBuilder, ScopedPredicateId,
-    StarImportPlaceholderPredicate,
 };
 use crate::semantic_index::reachability_constraints::{
     ReachabilityConstraints, ReachabilityConstraintsBuilder, ScopedReachabilityConstraintId,
@@ -844,7 +843,7 @@ impl<'db> UseDefMapBuilder<'db> {
     /// This method exists solely for handling `*`-import reachability constraints.
     ///
     /// The reason why we add reachability constraints for [`Definition`]s created by `*` imports
-    /// is laid out in the doc-comment for [`StarImportPlaceholderPredicate`]. But treating these
+    /// is laid out in the doc-comment for `StarImportPlaceholderPredicate`. But treating these
     /// reachability constraints in the use-def map the same way as all other reachability constraints
     /// was shown to lead to [significant regressions] for small codebases where typeshed
     /// dominates. (Although `*` imports are not common generally, they are used in several
@@ -872,12 +871,10 @@ impl<'db> UseDefMapBuilder<'db> {
     /// [significant regressions]: https://github.com/astral-sh/ruff/pull/17286#issuecomment-2786755746
     pub(super) fn record_and_negate_star_import_reachability_constraint(
         &mut self,
-        star_import: StarImportPlaceholderPredicate<'db>,
+        reachability_id: ScopedReachabilityConstraintId,
         symbol: ScopedPlaceId,
         pre_definition_state: PlaceState,
     ) {
-        let predicate_id = self.add_predicate(star_import.into());
-        let reachability_id = self.reachability_constraints.add_atom(predicate_id);
         let negated_reachability_id = self
             .reachability_constraints
             .add_not_constraint(reachability_id);