From c5218f2d4e9d1c3b971e699d73a4df4eecaa4a69 Mon Sep 17 00:00:00 2001 From: Nicholas Shahan Date: Tue, 2 Apr 2024 19:57:25 +0000 Subject: [PATCH] [ddc] Refactor `visitFunctionTearOff` Stop passing `null` as the member parameter of `_emitPropertyGet`. Avoids the use of one method with multiple code paths and returns to handle any situation because it becomes very hard to reason about what original source code leads to each path. Issue: https://github.com/dart-lang/sdk/issues/54463 Change-Id: I1255817c7cf76578b256650789f6530b456b2f03 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357207 Reviewed-by: Srujan Gaddam Reviewed-by: Sigmund Cherem Reviewed-by: Bob Nystrom Commit-Queue: Nicholas Shahan --- pkg/dev_compiler/lib/src/kernel/compiler.dart | 39 ++++++++++++++----- .../lib/src/kernel/js_interop.dart | 31 +++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index 45b47bcf547e..327bef9b8380 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -4896,6 +4896,13 @@ class ProgramCompiler extends ComputeOnceConstantVisitor node.receiver, node.interfaceTarget, node.name.text); } + /// Returns `true` when [member] is a `.call` member (field, getter or method) + /// of a non-static JavaScript Interop class. + bool _isNonStaticJsInteropCallMember(Member? member) => + member != null && + member.name.text == 'call' && + isNonStaticJsInterop(member); + @override js_ast.Expression visitDynamicSet(DynamicSet node) { return _emitPropertySet(node.receiver, null, node.value, node.name.text); @@ -4958,16 +4965,15 @@ class ProgramCompiler extends ComputeOnceConstantVisitor // TODO(54463): Refactor and specialize this code for each type of 'get'. js_ast.Expression _emitPropertyGet( Expression receiver, Member? member, String memberName) { - // TODO(jmesserly): should tearoff of `.call` on a function type be - // encoded as a different node, or possibly eliminated? - // (Regardless, we'll still need to handle the callable JS interop classes.) - if (memberName == 'call' && - _isDirectCallable(receiver.getStaticType(_staticTypeContext))) { - // Tearoff of `call` on a function type is a no-op; - return _visitExpression(receiver); + var jsReceiver = _visitExpression(receiver); + if (_isNonStaticJsInteropCallMember(member)) { + // Historically DDC has treated this as a "callable class" and the + // tearoff of `.call` as a no-op. This is still needed here to preserve + // the existing behavior for the non-static JavaScript interop (including + // potentially failing cases). + return jsReceiver; } var jsName = _emitMemberName(memberName, member: member); - var jsReceiver = _visitExpression(receiver); // TODO(jmesserly): we need to mark an end span for property accessors so // they can be hovered. Unfortunately this is not possible as Kernel does @@ -7391,7 +7397,22 @@ class ProgramCompiler extends ComputeOnceConstantVisitor @override js_ast.Expression visitFunctionTearOff(FunctionTearOff node) { - return _emitPropertyGet(node.receiver, null, 'call'); + var receiver = node.receiver; + var receiverType = receiver.getStaticType(_staticTypeContext); + var jsReceiver = _visitExpression(receiver); + if (receiverType is InterfaceType && + receiverType.classNode == _coreTypes.functionClass) { + // Historically DDC has treated this case as a dynamic get and allowed it + // to evaluate at runtime. + // + // This is here to preserve the existing behavior for the non-static + // JavaScript interop (including some failing cases) but could potentially + // be cleaned up as a breaking change. + return runtimeCall( + 'dload$_replSuffix(#, #)', [jsReceiver, js.string('call')]); + } + // Otherwise, tearoff of `call` on a function type is a no-op. + return jsReceiver; } /// Creates header comments with helpful compilation information. diff --git a/pkg/dev_compiler/lib/src/kernel/js_interop.dart b/pkg/dev_compiler/lib/src/kernel/js_interop.dart index e6c13dacc061..5c4ef902be9a 100644 --- a/pkg/dev_compiler/lib/src/kernel/js_interop.dart +++ b/pkg/dev_compiler/lib/src/kernel/js_interop.dart @@ -2,6 +2,7 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'package:_js_interop_checks/src/js_interop.dart'; import 'package:kernel/kernel.dart'; import 'kernel_helpers.dart'; @@ -158,3 +159,33 @@ bool usesJSInterop(NamedNode n) { } return false; } + +/// Returns `true` when [member] is a JavaScript interop member using the +/// package:js style of interop. +/// +/// Will return `false` when the newer static interop style is used or when +/// [member] is not a JavaScript interop member at all. +bool isNonStaticJsInterop(Member member) { + return member.isExternal && + !member.isExtensionMember && + !member.isExtensionTypeMember && + _usesNonStaticInteropAnnotation(member); +} + +/// Returns `true` when [member] is a JavaScript interop member using the +/// package:js style of interop annotation. +/// +/// This does not mean [member] has the annotation directly because it can +/// be applied on the class or the library level. +/// +/// Will return `false` when the newer static interop style annotation is +/// found or when [member] is not a JavaScript interop member at all. +bool _usesNonStaticInteropAnnotation(Member member) { + var enclosingClass = member.enclosingClass; + if (enclosingClass != null) { + return hasPackageJSAnnotation(enclosingClass) && + !hasStaticInteropAnnotation(enclosingClass); + } + return hasPackageJSAnnotation(member) || + hasPackageJSAnnotation(member.enclosingLibrary); +}