From 93aff361472496e140fb4f4dc9e4b8efc3941f0e Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Sat, 15 Feb 2025 18:22:35 +0000 Subject: [PATCH] [red-knot] Improve handling of inherited class attributes (#16160) --- .../resources/mdtest/attributes.md | 61 +++++++++++++ crates/red_knot_python_semantic/src/symbol.rs | 85 ++++++++++++++++--- crates/red_knot_python_semantic/src/types.rs | 44 +++++++--- .../src/types/infer.rs | 77 +++++++++-------- 4 files changed, 209 insertions(+), 58 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/attributes.md b/crates/red_knot_python_semantic/resources/mdtest/attributes.md index c51a899131..0f7ee18380 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/attributes.md +++ b/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 +` & 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: diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 354c271f6a..0d3bdd8ead 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/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> 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, LookupError<'db>>; + #[cfg(test)] mod tests { use super::*; diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 4e932798b9..e5d26e0726 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/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> = 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::Class(class) => { - let member = class.own_class_member(db, name); - if !member.is_unbound() { - return member; - } + 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) => { + 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`. diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 2ad1294df7..bf1fef8003 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/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) => { - self.context.report_lint( - &POSSIBLY_UNBOUND_ATTRIBUTE, - attribute.into(), - format_args!( - "Attribute `{}` on type `{}` is possibly unbound", - attr.id, - value_ty.display(self.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() - } - } + 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_type.display(db), + ), + ); + 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) {