From 0959b03fcdcdc2456b0c661c70bb2e8ce175b69c Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Tue, 4 Aug 2026 17:59:17 -0700 Subject: [PATCH 1/6] Relax overly strict metaclass conflict detection Summary: When a class has multiple possible metaclasses through a `metaclass=...` declaration and/or inherited metaclasses, pyrefly grabbed the first metaclass using MRO precedence and then checked if it was legal. What the runtime actually does is look at all the possible metaclasses and choose a legal one, if one exists. This diff adjusts pyrefly's logic to match the runtime. The error message for metaclass conflicts has also been tweaked. The new selection process doesn't always select the direct metaclass in case of a conflict, so the "is not a subclass" language sometimes misleadingly implied that a base's metaclass needed to be modified. Finally, I modified `Metaclass::Inherited` to also store the direct `metaclass=...` declaration, so that `implicit-abstract-class` detection continues to work. Reviewed By: grievejia Differential Revision: D114434102 --- pyrefly/lib/alt/class/class_metadata.rs | 174 +++++++++++++----------- pyrefly/lib/alt/types/class_metadata.rs | 34 +++-- pyrefly/lib/test/abstract_methods.rs | 13 ++ pyrefly/lib/test/class_keywords.rs | 48 ++++++- pyrefly/lib/test/constructors.rs | 12 ++ pyrefly/lib/test/enums.rs | 4 +- pyrefly/lib/test/protocol.rs | 4 +- 7 files changed, 189 insertions(+), 100 deletions(-) diff --git a/pyrefly/lib/alt/class/class_metadata.rs b/pyrefly/lib/alt/class/class_metadata.rs index 98b5ac6c06..1c9995ac47 100644 --- a/pyrefly/lib/alt/class/class_metadata.rs +++ b/pyrefly/lib/alt/class/class_metadata.rs @@ -185,8 +185,31 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { errors, ); - // Compute base classes with metadata. - let bases_with_metadata = self.bases_with_metadata(parsed_results, is_new_type, errors); + let protocol_base_name = Name::new_static("Protocol"); + let base_metaclasses = bases + .iter() + .zip(&parsed_results) + .filter_map(|(base, parsed)| match (base, parsed) { + ( + BaseClass::Generic(BaseClassGeneric { + kind: BaseClassGenericKind::Protocol, + .. + }), + _, + ) if !self.module().path().is_interface() => { + // `Protocol` has metaclass `_ProtocolMeta`. `Protocol` is a special form in typeshed, + // so we inject the metaclass here so that metaclass-driven checks work. Stubs often + // model things as protocols even when they aren't at runtime, so we can be confident + // that the class has `_ProtocolMeta` only when it is defined in a source (.py) file. + Some((&protocol_base_name, self.stdlib.protocol_meta())) + } + (_, BaseClassParseResult::Parsed(parsed)) => parsed + .metadata + .custom_metaclass() + .map(|metaclass| (parsed.class_object.name(), metaclass)), + _ => None, + }) + .collect::>(); // Compute class keywords, including the metaclass. let (metaclasses, keyword_annotations): (Vec<_>, Vec<(_, _)>) = @@ -196,40 +219,26 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { }); let keyword_annotations = keyword_annotations.into_map(|(name, annot)| (name.id, annot)); - let protocol_base_name = Name::new_static("Protocol"); - let mut base_metaclasses = bases_with_metadata - .iter() - .filter_map(|(b, metadata)| metadata.custom_metaclass().map(|m| (b.name(), m))) - .collect::>(); - if protocol_metadata.is_some() && !self.module().path().is_interface() { - // `Protocol` has metaclass `_ProtocolMeta`. `Protocol` is a special form in typeshed, - // so we inject the metaclass here so that metaclass-driven checks work. Stubs often - // model things as protocols even when they aren't at runtime, so we can be confident - // 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)); + if let Some(metaclass) = &direct_metaclass + && 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(), + ); } + let mut calculated_metaclass = + self.calculate_metaclass(cls, direct_metaclass, &base_metaclasses, errors); // If the metaclass has unresolved type variables, replace them with their // gradual types (e.g. Any) to avoid cascading errors from bare TypeVars. // We do a targeted substitution inside each targ so that e.g. Meta[list[T]] @@ -260,6 +269,8 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { } } let metaclass = calculated_metaclass.get(); + // Compute base classes with metadata. + let bases_with_metadata = self.bases_with_metadata(parsed_results, is_new_type, errors); self.check_init_subclass_keywords(cls, &bases_with_metadata, metaclass, keywords, errors); let mut directly_inherits_model = false; @@ -1836,61 +1847,66 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { fn calculate_metaclass( &self, cls: &Class, - raw_metaclass: Option<&Expr>, + direct_metaclass: Option, 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) - } else { - let mut inherited_meta: Option = 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); + // 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 candidate = direct_metaclass + .as_ref() + .map(|m| (None, self.heap.mk_class_type(m.clone()))); + for (base_name, base_metaclass) in base_metaclasses { + let base_metaclass_type = self.heap.mk_class_type((*base_metaclass).clone()); + if let Some((candidate_name, candidate_metaclass_type)) = &candidate { + if self.is_subset_eq(candidate_metaclass_type, &base_metaclass_type) { + // Keep the current candidate. + } else if self.is_subset_eq(&base_metaclass_type, candidate_metaclass_type) { + candidate = Some((Some(base_name), base_metaclass_type)); + } else { + let origin = |base_name| { + if let Some(name) = base_name { + format!(" from base class `{name}`") + } else { + "".to_owned() + } + }; + self.error(errors, + cls.range(), + ErrorKind::InvalidInheritance, + format!( + "Class `{}` has metaclass `{}`{} which is not compatible with metaclass `{}`{}", + cls.name(), + self.for_display(candidate_metaclass_type.clone()), + origin(*candidate_name), + self.for_display(base_metaclass_type), + origin(Some(base_name)), + ), + ); + break; } + } else { + // All custom metaclasses are subclasses of `type`. + candidate = Some((Some(base_name), base_metaclass_type)); } - 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) { - self.error(errors, - cls.range(), - ErrorKind::InvalidInheritance, - format!( - "Class `{}` has metaclass `{}` which is not a subclass of metaclass `{}` from base class `{}`", - cls.name(), - self.for_display(metaclass_type.clone()), - self.for_display(base_metaclass_type), - base_name, - ), - ); + match candidate { + Some((candidate_name, Type::ClassType(candidate_metaclass))) => { + if candidate_name.is_some() { + Metaclass::Inherited { + metaclass: candidate_metaclass, + is_explicitly_abstract: direct_metaclass.is_some_and(|direct_metaclass| { + direct_metaclass + .class_object() + .has_toplevel_qname("abc", "ABCMeta") + }), + } + } else { + Metaclass::Direct(candidate_metaclass) + } } + Some(_) => unreachable!("Metaclass must be a ClassType"), + None => Metaclass::None, } } diff --git a/pyrefly/lib/alt/types/class_metadata.rs b/pyrefly/lib/alt/types/class_metadata.rs index 19fa8aef63..89a89b5eef 100644 --- a/pyrefly/lib/alt/types/class_metadata.rs +++ b/pyrefly/lib/alt/types/class_metadata.rs @@ -278,10 +278,6 @@ impl ClassMetadata { self.metaclass.get() } - pub fn custom_metaclass_raw(&self) -> &Metaclass { - &self.metaclass - } - /// The class's metaclass. pub fn metaclass<'a>(&'a self, stdlib: &'a Stdlib) -> &'a ClassType { self.custom_metaclass() @@ -341,15 +337,16 @@ impl ClassMetadata { return true; } } - // Only check the metaclass if it's directly specified on this class - if let Metaclass::Direct(metaclass) = self.custom_metaclass_raw() - && metaclass + match &self.metaclass { + Metaclass::Direct(metaclass) => metaclass .class_object() - .has_toplevel_qname("abc", "ABCMeta") - { - return true; + .has_toplevel_qname("abc", "ABCMeta"), + Metaclass::Inherited { + is_explicitly_abstract, + .. + } => *is_explicitly_abstract, + Metaclass::None => false, } - false } pub fn deprecation(&self) -> Option<&Deprecation> { @@ -544,7 +541,14 @@ impl Display for ClassSynthesizedFields { #[derive(Clone, Debug, TypeEq, PartialEq, Eq, Default)] pub enum Metaclass { Direct(ClassType), - Inherited(ClassType), + Inherited { + /// The actual metaclass, which is inherited from a parent. + metaclass: ClassType, + /// Whether the class has a `metaclass=...` declaration that marks the class as explicitly + /// abstract. Note that in this case the declared metaclass did *not* end up being the + /// class's resolved metaclass, but we still use it to determine intended abstract-ness. + is_explicitly_abstract: bool, + }, #[default] None, } @@ -553,7 +557,7 @@ impl Display for Metaclass { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match &self { Self::Direct(metaclass) => write!(f, "{metaclass}"), - Self::Inherited(metaclass) => write!(f, "inherited({metaclass})"), + Self::Inherited { metaclass, .. } => write!(f, "inherited({metaclass})"), Self::None => write!(f, "type"), } } @@ -564,7 +568,7 @@ impl Metaclass { pub fn get(&self) -> Option<&ClassType> { match self { Self::Direct(metaclass) => Some(metaclass), - Self::Inherited(metaclass) => Some(metaclass), + Self::Inherited { metaclass, .. } => Some(metaclass), Self::None => None, } } @@ -572,7 +576,7 @@ impl Metaclass { pub fn get_mut(&mut self) -> Option<&mut ClassType> { match self { Self::Direct(metaclass) => Some(metaclass), - Self::Inherited(metaclass) => Some(metaclass), + Self::Inherited { metaclass, .. } => Some(metaclass), Self::None => None, } } diff --git a/pyrefly/lib/test/abstract_methods.rs b/pyrefly/lib/test/abstract_methods.rs index 58a49b7da9..193ed170a3 100644 --- a/pyrefly/lib/test/abstract_methods.rs +++ b/pyrefly/lib/test/abstract_methods.rs @@ -497,6 +497,19 @@ class D(C): # E: Class `D` has unimplemented abstract members: `bar` "#, ); +testcase!( + test_inherited_metaclass_and_explicit_abstractness, + TestEnv::new().enable_implicit_abstract_class_error(), + r#" +from abc import ABCMeta, abstractmethod +class M(ABCMeta): ... +class A(metaclass=M): ... +class B(A, metaclass=ABCMeta): + @abstractmethod + def f(self) -> None: ... + "#, +); + testcase!( test_uninit_classvar_abc, r#" diff --git a/pyrefly/lib/test/class_keywords.rs b/pyrefly/lib/test/class_keywords.rs index 946e37ab60..33ef35bc2a 100644 --- a/pyrefly/lib/test/class_keywords.rs +++ b/pyrefly/lib/test/class_keywords.rs @@ -113,7 +113,7 @@ testcase!( class M0(type): pass class M1(type): pass class B(metaclass=M0): pass -class A(B, metaclass=M1): # E: Class `A` has metaclass `M1` which is not a subclass of metaclass `M0` from base class `B` +class A(B, metaclass=M1): # E: Class `A` has metaclass `M1` which is not compatible with metaclass `M0` from base class `B` pass "#, ); @@ -125,7 +125,7 @@ class M0(type): pass class M1(type): pass class B0(metaclass=M0): pass class B1(metaclass=M1): pass -class A(B0, B1): # E: Class `A` has metaclass `M0` which is not a subclass of metaclass `M1` from base class `B1` +class A(B0, B1): # E: Class `A` has metaclass `M0` from base class `B0` which is not compatible with metaclass `M1` from base class `B1` pass "#, ); @@ -176,3 +176,47 @@ class A(**f): # E: Unpacking is not supported in class header pass "#, ); + +testcase!( + test_metaclasses_ok_any_order, + r#" +from typing import assert_type + +class Meta1(type): ... +class Meta2(Meta1): + x: int = 0 + +class A1(metaclass=Meta1): ... +class A2(A1, metaclass=Meta2): ... + +class B1(metaclass=Meta2): ... +# B2's metaclass has to be a (non-strict) subclass of Meta1 and Meta2. The only legal metaclass is +# Meta2, which is what the runtime chooses (despite the explicit metaclass=Meta1 declaration). +class B2(B1, metaclass=Meta1): ... + +assert_type(A2.__class__.x, int) +assert_type(B2.__class__.x, int) + "#, +); + +testcase!( + test_metaclass_conflict_preserves_running_winner, + r#" +from typing import assert_type + +class MA(type): + x: int +class MB(type): ... +class MC(MA, MB): ... + +class A(metaclass=MA): ... +class B(metaclass=MB): ... +class C(metaclass=MC): ... + +class X(A, B, C): ... # E: not compatible with metaclass +class Y(A, C, B): ... + +assert_type(X.x, int) +assert_type(Y.x, int) + "#, +); diff --git a/pyrefly/lib/test/constructors.rs b/pyrefly/lib/test/constructors.rs index 419c63d30d..ebfa9b2991 100644 --- a/pyrefly/lib/test/constructors.rs +++ b/pyrefly/lib/test/constructors.rs @@ -187,6 +187,18 @@ assert_type(C(), C[Any]) # Correct, because invalid metaclass. "#, ); +testcase!( + test_metaclass_invalid_losing_direct_generic, + r#" +from typing import Any + +class Meta1[T](type): ... +class Meta2[T](Meta1[T]): ... +class A(metaclass=Meta2[Any]): ... +class B[T](A, metaclass=Meta1[T]): ... # E: Metaclass may not be an unbound generic + "#, +); + testcase!( test_init_subclass_class_keywords, r#" diff --git a/pyrefly/lib/test/enums.rs b/pyrefly/lib/test/enums.rs index bb6afd19f4..84065d8383 100644 --- a/pyrefly/lib/test/enums.rs +++ b/pyrefly/lib/test/enums.rs @@ -1210,7 +1210,7 @@ class MyMeta(type): def __getitem__(cls, item) -> str: ... def __len__(cls) -> int: ... -class Base(Enum, metaclass=MyMeta): # E: Class `Base` has metaclass `MyMeta` which is not a subclass of metaclass `EnumMeta` from base class `Enum` +class Base(Enum, metaclass=MyMeta): # E: Class `Base` has metaclass `MyMeta` which is not compatible with metaclass `EnumMeta` from base class `Enum` @classmethod def where(cls, pred: bool, a: Self, b: Self) -> Self: ... @@ -1237,7 +1237,7 @@ from enum import Enum class MyMeta(type): pass -class E(Enum, metaclass=MyMeta): # E: Class `E` has metaclass `MyMeta` which is not a subclass of metaclass `EnumMeta` from base class `Enum` +class E(Enum, metaclass=MyMeta): # E: Class `E` has metaclass `MyMeta` which is not compatible with metaclass `EnumMeta` from base class `Enum` A = 1 B = 2 C = 3 diff --git a/pyrefly/lib/test/protocol.rs b/pyrefly/lib/test/protocol.rs index dbacc05e3c..fc18672ddd 100644 --- a/pyrefly/lib/test/protocol.rs +++ b/pyrefly/lib/test/protocol.rs @@ -67,8 +67,8 @@ from typing import Protocol class M(type): ... class P(Protocol): ... -class A(P, metaclass=M): ... # E: has metaclass `M` which is not a subclass of metaclass `_ProtocolMeta` -class E(Enum, P): ... # E: has metaclass `EnumMeta` which is not a subclass of metaclass `_ProtocolMeta` +class A(P, metaclass=M): ... # E: has metaclass `M` which is not compatible with metaclass `_ProtocolMeta` +class E(Enum, P): ... # E: has metaclass `EnumMeta` from base class `Enum` which is not compatible with metaclass `_ProtocolMeta` from base class `P` "#, ); From 3b285aee1e565c128540672ea7d52d62596c3e4a Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Tue, 4 Aug 2026 17:59:17 -0700 Subject: [PATCH 2/6] Allow redundant `metaclass=type` declaration Summary: It's pointless but perfectly legal to write `metaclass=type`. ~~The conformance change is because pyrefly previously accidentally passed a test due to emitting a FP on a line on which an entirely different error is expected. I believe pyrefly's behavior as of this diff is correct and the test is overly strict: I've opened https://github.com/python/typing/pull/2327 to adjust the test.~~ The conformance test has been updated :) Reviewed By: yangdanny97 Differential Revision: D114434451 --- pyrefly/lib/alt/class/class_metadata.rs | 1 + pyrefly/lib/test/class_keywords.rs | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/pyrefly/lib/alt/class/class_metadata.rs b/pyrefly/lib/alt/class/class_metadata.rs index 1c9995ac47..78b39d71d4 100644 --- a/pyrefly/lib/alt/class/class_metadata.rs +++ b/pyrefly/lib/alt/class/class_metadata.rs @@ -1937,6 +1937,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { None } } + Type::Type(inner) if inner.is_any() => None, // redundant but legal ty => { self.error( errors, diff --git a/pyrefly/lib/test/class_keywords.rs b/pyrefly/lib/test/class_keywords.rs index 33ef35bc2a..cd3d050948 100644 --- a/pyrefly/lib/test/class_keywords.rs +++ b/pyrefly/lib/test/class_keywords.rs @@ -220,3 +220,12 @@ assert_type(X.x, int) assert_type(Y.x, int) "#, ); + +testcase!( + test_redundant_type_metaclass_is_ok, + r#" +from typing import Any +class A(metaclass=type): ... +class B(metaclass=type[Any]): ... + "#, +); From 8ef102c4ae36a3469bfc1663d2d5b49c2a712c27 Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Tue, 4 Aug 2026 17:59:17 -0700 Subject: [PATCH 3/6] Extract `is_implemented_in_class` out of `calculate_abstract_members` Summary: Small readability improvement. Reviewed By: yangdanny97 Differential Revision: D114454605 --- pyrefly/lib/alt/class/class_metadata.rs | 46 ++++++++++++++----------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/pyrefly/lib/alt/class/class_metadata.rs b/pyrefly/lib/alt/class/class_metadata.rs index 78b39d71d4..128481633c 100644 --- a/pyrefly/lib/alt/class/class_metadata.rs +++ b/pyrefly/lib/alt/class/class_metadata.rs @@ -2082,6 +2082,27 @@ 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; @@ -2109,27 +2130,10 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { .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 abstract_members = fields_to_check + .into_iter() + .filter(|field_name| !self.is_implemented_in_class(cls, field_name)) + .collect(); AbstractClassMembers::new(abstract_members) } From f3ed9a279713c6907283f9608632baedfbc5d1e8 Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Tue, 4 Aug 2026 17:59:17 -0700 Subject: [PATCH 4/6] Collect all unimplemented abstract methods from bases Summary: In `calculate_abstract_members`, we had an incorrect filter that prevented us from transitively collecting unimplemented abstract methods. E.g., we recognized that `Sequence` was uninstantiable, but not that a subclass of `Sequence` was also uninstantiable. This diff removes the filter. Removing the filter causes us to incorrectly propagate unimplemented Mapping methods to TypedDicts, because of a fake `TypedDictFallback` base that doesn't exist at runtime. We detect and ignore these methods. There's still a bug where we only pick up one of `Sequence`'s two unimplemented abstract methods. The rest of the stack will fix this. Reviewed By: fangyi-zhou Differential Revision: D114462164 --- pyrefly/lib/alt/class/class_metadata.rs | 15 +++-- pyrefly/lib/test/abstract_methods.rs | 67 +++++++++++++++++++ .../test_laziness/test_attribute_inherited.md | 2 +- .../test_attribute_on_class_itself.md | 2 +- ...ltiple_inheritance_solves_unique_fields.md | 2 + 5 files changed, 80 insertions(+), 8 deletions(-) diff --git a/pyrefly/lib/alt/class/class_metadata.rs b/pyrefly/lib/alt/class/class_metadata.rs index 128481633c..095eecbf2b 100644 --- a/pyrefly/lib/alt/class/class_metadata.rs +++ b/pyrefly/lib/alt/class/class_metadata.rs @@ -2104,6 +2104,15 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { } pub fn calculate_abstract_members(&self, cls: &Class) -> AbstractClassMembers { + 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); let mut fields_to_check: SmallSet; if metadata.extends_abc() || metadata.is_protocol() { @@ -2116,12 +2125,6 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { } // Check inherited abstract methods + all fields defined in the current class 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( base_class_abstract_members diff --git a/pyrefly/lib/test/abstract_methods.rs b/pyrefly/lib/test/abstract_methods.rs index 193ed170a3..5d1d40dd59 100644 --- a/pyrefly/lib/test/abstract_methods.rs +++ b/pyrefly/lib/test/abstract_methods.rs @@ -609,6 +609,62 @@ A() # E: Cannot instantiate `A` "#, ); +testcase!( + bug = "Unimplemented abstract method `__getitem__` is missing", + test_final_class_with_unimplemented_sequence_methods, + r#" +from typing import final +from collections.abc import Sequence +@final +class A[T](Sequence[T]): # E: cannot have unimplemented abstract members: `__len__` + ... + "#, +); + +testcase!( + test_final_class_with_unimplemented_collection_and_reversible_methods, + r#" +from typing import final +from collections.abc import Collection, Reversible +@final +class B[T](Collection[T], Reversible[T]): # E: cannot have unimplemented abstract members: `__len__`, `__iter__`, `__contains__`, `__reversed__` + ... + "#, +); + +testcase!( + test_final_class_with_unimplemented_abstract_methods, + r#" +from typing import final +from abc import ABC, abstractmethod + +class A1(ABC): + @abstractmethod + def a(): ... + +class B1(ABC): + @abstractmethod + def b(): ... + +class C(A1, B1): ... + +@final +class D(C): ... # E: cannot have unimplemented abstract members: `a`, `b` + "#, +); + +// Even though a TypedDict's fake TypedDictFallback base inherits from Mapping, which has abstract +// methods, we must never consider a TypedDict to have unimplemented abstract methods. +testcase!( + test_typed_dict_is_not_abstract, + r#" +from typing import TypedDict, final +@final +class FinalTD(TypedDict): + year: int +"#, +); + // Tests for invalid-abstract-method: @abstractmethod in a non-abstract class. testcase!( @@ -728,6 +784,17 @@ class Child(Base): "#, ); +testcase!( + test_invalid_abstract_method_tuple_child_is_not_abstract, + TestEnv::new().enable_invalid_abstract_method_error(), + r#" +from abc import abstractmethod +class A(tuple): + @abstractmethod + def f(self): ... # E: `A` is not an abstract class +"#, +); + testcase!( test_invalid_abstract_method_off_by_default, r#" diff --git a/pyrefly/test_laziness/test_attribute_inherited.md b/pyrefly/test_laziness/test_attribute_inherited.md index 89b6fe6e9d..72de6cbda7 100644 --- a/pyrefly/test_laziness/test_attribute_inherited.md +++ b/pyrefly/test_laziness/test_attribute_inherited.md @@ -69,7 +69,7 @@ a -> b::KeyClassMetadata(ClassDefIndex(0)) b -> c::KeyClassMetadata(ClassDefIndex(0)) a -> b::KeyClassMetadata(ClassDefIndex(0)) a -> b::KeyAbstractClassCheck(ClassDefIndex(0)) - b -> c::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyAbstractClassCheck(ClassDefIndex(0)) a -> b::KeyClassSynthesizedFields(ClassDefIndex(0)) a -> b::KeyClassMro(ClassDefIndex(0)) b -> c::KeyClassMetadata(ClassDefIndex(0)) diff --git a/pyrefly/test_laziness/test_attribute_on_class_itself.md b/pyrefly/test_laziness/test_attribute_on_class_itself.md index 9e1bec387b..0723dceacc 100644 --- a/pyrefly/test_laziness/test_attribute_on_class_itself.md +++ b/pyrefly/test_laziness/test_attribute_on_class_itself.md @@ -71,7 +71,7 @@ a -> b::KeyClassMetadata(ClassDefIndex(0)) b -> c::KeyClassMetadata(ClassDefIndex(0)) a -> b::KeyClassMetadata(ClassDefIndex(0)) a -> b::KeyAbstractClassCheck(ClassDefIndex(0)) - b -> c::KeyClassMetadata(ClassDefIndex(0)) + b -> c::KeyAbstractClassCheck(ClassDefIndex(0)) a -> b::KeyClassSynthesizedFields(ClassDefIndex(0)) a -> b::KeyClassMro(ClassDefIndex(0)) b -> c::KeyClassMetadata(ClassDefIndex(0)) diff --git a/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md b/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md index b13cad8b86..9341c012bf 100644 --- a/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md +++ b/pyrefly/test_laziness/test_multiple_inheritance_solves_unique_fields.md @@ -74,6 +74,8 @@ a -> c::KeyClassField(ClassDefIndex(0), Name("shared")) a -> b::KeyClassField(ClassDefIndex(0), Name("shared")) a -> b::KeyClassDisjointBase(ClassDefIndex(0)) a -> c::KeyClassDisjointBase(ClassDefIndex(0)) +a -> b::KeyAbstractClassCheck(ClassDefIndex(0)) +a -> c::KeyAbstractClassCheck(ClassDefIndex(0)) a -> b::KeyClassSynthesizedFields(ClassDefIndex(0)) a -> c::KeyClassSynthesizedFields(ClassDefIndex(0)) ``` From b14f327d5316ab68c57159fd450eabf9dec2b54f Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Tue, 4 Aug 2026 17:59:17 -0700 Subject: [PATCH 5/6] Check inherited methods first in `calculate_class_members` Summary: The only user-facing effect of this change is that the order in which methods are listed in some error messages is shuffled around. I split this out of the next diff so that its impact can be evaluated without the noise of the changing error messages. Reviewed By: shobhitmehro Differential Revision: D114455661 --- pyrefly/lib/alt/class/class_metadata.rs | 32 +++++++++++++++---------- pyrefly/lib/test/abstract_methods.rs | 2 +- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/pyrefly/lib/alt/class/class_metadata.rs b/pyrefly/lib/alt/class/class_metadata.rs index 095eecbf2b..ac790c0244 100644 --- a/pyrefly/lib/alt/class/class_metadata.rs +++ b/pyrefly/lib/alt/class/class_metadata.rs @@ -2114,29 +2114,35 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { return AbstractClassMembers::new(SmallSet::new()); } let metadata = self.get_metadata_for_class(cls); - let mut fields_to_check: SmallSet; - 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(); - } // 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_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 abstract_members = fields_to_check - .into_iter() + let mut abstract_members = inherited_fields_to_check + .iter() .filter(|field_name| !self.is_implemented_in_class(cls, field_name)) - .collect(); + .cloned() + .collect::>(); + if (metadata.extends_abc() || metadata.is_protocol()) + && 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) } diff --git a/pyrefly/lib/test/abstract_methods.rs b/pyrefly/lib/test/abstract_methods.rs index 5d1d40dd59..e315595886 100644 --- a/pyrefly/lib/test/abstract_methods.rs +++ b/pyrefly/lib/test/abstract_methods.rs @@ -627,7 +627,7 @@ testcase!( from typing import final from collections.abc import Collection, Reversible @final -class B[T](Collection[T], Reversible[T]): # E: cannot have unimplemented abstract members: `__len__`, `__iter__`, `__contains__`, `__reversed__` +class B[T](Collection[T], Reversible[T]): # E: cannot have unimplemented abstract members: `__iter__`, `__contains__`, `__len__`, `__reversed__` ... "#, ); From 8367e6c8a5b57a952efe2c331d28d9f251f2ba08 Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Tue, 4 Aug 2026 17:59:17 -0700 Subject: [PATCH 6/6] Expand when unimplemented abstract methods are collected (#4396) Summary: Fixes https://github.com/facebook/pyrefly/issues/2797. Previously, we only treated a class's `abstractmethod` methods as unimplemented abstract methods when the class extended ABC or was a protocol. This missed some abstract methods because we can't track ABC inheritance properly in stub files. This diff adds a rule that if a class has a base with unimplemented abstract methods, the class can also collect unimplemented abstract methods. This fixes the linked issue while ensuring we don't accidentally mark concrete classes as abstract. Reviewed By: fangyi-zhou Differential Revision: D114166584 --- pyrefly/lib/alt/class/class_metadata.rs | 15 ++++++++++++++- pyrefly/lib/test/abstract_methods.rs | 3 +-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/pyrefly/lib/alt/class/class_metadata.rs b/pyrefly/lib/alt/class/class_metadata.rs index ac790c0244..af91110fc2 100644 --- a/pyrefly/lib/alt/class/class_metadata.rs +++ b/pyrefly/lib/alt/class/class_metadata.rs @@ -2130,7 +2130,20 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { .filter(|field_name| !self.is_implemented_in_class(cls, field_name)) .cloned() .collect::>(); - if (metadata.extends_abc() || metadata.is_protocol()) + // 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( diff --git a/pyrefly/lib/test/abstract_methods.rs b/pyrefly/lib/test/abstract_methods.rs index e315595886..0ff559d78c 100644 --- a/pyrefly/lib/test/abstract_methods.rs +++ b/pyrefly/lib/test/abstract_methods.rs @@ -610,13 +610,12 @@ A() # E: Cannot instantiate `A` ); testcase!( - bug = "Unimplemented abstract method `__getitem__` is missing", test_final_class_with_unimplemented_sequence_methods, r#" from typing import final from collections.abc import Sequence @final -class A[T](Sequence[T]): # E: cannot have unimplemented abstract members: `__len__` +class A[T](Sequence[T]): # E: cannot have unimplemented abstract members: `__len__`, `__getitem__` ... "#, );