[red-knot] Improve handling of inherited class attributes (#16160)

crates/red_knot_python_semantic/resources/mdtest/attributes.md

@@ -804,6 +804,67 @@ def _(flag: bool, flag1: bool, flag2: bool):
     reveal_type(C.x)  # revealed: Unknown | Literal[1, 2, 3]
 ```
 
+### Attribute possibly unbound on a subclass but not on a superclass
+
+```py
+def _(flag: bool):
+    class Foo:
+        x = 1
+
+    class Bar(Foo):
+        if flag:
+            x = 2
+
+    reveal_type(Bar.x)  # revealed: Unknown | Literal[2, 1]
+```
+
+### Attribute possibly unbound on a subclass and on a superclass
+
+```py
+def _(flag: bool):
+    class Foo:
+        if flag:
+            x = 1
+
+    class Bar(Foo):
+        if flag:
+            x = 2
+
+    # error: [possibly-unbound-attribute]
+    reveal_type(Bar.x)  # revealed: Unknown | Literal[2, 1]
+```
+
+### Attribute access on `Any`
+
+The union of the set of types that `Any` could materialise to is equivalent to `object`. It follows
+from this that attribute access on `Any` resolves to `Any` if the attribute does not exist on
+`object` -- but if the attribute *does* exist on `object`, the type of the attribute is
+`<type as it exists on object> & Any`.
+
+```py
+from typing import Any
+
+class Foo(Any): ...
+
+reveal_type(Foo.bar)  # revealed: Any
+reveal_type(Foo.__repr__)  # revealed: Literal[__repr__] & Any
+```
+
+Similar principles apply if `Any` appears in the middle of an inheritance hierarchy:
+
+```py
+from typing import ClassVar, Literal
+
+class A:
+    x: ClassVar[Literal[1]] = 1
+
+class B(Any): ...
+class C(B, A): ...
+
+reveal_type(C.__mro__)  # revealed: tuple[Literal[C], Literal[B], Any, Literal[A], Literal[object]]
+reveal_type(C.x)  # revealed: Literal[1] & Any
+```
+
 ### Unions with all paths unbound
 
 If the symbol is unbound in all elements of the union, we detect that:

crates/red_knot_python_semantic/src/symbol.rs

@@ -40,7 +40,7 @@ impl<'db> Symbol<'db> {
 
     /// Constructor that creates a [`Symbol`] with a [`crate::types::TodoType`] type
     /// and boundness [`Boundness::Bound`].
-    #[allow(unused_variables)]
+    #[allow(unused_variables)] // Only unused in release builds
     pub(crate) fn todo(message: &'static str) -> Self {
         Symbol::Type(todo_type!(message), Boundness::Bound)
     }
@@ -67,6 +67,30 @@ impl<'db> Symbol<'db> {
             .expect("Expected a (possibly unbound) type, not an unbound symbol")
     }
 
+    /// Transform the symbol into a [`LookupResult`],
+    /// a [`Result`] type in which the `Ok` variant represents a definitely bound symbol
+    /// and the `Err` variant represents a symbol that is either definitely or possibly unbound.
+    pub(crate) fn into_lookup_result(self) -> LookupResult<'db> {
+        match self {
+            Symbol::Type(ty, Boundness::Bound) => Ok(ty),
+            Symbol::Type(ty, Boundness::PossiblyUnbound) => Err(LookupError::PossiblyUnbound(ty)),
+            Symbol::Unbound => Err(LookupError::Unbound),
+        }
+    }
+
+    /// Safely unwrap the symbol into a [`Type`].
+    ///
+    /// If the symbol is definitely unbound or possibly unbound, it will be transformed into a
+    /// [`LookupError`] and `diagnostic_fn` will be applied to the error value before returning
+    /// the result of `diagnostic_fn` (which will be a [`Type`]). This allows the caller to ensure
+    /// that a diagnostic is emitted if the symbol is possibly or definitely unbound.
+    pub(crate) fn unwrap_with_diagnostic(
+        self,
+        diagnostic_fn: impl FnOnce(LookupError<'db>) -> Type<'db>,
+    ) -> Type<'db> {
+        self.into_lookup_result().unwrap_or_else(diagnostic_fn)
+    }
+
     /// Fallback (partially or fully) to another symbol if `self` is partially or fully unbound.
     ///
     /// 1. If `self` is definitely bound, return `self` without evaluating `fallback_fn()`.
@@ -83,17 +107,9 @@ impl<'db> Symbol<'db> {
         db: &'db dyn Db,
         fallback_fn: impl FnOnce() -> Self,
     ) -> Self {
-        match self {
-            Symbol::Type(_, Boundness::Bound) => self,
-            Symbol::Unbound => fallback_fn(),
-            Symbol::Type(self_ty, Boundness::PossiblyUnbound) => match fallback_fn() {
-                Symbol::Unbound => self,
-                Symbol::Type(fallback_ty, fallback_boundness) => Symbol::Type(
-                    UnionType::from_elements(db, [self_ty, fallback_ty]),
-                    fallback_boundness,
-                ),
-            },
-        }
+        self.into_lookup_result()
+            .or_else(|lookup_error| lookup_error.or_fall_back_to(db, fallback_fn()))
+            .into()
     }
 
     #[must_use]
@@ -105,6 +121,51 @@ impl<'db> Symbol<'db> {
     }
 }
 
+impl<'db> From<LookupResult<'db>> for Symbol<'db> {
+    fn from(value: LookupResult<'db>) -> Self {
+        match value {
+            Ok(ty) => Symbol::Type(ty, Boundness::Bound),
+            Err(LookupError::Unbound) => Symbol::Unbound,
+            Err(LookupError::PossiblyUnbound(ty)) => Symbol::Type(ty, Boundness::PossiblyUnbound),
+        }
+    }
+}
+
+/// Possible ways in which a symbol lookup can (possibly or definitely) fail.
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub(crate) enum LookupError<'db> {
+    Unbound,
+    PossiblyUnbound(Type<'db>),
+}
+
+impl<'db> LookupError<'db> {
+    /// Fallback (wholly or partially) to `fallback` to create a new [`LookupResult`].
+    pub(crate) fn or_fall_back_to(
+        self,
+        db: &'db dyn Db,
+        fallback: Symbol<'db>,
+    ) -> LookupResult<'db> {
+        let fallback = fallback.into_lookup_result();
+        match (&self, &fallback) {
+            (LookupError::Unbound, _) => fallback,
+            (LookupError::PossiblyUnbound { .. }, Err(LookupError::Unbound)) => Err(self),
+            (LookupError::PossiblyUnbound(ty), Ok(ty2)) => {
+                Ok(UnionType::from_elements(db, [ty, ty2]))
+            }
+            (LookupError::PossiblyUnbound(ty), Err(LookupError::PossiblyUnbound(ty2))) => Err(
+                LookupError::PossiblyUnbound(UnionType::from_elements(db, [ty, ty2])),
+            ),
+        }
+    }
+}
+
+/// A [`Result`] type in which the `Ok` variant represents a definitely bound symbol
+/// and the `Err` variant represents a symbol that is either definitely or possibly unbound.
+///
+/// Note that this type is exactly isomorphic to [`Symbol`].
+/// In the future, we could possibly consider removing `Symbol` and using this type everywhere instead.
+pub(crate) type LookupResult<'db> = Result<Type<'db>, LookupError<'db>>;
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/red_knot_python_semantic/src/types.rs

@@ -35,7 +35,7 @@ use crate::semantic_index::{
 };
 use crate::stdlib::{known_module_symbol, typing_extensions_symbol};
 use crate::suppression::check_suppressions;
-use crate::symbol::{Boundness, Symbol};
+use crate::symbol::{Boundness, LookupError, LookupResult, Symbol};
 use crate::types::call::{
     bind_call, CallArguments, CallBinding, CallDunderResult, CallOutcome, StaticAssertionErrorKind,
 };
@@ -4252,21 +4252,45 @@ impl<'db> Class<'db> {
             return Symbol::bound(TupleType::from_elements(db, tuple_elements));
         }
 
+        // If we encounter a dynamic type in this class's MRO, we'll save that dynamic type
+        // in this variable. After we've traversed the MRO, we'll either:
+        // (1) Use that dynamic type as the type for this attribute,
+        //     if no other classes in the MRO define the attribute; or,
+        // (2) Intersect that dynamic type with the type of the attribute
+        //     from the non-dynamic members of the class's MRO.
+        let mut dynamic_type_to_intersect_with: Option<Type<'db>> = None;
+
+        let mut lookup_result: LookupResult<'db> = Err(LookupError::Unbound);
+
         for superclass in self.iter_mro(db) {
             match superclass {
-                // TODO we may instead want to record the fact that we encountered dynamic, and intersect it with
-                // the type found on the next "real" class.
-                ClassBase::Dynamic(_) => return Type::from(superclass).member(db, name),
+                ClassBase::Dynamic(_) => {
+                    // Note: calling `Type::from(superclass).member()` would be incorrect here.
+                    // What we'd really want is a `Type::Any.own_class_member()` method,
+                    // but adding such a method wouldn't make much sense -- it would always return `Any`!
+                    dynamic_type_to_intersect_with.get_or_insert(Type::from(superclass));
+                }
                 ClassBase::Class(class) => {
-                    let member = class.own_class_member(db, name);
-                    if !member.is_unbound() {
-                        return member;
+                    lookup_result = lookup_result.or_else(|lookup_error| {
+                        lookup_error.or_fall_back_to(db, class.own_class_member(db, name))
+                    });
                 }
             }
+            if lookup_result.is_ok() {
+                break;
             }
         }
 
-        Symbol::Unbound
+        match (Symbol::from(lookup_result), dynamic_type_to_intersect_with) {
+            (symbol, None) => symbol,
+            (Symbol::Type(ty, _), Some(dynamic_type)) => Symbol::bound(
+                IntersectionBuilder::new(db)
+                    .add_positive(ty)
+                    .add_positive(dynamic_type)
+                    .build(),
+            ),
+            (Symbol::Unbound, Some(dynamic_type)) => Symbol::bound(dynamic_type),
+        }
     }
 
     /// Returns the inferred type of the class member named `name`.

crates/red_knot_python_semantic/src/types/infer.rs

@@ -49,6 +49,7 @@ use crate::semantic_index::semantic_index;
 use crate::semantic_index::symbol::{NodeWithScopeKind, NodeWithScopeRef, ScopeId};
 use crate::semantic_index::SemanticIndex;
 use crate::stdlib::builtins_module_scope;
+use crate::symbol::LookupError;
 use crate::types::call::{Argument, CallArguments};
 use crate::types::diagnostic::{
     report_invalid_arguments_to_annotated, report_invalid_assignment,
@@ -3421,17 +3422,16 @@ impl<'db> TypeInferenceBuilder<'db> {
                 })
         });
 
-        match symbol {
-            Symbol::Type(ty, Boundness::Bound) => ty,
-            Symbol::Type(ty, Boundness::PossiblyUnbound) => {
-                report_possibly_unresolved_reference(&self.context, name_node);
-                ty
-            }
-            Symbol::Unbound => {
+        symbol.unwrap_with_diagnostic(|lookup_error| match lookup_error {
+            LookupError::Unbound => {
                 report_unresolved_reference(&self.context, name_node);
                 Type::unknown()
             }
+            LookupError::PossiblyUnbound(type_when_bound) => {
+                report_possibly_unresolved_reference(&self.context, name_node);
+                type_when_bound
             }
+        })
     }
 
     fn infer_name_expression(&mut self, name: &ast::ExprName) -> Type<'db> {
@@ -3451,34 +3451,37 @@ impl<'db> TypeInferenceBuilder<'db> {
             ctx: _,
         } = attribute;
 
-        let value_ty = self.infer_expression(value);
-        match value_ty.member(self.db(), &attr.id) {
-            Symbol::Type(member_ty, Boundness::Bound) => member_ty,
-            Symbol::Type(member_ty, Boundness::PossiblyUnbound) => {
+        let value_type = self.infer_expression(value);
+        let db = self.db();
+
+        value_type
+            .member(db, &attr.id)
+            .unwrap_with_diagnostic(|lookup_error| match lookup_error {
+                LookupError::Unbound => {
+                    self.context.report_lint(
+                        &UNRESOLVED_ATTRIBUTE,
+                        attribute.into(),
+                        format_args!(
+                            "Type `{}` has no attribute `{}`",
+                            value_type.display(db),
+                            attr.id
+                        ),
+                    );
+                    Type::unknown()
+                }
+                LookupError::PossiblyUnbound(type_when_bound) => {
                     self.context.report_lint(
                         &POSSIBLY_UNBOUND_ATTRIBUTE,
                         attribute.into(),
                         format_args!(
                             "Attribute `{}` on type `{}` is possibly unbound",
                             attr.id,
-                        value_ty.display(self.db()),
+                            value_type.display(db),
                         ),
                     );
-                member_ty
-            }
-            Symbol::Unbound => {
-                self.context.report_lint(
-                    &UNRESOLVED_ATTRIBUTE,
-                    attribute.into(),
-                    format_args!(
-                        "Type `{}` has no attribute `{}`",
-                        value_ty.display(self.db()),
-                        attr.id
-                    ),
-                );
-                Type::unknown()
-            }
+                    type_when_bound
                 }
+            })
     }
 
     fn infer_attribute_expression(&mut self, attribute: &ast::ExprAttribute) -> Type<'db> {
@@ -3836,6 +3839,8 @@ impl<'db> TypeInferenceBuilder<'db> {
                 if left_ty != right_ty && right_ty.is_subtype_of(self.db(), left_ty) {
                     let reflected_dunder = op.reflected_dunder();
                     let rhs_reflected = right_class.member(self.db(), reflected_dunder);
+                    // TODO: if `rhs_reflected` is possibly unbound, we should union the two possible
+                    // CallOutcomes together
                     if !rhs_reflected.is_unbound()
                         && rhs_reflected != left_class.member(self.db(), reflected_dunder)
                     {