[dart2wasm] Fix dynamic modules not generating code for dynamic module callable members if they are inherited by a concrete class.

Prior to this we were using the dispatch table to get the class IDs in order to register the callable references via `recordClassTargetUse`. However, the dispatch table only contains concrete classes.

A callable member may exist on an abstract class with no concrete subclasses. In this case the dispatch table would not contain any classes and the code for the member would never get generated. This is problematic when the abstract class (or a subclass) is dynamic module extendable. Then the callable member could be inherited onto a subclass in the dynamic module.

Also makes sure to mark members that are callable AND overrideable as invoked so that the updateable dispatch mechanism registers the member.

Change-Id: Icc079f76ab300aa761a47c8fbe24b8d9583d0da7
Fixes: #60602
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/423260
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Nate Biggs <[email protected]>

Author: natebiggs

pkg/dart2wasm/lib/dynamic_module_kernel_metadata.dart

@@ -221,7 +221,7 @@ class MainModuleMetadata {
   late final DispatchTable dispatchTable;
 
   /// Contains each invoked reference that targets an updateable function.
-  final Set<Reference> invokedReferences;
+  final Set<Reference> invokedOverrideableReferences;
 
   /// Maps invocation keys (either selector or builtin) to the implementation's
   /// index in the runtime table. Key includes whether the key was invoked
@@ -240,7 +240,7 @@ class MainModuleMetadata {
       this.classMetadata,
       this.callableReferenceIds,
       this.dispatchTable,
-      this.invokedReferences,
+      this.invokedOverrideableReferences,
       this.keyInvocationToIndex,
       this.dfsOrderClassIds,
       this.mainModuleTranslatorOptions,
@@ -250,7 +250,7 @@ class MainModuleMetadata {
       this.mainModuleTranslatorOptions, this.mainModuleEnvironment)
       : classMetadata = {},
         callableReferenceIds = {},
-        invokedReferences = {},
+        invokedOverrideableReferences = {},
         keyInvocationToIndex = {},
         dfsOrderClassIds = [];
 
@@ -299,7 +299,7 @@ class MainModuleMetadata {
 
     dispatchTable.serialize(sink);
 
-    sink.writeList(invokedReferences, sink.writeReference);
+    sink.writeList(invokedOverrideableReferences, sink.writeReference);
 
     sink.writeMap(keyInvocationToIndex, sink.writeInt, sink.writeInt);
 

pkg/dart2wasm/lib/dynamic_modules.dart

@@ -430,93 +430,114 @@ class DynamicModuleInfo {
     b.global_set(moduleIdGlobal);
   }
 
-  void _initializeCallableReferences() {
-    void collectCallableReference(Reference reference) {
-      final member = reference.asMember;
+  bool _isClassDynamicModuleInstantiable(Class cls) {
+    return cls.isDynamicModuleExtendable(translator.coreTypes) ||
+        cls.constructors
+            .any((e) => e.isDynamicModuleCallable(translator.coreTypes)) ||
+        cls.procedures.any((e) =>
+            e.isFactory && e.isDynamicModuleCallable(translator.coreTypes));
+  }
 
-      if (member.isExternal) {
-        final isGeneratedIntrinsic = member is Procedure &&
-            MemberIntrinsic.fromProcedure(translator.coreTypes, member) != null;
-        if (!isGeneratedIntrinsic) return;
+  void _initializeCallableReferences() {
+    for (final lib in translator.component.libraries) {
+      for (final member in lib.members) {
+        if (!member.isDynamicModuleCallable(translator.coreTypes)) continue;
+        _forEachMemberReference(member, _registerStaticCallableTarget);
       }
+    }
 
-      if (!member.isInstanceMember) {
-        // Generate static members immediately since they are unconditionally
-        // callable.
-        metadata.callableReferenceIds[reference] ??=
-            metadata.callableReferenceIds.length;
-        translator.functions.getFunction(reference);
-        return;
-      }
+    for (final classInfo in translator.classesSupersFirst) {
+      final cls = classInfo.cls;
+      if (cls == null) continue;
 
-      final selector = translator.dispatchTable.selectorForTarget(reference);
-      final targetRanges = selector
-          .targets(unchecked: false)
-          .targetRanges
-          .followedBy(selector.targets(unchecked: true).targetRanges);
-      // Record usage of the target on all classes that inherit the member. The
-      // class must be allocated in order for the target to be live.
-      for (final (:range, :target) in targetRanges) {
-        // Match any reference for the callable member, even if the reference
-        // kind is not the same.
-        if (target.asMember != member) continue;
-        metadata.callableReferenceIds[target] ??=
-            metadata.callableReferenceIds.length;
-        for (int classId = range.start; classId <= range.end; ++classId) {
-          translator.functions.recordClassTargetUse(classId, target);
+      // Register any callable functions defined within this class.
+      for (final member in cls.members) {
+        if (!member.isDynamicModuleCallable(translator.coreTypes)) continue;
+
+        if (!member.isInstanceMember) {
+          // Generate static members immediately since they are unconditionally
+          // callable.
+          _forEachMemberReference(member, _registerStaticCallableTarget);
+          continue;
         }
-      }
-    }
 
-    void collectCallableReferences(Member member) {
-      if (member is Procedure) {
-        collectCallableReference(member.reference);
-        // We ignore the tear-off and let each dynamic module generate it for
-        // itself.
-      } else if (member is Field) {
-        collectCallableReference(member.getterReference);
-        if (member.hasSetter) {
-          collectCallableReference(member.setterReference!);
+        // Consider callable references invoked and therefore if they're
+        // overrideable include them in the runtime dispatch table.
+        if (member.isDynamicModuleOverrideable(translator.coreTypes)) {
+          _forEachMemberReference(
+              member, metadata.invokedOverrideableReferences.add);
         }
-      } else if (member is Constructor &&
-          // Skip types that don't extend Object in the wasm type hierarchy.
-          // These types do not have directly invokable constructors.
-          translator.classInfo[member.enclosingClass]!.struct
-              .isSubtypeOf(translator.objectInfo.struct)) {
-        collectCallableReference(member.reference);
-        collectCallableReference(member.initializerReference);
-        collectCallableReference(member.constructorBodyReference);
       }
-    }
 
-    for (final lib in translator.component.libraries) {
-      for (final member in lib.members) {
-        if (member.isDynamicModuleCallable(translator.coreTypes)) {
-          collectCallableReferences(member);
-        }
+      // Anonymous mixins' targets don't need to be registered since they aren't
+      // directly allocatable.
+      if (cls.isAnonymousMixin) continue;
+
+      if (cls.isAbstract && !_isClassDynamicModuleInstantiable(cls)) {
+        continue;
       }
 
-      for (final cls in lib.classes) {
-        for (final member in cls.members) {
-          if (member.isDynamicModuleCallable(translator.coreTypes)) {
-            collectCallableReferences(member);
-          }
-        }
+      // For each dispatch target, register the member as callable from this
+      // class.
+      final targets = translator.hierarchy.getDispatchTargets(cls).followedBy(
+          translator.hierarchy.getDispatchTargets(cls, setters: true));
+      for (final member in targets) {
+        if (!member.isDynamicModuleCallable(translator.coreTypes)) continue;
+
+        _forEachMemberReference(member,
+            (reference) => _registerCallableDispatchTarget(reference, cls));
       }
     }
   }
 
+  /// If class [cls] is marked allocated then ensure we compile [target].
+  ///
+  /// The [cls] may be marked allocated in
+  /// [_initializeDynamicAllocatableClasses] which (together with this) will
+  /// enqueue the [target] for compilation. Otherwise the [cls] must be
+  /// allocated via a constructor call in the program itself.
+  void _registerCallableDispatchTarget(Reference target, Class cls) {
+    final member = target.asMember;
+
+    if (member.isExternal) {
+      final isGeneratedIntrinsic = member is Procedure &&
+          MemberIntrinsic.fromProcedure(translator.coreTypes, member) != null;
+      if (!isGeneratedIntrinsic) return;
+    }
+
+    final classId =
+        (translator.classInfo[cls]!.classId as AbsoluteClassId).value;
+
+    metadata.callableReferenceIds[target] ??=
+        metadata.callableReferenceIds.length;
+    // The class must be allocated in order for the target to be live.
+    translator.functions.recordClassTargetUse(classId, target);
+  }
+
+  void _registerStaticCallableTarget(Reference target) {
+    final member = target.asMember;
+
+    if (member.isExternal) {
+      final isGeneratedIntrinsic = member is Procedure &&
+          MemberIntrinsic.fromProcedure(translator.coreTypes, member) != null;
+      if (!isGeneratedIntrinsic) return;
+    }
+
+    // Generate static members immediately since they are unconditionally
+    // callable.
+    metadata.callableReferenceIds[target] ??=
+        metadata.callableReferenceIds.length;
+    translator.functions.getFunction(target);
+  }
+
   void _initializeDynamicAllocatableClasses() {
-    for (final lib in translator.component.libraries) {
-      for (final cls in lib.classes) {
-        if (cls.isAnonymousMixin) continue;
-
-        if (cls.isDynamicModuleExtendable(translator.coreTypes) ||
-            cls.constructors
-                .any((e) => e.isDynamicModuleCallable(translator.coreTypes))) {
-          translator.functions
-              .recordClassAllocation(translator.classInfo[cls]!.classId);
-        }
+    for (final classInfo in translator.classesSupersFirst) {
+      final cls = classInfo.cls;
+      if (cls == null) continue;
+      if (cls.isAnonymousMixin) continue;
+
+      if (_isClassDynamicModuleInstantiable(cls)) {
+        translator.functions.recordClassAllocation(classInfo.classId);
       }
     }
   }
@@ -529,11 +550,12 @@ class DynamicModuleInfo {
           name: '#r_${builtin.name}');
     }
 
-    for (final reference in metadata.invokedReferences) {
+    for (final reference in metadata.invokedOverrideableReferences) {
       final selector = translator.dispatchTable.selectorForTarget(reference);
       translator.functions.recordSelectorUse(selector, false);
 
-      final mainSelector = translator.dynamicMainModuleDispatchTable!
+      final mainSelector = (translator.dynamicMainModuleDispatchTable ??
+              translator.dispatchTable)
           .selectorForTarget(reference);
       final signature = _getGeneralizedSignature(mainSelector);
       final buildMain = buildSelectorBranch(reference, mainSelector);
@@ -547,6 +569,39 @@ class DynamicModuleInfo {
     }
   }
 
+  void _forEachMemberReference(Member member, void Function(Reference) f) {
+    void passReference(Reference reference) {
+      final checkedReference =
+          translator.getFunctionEntry(reference, uncheckedEntry: false);
+      f(checkedReference);
+
+      final uncheckedReference =
+          translator.getFunctionEntry(reference, uncheckedEntry: true);
+      if (uncheckedReference != checkedReference) {
+        f(uncheckedReference);
+      }
+    }
+
+    if (member is Procedure) {
+      passReference(member.reference);
+      // We ignore the tear-off and let each dynamic module generate it for
+      // itself.
+    } else if (member is Field) {
+      passReference(member.getterReference);
+      if (member.hasSetter) {
+        passReference(member.setterReference!);
+      }
+    } else if (member is Constructor &&
+        // Skip types that don't extend Object in the wasm type hierarchy.
+        // These types do not have directly invokable constructors.
+        translator.classInfo[member.enclosingClass]!.struct
+            .isSubtypeOf(translator.objectInfo.struct)) {
+      passReference(member.reference);
+      passReference(member.initializerReference);
+      passReference(member.constructorBodyReference);
+    }
+  }
+
   void finishDynamicModule() {
     _registerModuleRefs(
         isDynamicModule ? initFunction.body : translator.initFunction.body);
@@ -776,7 +831,7 @@ class DynamicModuleInfo {
   void callOverrideableDispatch(
       w.InstructionsBuilder b, SelectorInfo selector, Reference interfaceTarget,
       {required bool useUncheckedEntry}) {
-    metadata.invokedReferences.add(interfaceTarget);
+    metadata.invokedOverrideableReferences.add(interfaceTarget);
 
     final localSignature = selector.signature;
     // If any input is not a RefType (i.e. it's an unboxed value) then wrap it

pkg/dynamic_modules/test/data/override_extra_params/dynamic_interface.yaml

@@ -15,9 +15,15 @@ can-be-overridden:
   - library: 'shared/shared.dart'
     class: 'Base'
     member: 'method3'
+  - library: 'shared/shared.dart'
+    class: 'Base'
+    member: 'method4'
 
 # TODO(sigmund): consider implying this for all extendable types.
 callable:
   - library: 'shared/shared.dart'
     class: 'Base'
     member: ''
+  - library: 'shared/shared.dart'
+    class: 'Base'
+    member: 'method4'

pkg/dynamic_modules/test/data/override_extra_params/modules/entry1.dart

@@ -15,5 +15,17 @@ class Child extends Base {
   int method3<T>(int i, {int? j, T? a}) => i + (j ?? 0);
 }
 
+class OtherChild extends Base {
+  @override
+  int method4<T>(int i, {int? j, T? a}) => i + (j ?? 0);
+}
+
 @pragma('dyn-module:entry-point')
-Object? dynamicModuleEntrypoint() => Child();
+Object? dynamicModuleEntrypoint() {
+  final c = Child();
+  c.method1(2, 4);
+  c.method2(2, j: 5, a: 'hello');
+  c.method3(2, j: 5, a: null);
+  c.method4(2, j: 5);
+  return c;
+}

pkg/dynamic_modules/test/data/override_extra_params/shared/shared.dart

@@ -8,4 +8,6 @@ abstract class Base {
   int method2(int i, {int? j}) => i;
 
   int method3<T>(int i, {int? j}) => i;
+
+  int method4<T>(int i, {int? j}) => i;
 }