From 776c0586aa7c7272bfed897ec1de62a4d4ada7fe Mon Sep 17 00:00:00 2001 From: Nicholas Shahan Date: Mon, 28 Apr 2025 17:42:43 -0700 Subject: [PATCH] [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 Reviewed-by: Nate Biggs Commit-Queue: Nicholas Shahan --- pkg/dev_compiler/lib/src/kernel/compiler.dart | 140 +++++++++--------- .../lib/src/kernel/compiler_new.dart | 139 ++++++++--------- 2 files changed, 142 insertions(+), 137 deletions(-) diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index da5f843af18..6f3dd4762b4 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -5293,8 +5293,7 @@ class ProgramCompiler extends ComputeOnceConstantVisitor @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 @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 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 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,19 +5431,11 @@ class ProgramCompiler extends ComputeOnceConstantVisitor 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 @@ -5402,55 +5443,21 @@ class ProgramCompiler extends ComputeOnceConstantVisitor // 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); } } - + 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); - - // 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]); - } - } - } return js.call('#.#(#)', [jsReceiver, jsName, args]); } @@ -5505,11 +5512,6 @@ class ProgramCompiler extends ComputeOnceConstantVisitor 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; diff --git a/pkg/dev_compiler/lib/src/kernel/compiler_new.dart b/pkg/dev_compiler/lib/src/kernel/compiler_new.dart index ea56acda5d2..5740571ad79 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler_new.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler_new.dart @@ -5742,8 +5742,7 @@ class LibraryCompiler extends ComputeOnceConstantVisitor @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; @@ -5752,13 +5751,64 @@ class LibraryCompiler extends ComputeOnceConstantVisitor @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'); @@ -5779,20 +5829,15 @@ class LibraryCompiler extends ComputeOnceConstantVisitor 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; @@ -5822,6 +5867,10 @@ class LibraryCompiler extends ComputeOnceConstantVisitor 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) { @@ -5831,18 +5880,11 @@ class LibraryCompiler extends ComputeOnceConstantVisitor 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 @@ -5850,55 +5892,21 @@ class LibraryCompiler extends ComputeOnceConstantVisitor // 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); } } - + if (_isObjectMethodCall(name, arguments) && + _shouldCallObjectMemberHelper(receiver)) { + // Handle Object methods when the receiver could potentially be `null` or + // 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); - - // 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]); - } - } - } return js.call('#.#(#)', [jsReceiver, jsName, args]); } @@ -5953,11 +5961,6 @@ class LibraryCompiler extends ComputeOnceConstantVisitor 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;