From 4f74db56306e4169171d5e873d7f80a93f6270f5 Mon Sep 17 00:00:00 2001 From: David Peter Date: Thu, 7 Nov 2024 19:58:31 +0100 Subject: [PATCH] [red-knot] Improve `Symbol` API for callable types (#14137) ## Summary - Get rid of `Symbol::unwrap_or` (unclear semantics, not needed anymore) - Introduce `Type::call_dunder` - Emit new diagnostic for possibly-unbound `__iter__` methods - Better diagnostics for callables with possibly-unbound / possibly-non-callable `__call__` methods part of: #14022 closes #14016 ## Test Plan - Updated test for iterables with possibly-unbound `__iter__` methods. - New tests for callables --- .../mdtest/call/callable_instance.md | 55 +++++ .../resources/mdtest/call/function.md | 13 ++ .../resources/mdtest/loops/for_loop.md | 2 +- crates/red_knot_python_semantic/src/symbol.rs | 10 +- crates/red_knot_python_semantic/src/types.rs | 191 +++++++++++++----- .../src/types/diagnostic.rs | 17 ++ .../src/types/infer.rs | 16 +- 7 files changed, 241 insertions(+), 63 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md b/crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md index 1719601372..95e82fc572 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md +++ b/crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md @@ -18,3 +18,58 @@ class Unit: ... b = Unit()(3.0) # error: "Object of type `Unit` is not callable" reveal_type(b) # revealed: Unknown ``` + +## Possibly unbound `__call__` method + +```py +def flag() -> bool: ... + +class PossiblyNotCallable: + if flag(): + def __call__(self) -> int: ... + +a = PossiblyNotCallable() +result = a() # error: "Object of type `PossiblyNotCallable` is not callable (possibly unbound `__call__` method)" +reveal_type(result) # revealed: int +``` + +## Possibly unbound callable + +```py +def flag() -> bool: ... + +if flag(): + class PossiblyUnbound: + def __call__(self) -> int: ... + +# error: [possibly-unresolved-reference] +a = PossiblyUnbound() +reveal_type(a()) # revealed: int +``` + +## Non-callable `__call__` + +```py +class NonCallable: + __call__ = 1 + +a = NonCallable() +# error: "Object of type `NonCallable` is not callable" +reveal_type(a()) # revealed: Unknown +``` + +## Possibly non-callable `__call__` + +```py +def flag() -> bool: ... + +class NonCallable: + if flag(): + __call__ = 1 + else: + def __call__(self) -> int: ... + +a = NonCallable() +# error: "Object of type `Literal[1] | Literal[__call__]` is not callable (due to union element `Literal[1]`)" +reveal_type(a()) # revealed: Unknown | int +``` diff --git a/crates/red_knot_python_semantic/resources/mdtest/call/function.md b/crates/red_knot_python_semantic/resources/mdtest/call/function.md index a87285a9ed..6b8cbd214b 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/call/function.md +++ b/crates/red_knot_python_semantic/resources/mdtest/call/function.md @@ -44,3 +44,16 @@ reveal_type(bar()) # revealed: @Todo nonsense = 123 x = nonsense() # error: "Object of type `Literal[123]` is not callable" ``` + +## Potentially unbound function + +```py +def flag() -> bool: ... + +if flag(): + def foo() -> int: + return 42 + +# error: [possibly-unresolved-reference] +reveal_type(foo()) # revealed: int +``` diff --git a/crates/red_knot_python_semantic/resources/mdtest/loops/for_loop.md b/crates/red_knot_python_semantic/resources/mdtest/loops/for_loop.md index d0ed8073c8..09a9bc8187 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/loops/for_loop.md +++ b/crates/red_knot_python_semantic/resources/mdtest/loops/for_loop.md @@ -238,7 +238,7 @@ class Test: def coinflip() -> bool: return True -# TODO: we should emit a diagnostic here (it might not be iterable) +# error: [not-iterable] "Object of type `Test | Literal[42]` is not iterable because its `__iter__` method is possibly unbound" for x in Test() if coinflip() else 42: reveal_type(x) # revealed: int ``` diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index f9889fa3e1..f8cc834803 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -3,7 +3,7 @@ use crate::{ Db, }; -#[derive(Debug, Clone, Copy, PartialEq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Boundness { Bound, MayBeUnbound, @@ -44,17 +44,13 @@ impl<'db> Symbol<'db> { } } - pub(crate) fn unwrap_or(&self, other: Type<'db>) -> Type<'db> { + pub(crate) fn unwrap_or_unknown(&self) -> Type<'db> { match self { Symbol::Type(ty, _) => *ty, - Symbol::Unbound => other, + Symbol::Unbound => Type::Unknown, } } - pub(crate) fn unwrap_or_unknown(&self) -> Type<'db> { - self.unwrap_or(Type::Unknown) - } - pub(crate) fn as_type(&self) -> Option> { match self { Symbol::Type(ty, _) => Some(*ty), diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index df5e069f93..e2b0efc816 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -1208,17 +1208,33 @@ impl<'db> Type<'db> { }) } - Type::Instance(InstanceType { class, .. }) => { - // Since `__call__` is a dunder, we need to access it as an attribute on the class - // rather than the instance (matching runtime semantics). - match class.class_member(db, "__call__") { - Symbol::Type(dunder_call_method, Boundness::Bound) => { - let args = std::iter::once(self) - .chain(arg_types.iter().copied()) - .collect::>(); - dunder_call_method.call(db, &args) + instance_ty @ Type::Instance(InstanceType { .. }) => { + let args = std::iter::once(self) + .chain(arg_types.iter().copied()) + .collect::>(); + match instance_ty.call_dunder(db, "__call__", &args) { + CallDunderResult::CallOutcome(CallOutcome::NotCallable { .. }) => { + // Turn "`` not callable" into + // "`X` not callable" + CallOutcome::NotCallable { + not_callable_ty: self, + } + } + CallDunderResult::CallOutcome(outcome) => outcome, + CallDunderResult::PossiblyUnbound(call_outcome) => { + // Turn "possibly unbound object of type `Literal['__call__']`" + // into "`X` not callable (possibly unbound `__call__` method)" + CallOutcome::PossiblyUnboundDunderCall { + called_ty: self, + call_outcome: Box::new(call_outcome), + } + } + CallDunderResult::MethodNotAvailable => { + // Turn "`X.__call__` unbound" into "`X` not callable" + CallOutcome::NotCallable { + not_callable_ty: self, + } } - _ => CallOutcome::not_callable(self), } } @@ -1244,6 +1260,24 @@ impl<'db> Type<'db> { } } + /// Look up a dunder method on the meta type of `self` and call it. + fn call_dunder( + self, + db: &'db dyn Db, + name: &str, + arg_types: &[Type<'db>], + ) -> CallDunderResult<'db> { + match self.to_meta_type(db).member(db, name) { + Symbol::Type(callable_ty, Boundness::Bound) => { + CallDunderResult::CallOutcome(callable_ty.call(db, arg_types)) + } + Symbol::Type(callable_ty, Boundness::MayBeUnbound) => { + CallDunderResult::PossiblyUnbound(callable_ty.call(db, arg_types)) + } + Symbol::Unbound => CallDunderResult::MethodNotAvailable, + } + } + /// Given the type of an object that is iterated over in some way, /// return the type of objects that are yielded by that iteration. /// @@ -1265,34 +1299,35 @@ impl<'db> Type<'db> { return IterationOutcome::Iterable { element_ty: self }; } - // `self` represents the type of the iterable; - // `__iter__` and `__next__` are both looked up on the class of the iterable: - let iterable_meta_type = self.to_meta_type(db); - - let dunder_iter_method = iterable_meta_type.member(db, "__iter__"); - if let Symbol::Type(dunder_iter_method, _) = dunder_iter_method { - let Some(iterator_ty) = dunder_iter_method.call(db, &[self]).return_ty(db) else { - return IterationOutcome::NotIterable { - not_iterable_ty: self, - }; - }; - - match iterator_ty.to_meta_type(db).member(db, "__next__") { - Symbol::Type(dunder_next_method, Boundness::Bound) => { - return dunder_next_method - .call(db, &[iterator_ty]) - .return_ty(db) - .map(|element_ty| IterationOutcome::Iterable { element_ty }) - .unwrap_or(IterationOutcome::NotIterable { - not_iterable_ty: self, - }); - } - _ => { + let dunder_iter_result = self.call_dunder(db, "__iter__", &[self]); + match dunder_iter_result { + CallDunderResult::CallOutcome(ref call_outcome) + | CallDunderResult::PossiblyUnbound(ref call_outcome) => { + let Some(iterator_ty) = call_outcome.return_ty(db) else { return IterationOutcome::NotIterable { not_iterable_ty: self, }; - } + }; + + return if let Some(element_ty) = iterator_ty + .call_dunder(db, "__next__", &[iterator_ty]) + .return_ty(db) + { + if matches!(dunder_iter_result, CallDunderResult::PossiblyUnbound(..)) { + IterationOutcome::PossiblyUnboundDunderIter { + iterable_ty: self, + element_ty, + } + } else { + IterationOutcome::Iterable { element_ty } + } + } else { + IterationOutcome::NotIterable { + not_iterable_ty: self, + } + }; } + CallDunderResult::MethodNotAvailable => {} } // Although it's not considered great practice, @@ -1301,17 +1336,15 @@ impl<'db> Type<'db> { // // TODO(Alex) this is only valid if the `__getitem__` method is annotated as // accepting `int` or `SupportsIndex` - match iterable_meta_type.member(db, "__getitem__") { - Symbol::Type(dunder_get_item_method, Boundness::Bound) => dunder_get_item_method - .call(db, &[self, KnownClass::Int.to_instance(db)]) - .return_ty(db) - .map(|element_ty| IterationOutcome::Iterable { element_ty }) - .unwrap_or(IterationOutcome::NotIterable { - not_iterable_ty: self, - }), - _ => IterationOutcome::NotIterable { + if let Some(element_ty) = self + .call_dunder(db, "__getitem__", &[self, KnownClass::Int.to_instance(db)]) + .return_ty(db) + { + IterationOutcome::Iterable { element_ty } + } else { + IterationOutcome::NotIterable { not_iterable_ty: self, - }, + } } } @@ -1632,6 +1665,10 @@ enum CallOutcome<'db> { called_ty: Type<'db>, outcomes: Box<[CallOutcome<'db>]>, }, + PossiblyUnboundDunderCall { + called_ty: Type<'db>, + call_outcome: Box>, + }, } impl<'db> CallOutcome<'db> { @@ -1689,6 +1726,7 @@ impl<'db> CallOutcome<'db> { } }) .map(UnionBuilder::build), + Self::PossiblyUnboundDunderCall { call_outcome, .. } => call_outcome.return_ty(db), } } @@ -1747,6 +1785,20 @@ impl<'db> CallOutcome<'db> { ); return_ty } + Err(NotCallableError::PossiblyUnboundDunderCall { + callable_ty: called_ty, + return_ty, + }) => { + diagnostics.add( + node, + "call-non-callable", + format_args!( + "Object of type `{}` is not callable (possibly unbound `__call__` method)", + called_ty.display(db) + ), + ); + return_ty + } } } @@ -1774,6 +1826,13 @@ impl<'db> CallOutcome<'db> { not_callable_ty: *not_callable_ty, return_ty: Type::Unknown, }), + Self::PossiblyUnboundDunderCall { + called_ty, + call_outcome, + } => Err(NotCallableError::PossiblyUnboundDunderCall { + callable_ty: *called_ty, + return_ty: call_outcome.return_ty(db).unwrap_or(Type::Unknown), + }), Self::Union { outcomes, called_ty, @@ -1825,6 +1884,22 @@ impl<'db> CallOutcome<'db> { } } +enum CallDunderResult<'db> { + CallOutcome(CallOutcome<'db>), + PossiblyUnbound(CallOutcome<'db>), + MethodNotAvailable, +} + +impl<'db> CallDunderResult<'db> { + fn return_ty(&self, db: &'db dyn Db) -> Option> { + match self { + Self::CallOutcome(outcome) => outcome.return_ty(db), + Self::PossiblyUnbound { .. } => None, + Self::MethodNotAvailable => None, + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] enum NotCallableError<'db> { /// The type is not callable. @@ -1844,6 +1919,10 @@ enum NotCallableError<'db> { called_ty: Type<'db>, return_ty: Type<'db>, }, + PossiblyUnboundDunderCall { + callable_ty: Type<'db>, + return_ty: Type<'db>, + }, } impl<'db> NotCallableError<'db> { @@ -1853,6 +1932,7 @@ impl<'db> NotCallableError<'db> { Self::Type { return_ty, .. } => *return_ty, Self::UnionElement { return_ty, .. } => *return_ty, Self::UnionElements { return_ty, .. } => *return_ty, + Self::PossiblyUnboundDunderCall { return_ty, .. } => *return_ty, } } @@ -1867,14 +1947,26 @@ impl<'db> NotCallableError<'db> { } => *not_callable_ty, Self::UnionElement { called_ty, .. } => *called_ty, Self::UnionElements { called_ty, .. } => *called_ty, + Self::PossiblyUnboundDunderCall { + callable_ty: called_ty, + .. + } => *called_ty, } } } #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum IterationOutcome<'db> { - Iterable { element_ty: Type<'db> }, - NotIterable { not_iterable_ty: Type<'db> }, + Iterable { + element_ty: Type<'db>, + }, + NotIterable { + not_iterable_ty: Type<'db>, + }, + PossiblyUnboundDunderIter { + iterable_ty: Type<'db>, + element_ty: Type<'db>, + }, } impl<'db> IterationOutcome<'db> { @@ -1889,6 +1981,13 @@ impl<'db> IterationOutcome<'db> { diagnostics.add_not_iterable(iterable_node, not_iterable_ty); Type::Unknown } + Self::PossiblyUnboundDunderIter { + iterable_ty, + element_ty, + } => { + diagnostics.add_not_iterable_possibly_unbound(iterable_node, iterable_ty); + element_ty + } } } } diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 8b3c75e971..54b14105e7 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -164,6 +164,23 @@ impl<'db> TypeCheckDiagnosticsBuilder<'db> { ); } + /// Emit a diagnostic declaring that the object represented by `node` is not iterable + /// because its `__iter__` method is possibly unbound. + pub(super) fn add_not_iterable_possibly_unbound( + &mut self, + node: AnyNodeRef, + element_ty: Type<'db>, + ) { + self.add( + node, + "not-iterable", + format_args!( + "Object of type `{}` is not iterable because its `__iter__` method is possibly unbound", + element_ty.display(self.db) + ), + ); + } + /// Emit a diagnostic declaring that an index is out of bounds for a tuple. pub(super) fn add_index_out_of_bounds( &mut self, diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 2e17e1c18e..aae55c04c6 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -2957,22 +2957,19 @@ impl<'db> TypeInferenceBuilder<'db> { op, ), - (Type::Instance(left), Type::Instance(right), op) => { + (left_ty @ Type::Instance(left), right_ty @ Type::Instance(right), op) => { if left != right && right.is_instance_of(self.db, left.class) { let reflected_dunder = op.reflected_dunder(); let rhs_reflected = right.class.class_member(self.db, reflected_dunder); if !rhs_reflected.is_unbound() && rhs_reflected != left.class.class_member(self.db, reflected_dunder) { - return rhs_reflected - .unwrap_or(Type::Never) - .call(self.db, &[right_ty, left_ty]) + return right_ty + .call_dunder(self.db, reflected_dunder, &[right_ty, left_ty]) .return_ty(self.db) .or_else(|| { - left.class - .class_member(self.db, op.dunder()) - .unwrap_or(Type::Never) - .call(self.db, &[left_ty, right_ty]) + left_ty + .call_dunder(self.db, op.dunder(), &[left_ty, right_ty]) .return_ty(self.db) }); } @@ -4398,7 +4395,8 @@ fn perform_membership_test_comparison<'db>( // iteration-based membership test match Type::Instance(right).iterate(db) { IterationOutcome::Iterable { .. } => Some(KnownClass::Bool.to_instance(db)), - IterationOutcome::NotIterable { .. } => None, + IterationOutcome::NotIterable { .. } + | IterationOutcome::PossiblyUnboundDunderIter { .. } => None, } } };