Skip to content

Commit 1c534e5

Browse files
srujzsCommit Queue
authored and
Commit Queue
committed
[dart2wasm/ddc/dart2js] Lower Function.toJS and JSExportedDartFunction.toDart
Lowers these extension methods to some helper functions instead of allowInterop to improve performance and get consistent semantics. Specifically: - Function.toJS no longer gives you the same JSFunction when calling toJS on the same function multiple times in the JS compilers. - Adds fast calling syntax for functions with 0-5 args in the JS compilers. - Allows additional args to be passed to converted functions that are omitted in the JS compilers. - The static type now determines the number of args that can be passed to the JS function in the JS compilers. - Fixes an issue in dart2wasm where if too few arguments are passed, the call may succeed due to conversion of undefined to null. - Adds throws when a user tries to wrap a JS function that returned from Function.toJS in the JS compilers. Closes #55515 Addresses #48186 CoreLibraryReviewExempt: Changes to docs only in API surface. Change-Id: I41d864dc5e02b597d9f1c16c88e3c04872f28225 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/368065 Reviewed-by: Nicholas Shahan <[email protected]> Reviewed-by: Stephen Adams <[email protected]> Commit-Queue: Srujan Gaddam <[email protected]> Reviewed-by: Martin Kustermann <[email protected]>
1 parent 4b88698 commit 1c534e5

File tree

15 files changed

+1114
-109
lines changed

15 files changed

+1114
-109
lines changed

CHANGELOG.md

+8
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,19 @@
6464
`ExternalDartReferenceToObject` and `ObjectToExternalDartReference` are now
6565
extensions on `T` and `ExternalDartReference<T>`, respectively, where `T
6666
extends Object?`. See [#55342][] and [#55536][] for more details.
67+
- Fixes some consistency issues with `Function.toJS` across all compilers.
68+
Specifically, calling `Function.toJS` on the same function gives you a new JS
69+
function (see issue [#55515][]), the max number of args that are passed to the
70+
JS function is determined by the static type of the Dart function, and extra
71+
args are dropped when passed to the JS function in all compilers (see
72+
[#48186][]).
6773

6874
[#55508]: https://github.com/dart-lang/sdk/issues/55508
6975
[#55267]: https://github.com/dart-lang/sdk/issues/55267
7076
[#55342]: https://github.com/dart-lang/sdk/issues/55342
7177
[#55536]: https://github.com/dart-lang/sdk/issues/55536
78+
[#55515]: https://github.com/dart-lang/sdk/issues/55515
79+
[#48186]: https://github.com/dart-lang/sdk/issues/48186
7280

7381
### Tools
7482

pkg/_js_interop_checks/lib/src/transformations/js_util_optimizer.dart

+61
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,14 @@ class JsUtilOptimizer extends Transformer {
4141
final List<Procedure> _callConstructorUncheckedTargets;
4242
final CloneVisitorNotMembers _cloner = CloneVisitorWithMembers();
4343
final Map<Member, _InvocationBuilder?> _externalInvocationBuilders = {};
44+
final Procedure _functionToJSTarget;
45+
final List<Procedure> _functionToJSTargets;
46+
final Procedure _functionToJSNTarget;
4447
final Procedure _getPropertyTarget;
4548
final Procedure _getPropertyTrustTypeTarget;
4649
final Procedure _globalContextTarget;
50+
final Procedure _jsExportedDartFunctionToDartTarget;
51+
final Procedure _jsFunctionToDart;
4752
final InterfaceType _objectType;
4853
final Procedure _setPropertyTarget;
4954
final Procedure _setPropertyUncheckedTarget;
@@ -88,12 +93,25 @@ class JsUtilOptimizer extends Transformer {
8893
5,
8994
(i) => _coreTypes.index.getTopLevelProcedure(
9095
'dart:js_util', '_callConstructorUnchecked$i')),
96+
_functionToJSTarget = _coreTypes.index.getTopLevelProcedure(
97+
'dart:js_interop', 'FunctionToJSExportedDartFunction|get#toJS'),
98+
_functionToJSTargets = List<Procedure>.generate(
99+
6,
100+
(i) => _coreTypes.index
101+
.getTopLevelProcedure('dart:js_util', '_functionToJS$i')),
102+
_functionToJSNTarget = _coreTypes.index
103+
.getTopLevelProcedure('dart:js_util', '_functionToJSN'),
91104
_getPropertyTarget = _coreTypes.index
92105
.getTopLevelProcedure('dart:js_util', 'getProperty'),
93106
_getPropertyTrustTypeTarget = _coreTypes.index
94107
.getTopLevelProcedure('dart:js_util', '_getPropertyTrustType'),
95108
_globalContextTarget = _coreTypes.index.getTopLevelProcedure(
96109
'dart:_js_helper', 'get:staticInteropGlobalContext'),
110+
_jsExportedDartFunctionToDartTarget = _coreTypes.index
111+
.getTopLevelProcedure('dart:js_interop',
112+
'JSExportedDartFunctionToFunction|get#toDart'),
113+
_jsFunctionToDart = _coreTypes.index
114+
.getTopLevelProcedure('dart:js_util', '_jsFunctionToDart'),
97115
_objectType = hierarchy.coreTypes.objectNonNullableRawType,
98116
_setPropertyTarget = _coreTypes.index
99117
.getTopLevelProcedure('dart:js_util', 'setProperty'),
@@ -480,6 +498,10 @@ class JsUtilOptimizer extends Transformer {
480498
invocation = _lowerCallConstructor(node);
481499
// TODO(srujzs): Delete the `isPatchedMember` check once
482500
// https://github.com/dart-lang/sdk/issues/53367 is resolved.
501+
} else if (target == _functionToJSTarget) {
502+
invocation = _lowerFunctionToJS(node);
503+
} else if (target == _jsExportedDartFunctionToDartTarget) {
504+
invocation = _lowerJSExportedDartFunctionToDart(node);
483505
} else if (target.isExternal && !JsInteropChecks.isPatchedMember(target)) {
484506
final builder = _externalInvocationBuilders.putIfAbsent(
485507
target, () => _getExternalInvocationBuilder(target));
@@ -662,6 +684,45 @@ class JsUtilOptimizer extends Transformer {
662684
..parent = nodeParent;
663685
}
664686

687+
/// For the given `dart:js_interop` `Function.toJS` invocation [node], returns
688+
/// an invocation of `_functionToJSX` with the given `Function` argument,
689+
/// where X is the number of the positional arguments.
690+
///
691+
/// If the number of the positional arguments is larger than 5, returns an
692+
/// invocation of `_functionToJSN` instead.
693+
StaticInvocation _lowerFunctionToJS(StaticInvocation node) {
694+
// JS interop checks assert that the static type is available, and that
695+
// there are no named arguments or type arguments.
696+
final function = node.arguments.positional.single;
697+
final functionType =
698+
function.getStaticType(_staticTypeContext) as FunctionType;
699+
final argumentsLength = functionType.positionalParameters.length;
700+
Procedure target;
701+
Arguments arguments;
702+
if (argumentsLength < _functionToJSTargets.length) {
703+
target = _functionToJSTargets[argumentsLength];
704+
arguments = Arguments([function]);
705+
} else {
706+
target = _functionToJSNTarget;
707+
arguments = Arguments([function, IntLiteral(argumentsLength)]);
708+
}
709+
return StaticInvocation(
710+
target, arguments..fileOffset = node.arguments.fileOffset)
711+
..fileOffset = node.fileOffset
712+
..parent = node.parent;
713+
}
714+
715+
/// For the given `dart:js_interop` `JSExportedDartFunction.toDart` invocation
716+
/// [node], returns an invocation of `_jsFunctionToDart` with the given
717+
/// `JSExportedDartFunction` argument.
718+
StaticInvocation _lowerJSExportedDartFunctionToDart(StaticInvocation node) =>
719+
StaticInvocation(
720+
_jsFunctionToDart,
721+
Arguments([node.arguments.positional[0]])
722+
..fileOffset = node.arguments.fileOffset)
723+
..fileOffset = node.fileOffset
724+
..parent = node.parent;
725+
665726
/// Returns whether the given [node] is guaranteed to be allowed to interop
666727
/// with JS.
667728
///

pkg/dart2wasm/lib/js/callback_specializer.dart

+51-53
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@ class CallbackSpecializer {
1717
CallbackSpecializer(
1818
this._staticTypeContext, this._util, this._methodCollector);
1919

20-
bool _needsArgumentsLength(FunctionType type) =>
21-
type.requiredParameterCount < type.positionalParameters.length;
22-
2320
Statement _generateDispatchCase(
2421
FunctionType function,
2522
VariableDeclaration callbackVariable,
@@ -54,12 +51,11 @@ class CallbackSpecializer {
5451
/// Creates a callback trampoline for the given [function].
5552
///
5653
/// This callback trampoline expects a Dart callback as its first argument,
57-
/// then an integer value(double type) indicating the position of the last
58-
/// defined argument(only for callbacks that take optional parameters),
59-
/// followed by all of the arguments to the Dart callback as JS objects. The
60-
/// trampoline will `dartifyRaw` or box all incoming JS objects and then cast
61-
/// them to their appropriate types, dispatch, and then `jsifyRaw` or box any
62-
/// returned value.
54+
/// then an integer value(double type) indicating the number of arguments
55+
/// passed, followed by all of the arguments to the Dart callback as JS
56+
/// objects. The trampoline will `dartifyRaw` or box all incoming JS objects
57+
/// and then cast them to their appropriate types, dispatch, and then
58+
/// `jsifyRaw` or box any returned value.
6359
///
6460
/// Returns a [String] function name representing the name of the wrapping
6561
/// function.
@@ -69,20 +65,18 @@ class CallbackSpecializer {
6965
// arguments will be JS objects. The generated wrapper will cast each
7066
// argument to the correct type. The first argument to this function will
7167
// be the Dart callback, which will be cast to the supplied [FunctionType]
72-
// before being invoked. If the callback takes optional parameters then, the
73-
// second argument will be a `double` indicating the last defined argument.
68+
// before being invoked. The second argument will be a `double` indicating
69+
// the number of arguments passed.
7470
int parameterId = 1;
7571
final callbackVariable = VariableDeclaration('callback',
7672
type: _util.nonNullableObjectType, isSynthesized: true);
77-
VariableDeclaration? argumentsLength;
78-
if (_needsArgumentsLength(function)) {
79-
argumentsLength = VariableDeclaration('argumentsLength',
80-
type: _util.coreTypes.doubleNonNullableRawType, isSynthesized: true);
81-
}
73+
final argumentsLength = VariableDeclaration('argumentsLength',
74+
type: _util.coreTypes.doubleNonNullableRawType, isSynthesized: true);
8275

8376
// Initialize variable declarations.
8477
List<VariableDeclaration> positionalParameters = [];
85-
for (int j = 0; j < function.positionalParameters.length; j++) {
78+
final positionalParametersLength = function.positionalParameters.length;
79+
for (int j = 0; j < positionalParametersLength; j++) {
8680
positionalParameters.add(VariableDeclaration('x${parameterId++}',
8781
type: _util.nullableWasmExternRefType, isSynthesized: true));
8882
}
@@ -91,26 +85,42 @@ class CallbackSpecializer {
9185
// find the last defined argument in JS, that is the last argument which was
9286
// explicitly passed by the user, and then we dispatch to a Dart function
9387
// with the right number of arguments.
94-
//
95-
// First we handle cases where some or all arguments are undefined.
96-
// TODO(joshualitt): Consider using a switch instead.
9788
List<Statement> dispatchCases = [];
98-
for (int i = function.requiredParameterCount + 1;
99-
i <= function.positionalParameters.length;
100-
i++) {
89+
// If more arguments were passed than there are parameters, ignore the extra
90+
// arguments.
91+
dispatchCases.add(IfStatement(
92+
_util.variableGreaterThanOrEqualToConstant(
93+
argumentsLength, IntConstant(positionalParametersLength)),
94+
_generateDispatchCase(function, callbackVariable, positionalParameters,
95+
positionalParametersLength,
96+
boxExternRef: boxExternRef),
97+
null));
98+
// TODO(srujzs): Consider using a switch instead.
99+
for (int i = positionalParametersLength - 1;
100+
i >= function.requiredParameterCount;
101+
i--) {
101102
dispatchCases.add(IfStatement(
102103
_util.variableCheckConstant(
103-
argumentsLength!, DoubleConstant(i.toDouble())),
104+
argumentsLength, DoubleConstant(i.toDouble())),
104105
_generateDispatchCase(
105106
function, callbackVariable, positionalParameters, i,
106107
boxExternRef: boxExternRef),
107108
null));
108109
}
109110

110-
// Finally handle the case where only required parameters are passed.
111-
dispatchCases.add(_generateDispatchCase(function, callbackVariable,
112-
positionalParameters, function.requiredParameterCount,
113-
boxExternRef: boxExternRef));
111+
// Throw since we have too few arguments. Alternatively, we can continue
112+
// checking lengths and try to call the callback, which will then throw, but
113+
// that's unnecessary extra code. Note that we can't exclude this and assume
114+
// the last dispatch case will catch this. Since arguments that are not
115+
// passed are `undefined` and `undefined` gets converted to `null`, they may
116+
// be treated as valid `null` arguments to the Dart function even though
117+
// they were never passed.
118+
dispatchCases.add(ExpressionStatement(Throw(StringConcatenation([
119+
StringLiteral('Too few arguments passed. '
120+
'Expected ${function.requiredParameterCount} or more, got '),
121+
invokeMethod(argumentsLength, _util.numToIntTarget),
122+
StringLiteral(' instead.')
123+
]))));
114124
Statement functionTrampolineBody = Block(dispatchCases);
115125

116126
// Create a new procedure for the callback trampoline. This procedure will
@@ -124,8 +134,9 @@ class CallbackSpecializer {
124134
FunctionNode(functionTrampolineBody,
125135
positionalParameters: [
126136
callbackVariable,
127-
if (argumentsLength != null) argumentsLength
128-
].followedBy(positionalParameters).toList(),
137+
argumentsLength,
138+
...positionalParameters
139+
],
129140
returnType: _util.nullableWasmExternRefType)
130141
..fileOffset = node.fileOffset,
131142
node.fileUri,
@@ -144,11 +155,7 @@ class CallbackSpecializer {
144155
jsParameters.add('x$i');
145156
}
146157
String jsParametersString = jsParameters.join(',');
147-
String dartArguments = 'f';
148-
bool needsArguments = _needsArgumentsLength(type);
149-
if (needsArguments) {
150-
dartArguments = '$dartArguments,arguments.length';
151-
}
158+
String dartArguments = 'f,arguments.length';
152159
if (jsParameters.isNotEmpty) {
153160
dartArguments = '$dartArguments,$jsParametersString';
154161
}
@@ -171,24 +178,12 @@ class CallbackSpecializer {
171178
// Create JS method.
172179
// Note: We have to use a regular function for the inner closure in some
173180
// cases because we need access to `arguments`.
174-
if (needsArguments) {
175-
_methodCollector.addMethod(
176-
dartProcedure,
177-
jsMethodName,
178-
"f => finalizeWrapper(f, function($jsParametersString) {"
179-
" return dartInstance.exports.$functionTrampolineName($dartArguments) "
180-
"})");
181-
} else {
182-
if (parametersNeedParens(jsParameters)) {
183-
jsParametersString = '($jsParametersString)';
184-
}
185-
_methodCollector.addMethod(
186-
dartProcedure,
187-
jsMethodName,
188-
"f => "
189-
"finalizeWrapper(f,$jsParametersString => "
190-
"dartInstance.exports.$functionTrampolineName($dartArguments))");
191-
}
181+
_methodCollector.addMethod(
182+
dartProcedure,
183+
jsMethodName,
184+
"f => finalizeWrapper(f, function($jsParametersString) {"
185+
" return dartInstance.exports.$functionTrampolineName($dartArguments) "
186+
"})");
192187

193188
return dartProcedure;
194189
}
@@ -207,6 +202,9 @@ class CallbackSpecializer {
207202
/// these Dart functions flow to JS, they are replaced by their wrappers. If
208203
/// the wrapper should ever flow back into Dart then it will be replaced by
209204
/// the original Dart function.
205+
// TODO(srujzs): It looks like there's no more code that references this
206+
// function anymore in dart2wasm. Should we delete this lowering and related
207+
// code?
210208
Expression allowInterop(StaticInvocation staticInvocation) {
211209
final argument = staticInvocation.arguments.positional.single;
212210
final type = argument.getStaticType(_staticTypeContext) as FunctionType;

pkg/dart2wasm/lib/js/util.dart

+14
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ class CoreTypesUtil {
1616
final Procedure allowInteropTarget;
1717
final Procedure dartifyRawTarget;
1818
final Procedure functionToJSTarget;
19+
final Procedure greaterThanOrEqualToTarget;
1920
final Procedure inlineJSTarget;
2021
final Procedure isDartFunctionWrappedTarget;
2122
final Procedure jsifyRawTarget;
@@ -33,6 +34,8 @@ class CoreTypesUtil {
3334
.getTopLevelProcedure('dart:_js_helper', 'dartifyRaw'),
3435
functionToJSTarget = coreTypes.index.getTopLevelProcedure(
3536
'dart:js_interop', 'FunctionToJSExportedDartFunction|get#toJS'),
37+
greaterThanOrEqualToTarget =
38+
coreTypes.index.getProcedure('dart:core', 'num', '>='),
3639
inlineJSTarget =
3740
coreTypes.index.getTopLevelProcedure('dart:_js_helper', 'JS'),
3841
isDartFunctionWrappedTarget = coreTypes.index
@@ -94,6 +97,17 @@ class CoreTypesUtil {
9497
StaticInvocation(coreTypes.identicalProcedure,
9598
Arguments([VariableGet(variable), ConstantExpression(constant)]));
9699

100+
Expression variableGreaterThanOrEqualToConstant(
101+
VariableDeclaration variable, Constant constant) =>
102+
InstanceInvocation(
103+
InstanceAccessKind.Instance,
104+
VariableGet(variable),
105+
greaterThanOrEqualToTarget.name,
106+
Arguments([ConstantExpression(constant)]),
107+
interfaceTarget: greaterThanOrEqualToTarget,
108+
functionType: greaterThanOrEqualToTarget.getterType as FunctionType,
109+
);
110+
97111
/// Cast the [invocation] if needed to conform to the expected [returnType].
98112
Expression castInvocationForReturn(
99113
Expression invocation, DartType returnType) {

0 commit comments

Comments
 (0)