Drop the dead incomplete_attributes computation in pyrefly coverage

Author: jorenham

pyrefly/lib/commands/coverage/collect.rs

@@ -1009,24 +1009,6 @@ fn return_annotation_range(bindings: &Bindings, return_idx: Idx<Key>) -> Option<
     }
 }
 
-/// Only the first parameter (`self`/`cls`) is allowed to be unannotated.
-fn is_function_completely_annotated(
-    bindings: &Bindings,
-    answers: &Answers,
-    undecorated_idx: Idx<KeyUndecoratedFunction>,
-) -> bool {
-    let fun = bindings.get(undecorated_idx);
-    let return_idx = bindings.key_to_idx(&Key::ReturnType(ShortIdentifier::new(&fun.def.name)));
-    if return_annotation_range(bindings, return_idx).is_none() {
-        return false;
-    }
-
-    let implicit_receiver = has_implicit_receiver(fun, answers, undecorated_idx);
-    params_with_keys(&fun.def.parameters, implicit_receiver)
-        .iter()
-        .all(|(key, param)| key.is_none() || param.annotation.is_some())
-}
-
 /// Only a bare `Any` counts as unknown; container types like `list[Any]` are known.
 fn is_type_known(ty: &Type) -> bool {
     !ty.is_any()
@@ -1224,36 +1206,16 @@ fn parse_classes(
     module: &Module,
     bindings: &Bindings,
     answers: &Answers,
-    transaction: &Transaction,
-    handle: &Handle,
     tco_classes: &SmallSet<Idx<KeyClass>>,
 ) -> Vec<ReportClass> {
     let mut classes = Vec::new();
 
-    // group method definitions by class
-    let mut methods_by_class: HashMap<Idx<KeyClass>, Vec<Idx<KeyUndecoratedFunction>>> =
-        HashMap::new();
-    for idx in bindings.keys::<Key>() {
-        if let Key::Definition(_) = bindings.idx_to_key(idx)
-            && let Binding::Function(x, _pred, _class_meta) = bindings.get(idx)
-        {
-            let undecorated_idx = bindings.get(*x).undecorated_idx;
-            if let Some(class_key) = bindings.get(undecorated_idx).class_key {
-                methods_by_class
-                    .entry(class_key)
-                    .or_default()
-                    .push(undecorated_idx);
-            }
-        }
-    }
-
     for class_idx in bindings.keys::<KeyClass>() {
         // Skip @type_check_only classes.
         if tco_classes.contains(&class_idx) {
             continue;
         }
-        let binding_class = bindings.get(class_idx);
-        let cls_binding = match binding_class {
+        let cls_binding = match bindings.get(class_idx) {
             BindingClass::ClassDef(cls) => cls,
             BindingClass::FunctionalClassDef(..) => continue,
         };
@@ -1266,65 +1228,13 @@ fn parse_classes(
         if is_deleted_class(module, bindings, cls_binding) {
             continue;
         }
-        let class_type = match answers.get_idx(class_idx) {
-            Some(result) => match &result.0 {
-                Some(cls) => cls.clone(),
-                None => continue,
-            },
-            None => continue,
-        };
-        let class_name = class_fqn(module, parent, name);
-        let mro = class_mro(bindings, answers, &class_type);
-        // Check methods defined directly on this class
-        let mut incomplete_attributes = Vec::new();
-        for &undecorated_idx in methods_by_class.get(&class_idx).into_iter().flatten() {
-            if !is_function_completely_annotated(bindings, answers, undecorated_idx) {
-                incomplete_attributes.push(IncompleteAttribute {
-                    name: bindings.get(undecorated_idx).def.name.to_string(),
-                    declared_in: class_name.clone(),
-                });
-            }
-        }
-        // Check inherited methods
-        for ancestor_class_type in mro.ancestors_no_object() {
-            let ancestor_class = ancestor_class_type.class_object();
-            // Skip methods inherited from builtins
-            if ancestor_class.module_name().as_str() == "builtins" {
-                continue;
-            }
-            let ancestor_name = class_fqn(
-                ancestor_class.module(),
-                ancestor_class.qname().parent(),
-                ancestor_class.name(),
-            );
-            let Some(ancestor_class_fields) = transaction.get_class_fields(handle, ancestor_class)
-            else {
-                continue;
-            };
-            for field_name in ancestor_class_fields.names() {
-                let field_name_str = field_name.to_string();
-                // Skip if we already have this attribute listed (it has been overridden
-                // by the current class or another class in the MRO)
-                if incomplete_attributes
-                    .iter()
-                    .any(|a| a.name == field_name_str)
-                {
-                    continue;
-                }
-                if !ancestor_class_fields.is_field_annotated(field_name) {
-                    incomplete_attributes.push(IncompleteAttribute {
-                        name: field_name_str,
-                        declared_in: ancestor_name.clone(),
-                    });
-                }
-            }
+        // Skip classes that fail to resolve.
+        if answers.get_idx(class_idx).is_none_or(|c| c.0.is_none()) {
+            continue;
         }
-        let location = range_to_location(module, cls_binding.def.range);
-        incomplete_attributes.sort();
         classes.push(ReportClass {
-            name: class_name,
-            incomplete_attributes,
-            location,
+            name: class_fqn(module, parent, name),
+            location: range_to_location(module, cls_binding.def.range),
         });
     }
     classes.sort();
@@ -1442,14 +1352,7 @@ impl ModuleSymbols {
             &tco_classes,
         );
         merge_overloads(&mut functions);
-        let classes = parse_classes(
-            &module,
-            &bindings,
-            &answers,
-            transaction,
-            handle,
-            &tco_classes,
-        );
+        let classes = parse_classes(&module, &bindings, &answers, &tco_classes);
         let mut variables = parse_variables(
             &module,
             &bindings,
@@ -3127,20 +3030,19 @@ def g(x: int) -> int:
         (env, base, derived)
     }
 
-    /// Collect `derived` from a run that retains all bindings/answers, as ground truth.
-    fn base_derived_reference() -> ModuleSymbols {
+    /// Collect `derived`'s class members from a run that retains all bindings/answers,
+    /// as ground truth. Inherited members are looked up in the base module via
+    /// `get_class_fields`, so they exercise the cross-module path.
+    fn base_derived_reference() -> SmallSet<String> {
         let (env, _, derived) = base_derived_env();
         let (state, _) = env.to_state();
-        let symbols = ModuleSymbols::collect(&state.transaction(), &derived, false).unwrap();
+        let symbols = ModuleSymbols::collect(&state.transaction(), &derived, true).unwrap();
+        let members = symbols.stub_merge.unwrap().class_members;
         assert!(
-            symbols
-                .classes
-                .iter()
-                .any(|c| !c.incomplete_attributes.is_empty()),
-            "Derived should report an inherited incomplete attribute from Base: {:?}",
-            symbols.classes
+            members.contains("derived.Derived.x"),
+            "Derived should report the member `x` inherited from Base: {members:?}"
         );
-        symbols
+        members
     }
 
     /// Inherited members survive base-bindings eviction via retained `Solutions` metadata.
@@ -3159,11 +3061,11 @@ def g(x: int) -> int:
         );
 
         transaction.run(&[derived.dupe()], Require::Everything, None);
-        let symbols = ModuleSymbols::collect(&transaction, &derived, false).unwrap();
+        let symbols = ModuleSymbols::collect(&transaction, &derived, true).unwrap();
         assert_eq!(
-            symbols.classes,
-            base_derived_reference().classes,
-            "evicting the base module's bindings must not change the derived class report"
+            symbols.stub_merge.unwrap().class_members,
+            base_derived_reference(),
+            "evicting the base module's bindings must not change the derived class members"
         );
     }
 
@@ -3176,7 +3078,7 @@ def g(x: int) -> int:
         let mut transaction = state.new_transaction(Require::Exports, None);
         transaction.set_memory(env.get_memory());
         transaction.set_solutions_hook(Some(Box::new(|handle, transaction| {
-            if let Some(symbols) = ModuleSymbols::collect(transaction, handle, false) {
+            if let Some(symbols) = ModuleSymbols::collect(transaction, handle, true) {
                 collected.lock().insert(handle.dupe(), symbols);
             }
         })));
@@ -3189,9 +3091,9 @@ def g(x: int) -> int:
 
         let symbols = collected.lock().remove(&derived).unwrap();
         assert_eq!(
-            symbols.classes,
-            base_derived_reference().classes,
-            "hook-collected symbols must match symbols collected with bindings retained"
+            symbols.stub_merge.unwrap().class_members,
+            base_derived_reference(),
+            "hook-collected members must match members collected with bindings retained"
         );
     }
 }

pyrefly/lib/commands/coverage/types.rs

@@ -189,16 +189,9 @@ pub struct Function {
     pub range: TextRange,
 }
 
-#[derive(Debug, Serialize, PartialEq, Eq, PartialOrd, Ord)]
-pub struct IncompleteAttribute {
-    pub name: String,
-    pub declared_in: String,
-}
-
 #[derive(Debug, Serialize, PartialEq, Eq, PartialOrd, Ord)]
 pub struct ReportClass {
     pub name: String,
-    pub incomplete_attributes: Vec<IncompleteAttribute>,
     pub location: Location,
 }