diff --git a/conformance/third_party/conformance.exp b/conformance/third_party/conformance.exp index 704245668..12627d510 100644 --- a/conformance/third_party/conformance.exp +++ b/conformance/third_party/conformance.exp @@ -11242,13 +11242,46 @@ { "code": -2, "column": 5, - "concise_description": "Class member `F3.a` overrides parent class `F1` in an inconsistent manner", - "description": "Class member `F3.a` overrides parent class `F1` in an inconsistent manner\n `F3.a` is read-only, but `F1.a` is read-write", + "concise_description": "TypedDict field `a` in `F3` cannot be marked read-only; parent TypedDict `F1` defines it as mutable", + "description": "TypedDict field `a` in `F3` cannot be marked read-only; parent TypedDict `F1` defines it as mutable", "line": 94, - "name": "bad-override", + "name": "bad-typed-dict-key", "severity": "error", "stop_column": 6, "stop_line": 94 + }, + { + "code": -2, + "column": 5, + "concise_description": "TypedDict field `a` in `F4` must remain required because parent TypedDict `F1` defines it as required", + "description": "TypedDict field `a` in `F4` must remain required because parent TypedDict `F1` defines it as required", + "line": 98, + "name": "bad-typed-dict-key", + "severity": "error", + "stop_column": 6, + "stop_line": 98 + }, + { + "code": -2, + "column": 5, + "concise_description": "TypedDict field `c` in `F6` cannot be made non-required; parent TypedDict `F1` defines it as required", + "description": "TypedDict field `c` in `F6` cannot be made non-required; parent TypedDict `F1` defines it as required", + "line": 106, + "name": "bad-typed-dict-key", + "severity": "error", + "stop_column": 6, + "stop_line": 106 + }, + { + "code": -2, + "column": 7, + "concise_description": "TypedDict field `x` in `TD_B` cannot be made non-required; parent TypedDict `TD_B2` defines it as required", + "description": "TypedDict field `x` in `TD_B` cannot be made non-required; parent TypedDict `TD_B2` defines it as required", + "line": 132, + "name": "bad-typed-dict-key", + "severity": "error", + "stop_column": 11, + "stop_line": 132 } ], "typeddicts_readonly_kwargs.py": [ diff --git a/conformance/third_party/conformance.result b/conformance/third_party/conformance.result index 96ac7eeee..67af0c096 100644 --- a/conformance/third_party/conformance.result +++ b/conformance/third_party/conformance.result @@ -331,10 +331,7 @@ "typeddicts_readonly.py": [], "typeddicts_readonly_consistency.py": [], "typeddicts_readonly_inheritance.py": [ - "Line 98: Expected 1 errors", - "Line 106: Expected 1 errors", - "Line 119: Expected 1 errors", - "Line 132: Expected 1 errors" + "Line 119: Expected 1 errors" ], "typeddicts_readonly_kwargs.py": [], "typeddicts_readonly_update.py": [], diff --git a/conformance/third_party/results.json b/conformance/third_party/results.json index 5fed80afc..0d0d9a4cd 100644 --- a/conformance/third_party/results.json +++ b/conformance/third_party/results.json @@ -3,7 +3,7 @@ "pass": 97, "fail": 41, "pass_rate": 0.7, - "differences": 164, + "differences": 161, "passing": [ "aliases_explicit.py", "aliases_newtype.py", @@ -143,7 +143,7 @@ "qualifiers_final_annotation.py": 12, "specialtypes_never.py": 1, "specialtypes_type.py": 8, - "typeddicts_readonly_inheritance.py": 4, + "typeddicts_readonly_inheritance.py": 1, "typeddicts_required.py": 1 }, "comment": "@generated" diff --git a/pyrefly/lib/alt/class/class_field.rs b/pyrefly/lib/alt/class/class_field.rs index 8723c6cba..888ca1c03 100644 --- a/pyrefly/lib/alt/class/class_field.rs +++ b/pyrefly/lib/alt/class/class_field.rs @@ -1780,6 +1780,100 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { .is_some_and(|metadata| metadata.fields.contains_key(field_name)) } + fn typed_dict_field_info( + &self, + metadata: &ClassMetadata, + field_name: &Name, + field: &ClassField, + ) -> Option { + metadata + .typed_dict_metadata() + .and_then(|typed_dict| typed_dict.fields.get(field_name)) + .and_then(|is_total| field.clone().as_typed_dict_field_info(*is_total)) + } + + fn validate_typed_dict_field_override( + &self, + child_cls: &Class, + child_metadata: &ClassMetadata, + parent_cls: &Class, + parent_metadata: &ClassMetadata, + field_name: &Name, + child_field: &ClassField, + parent_field: &ClassField, + range: TextRange, + errors: &ErrorCollector, + ) -> bool { + let Some(child_info) = self.typed_dict_field_info(child_metadata, field_name, child_field) + else { + return true; + }; + let Some(parent_info) = + self.typed_dict_field_info(parent_metadata, field_name, parent_field) + else { + return true; + }; + + let parent_mutable = !parent_info.is_read_only(); + let child_mutable = !child_info.is_read_only(); + + if parent_mutable { + if !child_mutable { + self.error( + errors, + range, + ErrorInfo::Kind(ErrorKind::BadTypedDictKey), + format!( + "TypedDict field `{field_name}` in `{}` cannot be marked read-only; parent TypedDict `{}` defines it as mutable", + child_cls.name(), + parent_cls.name() + ), + ); + return false; + } + if parent_info.required && !child_info.required { + self.error( + errors, + range, + ErrorInfo::Kind(ErrorKind::BadTypedDictKey), + format!( + "TypedDict field `{field_name}` in `{}` must remain required because parent TypedDict `{}` defines it as required", + child_cls.name(), + parent_cls.name() + ), + ); + return false; + } + if !parent_info.required && child_info.required { + self.error( + errors, + range, + ErrorInfo::Kind(ErrorKind::BadTypedDictKey), + format!( + "TypedDict field `{field_name}` in `{}` cannot be made required; parent TypedDict `{}` defines it as non-required", + child_cls.name(), + parent_cls.name() + ), + ); + return false; + } + } else if parent_info.required && !child_info.required { + self.error( + errors, + range, + ErrorInfo::Kind(ErrorKind::BadTypedDictKey), + format!( + "TypedDict field `{field_name}` in `{}` cannot be made non-required; parent TypedDict `{}` defines it as required", + child_cls.name(), + parent_cls.name() + ), + ); + return false; + } + + true + } + fn should_check_field_for_override_consistency(&self, field_name: &Name) -> bool { // Object construction (`__new__`, `__init__`, `__init_subclass__`) should not participate in override checks if field_name == &dunder::NEW @@ -1855,8 +1949,8 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { let mut got_attribute = None; let mut parent_attr_found = false; let mut parent_has_any = false; - let is_typed_dict_field = - self.is_typed_dict_field(&self.get_metadata_for_class(cls), field_name); + let metadata = self.get_metadata_for_class(cls); + let is_typed_dict_field = self.is_typed_dict_field(metadata.as_ref(), field_name); let bases_to_check: Box> = if bases.is_empty() { // If the class doesn't have any base type, we should just use `object` as base to ensure @@ -1935,6 +2029,21 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { // keys but not regular fields. continue; } + if is_typed_dict_field + && !self.validate_typed_dict_field_override( + cls, + metadata.as_ref(), + parent_cls, + parent_metadata.as_ref(), + field_name, + class_field, + &want_class_field, + range, + errors, + ) + { + continue; + } // Substitute `Self` with derived class to support contravariant occurrences of `Self` let want_attribute = self.as_instance_attribute( &want_class_field, @@ -2005,38 +2114,56 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { /// For classes with multiple inheritance, check that fields inherited from multiple base classes are consistent. pub fn check_consistent_multiple_inheritance(&self, cls: &Class, errors: &ErrorCollector) { - // Maps field from inherited class + struct InheritedFieldInfo { + class: Class, + metadata: Arc, + field: ClassField, + ty: Type, + } + let mro = self.get_mro_for_class(cls); - let mut inherited_fields: SmallMap<&Name, Vec<(&Name, Type)>> = SmallMap::new(); + let mut inherited_fields: SmallMap<&Name, Vec> = SmallMap::new(); let current_class_fields: SmallSet<_> = cls.fields().collect(); for parent_cls in mro.ancestors_no_object().iter() { - let class_fields = parent_cls.class_object().fields(); + let class_object = parent_cls.class_object(); + let class_fields = class_object.fields(); + let parent_metadata = self.get_metadata_for_class(class_object); for field in class_fields { - // Skip fields that are not relevant for override consistency checks if !self.should_check_field_for_override_consistency(field) { continue; } - let key = KeyClassField(parent_cls.class_object().index(), field.clone()); + if current_class_fields.contains(field) { + continue; + } + let key = KeyClassField(class_object.index(), field.clone()); let field_entry = self.get_from_class(cls, &key); - if let Some(field_entry) = field_entry.as_ref() { - // Skip fields that are overridden in the current class, - // they will have been checked by - // `check_consistent_override_for_field` already - if current_class_fields.contains(field) { - continue; - } + let Some(field_entry) = field_entry.as_ref() else { + continue; + }; + if let Some(parent_member) = self.get_class_member(class_object, field) { + let parent_field = Arc::unwrap_or_clone(parent_member.value.clone()); inherited_fields .entry(field) .or_default() - .push((parent_cls.name(), field_entry.ty())); + .push(InheritedFieldInfo { + class: class_object.dupe(), + metadata: parent_metadata.clone(), + field: parent_field, + ty: field_entry.ty(), + }); } } } - for (field_name, class_and_types) in inherited_fields.iter() { - if class_and_types.len() > 1 { - let types: Vec = class_and_types.iter().map(|(_, ty)| ty.clone()).collect(); + let child_metadata = self.get_metadata_for_class(cls); + + for (field_name, inherited_field_infos_by_ancestor) in inherited_fields.iter() { + if inherited_field_infos_by_ancestor.len() > 1 { + let types: Vec = inherited_field_infos_by_ancestor + .iter() + .map(|info| info.ty.clone()) + .collect(); if types.iter().any(|ty| { matches!( ty, @@ -2046,8 +2173,6 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { | Type::Overload(_) ) }) { - // TODO(fangyizhou): Handle bound methods and functions properly. - // This is a leftover from https://github.com/facebook/pyrefly/pull/1196 continue; } let intersect = self.intersects(&types); @@ -2058,11 +2183,11 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { ), "Inherited types include:".to_owned() ]; - for (cls, ty) in class_and_types.iter() { + for info in inherited_field_infos_by_ancestor.iter() { error_msg.push(format!( " `{}` from `{}`", - self.for_display(ty.clone()), - cls + self.for_display(info.ty.clone()), + info.class.name() )); } errors.add( @@ -2071,6 +2196,38 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { error_msg, ); } + + if let Some(child_member) = self.get_class_member(cls, field_name) { + let child_field = Arc::unwrap_or_clone(child_member.value.clone()); + if self + .typed_dict_field_info(child_metadata.as_ref(), field_name, &child_field) + .is_some() + { + for info in inherited_field_infos_by_ancestor { + if self + .typed_dict_field_info( + info.metadata.as_ref(), + field_name, + &info.field, + ) + .is_some() + && !self.validate_typed_dict_field_override( + cls, + child_metadata.as_ref(), + &info.class, + info.metadata.as_ref(), + field_name, + &child_field, + &info.field, + cls.range(), + errors, + ) + { + continue; + } + } + } + } } } } diff --git a/pyrefly/lib/test/typed_dict.rs b/pyrefly/lib/test/typed_dict.rs index d3ff5cc3f..d098c3bd9 100644 --- a/pyrefly/lib/test/typed_dict.rs +++ b/pyrefly/lib/test/typed_dict.rs @@ -173,6 +173,31 @@ def test(a: A) -> None: "#, ); +testcase!( + test_typed_dict_inheritance_field_qualifiers, + r#" +from typing import NotRequired, ReadOnly, Required, TypedDict + +class ParentRequired(TypedDict): + x: int + +class ChildOptional(ParentRequired): + x: NotRequired[int] # E: TypedDict field `x` in `ChildOptional` must remain required because parent TypedDict `ParentRequired` defines it as required + +class ParentOptional(TypedDict, total=False): + x: int + +class ChildRequired(ParentOptional): + x: Required[int] # E: TypedDict field `x` in `ChildRequired` cannot be made required; parent TypedDict `ParentOptional` defines it as non-required + +class ParentMutable(TypedDict): + x: int + +class ChildReadOnly(ParentMutable): + x: ReadOnly[int] # E: TypedDict field `x` in `ChildReadOnly` cannot be marked read-only; parent TypedDict `ParentMutable` defines it as mutable +"#, +); + testcase!( test_typed_dict_contextual, r#"