[ty] Proper assignability/subtyping checks for protocols with method members (#20165)

crates/ty_python_semantic/src/types.rs

@@ -1658,12 +1658,15 @@ impl<'db> Type<'db> {
                 | Type::EnumLiteral(_),
             ) => ConstraintSet::from(false),
 
-            (Type::Callable(self_callable), Type::Callable(other_callable)) => {
-                self_callable.has_relation_to_impl(db, other_callable, relation, visitor)
-            }
+            (Type::Callable(self_callable), Type::Callable(other_callable)) => visitor
+                .visit((self, target, relation), || {
+                    self_callable.has_relation_to_impl(db, other_callable, relation, visitor)
+                }),
 
-            (_, Type::Callable(_)) => self.into_callable(db).when_some_and(|callable| {
-                callable.has_relation_to_impl(db, target, relation, visitor)
+            (_, Type::Callable(_)) => visitor.visit((self, target, relation), || {
+                self.into_callable(db).when_some_and(|callable| {
+                    callable.has_relation_to_impl(db, target, relation, visitor)
+                })
             }),
 
             (_, Type::ProtocolInstance(protocol)) => {
@@ -3265,6 +3268,13 @@ impl<'db> Type<'db> {
         policy: InstanceFallbackShadowsNonDataDescriptor,
         member_policy: MemberLookupPolicy,
     ) -> PlaceAndQualifiers<'db> {
+        // TODO: this is a workaround for the fact that looking up the `__call__` attribute on the
+        // meta-type of a `Callable` type currently returns `Unbound`. We should fix this by inferring
+        // a more sophisticated meta-type for `Callable` types; that would allow us to remove this branch.
+        if name == "__call__" && matches!(self, Type::Callable(_) | Type::DataclassTransformer(_)) {
+            return Place::bound(self).into();
+        }
+
         let (
             PlaceAndQualifiers {
                 place: meta_attr,

crates/ty_python_semantic/src/types/property_tests.rs

@@ -68,7 +68,7 @@ macro_rules! type_property_test {
 
 mod stable {
     use super::union;
-    use crate::types::{CallableType, Type};
+    use crate::types::{CallableType, KnownClass, Type};
 
     // Reflexivity: `T` is equivalent to itself.
     type_property_test!(
@@ -205,6 +205,25 @@ mod stable {
         all_fully_static_type_pairs_are_subtype_of_their_union, db,
         forall fully_static_types s, t. s.is_subtype_of(db, union(db, [s, t])) && t.is_subtype_of(db, union(db, [s, t]))
     );
+
+    // Any type assignable to `Iterable[object]` should be considered iterable.
+    //
+    // Note that the inverse is not true, due to the fact that we recognize the old-style
+    // iteration protocol as well as the new-style iteration protocol: not all objects that
+    // we consider iterable are assignable to `Iterable[object]`.
+    //
+    // Note also that (like other property tests in this module),
+    // this invariant will only hold true for Liskov-compliant types assignable to `Iterable`.
+    // Since protocols can participate in nominal assignability/subtyping as well as
+    // structural assignability/subtyping, it is possible to construct types that a type
+    // checker must consider to be subtypes of `Iterable` even though they are not in fact
+    // iterable (as long as the user `type: ignore`s any type-checker errors stemming from
+    // the Liskov violation). All you need to do is to create a class that subclasses
+    // `Iterable` but assigns `__iter__ = None` in the class body (or similar).
+    type_property_test!(
+        all_type_assignable_to_iterable_are_iterable, db,
+        forall types t. t.is_assignable_to(db, KnownClass::Iterable.to_specialized_instance(db, [Type::object()])) => t.try_iterate(db).is_ok()
+    );
 }
 
 /// This module contains property tests that currently lead to many false positives.
@@ -217,8 +236,6 @@ mod stable {
 mod flaky {
     use itertools::Itertools;
 
-    use crate::types::{KnownClass, Type};
-
     use super::{intersection, union};
 
     // Negating `T` twice is equivalent to `T`.
@@ -313,25 +330,4 @@ mod flaky {
         bottom_materialization_of_type_is_assigneble_to_type, db,
         forall types t. t.bottom_materialization(db).is_assignable_to(db, t)
     );
-
-    // Any type assignable to `Iterable[object]` should be considered iterable.
-    //
-    // Note that the inverse is not true, due to the fact that we recognize the old-style
-    // iteration protocol as well as the new-style iteration protocol: not all objects that
-    // we consider iterable are assignable to `Iterable[object]`.
-    //
-    // Note also that (like other property tests in this module),
-    // this invariant will only hold true for Liskov-compliant types assignable to `Iterable`.
-    // Since protocols can participate in nominal assignability/subtyping as well as
-    // structural assignability/subtyping, it is possible to construct types that a type
-    // checker must consider to be subtypes of `Iterable` even though they are not in fact
-    // iterable (as long as the user `type: ignore`s any type-checker errors stemming from
-    // the Liskov violation). All you need to do is to create a class that subclasses
-    // `Iterable` but assigns `__iter__ = None` in the class body (or similar).
-    //
-    // Currently flaky due to <https://github.com/astral-sh/ty/issues/889>
-    type_property_test!(
-        all_type_assignable_to_iterable_are_iterable, db,
-        forall types t. t.is_assignable_to(db, KnownClass::Iterable.to_specialized_instance(db, [Type::object()])) => t.try_iterate(db).is_ok()
-    );
 }

crates/ty_python_semantic/src/types/protocol_class.rs

@@ -6,23 +6,24 @@ use itertools::Itertools;
 use ruff_python_ast::name::Name;
 use rustc_hash::FxHashMap;
 
-use super::TypeVarVariance;
-use crate::semantic_index::place::ScopedPlaceId;
-use crate::semantic_index::{SemanticIndex, place_table};
-use crate::types::ClassType;
-use crate::types::context::InferContext;
-use crate::types::diagnostic::report_undeclared_protocol_member;
 use crate::{
     Db, FxOrderSet,
     place::{Boundness, Place, PlaceAndQualifiers, place_from_bindings, place_from_declarations},
-    semantic_index::{definition::Definition, use_def_map},
+    semantic_index::{
+        SemanticIndex, definition::Definition, place::ScopedPlaceId, place_table, use_def_map,
+    },
     types::{
         ApplyTypeMappingVisitor, BoundTypeVarInstance, CallableType, ClassBase, ClassLiteral,
-        FindLegacyTypeVarsVisitor, HasRelationToVisitor, IsDisjointVisitor, KnownFunction,
-        NormalizedVisitor, PropertyInstanceType, Signature, Type, TypeMapping, TypeQualifiers,
-        TypeRelation, VarianceInferable,
+        ClassType, FindLegacyTypeVarsVisitor, HasRelationToVisitor,
+        InstanceFallbackShadowsNonDataDescriptor, IsDisjointVisitor, KnownFunction,
+        MemberLookupPolicy, NormalizedVisitor, PropertyInstanceType, Signature, Type, TypeMapping,
+        TypeQualifiers, TypeRelation, TypeVarVariance, VarianceInferable,
         constraints::{ConstraintSet, IteratorConstraintsExtension},
+        context::InferContext,
+        diagnostic::report_undeclared_protocol_member,
         signatures::{Parameter, Parameters},
+        todo_type,
+        visitor::any_over_type,
     },
 };
 
@@ -539,12 +540,36 @@ impl<'a, 'db> ProtocolMember<'a, 'db> {
         visitor: &HasRelationToVisitor<'db>,
     ) -> ConstraintSet<'db> {
         match &self.kind {
-            // TODO: consider the types of the attribute on `other` for method members
-            ProtocolMemberKind::Method(_) => ConstraintSet::from(matches!(
-                other.to_meta_type(db).member(db, self.name).place,
-                Place::Type(ty, Boundness::Bound)
-                if ty.is_assignable_to(db, CallableType::single(db, Signature::dynamic(Type::any())))
-            )),
+            ProtocolMemberKind::Method(method) => {
+                let Place::Type(attribute_type, Boundness::Bound) = other
+                    .invoke_descriptor_protocol(
+                        db,
+                        self.name,
+                        Place::Unbound.into(),
+                        InstanceFallbackShadowsNonDataDescriptor::No,
+                        MemberLookupPolicy::default(),
+                    )
+                    .place
+                else {
+                    return ConstraintSet::from(false);
+                };
+
+                let proto_member_as_bound_method = method.bind_self(db);
+
+                if any_over_type(db, proto_member_as_bound_method, &|t| {
+                    matches!(t, Type::TypeVar(_))
+                }) {
+                    // TODO: proper validation for generic methods on protocols
+                    return ConstraintSet::from(true);
+                }
+
+                attribute_type.has_relation_to_impl(
+                    db,
+                    proto_member_as_bound_method,
+                    relation,
+                    visitor,
+                )
+            }
             // TODO: consider the types of the attribute on `other` for property members
             ProtocolMemberKind::Property(_) => ConstraintSet::from(matches!(
                 other.member(db, self.name).place,
@@ -687,6 +712,13 @@ fn cached_protocol_interface<'db>(
                 {
                     ProtocolMemberKind::Method(callable)
                 }
+                Type::FunctionLiteral(function)
+                    if function.is_staticmethod(db) || function.is_classmethod(db) =>
+                {
+                    ProtocolMemberKind::Other(todo_type!(
+                        "classmethod and staticmethod protocol members"
+                    ))
+                }
                 Type::FunctionLiteral(function) if bound_on_class.is_yes() => {
                     ProtocolMemberKind::Method(function.into_callable_type(db))
                 }