[ty] simplify return type of place_from_declarations

Author: carljm

crates/ty_python_semantic/resources/mdtest/annotations/invalid.md

@@ -56,6 +56,7 @@ def _(
 def bar() -> None:
     return None
 
+async def baz(): ...
 async def outer():  # avoid unrelated syntax errors on yield, yield from, and await
     def _(
         a: 1,  # error: [invalid-type-form] "Int literals are not allowed in this context in a type expression"
@@ -69,7 +70,7 @@ async def outer():  # avoid unrelated syntax errors on yield, yield from, and aw
         i: not 1,  # error: [invalid-type-form] "Unary operations are not allowed in type expressions"
         j: lambda: 1,  # error: [invalid-type-form] "`lambda` expressions are not allowed in type expressions"
         k: 1 if True else 2,  # error: [invalid-type-form] "`if` expressions are not allowed in type expressions"
-        l: await 1,  # error: [invalid-type-form] "`await` expressions are not allowed in type expressions"
+        l: await baz(),  # error: [invalid-type-form] "`await` expressions are not allowed in type expressions"
         m: (yield 1),  # error: [invalid-type-form] "`yield` expressions are not allowed in type expressions"
         n: (yield from [1]),  # error: [invalid-type-form] "`yield from` expressions are not allowed in type expressions"
         o: 1 < 2,  # error: [invalid-type-form] "Comparison expressions are not allowed in type expressions"

crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md

@@ -124,6 +124,9 @@ match obj:
 ## `return`, `yield`, `yield from`, and `await` outside function
 
 ```py
+class C:
+    def __await__(self): ...
+
 # error: [invalid-syntax] "`return` statement outside of a function"
 return
 
@@ -135,11 +138,11 @@ yield from []
 
 # error: [invalid-syntax] "`await` statement outside of a function"
 # error: [invalid-syntax] "`await` outside of an asynchronous function"
-await 1
+await C()
 
 def f():
     # error: [invalid-syntax] "`await` outside of an asynchronous function"
-    await 1
+    await C()
 ```
 
 Generators are evaluated lazily, so `await` is allowed, even outside of a function.
@@ -330,7 +333,8 @@ async def elements(n):
 
 def _():
     # error: [invalid-syntax] "`await` outside of an asynchronous function"
-    await 1
+    await elements(1)
+
     # error: [invalid-syntax] "`async for` outside of an asynchronous function"
     async for _ in elements(1):
         ...

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

@@ -40,6 +40,22 @@ class C:
 reveal_type(C.y)  # revealed: Unknown | Literal[1, "abc"]
 ```
 
+## Possibly unbound in class scope with multiple declarations
+
+```py
+def coinflip() -> bool:
+    return True
+
+class C:
+    if coinflip():
+        x: int = 1
+    elif coinflip():
+        x: str = "abc"
+
+# error: [possibly-unbound-attribute]
+reveal_type(C.x)  # revealed: int | str
+```
+
 ## Unbound function local
 
 An unbound function local that has definitions in the scope does not fall back to globals.

crates/ty_python_semantic/src/place.rs

@@ -472,12 +472,56 @@ pub(crate) fn place_from_declarations<'db>(
     place_from_declarations_impl(db, declarations, RequiresExplicitReExport::No)
 }
 
-pub(crate) type DeclaredTypeAndConflictingTypes<'db> =
-    (TypeAndQualifiers<'db>, Box<indexmap::set::Slice<Type<'db>>>);
+type DeclaredTypeAndConflictingTypes<'db> = (
+    TypeAndQualifiers<'db>,
+    Option<Box<indexmap::set::Slice<Type<'db>>>>,
+);
 
 /// The result of looking up a declared type from declarations; see [`place_from_declarations`].
-pub(crate) type PlaceFromDeclarationsResult<'db> =
-    Result<PlaceAndQualifiers<'db>, DeclaredTypeAndConflictingTypes<'db>>;
+pub(crate) struct PlaceFromDeclarationsResult<'db> {
+    place_and_quals: PlaceAndQualifiers<'db>,
+    conflicting_types: Option<Box<indexmap::set::Slice<Type<'db>>>>,
+}
+
+impl<'db> PlaceFromDeclarationsResult<'db> {
+    fn conflict(
+        place_and_quals: PlaceAndQualifiers<'db>,
+        conflicting_types: Box<indexmap::set::Slice<Type<'db>>>,
+    ) -> Self {
+        PlaceFromDeclarationsResult {
+            place_and_quals,
+            conflicting_types: Some(conflicting_types),
+        }
+    }
+
+    pub(crate) fn ignore_conflicting_declarations(self) -> PlaceAndQualifiers<'db> {
+        self.place_and_quals
+    }
+
+    pub(crate) fn with_conflicting_declarations(
+        self,
+    ) -> (
+        PlaceAndQualifiers<'db>,
+        Option<Box<indexmap::set::Slice<Type<'db>>>>,
+    ) {
+        (self.place_and_quals, self.conflicting_types)
+    }
+}
+
+impl<'db> From<PlaceAndQualifiers<'db>> for PlaceFromDeclarationsResult<'db> {
+    fn from(place_and_quals: PlaceAndQualifiers<'db>) -> Self {
+        PlaceFromDeclarationsResult {
+            place_and_quals,
+            conflicting_types: None,
+        }
+    }
+}
+
+impl<'db> From<Place<'db>> for PlaceFromDeclarationsResult<'db> {
+    fn from(place: Place<'db>) -> Self {
+        PlaceFromDeclarationsResult::from(PlaceAndQualifiers::from(place))
+    }
+}
 
 /// A type with declaredness information, and a set of type qualifiers.
 ///
@@ -659,7 +703,8 @@ fn place_by_id<'db>(
         ConsideredDefinitions::AllReachable => use_def.all_reachable_declarations(place_id),
     };
 
-    let declared = place_from_declarations_impl(db, declarations, requires_explicit_reexport);
+    let declared = place_from_declarations_impl(db, declarations, requires_explicit_reexport)
+        .ignore_conflicting_declarations();
 
     let all_considered_bindings = || match considered_definitions {
         ConsideredDefinitions::EndOfScope => use_def.end_of_scope_bindings(place_id),
@@ -668,53 +713,41 @@ fn place_by_id<'db>(
 
     // 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
-        .as_ref()
-        .ok()
-        .and_then(PlaceAndQualifiers::is_bare_final)
-    {
+    if let Some(qualifiers) = declared.is_bare_final() {
         let bindings = all_considered_bindings();
         return place_from_bindings_impl(db, bindings, requires_explicit_reexport)
             .with_qualifiers(qualifiers);
     }
 
-    // Handle bare `ClassVar` annotations by falling back to the union of `Unknown` and the
-    // inferred type.
     match declared {
-        Ok(PlaceAndQualifiers {
+        // Handle bare `ClassVar` annotations by falling back to the union of `Unknown` and the
+        // inferred type.
+        PlaceAndQualifiers {
             place: Place::Type(Type::Dynamic(DynamicType::Unknown), declaredness),
             qualifiers,
-        }) if qualifiers.contains(TypeQualifiers::CLASS_VAR) => {
+        } if qualifiers.contains(TypeQualifiers::CLASS_VAR) => {
             let bindings = all_considered_bindings();
             match place_from_bindings_impl(db, bindings, requires_explicit_reexport) {
-                Place::Type(inferred, boundness) => {
-                    return Place::Type(
-                        UnionType::from_elements(db, [Type::unknown(), inferred]),
-                        boundness,
-                    )
-                    .with_qualifiers(qualifiers);
-                }
+                Place::Type(inferred, boundness) => Place::Type(
+                    UnionType::from_elements(db, [Type::unknown(), inferred]),
+                    boundness,
+                )
+                .with_qualifiers(qualifiers),
                 Place::Unbound => {
-                    return Place::Type(Type::unknown(), declaredness).with_qualifiers(qualifiers);
+                    Place::Type(Type::unknown(), declaredness).with_qualifiers(qualifiers)
                 }
             }
         }
-        _ => {}
-    }
-
-    match declared {
         // Place is declared, trust the declared type
-        Ok(
-            place_and_quals @ PlaceAndQualifiers {
-                place: Place::Type(_, Boundness::Bound),
-                qualifiers: _,
-            },
-        ) => place_and_quals,
+        place_and_quals @ PlaceAndQualifiers {
+            place: Place::Type(_, Boundness::Bound),
+            qualifiers: _,
+        } => place_and_quals,
         // Place is possibly declared
-        Ok(PlaceAndQualifiers {
+        PlaceAndQualifiers {
             place: Place::Type(declared_ty, Boundness::PossiblyUnbound),
             qualifiers,
-        }) => {
+        } => {
             let bindings = all_considered_bindings();
             let boundness_analysis = bindings.boundness_analysis;
             let inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport);
@@ -741,10 +774,10 @@ fn place_by_id<'db>(
             PlaceAndQualifiers { place, qualifiers }
         }
         // Place is undeclared, return the union of `Unknown` with the inferred type
-        Ok(PlaceAndQualifiers {
+        PlaceAndQualifiers {
             place: Place::Unbound,
             qualifiers: _,
-        }) => {
+        } => {
             let bindings = all_considered_bindings();
             let boundness_analysis = bindings.boundness_analysis;
             let mut inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport);
@@ -786,12 +819,6 @@ fn place_by_id<'db>(
                     .into()
             }
         }
-        // Place has conflicting declared types
-        Err((declared, _)) => {
-            // Intentionally ignore conflicting declared types; that's not our problem,
-            // it's the problem of the module we are importing from.
-            Place::bound(declared.inner_type()).with_qualifiers(declared.qualifiers())
-        }
     }
 
     // TODO (ticket: https://github.com/astral-sh/ruff/issues/14297) Our handling of boundness
@@ -1129,18 +1156,20 @@ impl<'db> DeclaredTypeBuilder<'db> {
     }
 
     fn build(mut self) -> DeclaredTypeAndConflictingTypes<'db> {
-        if !self.conflicting_types.is_empty() {
+        let type_and_quals = TypeAndQualifiers::new(self.inner.build(), self.qualifiers);
+        if self.conflicting_types.is_empty() {
+            (type_and_quals, None)
+        } else {
             self.conflicting_types.insert_before(
                 0,
                 self.first_type
                     .expect("there must be a first type if there are conflicting types"),
             );
+            (
+                type_and_quals,
+                Some(self.conflicting_types.into_boxed_slice()),
+            )
         }
-
-        (
-            TypeAndQualifiers::new(self.inner.build(), self.qualifiers),
-            self.conflicting_types.into_boxed_slice(),
-        )
     }
 }
 
@@ -1203,22 +1232,16 @@ fn place_from_declarations_impl<'db>(
     );
 
     if let Some(first) = types.next() {
-        let declared = if let Some(second) = types.next() {
+        let (declared, conflicting) = if let Some(second) = types.next() {
             let mut builder = DeclaredTypeBuilder::new(db);
             builder.add(first);
             builder.add(second);
             for element in types {
                 builder.add(element);
             }
-            let (union, conflicting) = builder.build();
-
-            if !conflicting.is_empty() {
-                return Err((union, conflicting));
-            }
-
-            union
+            builder.build()
         } else {
-            first
+            (first, None)
         };
 
         let boundness = match boundness_analysis {
@@ -1244,9 +1267,16 @@ fn place_from_declarations_impl<'db>(
             },
         };
 
-        Ok(Place::Type(declared.inner_type(), boundness).with_qualifiers(declared.qualifiers()))
+        let place_and_quals =
+            Place::Type(declared.inner_type(), boundness).with_qualifiers(declared.qualifiers());
+
+        if let Some(conflicting) = conflicting {
+            PlaceFromDeclarationsResult::conflict(place_and_quals, conflicting)
+        } else {
+            place_and_quals.into()
+        }
     } else {
-        Ok(Place::Unbound.into())
+        Place::Unbound.into()
     }
 }
 
@@ -1281,31 +1311,32 @@ mod implicit_globals {
     use crate::semantic_index::{place_table, use_def_map};
     use crate::types::{KnownClass, Type};
 
-    use super::{Place, PlaceFromDeclarationsResult, place_from_declarations};
+    use super::{Place, place_from_declarations};
 
     pub(crate) fn module_type_implicit_global_declaration<'db>(
         db: &'db dyn Db,
         name: &str,
-    ) -> PlaceFromDeclarationsResult<'db> {
+    ) -> PlaceAndQualifiers<'db> {
         if !module_type_symbols(db)
             .iter()
             .any(|module_type_member| module_type_member == name)
         {
-            return Ok(Place::Unbound.into());
+            return Place::Unbound.into();
         }
         let Type::ClassLiteral(module_type_class) = KnownClass::ModuleType.to_class_literal(db)
         else {
-            return Ok(Place::Unbound.into());
+            return Place::Unbound.into();
         };
         let module_type_scope = module_type_class.body_scope(db);
         let place_table = place_table(db, module_type_scope);
         let Some(symbol_id) = place_table.symbol_id(name) else {
-            return Ok(Place::Unbound.into());
+            return Place::Unbound.into();
         };
         place_from_declarations(
             db,
             use_def_map(db, module_type_scope).end_of_scope_symbol_declarations(symbol_id),
         )
+        .ignore_conflicting_declarations()
     }
 
     /// Looks up the type of an "implicit global symbol". Returns [`Place::Unbound`] if