Only use all bindings for within-module lookups

Author: sharkdp

crates/ty_python_semantic/resources/mdtest/public_types.md

@@ -89,8 +89,6 @@ def outer(flag: bool) -> None:
         x = A()
 
         def inner() -> None:
-            # TODO: this should not be an error
-            # error: [possibly-unresolved-reference]
             reveal_type(x)  # revealed: Unknown | A
         inner()
 ```
@@ -103,9 +101,7 @@ def outer(flag: bool) -> None:
         x = A()
 
     def inner() -> None:
-        # TODO: currently an error (good), but this diagnostic might go away if
-        # we try to silence the one above.
-        # error: [possibly-unresolved-reference]
+        # TODO: Ideally, we would emit a possibly-unresolved-reference error here.
         reveal_type(x)  # revealed: Unknown | A
     inner()
 ```
@@ -165,7 +161,6 @@ if flag():
     x = 1
 
     def f() -> None:
-        # error: [possibly-unresolved-reference]
         reveal_type(x)  # revealed: Unknown | Literal[1, 2]
     # Function only used inside this branch
     f()

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

@@ -13,7 +13,8 @@ if returns_bool():
     chr: int = 1
 
 def f():
-    reveal_type(chr)  # revealed: int | (def chr(i: SupportsIndex, /) -> str)
+    # TODO: should be `int | (def chr(i: SupportsIndex, /) -> str)`
+    reveal_type(chr)  # revealed: int
 ```
 
 ## Conditionally global or builtin, with annotation
@@ -28,5 +29,6 @@ if returns_bool():
     chr: int = 1
 
 def f():
-    reveal_type(chr)  # revealed: int | (def chr(i: SupportsIndex, /) -> str)
+    # TODO: should be `int | (def chr(i: SupportsIndex, /) -> str)`
+    reveal_type(chr)  # revealed: int
 ```

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

@@ -389,7 +389,8 @@ x = int
 class C:
     var: ClassVar[x]
 
-reveal_type(C.var)  # revealed: Unknown | str
+# TODO: should be `Unknown | str`
+reveal_type(C.var)  # revealed: Unknown | int | str
 
 x = str
 ```
@@ -404,7 +405,8 @@ x = int
 class C:
     var: ClassVar[x]
 
-reveal_type(C.var)  # revealed: str
+# TODO: should be `str`
+reveal_type(C.var)  # revealed: int | str
 
 x = str
 ```

crates/ty_python_semantic/resources/mdtest/statically_known_branches.md

@@ -1478,6 +1478,8 @@ if False:
 ```py
 # error: [unresolved-import]
 from module import symbol
+
+reveal_type(symbol)  # revealed: Unknown
 ```
 
 #### Always true, bound

crates/ty_python_semantic/src/place.rs

@@ -1,4 +1,5 @@
 use ruff_db::files::File;
+use salsa::Update;
 
 use crate::dunder_all::dunder_all_names;
 use crate::module_resolver::file_to_module;
@@ -202,8 +203,15 @@ pub(crate) fn symbol<'db>(
     db: &'db dyn Db,
     scope: ScopeId<'db>,
     name: &str,
+    considered_bindings: ConsideredBindings,
 ) -> PlaceAndQualifiers<'db> {
-    symbol_impl(db, scope, name, RequiresExplicitReExport::No)
+    symbol_impl(
+        db,
+        scope,
+        name,
+        RequiresExplicitReExport::No,
+        considered_bindings,
+    )
 }
 
 /// Infer the public type of a place (its type as seen from outside its scope) in the given
@@ -212,8 +220,15 @@ pub(crate) fn place<'db>(
     db: &'db dyn Db,
     scope: ScopeId<'db>,
     expr: &PlaceExpr,
+    considered_bindings: ConsideredBindings,
 ) -> PlaceAndQualifiers<'db> {
-    place_impl(db, scope, expr, RequiresExplicitReExport::No)
+    place_impl(
+        db,
+        scope,
+        expr,
+        RequiresExplicitReExport::No,
+        considered_bindings,
+    )
 }
 
 /// Infer the public type of a class symbol (its type as seen from outside its scope) in the given
@@ -226,7 +241,13 @@ pub(crate) fn class_symbol<'db>(
     place_table(db, scope)
         .place_id_by_name(name)
         .map(|symbol| {
-            let symbol_and_quals = place_by_id(db, scope, symbol, RequiresExplicitReExport::No);
+            let symbol_and_quals = place_by_id(
+                db,
+                scope,
+                symbol,
+                RequiresExplicitReExport::No,
+                ConsideredBindings::LiveBindingsAtUse,
+            );
 
             if symbol_and_quals.is_class_var() {
                 // For declared class vars we do not need to check if they have bindings,
@@ -241,8 +262,13 @@ pub(crate) fn class_symbol<'db>(
             {
                 // Otherwise, we need to check if the symbol has bindings
                 let use_def = use_def_map(db, scope);
-                let bindings = use_def.public_bindings(symbol);
-                let inferred = place_from_bindings_impl(db, bindings, RequiresExplicitReExport::No);
+                let bindings = use_def.end_of_scope_bindings(symbol);
+                let inferred = place_from_bindings_impl(
+                    db,
+                    bindings,
+                    RequiresExplicitReExport::No,
+                    ConsideredBindings::LiveBindingsAtUse,
+                );
 
                 // TODO: we should not need to calculate inferred type second time. This is a temporary
                 // solution until the notion of Boundness and Declaredness is split. See #16036, #16264
@@ -277,6 +303,7 @@ pub(crate) fn explicit_global_symbol<'db>(
         global_scope(db, file),
         name,
         RequiresExplicitReExport::No,
+        ConsideredBindings::AllReachable,
     )
 }
 
@@ -330,18 +357,22 @@ pub(crate) fn imported_symbol<'db>(
     // ignore `__getattr__`. Typeshed has a fake `__getattr__` on `types.ModuleType` to help out with
     // dynamic imports; we shouldn't use it for `ModuleLiteral` types where we know exactly which
     // module we're dealing with.
-    symbol_impl(db, global_scope(db, file), name, requires_explicit_reexport).or_fall_back_to(
+    symbol_impl(
         db,
-        || {
-            if name == "__getattr__" {
-                Place::Unbound.into()
-            } else if name == "__builtins__" {
-                Place::bound(Type::any()).into()
-            } else {
-                KnownClass::ModuleType.to_instance(db).member(db, name)
-            }
-        },
+        global_scope(db, file),
+        name,
+        requires_explicit_reexport,
+        ConsideredBindings::LiveBindingsAtUse,
     )
+    .or_fall_back_to(db, || {
+        if name == "__getattr__" {
+            Place::Unbound.into()
+        } else if name == "__builtins__" {
+            Place::bound(Type::any()).into()
+        } else {
+            KnownClass::ModuleType.to_instance(db).member(db, name)
+        }
+    })
 }
 
 /// Lookup the type of `symbol` in the builtins namespace.
@@ -361,6 +392,7 @@ pub(crate) fn builtins_symbol<'db>(db: &'db dyn Db, symbol: &str) -> PlaceAndQua
                     global_scope(db, file),
                     symbol,
                     RequiresExplicitReExport::Yes,
+                    ConsideredBindings::LiveBindingsAtUse,
                 )
                 .or_fall_back_to(db, || {
                     // We're looking up in the builtins namespace and not the module, so we should
@@ -431,8 +463,14 @@ fn core_module_scope(db: &dyn Db, core_module: KnownModule) -> Option<ScopeId<'_
 pub(super) fn place_from_bindings<'db>(
     db: &'db dyn Db,
     bindings_with_constraints: BindingWithConstraintsIterator<'_, 'db>,
+    considered_bindings: ConsideredBindings, // TODO: remove this and always use `LiveBindingsAtUse` here?
 ) -> Place<'db> {
-    place_from_bindings_impl(db, bindings_with_constraints, RequiresExplicitReExport::No)
+    place_from_bindings_impl(
+        db,
+        bindings_with_constraints,
+        RequiresExplicitReExport::No,
+        considered_bindings,
+    )
 }
 
 /// Build a declared type from a [`DeclarationsIterator`].
@@ -581,6 +619,7 @@ fn place_cycle_recover<'db>(
     _scope: ScopeId<'db>,
     _place_id: ScopedPlaceId,
     _requires_explicit_reexport: RequiresExplicitReExport,
+    _considered_bindings: ConsideredBindings,
 ) -> salsa::CycleRecoveryAction<PlaceAndQualifiers<'db>> {
     salsa::CycleRecoveryAction::Iterate
 }
@@ -590,6 +629,7 @@ fn place_cycle_initial<'db>(
     _scope: ScopeId<'db>,
     _place_id: ScopedPlaceId,
     _requires_explicit_reexport: RequiresExplicitReExport,
+    _considered_bindings: ConsideredBindings,
 ) -> PlaceAndQualifiers<'db> {
     Place::bound(Type::Never).into()
 }
@@ -600,6 +640,7 @@ fn place_by_id<'db>(
     scope: ScopeId<'db>,
     place_id: ScopedPlaceId,
     requires_explicit_reexport: RequiresExplicitReExport,
+    considered_bindings: ConsideredBindings,
 ) -> PlaceAndQualifiers<'db> {
     let use_def = use_def_map(db, scope);
 
@@ -609,6 +650,11 @@ fn place_by_id<'db>(
     let declarations = use_def.public_declarations(place_id);
     let declared = place_from_declarations_impl(db, declarations, requires_explicit_reexport);
 
+    let all_considered_bindings = || match considered_bindings {
+        ConsideredBindings::LiveBindingsAtUse => use_def.end_of_scope_bindings(place_id),
+        ConsideredBindings::AllReachable => use_def.potentially_reachable_bindings(place_id),
+    };
+
     match declared {
         // Place is declared, trust the declared type
         Ok(
@@ -622,8 +668,13 @@ fn place_by_id<'db>(
             place: Place::Type(declared_ty, Boundness::PossiblyUnbound),
             qualifiers,
         }) => {
-            let bindings = use_def.all_bindings(place_id);
-            let inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport);
+            let bindings = all_considered_bindings();
+            let inferred = place_from_bindings_impl(
+                db,
+                bindings,
+                requires_explicit_reexport,
+                considered_bindings,
+            );
 
             let place = match inferred {
                 // Place is possibly undeclared and definitely unbound
@@ -647,8 +698,13 @@ fn place_by_id<'db>(
             place: Place::Unbound,
             qualifiers: _,
         }) => {
-            let bindings = use_def.all_bindings(place_id);
-            let inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport);
+            let bindings = all_considered_bindings();
+            let inferred = place_from_bindings_impl(
+                db,
+                bindings,
+                requires_explicit_reexport,
+                considered_bindings,
+            );
 
             // `__slots__` is a symbol with special behavior in Python's runtime. It can be
             // modified externally, but those changes do not take effect. We therefore issue
@@ -707,6 +763,7 @@ fn symbol_impl<'db>(
     scope: ScopeId<'db>,
     name: &str,
     requires_explicit_reexport: RequiresExplicitReExport,
+    considered_bindings: ConsideredBindings,
 ) -> PlaceAndQualifiers<'db> {
     let _span = tracing::trace_span!("symbol", ?name).entered();
 
@@ -726,7 +783,15 @@ fn symbol_impl<'db>(
 
     place_table(db, scope)
         .place_id_by_name(name)
-        .map(|symbol| place_by_id(db, scope, symbol, requires_explicit_reexport))
+        .map(|symbol| {
+            place_by_id(
+                db,
+                scope,
+                symbol,
+                requires_explicit_reexport,
+                considered_bindings,
+            )
+        })
         .unwrap_or_default()
 }
 
@@ -736,12 +801,21 @@ fn place_impl<'db>(
     scope: ScopeId<'db>,
     expr: &PlaceExpr,
     requires_explicit_reexport: RequiresExplicitReExport,
+    considered_bindings: ConsideredBindings,
 ) -> PlaceAndQualifiers<'db> {
     let _span = tracing::trace_span!("place", ?expr).entered();
 
     place_table(db, scope)
         .place_id_by_expr(expr)
-        .map(|place| place_by_id(db, scope, place, requires_explicit_reexport))
+        .map(|place| {
+            place_by_id(
+                db,
+                scope,
+                place,
+                requires_explicit_reexport,
+                considered_bindings,
+            )
+        })
         .unwrap_or_default()
 }
 
@@ -754,6 +828,7 @@ fn place_from_bindings_impl<'db>(
     db: &'db dyn Db,
     bindings_with_constraints: BindingWithConstraintsIterator<'_, 'db>,
     requires_explicit_reexport: RequiresExplicitReExport,
+    considered_bindings: ConsideredBindings,
 ) -> Place<'db> {
     let predicates = bindings_with_constraints.predicates;
     let reachability_constraints = bindings_with_constraints.reachability_constraints;
@@ -868,15 +943,24 @@ fn place_from_bindings_impl<'db>(
     );
 
     if let Some(first) = types.next() {
-        let boundness = match unbound_reachability() {
-            Some(Truthiness::AlwaysTrue) => {
-                // unreachable!(
-                //     "If we have at least one binding, the implicit `unbound` binding should not be definitely visible"
-                // )
-                Boundness::Bound // TODO
+        let boundness = match considered_bindings {
+            ConsideredBindings::AllReachable => {
+                // TODO: explain why we do this
+                Boundness::Bound
+            }
+            ConsideredBindings::LiveBindingsAtUse => {
+                // If we have at least one binding, the implicit `unbound` binding should not be
+                // definitely visible.
+                match unbound_reachability() {
+                    Some(Truthiness::AlwaysTrue) => {
+                        unreachable!(
+                            "If we have at least one binding, the implicit `unbound` binding should not be definitely visible"
+                        )
+                    }
+                    Some(Truthiness::AlwaysFalse) | None => Boundness::Bound,
+                    Some(Truthiness::Ambiguous) => Boundness::PossiblyUnbound,
+                }
             }
-            Some(Truthiness::AlwaysFalse) | None => Boundness::Bound,
-            Some(Truthiness::Ambiguous) => Boundness::PossiblyUnbound,
         };
 
         let ty = if let Some(second) = types.next() {
@@ -1166,6 +1250,12 @@ impl RequiresExplicitReExport {
     }
 }
 
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Update)]
+pub(crate) enum ConsideredBindings {
+    AllReachable,
+    LiveBindingsAtUse,
+}
+
 /// Computes a possibly-widened type `Unknown | T_inferred` from the inferred type `T_inferred`
 /// of a symbol, unless the type is a known-instance type (e.g. `typing.Any`) or the symbol is
 /// considered non-modifiable (e.g. when the symbol is `@Final`). We need this for public uses

crates/ty_python_semantic/src/semantic_index.rs

@@ -116,7 +116,7 @@ pub(crate) fn attribute_assignments<'db, 's>(
         let place_table = index.place_table(function_scope_id);
         let place = place_table.place_id_by_instance_attribute_name(name)?;
         let use_def = &index.use_def_maps[function_scope_id];
-        Some((use_def.public_bindings(place), function_scope_id))
+        Some((use_def.end_of_scope_bindings(place), function_scope_id))
     })
 }
 
@@ -574,7 +574,7 @@ mod tests {
 
     impl UseDefMap<'_> {
         fn first_public_binding(&self, symbol: ScopedPlaceId) -> Option<Definition<'_>> {
-            self.public_bindings(symbol)
+            self.end_of_scope_bindings(symbol)
                 .find_map(|constrained_binding| constrained_binding.binding.definition())
         }