[ddc] Refactor visitInstanceGetterInvocation()

Moves the remaining special case logic out of `_emitMethodCall()`
simplifying the reasoning about where so hot reload soundness checks
can be added.

Change-Id: I13e2f451e61f6e067ea689bcc35039cf92946492
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/423603
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

@@ -5293,8 +5293,7 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
 
   @override
   js_ast.Expression visitInstanceInvocation(InstanceInvocation node) {
-    var invocation = _emitMethodCall(
-        node.receiver, node.interfaceTarget, node.arguments, node);
+    var invocation = _emitInstanceInvocation(node);
     return _isNullCheckableJsInterop(node.interfaceTarget)
         ? _wrapWithJsInteropNullCheck(invocation)
         : invocation;
@@ -5303,13 +5302,64 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
   @override
   js_ast.Expression visitInstanceGetterInvocation(
       InstanceGetterInvocation node) {
-    var getterInvocation = _emitMethodCall(
-        node.receiver, node.interfaceTarget, node.arguments, node);
+    if (node.functionType == null) {
+      // A `null` here implies the receiver must be typed as `dynamic` or
+      // `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 _emitDynamicInvocation(
+          node.receiver, node.name.text, node.arguments);
+    }
+    var getterInvocation = _emitInstanceGetterInvocation(node);
     return _isNullCheckableJsInterop(node.interfaceTarget)
         ? _wrapWithJsInteropNullCheck(getterInvocation)
         : getterInvocation;
   }
 
+  js_ast.Expression _emitInstanceGetterInvocation(
+      InstanceGetterInvocation node) {
+    var receiver = _visitExpression(node.receiver);
+    var arguments =
+        _emitArgumentList(node.arguments, target: node.interfaceTarget);
+    if (node.name.text == 'call') {
+      // 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 =
+          node.receiver.getStaticType(_staticTypeContext).extensionTypeErasure;
+      if (_isDirectCallable(receiverType)) {
+        // Call methods on function types should be handled as function calls.
+        return js_ast.Call(receiver, arguments);
+      }
+    }
+    var memberName =
+        _emitMemberName(node.name.text, member: node.interfaceTarget);
+    // We must erase the extension type to potentially find the `call` method.
+    // If the extension type has a runtime representation with a `call`:
+    //
+    // ```
+    // extension type Ext(C c) implements C {...}
+    // class C {
+    //   call() {...}
+    // }
+    // ```
+    //
+    // 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 erasedGetterType = node.interfaceTarget.getterType.extensionTypeErasure;
+    if (erasedGetterType is InterfaceType) {
+      var callName = _implicitCallTarget(erasedGetterType);
+      if (callName != null) {
+        return js.call('#.#.#(#)', [receiver, memberName, callName, arguments]);
+      }
+    }
+    return js.call('#.#(#)', [receiver, memberName, arguments]);
+  }
+
   @override
   js_ast.Expression visitLocalFunctionInvocation(LocalFunctionInvocation node) {
     assert(node.name.text == 'call');
@@ -5330,20 +5380,15 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
         negated: false);
   }
 
-  js_ast.Expression _emitMethodCall(Expression receiver, Member target,
-      Arguments arguments, InvocationExpression node) {
-    var name = node.name.text;
-
+  js_ast.Expression _emitInstanceInvocation(InstanceInvocation node) {
     /// Returns `true` when [node] represents an invocation of `List.add()` that
     /// can be optimized.
     ///
     /// The optimized add operation can skip checks for a growable or modifiable
     /// list and the element type is known to be invariant so it can skip the
     /// type check.
-    bool isNativeListInvariantAdd(InvocationExpression node) {
-      if (node is InstanceInvocation &&
-          node.isInvariant &&
-          node.name.text == 'add') {
+    bool isNativeListInvariantAdd(InstanceInvocation node) {
+      if (node.isInvariant && node.name.text == 'add') {
         // The call to add is marked as invariant, so the type check on the
         // parameter to add is not needed.
         var receiver = node.receiver;
@@ -5373,6 +5418,10 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
       return false;
     }
 
+    var name = node.name.text;
+    var receiver = node.receiver;
+    var arguments = node.arguments;
+    var target = node.interfaceTarget;
     if (isOperatorMethodName(name) && arguments.named.isEmpty) {
       var argLength = arguments.positional.length;
       if (argLength == 0) {
@@ -5382,75 +5431,33 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
             receiver, target, arguments.positional[0], node);
       }
     }
-
     var jsReceiver = _visitExpression(receiver);
     var args = _emitArgumentList(arguments, target: target);
-
     if (isNativeListInvariantAdd(node)) {
       return js.call('#.push(#)', [jsReceiver, args]);
     }
-
-    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 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) {
-        return _emitDynamicInvocation(receiver, name, arguments);
-      }
       if (_isDirectCallable(receiverType)) {
-        // Call methods on function types should be handled as function calls.
+        // Handle call methods on function types as function calls.
         return js_ast.Call(jsReceiver, args);
       }
     }
-
-    var jsName = _emitMemberName(name, member: target);
-
-    // Handle Object methods that are supported by `null` and potentially
-    // JavaScript interop values.
-    if (_isObjectMethodCall(name, arguments)) {
-      if (_shouldCallObjectMemberHelper(receiver)) {
-        // The names of the static helper methods in the runtime must match the
-        // names of the Object instance members.
-        return _runtimeCall('#(#, #)', [name, jsReceiver, args]);
-      }
-      // Otherwise generate this as a normal typed method call.
-    } 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`.
-    if (target is Field || target is Procedure && target.isAccessor) {
-      // We must erase the extension type to find the `call` method.
-      // If the extension type has a runtime representation with a `call`:
-      //
-      // ```
-      // extension type Ext(C c) implements C {...}
-      // class C {
-      //   call() {...}
-      // }
-      // ```
-      //
-      // 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;
-      if (fromType is InterfaceType) {
-        var callName = _implicitCallTarget(fromType);
-        if (callName != null) {
-          return js.call('#.#.#(#)', [jsReceiver, jsName, callName, args]);
-        }
-      }
+    if (_isObjectMethodCall(name, arguments) &&
+        _shouldCallObjectMemberHelper(receiver)) {
+      // Handle Object methods that are supported by `null` and possibly
+      // JavaScript interop values with static helper methods.
+      // The names of the static helper methods in the runtime must match the
+      // names of the Object instance members.
+      return _runtimeCall('#(#, #)', [name, jsReceiver, args]);
     }
+    // Otherwise generate this as a normal typed method call.
+    var jsName = _emitMemberName(name, member: target);
     return js.call('#.#(#)', [jsReceiver, jsName, args]);
   }
 
@@ -5505,11 +5512,6 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     return null;
   }
 
-  bool _isDynamicOrFunction(DartType t) =>
-      DartTypeEquivalence(_coreTypes, ignoreTopLevelNullability: true)
-          .areEqual(t, _coreTypes.functionNonNullableRawType) ||
-      t == const DynamicType();
-
   js_ast.Expression _emitUnaryOperator(
       Expression expr, Member? target, InvocationExpression node) {
     var op = node.name.text;