[ddc] Refactor visitDynamicInvocation()

Create a new helper `_emitArgumentGroups()` used for the calling
conventions where arguments are not passed in a flattened list.

Towards reducing the variety of code that flows through
`_emitMethodCall()` so hot reload soundness checks can be added when
they are appropriate.

Change-Id: I50ef9c4d54d71408d71b76fc2ca0acb73ef2af08
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/422367
Reviewed-by: Mark Zhou <[email protected]>
Reviewed-by: Nate Biggs <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>

Author: nshahan

pkg/dev_compiler/lib/src/kernel/compiler.dart

@@ -48,6 +48,15 @@ import 'type_environment.dart';
 import 'type_recipe_generator.dart';
 import 'type_table.dart';
 
+/// The groups of compiled invocation arguments ready to be flattened into a
+/// single [js_ast.Array] for statically sound calls or into separate packets
+/// for runtime checked calls.
+typedef _ArgumentGroups = ({
+  List<js_ast.Expression>? typeArguments,
+  List<js_ast.Expression>? positionalArguments,
+  List<js_ast.Property>? namedArguments
+});
+
 /// Public API for DDC compilers.
 abstract class Compiler {
   js_ast.Program emitModule(Component component);
@@ -5263,22 +5272,23 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
 
   @override
   js_ast.Expression visitDynamicInvocation(DynamicInvocation node) {
-    return _emitMethodCall(node.receiver, null, node.arguments, node);
+    return _emitDynamicInvocation(
+        node.receiver, node.name.text, node.arguments);
   }
 
   @override
   js_ast.Expression visitFunctionInvocation(FunctionInvocation node) {
-    assert(node.name.text == 'call');
-    var function = _visitExpression(node.receiver);
-    var arguments = _emitArgumentList(node.arguments);
+    var name = node.name.text;
+    assert(name == 'call');
     if (node.functionType == null) {
       // A `null` here implies the receiver is typed as `Function`. There isn't
       // any more type information available at compile time to know this
       // invocation is sound so a dynamic call will handle the checks at
       // runtime.
-      return _emitDynamicInvoke(function, null, arguments, node.arguments);
+      return _emitDynamicInvocation(node.receiver, name, node.arguments);
     }
-    return js_ast.Call(function, arguments);
+    return js_ast.Call(
+        _visitExpression(node.receiver), _emitArgumentList(node.arguments));
   }
 
   @override
@@ -5320,7 +5330,7 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
         negated: false);
   }
 
-  js_ast.Expression _emitMethodCall(Expression receiver, Member? target,
+  js_ast.Expression _emitMethodCall(Expression receiver, Member target,
       Arguments arguments, InvocationExpression node) {
     var name = node.name.text;
 
@@ -5380,22 +5390,22 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
       return js.call('#.push(#)', [jsReceiver, args]);
     }
 
-    var isCallingDynamicField = target is Member &&
-        target.hasGetter &&
+    var isCallingDynamicField = target.hasGetter &&
         // Erasing extension types here doesn't make sense. If there is an
         // extension type on dynamic or Function it will only be callable if it
         // defines a call method which would be invoked statically.
         _isDynamicOrFunction(target.getterType);
     if (name == 'call') {
-      // Erasing the extension types here to support existing callable behaivor
+      // Erasing the extension types here to support existing callable behavior
       // on the old style JS interop types that are callable. This should be
       // safe as it is a compile time error to try to dynamically invoke a call
       // method that is inherited from an extension type.
       var receiverType =
           receiver.getStaticType(_staticTypeContext).extensionTypeErasure;
-      if (isCallingDynamicField || _isDynamicOrFunction(receiverType)) {
-        return _emitDynamicInvoke(jsReceiver, null, args, arguments);
-      } else if (_isDirectCallable(receiverType)) {
+      if (isCallingDynamicField) {
+        return _emitDynamicInvocation(receiver, name, arguments);
+      }
+      if (_isDirectCallable(receiverType)) {
         // Call methods on function types should be handled as function calls.
         return js_ast.Call(jsReceiver, args);
       }
@@ -5412,8 +5422,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
         return _runtimeCall('#(#, #)', [name, jsReceiver, args]);
       }
       // Otherwise generate this as a normal typed method call.
-    } else if (target == null || isCallingDynamicField) {
-      return _emitDynamicInvoke(jsReceiver, jsName, args, arguments);
+    } else if (isCallingDynamicField) {
+      return _emitDynamicInvocation(receiver, name, arguments);
     }
     // TODO(jmesserly): remove when Kernel desugars this for us.
     // Handle `o.m(a)` where `o.m` is a getter returning a class with `call`.
@@ -5428,12 +5438,12 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
       // }
       // ```
       //
-      // We can always erase eagerly becuase:
-      //  - Extension types that do not implment an interface that exposes a
+      // We can always erase eagerly because:
+      //  - Extension types that do not implement an interface that exposes a
       //    `call` method will result in a static error at the call site.
       //  - Calls to extension types that implement their own call method are
       //    lowered by the CFE to top level static method calls.
-      var fromType = target!.getterType.extensionTypeErasure;
+      var fromType = target.getterType.extensionTypeErasure;
       if (fromType is InterfaceType) {
         var callName = _implicitCallTarget(fromType);
         if (callName != null) {
@@ -5444,41 +5454,42 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     return js.call('#.#(#)', [jsReceiver, jsName, args]);
   }
 
-  js_ast.Expression _emitDynamicInvoke(
-      js_ast.Expression fn,
-      js_ast.Expression? methodName,
-      Iterable<js_ast.Expression> args,
-      Arguments arguments) {
-    var jsArgs = <Object>[fn];
+  /// Returns an invocation of the runtime helpers `dcall`, `dgcall`, `dsend`,
+  /// or `dgsend` to perform dynamic checks before invoking [memberName] on
+  /// [receiver] and passing [arguments].
+  js_ast.Expression _emitDynamicInvocation(
+      Expression receiver, String memberName, Arguments arguments) {
+    var jsArgs = [_visitExpression(receiver)];
     String jsCode;
-
-    var typeArgs = arguments.types;
-    if (typeArgs.isNotEmpty) {
-      jsArgs.add(args.take(typeArgs.length));
-      args = args.skip(typeArgs.length);
-      if (methodName != null) {
-        jsCode = 'dgsend$_replSuffix(#, [#], #';
-        jsArgs.add(methodName);
+    var (:typeArguments, :positionalArguments, :namedArguments) =
+        _emitArgumentGroups(arguments, isJSInterop: false);
+    if (memberName == 'call') {
+      if (typeArguments != null) {
+        jsCode = 'dgcall(#, #';
+        jsArgs.add(js_ast.ArrayInitializer(typeArguments));
       } else {
-        jsCode = 'dgcall(#, [#]';
+        jsCode = 'dcall(#';
       }
-    } else if (methodName != null) {
-      jsCode = 'dsend$_replSuffix(#, #';
-      jsArgs.add(methodName);
     } else {
-      jsCode = 'dcall(#';
+      if (typeArguments != null) {
+        jsCode = 'dgsend$_replSuffix(#, #, #';
+        jsArgs.add(js_ast.ArrayInitializer(typeArguments));
+      } else {
+        jsCode = 'dsend$_replSuffix(#, #';
+      }
+      jsArgs.add(_emitMemberName(memberName));
     }
-
-    var hasNamed = arguments.named.isNotEmpty;
-    if (hasNamed) {
-      jsCode += ', [#], #)';
-      jsArgs.add(args.take(args.length - 1));
-      jsArgs.add(args.last);
+    if (positionalArguments != null) {
+      jsCode += ', #';
+      jsArgs.add(js_ast.ArrayInitializer(positionalArguments));
     } else {
-      jsArgs.add(args);
-      jsCode += ', [#])';
+      jsCode += ', []';
     }
-
+    if (namedArguments != null) {
+      jsCode += ', #';
+      jsArgs.add(js_ast.ObjectInitializer(namedArguments));
+    }
+    jsCode += ')';
     return _runtimeCall(jsCode, jsArgs);
   }
 
@@ -6384,29 +6395,65 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     return _emitTopLevelName(target);
   }
 
+  /// Returns all parts of [arguments] flattened into a list so they can be
+  /// passed in the calling convention for calls with no runtime checks.
+  ///
+  /// When [types] is `false` any type arguments present in [arguments] will be
+  /// omitted.
+  ///
+  /// Passing [target] when applicable allows for detection of an annotation to
+  /// omit passing type parameters and to detect if positional arguments should
+  /// be packaged to be passed to a JavaScript using an interop call.
   List<js_ast.Expression> _emitArgumentList(Arguments node,
       {bool types = true, Member? target}) {
     types = types && _reifyGenericFunction(target);
-    final isJsInterop = target != null && isJsMember(target);
+    var (:typeArguments, :positionalArguments, :namedArguments) =
+        _emitArgumentGroups(node,
+            isJSInterop: target != null && isJsMember(target));
     return [
-      if (types)
-        for (var typeArg in node.types) _emitType(typeArg),
-      for (var arg in node.positional)
-        if (arg is StaticInvocation &&
-            isJSSpreadInvocation(arg.target) &&
-            arg.arguments.positional.length == 1)
-          js_ast.Spread(_visitExpression(arg.arguments.positional[0]))
-        else if (isJsInterop)
-          _visitExpression(_assertInterop(arg))
-        else
-          _visitExpression(arg),
-      if (node.named.isNotEmpty)
-        js_ast.ObjectInitializer([
-          for (var arg in node.named) _emitNamedExpression(arg, isJsInterop)
-        ]),
+      if (types && typeArguments != null) ...typeArguments,
+      if (positionalArguments != null) ...positionalArguments,
+      if (namedArguments != null) js_ast.ObjectInitializer([...namedArguments]),
     ];
   }
 
+  /// Returns all [arguments] but kept in separate packets so they can be
+  /// further processed.
+  ///
+  /// Facilitates passing arguments in the calling convention used by runtime
+  /// helpers that check arguments.
+  ///
+  /// When [isJSInterop] is `true` the positional arguments are packaged to
+  /// be passed to JavaScript via an interop call.
+  _ArgumentGroups _emitArgumentGroups(Arguments node,
+      {required bool isJSInterop}) {
+    var typeArguments = node.types.isEmpty
+        ? null
+        : [for (var typeArgument in node.types) _emitType(typeArgument)];
+    var positionalArguments = node.positional.isEmpty
+        ? null
+        : [
+            for (var arg in node.positional)
+              if (arg is StaticInvocation &&
+                  isJSSpreadInvocation(arg.target) &&
+                  arg.arguments.positional.length == 1)
+                js_ast.Spread(_visitExpression(arg.arguments.positional[0]))
+              else if (isJSInterop)
+                _visitExpression(_assertInterop(arg))
+              else
+                _visitExpression(arg),
+          ];
+    var namedArguments = node.named.isEmpty
+        ? null
+        : [for (var arg in node.named) _emitNamedExpression(arg, isJSInterop)];
+
+    return (
+      typeArguments: typeArguments,
+      positionalArguments: positionalArguments,
+      namedArguments: namedArguments
+    );
+  }
+
   js_ast.Property _emitNamedExpression(NamedExpression arg,
       [bool isJsInterop = false]) {
     var value = isJsInterop ? _assertInterop(arg.value) : arg.value;