[ty] Use all reachable bindings for instance attributes and deferred lookups (#18955)

## Summary

Remove a hack in control flow modeling that was treating `return`
statements at the end of function bodies in a special way (basically
considering the state *just before* the `return` statement as the
end-of-scope state). This is not needed anymore now that #18750 has been
merged.

In order to make this work, we now use *all reachable bindings* for
purposes of finding implicit instance attribute assignments as well as
for deferred lookups of symbols. Both would otherwise be affected by
this change:
```py
def C:
    def f(self):
        self.x = 1  # a reachable binding that is not visible at the end of the scope
        return
```

```py
def f():
    class X: ...  # a reachable binding that is not visible at the end of the scope
    x: "X" = X()  # deferred use of `X`
    return
```

Implicit instance attributes also required another change. We previously
kept track of possibly-unbound instance attributes in some cases, but we
now give up on that completely and always consider *implicit* instance
attributes to be bound if we see a reachable binding in a reachable
method. The previous behavior was somewhat inconsistent anyway because
we also do not consider attributes possibly-unbound in other scenarios:
we do not (and can not) keep track of whether or not methods are called
that define these attributes.

closes astral-sh/ty#711

## Ecosystem analysis

I think this looks very positive!

* We see an unsurprising drop in `possibly-unbound-attribute`
diagnostics (599), mostly for classes that define attributes in `try …
except` blocks, `for` loops, or `if … else: raise …` constructs. There
might obviously also be true positives that got removed, but the vast
majority should be false positives.
* There is also a drop in `possibly-unresolved-reference` /
`unresolved-reference` diagnostics (279+13) from the change to deferred
lookups.
* Some `invalid-type-form` false positives got resolved (13), because we
can now properly look up the names in the annotations.
* There are some new *true* positives in `attrs`, since we understand
the `Attribute` annotation that was previously inferred as `Unknown`
because of a re-assignment after the class definition.


## Test Plan

The existing attributes.md test suite has sufficient coverage here.

Author: sharkdp

crates/ty_python_semantic/resources/mdtest/attributes.md

@@ -43,7 +43,6 @@ reveal_type(c_instance.declared_only)  # revealed: Unknown
 
 reveal_type(c_instance.declared_and_bound)  # revealed: bool
 
-# error: [possibly-unbound-attribute]
 reveal_type(c_instance.possibly_undeclared_unbound)  # revealed: str
 
 # This assignment is fine, as we infer `Unknown | Literal[1, "a"]` for `inferred_from_value`.
@@ -265,7 +264,7 @@ class C:
 
 # TODO: Mypy and pyright do not support this, but it would be great if we could
 # infer `Unknown | str` here (`Weird` is not a possible type for the `w` attribute).
-reveal_type(C().w)  # revealed: Unknown
+reveal_type(C().w)  # revealed: Unknown | Weird
 ```
 
 #### Attributes defined in tuple unpackings
@@ -342,10 +341,7 @@ class C:
         for self.z in NonIterable():
             pass
 
-# Iterable might be empty
-# error: [possibly-unbound-attribute]
 reveal_type(C().x)  # revealed: Unknown | int
-# error: [possibly-unbound-attribute]
 reveal_type(C().y)  # revealed: Unknown | str
 ```
 
@@ -453,8 +449,8 @@ reveal_type(c_instance.g)  # revealed: Unknown
 
 #### Conditionally declared / bound attributes
 
-Attributes are possibly unbound if they, or the method to which they are added are conditionally
-declared / bound.
+We currently treat implicit instance attributes to be bound, even if they are only conditionally
+defined:
 
 ```py
 def flag() -> bool:
@@ -472,13 +468,9 @@ class C:
 
 c_instance = C()
 
-# error: [possibly-unbound-attribute]
 reveal_type(c_instance.a1)  # revealed: str | None
-# error: [possibly-unbound-attribute]
 reveal_type(c_instance.a2)  # revealed: str | None
-# error: [possibly-unbound-attribute]
 reveal_type(c_instance.b1)  # revealed: Unknown | Literal[1]
-# error: [possibly-unbound-attribute]
 reveal_type(c_instance.b2)  # revealed: Unknown | Literal[1]
 ```
 
@@ -620,8 +612,10 @@ reveal_type(C(True).a)  # revealed: Unknown | Literal[1]
 # error: [unresolved-attribute]
 reveal_type(C(True).b)  # revealed: Unknown
 reveal_type(C(True).c)  # revealed: Unknown | Literal[3] | str
-# TODO: this attribute is possibly unbound
-reveal_type(C(True).d)  # revealed: Unknown | Literal[5]
+# Ideally, this would just be `Unknown | Literal[5]`, but we currently do not
+# attempt to analyze control flow within methods more closely. All reachable
+# attribute assignments are considered, so `self.x = 4` is also included:
+reveal_type(C(True).d)  # revealed: Unknown | Literal[4, 5]
 # error: [unresolved-attribute]
 reveal_type(C(True).e)  # revealed: Unknown
 ```
@@ -1289,6 +1283,10 @@ def _(flag: bool):
 
 ### Possibly unbound/undeclared instance attribute
 
+We currently treat implicit instance attributes to be bound, even if they are only conditionally
+defined within a method. If the class-level definition or the whole method is only conditionally
+available, we emit a `possibly-unbound-attribute` diagnostic.
+
 #### Possibly unbound and undeclared
 
 ```py
@@ -1320,10 +1318,8 @@ def _(flag: bool):
             else:
                 self.y = "b"
 
-    # error: [possibly-unbound-attribute]
     reveal_type(Foo().x)  # revealed: Unknown | Literal[1]
 
-    # error: [possibly-unbound-attribute]
     Foo().x = 2
 
     reveal_type(Foo().y)  # revealed: Unknown | Literal["a", "b"]

crates/ty_python_semantic/src/place.rs

@@ -62,10 +62,6 @@ impl<'db> Place<'db> {
         Place::Type(ty.into(), Boundness::Bound)
     }
 
-    pub(crate) fn possibly_unbound(ty: impl Into<Type<'db>>) -> Self {
-        Place::Type(ty.into(), Boundness::PossiblyUnbound)
-    }
-
     /// Constructor that creates a [`Place`] with a [`crate::types::TodoType`] type
     /// and boundness [`Boundness::Bound`].
     #[allow(unused_variables)] // Only unused in release builds

crates/ty_python_semantic/src/semantic_index.rs

@@ -118,7 +118,7 @@ pub(crate) fn attribute_assignments<'db, 's>(
         let place = place_table.place_id_by_instance_attribute_name(name)?;
         let use_def = &index.use_def_maps[function_scope_id];
         Some((
-            use_def.inner.end_of_scope_bindings(place),
+            use_def.inner.all_reachable_bindings(place),
             function_scope_id,
         ))
     })

crates/ty_python_semantic/src/semantic_index/builder.rs

@@ -1122,29 +1122,7 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> {
                             &mut first_parameter_name,
                         );
 
-                        // TODO: Fix how we determine the public types of symbols in a
-                        // function-like scope: https://github.com/astral-sh/ruff/issues/15777
-                        //
-                        // In the meantime, visit the function body, but treat the last statement
-                        // specially if it is a return. If it is, this would cause all definitions
-                        // in the function to be marked as non-visible with our current treatment
-                        // of terminal statements. Since we currently model the externally visible
-                        // definitions in a function scope as the set of bindings that are visible
-                        // at the end of the body, we then consider this function to have no
-                        // externally visible definitions. To get around this, we take a flow
-                        // snapshot just before processing the return statement, and use _that_ as
-                        // the "end-of-body" state that we resolve external references against.
-                        if let Some((last_stmt, first_stmts)) = body.split_last() {
-                            builder.visit_body(first_stmts);
-                            let pre_return_state = matches!(last_stmt, ast::Stmt::Return(_))
-                                .then(|| builder.flow_snapshot());
-                            builder.visit_stmt(last_stmt);
-                            let reachability = builder.current_use_def_map().reachability;
-                            if let Some(pre_return_state) = pre_return_state {
-                                builder.flow_restore(pre_return_state);
-                                builder.current_use_def_map_mut().reachability = reachability;
-                            }
-                        }
+                        builder.visit_body(body);
 
                         builder.current_first_parameter_name = first_parameter_name;
                         builder.pop_scope()

crates/ty_python_semantic/src/types/class.rs

@@ -1740,7 +1740,7 @@ impl<'db> ClassLiteral<'db> {
         // attribute might be externally modified.
         let mut union_of_inferred_types = UnionBuilder::new(db).add(Type::unknown());
 
-        let mut is_attribute_bound = Truthiness::AlwaysFalse;
+        let mut is_attribute_bound = false;
 
         let file = class_body_scope.file(db);
         let module = parsed_module(db, file).load(db);
@@ -1771,7 +1771,7 @@ impl<'db> ClassLiteral<'db> {
                     let method = index.expect_single_definition(method_def);
                     let method_place = class_table.place_id_by_name(&method_def.name).unwrap();
                     class_map
-                        .end_of_scope_bindings(method_place)
+                        .all_reachable_bindings(method_place)
                         .find_map(|bind| {
                             (bind.binding.is_defined_and(|def| def == method))
                                 .then(|| class_map.is_binding_reachable(db, &bind))
@@ -1806,13 +1806,8 @@ impl<'db> ClassLiteral<'db> {
                     .is_binding_reachable(db, &attribute_assignment)
                     .and(is_method_reachable)
                 {
-                    Truthiness::AlwaysTrue => {
-                        is_attribute_bound = Truthiness::AlwaysTrue;
-                    }
-                    Truthiness::Ambiguous => {
-                        if is_attribute_bound.is_always_false() {
-                            is_attribute_bound = Truthiness::Ambiguous;
-                        }
+                    Truthiness::AlwaysTrue | Truthiness::Ambiguous => {
+                        is_attribute_bound = true;
                     }
                     Truthiness::AlwaysFalse => {
                         continue;
@@ -1832,7 +1827,7 @@ impl<'db> ClassLiteral<'db> {
                     .and(is_method_reachable)
                     .is_always_true()
                 {
-                    is_attribute_bound = Truthiness::AlwaysTrue;
+                    is_attribute_bound = true;
                 }
 
                 match binding.kind(db) {
@@ -1849,17 +1844,12 @@ impl<'db> ClassLiteral<'db> {
                         );
 
                         // TODO: check if there are conflicting declarations
-                        match is_attribute_bound {
-                            Truthiness::AlwaysTrue => {
-                                return Place::bound(annotation_ty);
-                            }
-                            Truthiness::Ambiguous => {
-                                return Place::possibly_unbound(annotation_ty);
-                            }
-                            Truthiness::AlwaysFalse => unreachable!(
-                                "If the attribute assignments are all invisible, inference of their types should be skipped"
-                            ),
+                        if is_attribute_bound {
+                            return Place::bound(annotation_ty);
                         }
+                        unreachable!(
+                            "If the attribute assignments are all invisible, inference of their types should be skipped"
+                        );
                     }
                     DefinitionKind::Assignment(assign) => {
                         match assign.target_kind() {
@@ -1995,10 +1985,10 @@ impl<'db> ClassLiteral<'db> {
             }
         }
 
-        match is_attribute_bound {
-            Truthiness::AlwaysTrue => Place::bound(union_of_inferred_types.build()),
-            Truthiness::Ambiguous => Place::possibly_unbound(union_of_inferred_types.build()),
-            Truthiness::AlwaysFalse => Place::Unbound,
+        if is_attribute_bound {
+            Place::bound(union_of_inferred_types.build())
+        } else {
+            Place::Unbound
         }
     }
 

crates/ty_python_semantic/src/types/infer.rs

@@ -5665,7 +5665,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
         // If we're inferring types of deferred expressions, always treat them as public symbols
         if self.is_deferred() {
             let place = if let Some(place_id) = place_table.place_id_by_expr(expr) {
-                place_from_bindings(db, use_def.end_of_scope_bindings(place_id))
+                place_from_bindings(db, use_def.all_reachable_bindings(place_id))
             } else {
                 assert!(
                     self.deferred_state.in_string_annotation(),

crates/ty_python_semantic/src/types/signatures.rs

@@ -1650,8 +1650,8 @@ mod tests {
             panic!("expected one positional-or-keyword parameter");
         };
         assert_eq!(name, "a");
-        // Parameter resolution deferred; we should see B
-        assert_eq!(annotated_type.unwrap().display(&db).to_string(), "B");
+        // Parameter resolution deferred:
+        assert_eq!(annotated_type.unwrap().display(&db).to_string(), "A | B");
     }
 
     #[test]