fix Feature: inheritance checks for typed dictionaries (#1346)

Summary:
part of #52

Pull Request resolved: #1346

Reviewed By: yangdanny97

Differential Revision: D85366496

Pulled By: stroxler

fbshipit-source-id: 06604ecdb576d0043b9c1454b52d265df7bd6e39

Author: asukaminato0721

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": [

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": [],

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"

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<TypedDictField> {
+        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<dyn Iterator<Item = &ClassType>> = 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<ClassMetadata>,
+            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<InheritedFieldInfo>> = 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<Type> = 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<Type> = 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;
+                            }
+                        }
+                    }
+                }
             }
         }
     }

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#"