Expand when unimplemented abstract methods are collected

conformance/third_party/conformance.exp

@@ -11616,17 +11616,6 @@
       "stop_column": 16,
       "stop_line": 40
     },
-    {
-      "code": -2,
-      "column": 7,
-      "concise_description": "Metaclass of `BadTypedDict2` has type `type[Any]` that is not a simple class type",
-      "description": "Metaclass of `BadTypedDict2` has type `type[Any]` that is not a simple class type",
-      "line": 45,
-      "name": "invalid-inheritance",
-      "severity": "error",
-      "stop_column": 20,
-      "stop_line": 45
-    },
     {
       "code": -2,
       "column": 7,

conformance/third_party/conformance.result

@@ -145,7 +145,9 @@
   "tuples_type_form.py": [],
   "tuples_unpacked.py": [],
   "typeddicts_alt_syntax.py": [],
-  "typeddicts_class_syntax.py": [],
+  "typeddicts_class_syntax.py": [
+    "Line 45: Expected 1 errors"
+  ],
   "typeddicts_extra_items.py": [],
   "typeddicts_final.py": [],
   "typeddicts_inheritance.py": [],

conformance/third_party/results.json

@@ -1,9 +1,9 @@
 {
   "total": 140,
-  "pass": 134,
-  "fail": 6,
-  "pass_rate": 0.96,
-  "differences": 13,
+  "pass": 133,
+  "fail": 7,
+  "pass_rate": 0.95,
+  "differences": 14,
   "passing": [
     "aliases_explicit.py",
     "aliases_implicit.py",
@@ -125,7 +125,6 @@
     "tuples_type_form.py",
     "tuples_unpacked.py",
     "typeddicts_alt_syntax.py",
-    "typeddicts_class_syntax.py",
     "typeddicts_extra_items.py",
     "typeddicts_final.py",
     "typeddicts_inheritance.py",
@@ -146,7 +145,8 @@
     "constructors_callable.py": 2,
     "dataclasses_descriptors.py": 2,
     "exceptions_context_managers.py": 2,
-    "generics_scoping.py": 4
+    "generics_scoping.py": 4,
+    "typeddicts_class_syntax.py": 1
   },
   "comment": "@generated"
 }

pyrefly/lib/alt/class/class_metadata.rs

@@ -208,27 +208,25 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             // that the class has `_ProtocolMeta` only when it is defined in a source (.py) file.
             base_metaclasses.push((&protocol_base_name, self.stdlib.protocol_meta()));
         }
-        let mut calculated_metaclass = self.calculate_metaclass(
-            cls,
-            metaclasses.into_iter().next(),
-            &base_metaclasses,
-            errors,
-        );
-        if let Some(metaclass) = calculated_metaclass.get() {
-            self.check_base_class_metaclasses(cls, metaclass, &base_metaclasses, errors);
-            if metaclass
+        let direct_metaclass = metaclasses
+            .into_iter()
+            .next()
+            .and_then(|x| self.direct_metaclass(cls, x, errors));
+        let mut calculated_metaclass =
+            self.calculate_metaclass(cls, direct_metaclass, &base_metaclasses, errors);
+        if let Some(metaclass) = calculated_metaclass.get()
+            && metaclass
                 .targs()
                 .as_slice()
                 .iter()
                 .any(|targ| targ.contains_type_variable())
-            {
-                self.error(
-                    errors,
-                    cls.range(),
-                    ErrorKind::InvalidInheritance,
-                    "Metaclass may not be an unbound generic".to_owned(),
-                );
-            }
+        {
+            self.error(
+                errors,
+                cls.range(),
+                ErrorKind::InvalidInheritance,
+                "Metaclass may not be an unbound generic".to_owned(),
+            );
         }
         // If the metaclass has unresolved type variables, replace them with their
         // gradual types (e.g. Any) to avoid cascading errors from bare TypeVars.
@@ -1836,62 +1834,79 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
     fn calculate_metaclass(
         &self,
         cls: &Class,
-        raw_metaclass: Option<&Expr>,
+        direct_metaclass: Option<ClassType>,
         base_metaclasses: &[(&Name, &ClassType)],
         errors: &ErrorCollector,
     ) -> Metaclass {
-        let direct_meta = raw_metaclass.and_then(|x| self.direct_metaclass(cls, x, errors));
-
-        if let Some(metaclass) = direct_meta {
-            Metaclass::Direct(metaclass)
+        // Attempt to find a metaclass that is assignable to all candidate metaclasses from the current class and base classes.
+        // It is a runtime error if one does not exist.
+        let mut metaclasses = if let Some(direct_metaclass) = &direct_metaclass {
+            vec![(None, self.heap.mk_class_type(direct_metaclass.clone()))]
         } else {
-            let mut inherited_meta: Option<ClassType> = None;
-            for (_, m) in base_metaclasses {
-                let m = (*m).clone();
-                let accept_m = match &inherited_meta {
-                    None => true,
-                    Some(inherited) => self.is_subset_eq(
-                        &self.heap.mk_class_type(m.clone()),
-                        &self.heap.mk_class_type(inherited.clone()),
-                    ),
-                };
-                if accept_m {
-                    inherited_meta = Some(m);
-                }
+            Vec::new()
+        };
+        metaclasses.extend(base_metaclasses.iter().map(|(base_name, base_metaclass)| {
+            (
+                Some(base_name),
+                self.heap.mk_class_type((**base_metaclass).clone()),
+            )
+        }));
+        if metaclasses.is_empty() {
+            return Metaclass::None;
+        }
+        let mut candidate = 0;
+        for (i, (_, base_metaclass)) in metaclasses.iter().enumerate().skip(1) {
+            if !self.is_subset_eq(&metaclasses[candidate].1, base_metaclass) {
+                candidate = i;
             }
-            inherited_meta
-                .map(Metaclass::Inherited)
-                .unwrap_or(Metaclass::None)
         }
-    }
-
-    fn check_base_class_metaclasses(
-        &self,
-        cls: &Class,
-        metaclass: &ClassType,
-        base_metaclasses: &[(&Name, &ClassType)],
-        errors: &ErrorCollector,
-    ) {
-        // It is a runtime error to define a class whose metaclass (whether
-        // specified directly or through inheritance) is not a subtype of all
-        // base class metaclasses.
-        let metaclass_type = self.heap.mk_class_type(metaclass.clone());
-        for (base_name, m) in base_metaclasses {
-            let base_metaclass_type = self.heap.mk_class_type((*m).clone());
-            if !self.is_subset_eq(&metaclass_type, &base_metaclass_type) {
+        // We've found a candidate metaclass that is assignable to all candidates after it.
+        // Now check if it's assignable to all candidates before it.
+        let (candidate_name, candidate_metaclass) =
+            metaclasses.split_off(candidate).into_iter().next().unwrap();
+        for (base_name, base_metaclass) in metaclasses.into_iter() {
+            if !self.is_subset_eq(&candidate_metaclass, &base_metaclass) {
+                let origin = |base_name| {
+                    if let Some(name) = base_name {
+                        format!(" from base class `{name}`")
+                    } else {
+                        "".to_owned()
+                    }
+                };
+                // We end up selecting the metaclass from the last base in case of conflict, so we
+                // flip candidate and base for a more user-friendly error message.
                 self.error(errors,
                     cls.range(),
                     ErrorKind::InvalidInheritance,
                     format!(
-                        "Class `{}` has metaclass `{}` which is not a subclass of metaclass `{}` from base class `{}`",
+                        "Class `{}` has metaclass `{}`{} which is not compatible with metaclass `{}`{}",
                         cls.name(),
-                        self.for_display(metaclass_type.clone()),
-                        self.for_display(base_metaclass_type),
-                        base_name,
+                        self.for_display(base_metaclass),
+                        origin(base_name),
+                        self.for_display(candidate_metaclass),
+                        origin(candidate_name),
                     ),
                 );
+                // We still respect an explicit `metaclass=...` declaration.
+                return if let Some(direct_metaclass) = direct_metaclass {
+                    Metaclass::Direct(direct_metaclass)
+                } else {
+                    Metaclass::None
+                };
             }
         }
+        if let Type::ClassType(candidate_metaclass) = candidate_metaclass {
+            if candidate_name.is_some() {
+                Metaclass::Inherited {
+                    metaclass: candidate_metaclass,
+                    direct_declaration: direct_metaclass,
+                }
+            } else {
+                Metaclass::Direct(candidate_metaclass)
+            }
+        } else {
+            unreachable!("Metaclass must be a ClassType");
+        }
     }
 
     fn direct_metaclass(
@@ -1921,6 +1936,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     None
                 }
             }
+            Type::Type(inner) if inner.is_any() => None, // redundant but legal
             ty => {
                 self.error(
                     errors,
@@ -2065,53 +2081,79 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             .any(|name| !inherited_slot_names.contains(name))
     }
 
+    fn is_implemented_in_class(&self, cls: &Class, field_name: &Name) -> bool {
+        // If the class has a synthesized concrete implementation (e.g., `__dataclass_fields__`
+        // from @dataclass), that satisfies any protocol requirement for this field.
+        if self
+            .get_class_member(cls, field_name)
+            .is_some_and(|f| !f.is_abstract() && !f.is_uninit_class_var())
+        {
+            return true;
+        }
+        if let Some(field) =
+            self.get_non_synthesized_class_member_and_defining_class(cls, field_name)
+            && (field.value.is_abstract() ||
+                // Uninitialized class vars in protocols are considered abstract, unless it is in a stub file
+                (!cls.module().path().is_interface() && field.value.is_uninit_class_var() &&
+                self.get_metadata_for_class(&field.defining_class).is_protocol()))
+        {
+            return false;
+        }
+        true
+    }
+
     pub fn calculate_abstract_members(&self, cls: &Class) -> AbstractClassMembers {
-        let metadata = self.get_metadata_for_class(cls);
-        let mut fields_to_check: SmallSet<Name>;
-        if metadata.extends_abc() || metadata.is_protocol() {
-            fields_to_check = self
-                .get_class_fields(cls)
-                .map(|f| SmallSet::from_iter(f.names().cloned()))
-                .unwrap_or_default();
-        } else {
-            fields_to_check = SmallSet::new();
+        if cls.has_toplevel_qname(
+            ModuleName::type_checker_internals().as_str(),
+            "TypedDictFallback",
+        ) {
+            // TypedDictFallback is a fake base for TypedDict classes. Typeshed models it as
+            // inheriting from Mapping for convenience, but it should not get Mapping's
+            // unimplemented abstract methods.
+            return AbstractClassMembers::new(SmallSet::new());
         }
+        let metadata = self.get_metadata_for_class(cls);
         // Check inherited abstract methods + all fields defined in the current class
+        let mut inherited_fields_to_check = SmallSet::new();
         for base_class in metadata.base_class_objects() {
-            let base_class_metadata = self.get_metadata_for_class(base_class);
-            // For now, skip any non-protocols base classes that don't extend `ABC` or have metaclass `ABCMeta`
-            // Consider adding a stricter check in the future
-            if !base_class_metadata.extends_abc() && !base_class_metadata.is_protocol() {
-                continue;
-            }
             let base_class_abstract_members = self.get_abstract_members_for_class(base_class);
-            fields_to_check.extend(
+            inherited_fields_to_check.extend(
                 base_class_abstract_members
                     .unimplemented_abstract_methods()
                     .iter()
                     .cloned(),
             );
         }
-
-        let mut abstract_members = SmallSet::new();
-        for field_name in fields_to_check {
-            // If the class has a synthesized concrete implementation (e.g., `__dataclass_fields__`
-            // from @dataclass), that satisfies any protocol requirement for this field.
-            if self
-                .get_class_member(cls, &field_name)
-                .is_some_and(|f| !f.is_abstract() && !f.is_uninit_class_var())
-            {
-                continue;
-            }
-            if let Some(field) =
-                self.get_non_synthesized_class_member_and_defining_class(cls, &field_name)
-                && (field.value.is_abstract() ||
-                // Uninitialized class vars in protocols are considered absract, unless it is in a stub file
-                (!cls.module().path().is_interface() && field.value.is_uninit_class_var() &&
-                self.get_metadata_for_class(&field.defining_class).is_protocol()))
-            {
-                abstract_members.insert(field_name.clone());
-            }
+        let mut abstract_members = inherited_fields_to_check
+            .iter()
+            .filter(|field_name| !self.is_implemented_in_class(cls, field_name))
+            .cloned()
+            .collect::<SmallSet<_>>();
+        // Ideally, we would only check `extends_abc` here. What complicates things is that a class
+        // can implicitly extend ABC by inheriting from `Protocol`, because `_ProtocolMeta`
+        // inherits from `ABCMeta`. Stub files do not accurately mark classes that are protocols at
+        // runtime, so we cannot reliably follow `extends_abc` for protocols in stub files.
+        // Instead, we apply the following rules:
+        // * If a class is a protocol, we respect its abstract methods.
+        // * If a class inherits unimplemented abstract methods, it also inherits the judgment that
+        //   abstract methods are respected.
+        // Crucially, this means that if all inherited abstract methods have been implemented, we
+        // do not treat the class as abstract. So, for example, `typing.Sequence` is abstract
+        // because it inherits an unimplemented abstract `__len__` method from `Collection`, but
+        // `tuple` implements all of the abstract methods it inherits from `Sequence`, so `tuple`
+        // and its subclasses are not abstract.
+        if (metadata.extends_abc() || metadata.is_protocol() || !abstract_members.is_empty())
+            && let Some(fields) = self.get_class_fields(cls)
+        {
+            abstract_members.extend(
+                fields
+                    .names()
+                    .filter(|field_name| {
+                        !inherited_fields_to_check.contains(*field_name)
+                            && !self.is_implemented_in_class(cls, field_name)
+                    })
+                    .cloned(),
+            )
         }
         AbstractClassMembers::new(abstract_members)
     }