review comments

Author: AlexWaygood

crates/red_knot_python_semantic/src/types.rs

@@ -492,6 +492,7 @@ pub enum Type<'db> {
     /// Construct this variant using the `Type::instance` constructor function.
     NominalInstance(NominalInstanceType<'db>),
     /// The set of Python objects that conform to the interface described by a given protocol.
+    /// Construct this variant using the `Type::instance` constructor function.
     ProtocolInstance(ProtocolInstanceType<'db>),
     /// A single Python object that requires special treatment in the type system
     KnownInstance(KnownInstanceType<'db>),
@@ -1180,7 +1181,6 @@ impl<'db> Type<'db> {
                 left.is_subtype_of(db, right)
             }
             // A protocol instance can never be a subtype of a nominal type, with the *sole* exception of `object`.
-            // TODO: `Callable` types are also structural types.
             (Type::ProtocolInstance(_), _) => false,
             (_, Type::ProtocolInstance(protocol)) => self.satisfies_protocol(db, protocol),
 
@@ -1510,7 +1510,6 @@ impl<'db> Type<'db> {
             // Other than the dynamic types such as `Any`/`Unknown`/`Todo` handled above,
             // a protocol instance can never be assignable to a nominal type,
             // with the *sole* exception of `object`.
-            // TODO: `Callable` types are also structural types.
             (Type::ProtocolInstance(_), _) => false,
             (_, Type::ProtocolInstance(protocol)) => self.satisfies_protocol(db, protocol),
 
@@ -1537,8 +1536,8 @@ impl<'db> Type<'db> {
             (Type::NominalInstance(left), Type::NominalInstance(right)) => {
                 left.is_equivalent_to(db, right)
             }
-            (Type::ProtocolInstance(first), Type::ProtocolInstance(right)) => {
-                first.is_equivalent_to(db, right)
+            (Type::ProtocolInstance(left), Type::ProtocolInstance(right)) => {
+                left.is_equivalent_to(db, right)
             }
             (Type::ProtocolInstance(protocol), nominal @ Type::NominalInstance(n))
             | (nominal @ Type::NominalInstance(n), Type::ProtocolInstance(protocol)) => {
@@ -1594,8 +1593,8 @@ impl<'db> Type<'db> {
                 first.is_gradual_equivalent_to(db, second)
             }
 
-            (Type::ProtocolInstance(first), Type::ProtocolInstance(right)) => {
-                first.is_gradual_equivalent_to(db, right)
+            (Type::ProtocolInstance(first), Type::ProtocolInstance(second)) => {
+                first.is_gradual_equivalent_to(db, second)
             }
             (Type::ProtocolInstance(protocol), nominal @ Type::NominalInstance(n))
             | (nominal @ Type::NominalInstance(n), Type::ProtocolInstance(protocol)) => {

crates/red_knot_python_semantic/src/types/class.rs

@@ -534,13 +534,24 @@ impl<'db> ClassLiteral<'db> {
 
     /// Determine if this class is a protocol.
     ///
-    /// See the docs on [`KnownClass::is_protocol`] for why we hardcode knowledge
-    /// about certain special-cased classes, rather than treating all classes
-    /// the same way here.
+    /// This method relies on the accuracy of the [`KnownClass::is_protocol`] method,
+    /// which hardcodes knowledge about certain special-cased classes. See the docs on
+    /// that method for why we do this rather than relying on generalised logic for all
+    /// classes, including the special-cased ones that are included in the [`KnownClass`]
+    /// enum.
     pub(super) fn is_protocol(self, db: &'db dyn Db) -> bool {
         self.known(db)
             .map(KnownClass::is_protocol)
             .unwrap_or_else(|| {
+                // Iterate through the last three bases of the class
+                // searching for `Protocol` or `Protocol[]` in the bases list.
+                //
+                // If `Protocol` is present in the bases list of a valid protocol class, it must either:
+                //
+                // - be the last base
+                // - OR be the last-but-one base (with the final base being `Generic[]` or `object`)
+                // - OR be the last-but-two base (with the penultimate base being `Generic[]`
+                //                                and the final base being `object`)
                 self.explicit_bases(db).iter().rev().take(3).any(|base| {
                     matches!(
                         base,

crates/red_knot_python_semantic/src/types/instance.rs

@@ -22,6 +22,9 @@ impl<'db> Type<'db> {
     }
 
     /// Return `true` if `self` conforms to the interface described by `protocol`.
+    ///
+    /// TODO: we may need to split this into two methods in the future, once we start
+    /// differentiating between fully-static and non-fully-static protocols.
     pub(super) fn satisfies_protocol(
         self,
         db: &'db dyn Db,

crates/red_knot_python_semantic/src/types/type_ordering.rs

@@ -137,8 +137,6 @@ pub(super) fn union_or_intersection_elements_ordering<'db>(
         (_, Type::NominalInstance(_)) => Ordering::Greater,
 
         (Type::ProtocolInstance(left_proto), Type::ProtocolInstance(right_proto)) => {
-            debug_assert_eq!(*left, left_proto.normalized(db));
-            debug_assert_eq!(*right, right_proto.normalized(db));
             left_proto.cmp(right_proto)
         }
         (Type::ProtocolInstance(_), _) => Ordering::Less,