Skip to content

Commit 3b39c26

Browse files
srujzsCommit Queue
srujzs
authored and
Commit Queue
committed
[ddc] Separate _emitObjectLiteral out from _emitArgumentList
Currently _emitArgumentList handles arguments for Dart members, JS interop members, and object literal constructors. It requires an Arguments node, which we end up synthesizing, which messes with parent pointers erroneously. We also have to separate out the arguments and do additional checks later to detect whether the arguments passed correspond to an object literal constructor. Instead, we should create the resulting expression directly in a helper method. A map is passed so that the names of arguments can be easily changed when we handle @js renaming on object literal constructors. Lastly, we need to avoid wrapping the values with assertInterop in the case where the constructor is from a dart:js_interop interface. That library only statically allows Functions to be passed if they're externalized, in which case we shouldn't tell users to wrap the Function with allowInterop (which doesn't exist anyways in dart:js_interop). Change-Id: If6fdc706e80837ab2b698cca704ffd9f08aed28a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/446184 Reviewed-by: Nicholas Shahan <[email protected]> Commit-Queue: Srujan Gaddam <[email protected]>
1 parent 4ccfbc6 commit 3b39c26

File tree

3 files changed

+154
-60
lines changed

3 files changed

+154
-60
lines changed

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

Lines changed: 65 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import 'dart:convert';
77
import 'dart:io' as io;
88
import 'dart:math' show max, min;
99

10+
import 'package:_js_interop_checks/src/js_interop.dart'
11+
show hasDartJSInteropAnnotation;
1012
import 'package:_js_interop_checks/src/transformations/js_util_optimizer.dart'
1113
show ExtensionIndex;
1214
import 'package:front_end/src/api_unstable/ddc.dart';
@@ -6935,14 +6937,10 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
69356937
// JS interop checks assert that only external extension type constructors
69366938
// and factories have named parameters.
69376939
assert(target.function.positionalParameters.isEmpty);
6938-
return _emitObjectLiteral(
6939-
Arguments(
6940-
node.arguments.positional,
6941-
types: node.arguments.types,
6942-
named: node.arguments.named,
6943-
),
6944-
target,
6945-
);
6940+
var namedProperties = {
6941+
for (final named in node.arguments.named) named.name: named.value,
6942+
};
6943+
return _emitObjectLiteral(namedProperties, isDartJsInterop: true);
69466944
}
69476945
if (target == _coreTypes.identicalProcedure) {
69486946
return _emitCoreIdenticalCall(node.arguments.positional);
@@ -7110,10 +7108,10 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
71107108
return _emitTopLevelName(target);
71117109
}
71127110

7113-
/// Returns all parts of [arguments] flattened into a list so they can be
7114-
/// passed in the calling convention for calls with no runtime checks.
7111+
/// Returns all parts of [node] flattened into a list so they can be passed in
7112+
/// the calling convention for calls with no runtime checks.
71157113
///
7116-
/// When [types] is `false` any type arguments present in [arguments] will be
7114+
/// When [types] is `false` any type arguments present in [node] will be
71177115
/// omitted.
71187116
///
71197117
/// Passing [target] when applicable allows for detection of an annotation to
@@ -7140,8 +7138,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
71407138
];
71417139
}
71427140

7143-
/// Returns all [arguments] but kept in separate packets so they can be
7144-
/// further processed.
7141+
/// Returns all arguments from [node] but kept in separate packets so they can
7142+
/// be further processed.
71457143
///
71467144
/// Facilitates passing arguments in the calling convention used by runtime
71477145
/// helpers that check arguments.
@@ -7168,9 +7166,14 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
71687166
else
71697167
_visitExpression(arg),
71707168
];
7169+
// Named arguments with JS interop are only allowed in object literal
7170+
// constructors. Invocations to those are always transformed by
7171+
// `_emitObjectLiteral` and therefore we should never expect to see a JS
7172+
// interop invocation with named arguments here.
7173+
if (node.named.isNotEmpty) assert(!isJSInterop);
71717174
var namedArguments = node.named.isEmpty
71727175
? null
7173-
: [for (var arg in node.named) _emitNamedExpression(arg, isJSInterop)];
7176+
: [for (var arg in node.named) _emitNamedExpression(arg)];
71747177

71757178
return (
71767179
typeArguments: typeArguments,
@@ -7179,13 +7182,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
71797182
);
71807183
}
71817184

7182-
js_ast.Property _emitNamedExpression(
7183-
NamedExpression arg, [
7184-
bool isJsInterop = false,
7185-
]) {
7186-
var value = isJsInterop ? _assertInterop(arg.value) : arg.value;
7187-
return js_ast.Property(_propertyName(arg.name), _visitExpression(value));
7188-
}
7185+
js_ast.Property _emitNamedExpression(NamedExpression arg) =>
7186+
js_ast.Property(_propertyName(arg.name), _visitExpression(arg.value));
71897187

71907188
/// Emits code for the `JS(...)` macro.
71917189
js_ast.Node _emitInlineJSCode(StaticInvocation node) {
@@ -7411,7 +7409,15 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
74117409
var ctor = node.target;
74127410
var ctorClass = ctor.enclosingClass;
74137411
var args = node.arguments;
7414-
if (isJSAnonymousType(ctorClass)) return _emitObjectLiteral(args, ctor);
7412+
if (isJSAnonymousType(ctorClass)) {
7413+
var namedProperties = {
7414+
for (final named in node.arguments.named) named.name: named.value,
7415+
};
7416+
return _emitObjectLiteral(
7417+
namedProperties,
7418+
isDartJsInterop: hasDartJSInteropAnnotation(ctorClass),
7419+
);
7420+
}
74157421
// JS interop constructor calls do not provide an RTI at the call site.
74167422
var shouldProvideRti =
74177423
!isJSInteropMember(ctor) && _requiresRtiForInstantiation(ctorClass);
@@ -7499,7 +7505,15 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
74997505

75007506
js_ast.Expression _emitJSInteropNew(Member ctor, Arguments args) {
75017507
var ctorClass = ctor.enclosingClass!;
7502-
if (isJSAnonymousType(ctorClass)) return _emitObjectLiteral(args, ctor);
7508+
if (isJSAnonymousType(ctorClass)) {
7509+
var namedProperties = {
7510+
for (final named in args.named) named.name: named.value,
7511+
};
7512+
return _emitObjectLiteral(
7513+
namedProperties,
7514+
isDartJsInterop: hasDartJSInteropAnnotation(ctorClass),
7515+
);
7516+
}
75037517
// JS interop constructor calls do not require an RTI at the call site.
75047518
return js_ast.New(
75057519
_emitConstructorName(_coreTypes.nonNullableRawType(ctorClass), ctor),
@@ -7527,11 +7541,34 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
75277541
return InterfaceType(c, Nullability.nonNullable, typeArgs);
75287542
}
75297543

7530-
js_ast.Expression _emitObjectLiteral(Arguments node, Member ctor) {
7531-
var args = _emitArgumentList(node, types: false, target: ctor);
7532-
if (args.isEmpty) return js.call('{}');
7533-
assert(args.single is js_ast.ObjectInitializer);
7534-
return args.single;
7544+
/// Given a mapping [namedProperties] between names and their associated
7545+
/// expressions, returns an object literal with the same names mapped to the
7546+
/// transformed expressions.
7547+
///
7548+
/// This is used for object literal constructors in JS interop.
7549+
///
7550+
/// If [isDartJsInterop] is true, it implies that this is an object literal
7551+
/// constructor using `dart:js_interop`. If false, this method wraps the
7552+
/// expressions with an [_assertInterop] call.
7553+
js_ast.Expression _emitObjectLiteral(
7554+
Map<String, Expression> namedProperties, {
7555+
required bool isDartJsInterop,
7556+
}) {
7557+
if (namedProperties.isEmpty) return js.call('{}');
7558+
var properties = <js_ast.Property>[];
7559+
for (var name in namedProperties.keys) {
7560+
// `assertInterop` is not needed for `dart:js_interop`. It statically
7561+
// disallows `Function`s from being passed unless they were explicitly
7562+
// passed as an opaque reference, in which case, we shouldn't require the
7563+
// user to wrap them anyways.
7564+
var value = isDartJsInterop
7565+
? namedProperties[name]!
7566+
: _assertInterop(namedProperties[name]!);
7567+
properties.add(
7568+
js_ast.Property(_propertyName(name), _visitExpression(value)),
7569+
);
7570+
}
7571+
return js_ast.ObjectInitializer([...properties]);
75357572
}
75367573

75377574
@override

‎pkg/dev_compiler/lib/src/kernel/compiler_new.dart‎

Lines changed: 65 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import 'dart:convert';
77
import 'dart:io' as io;
88
import 'dart:math' show max, min;
99

10+
import 'package:_js_interop_checks/src/js_interop.dart'
11+
show hasDartJSInteropAnnotation;
1012
import 'package:_js_interop_checks/src/transformations/js_util_optimizer.dart'
1113
show ExtensionIndex;
1214
import 'package:front_end/src/api_unstable/ddc.dart';
@@ -7568,14 +7570,10 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
75687570
// JS interop checks assert that only external extension type constructors
75697571
// and factories have named parameters.
75707572
assert(target.function.positionalParameters.isEmpty);
7571-
return _emitObjectLiteral(
7572-
Arguments(
7573-
node.arguments.positional,
7574-
types: node.arguments.types,
7575-
named: node.arguments.named,
7576-
),
7577-
target,
7578-
);
7573+
var namedProperties = {
7574+
for (final named in node.arguments.named) named.name: named.value,
7575+
};
7576+
return _emitObjectLiteral(namedProperties, isDartJsInterop: true);
75797577
}
75807578
if (target == _coreTypes.identicalProcedure) {
75817579
return _emitCoreIdenticalCall(node.arguments.positional);
@@ -7905,10 +7903,10 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
79057903
return _emitTopLevelName(target);
79067904
}
79077905

7908-
/// Returns all parts of [arguments] flattened into a list so they can be
7909-
/// passed in the calling convention for calls with no runtime checks.
7906+
/// Returns all parts of [node] flattened into a list so they can be passed in
7907+
/// the calling convention for calls with no runtime checks.
79107908
///
7911-
/// When [types] is `false` any type arguments present in [arguments] will be
7909+
/// When [types] is `false` any type arguments present in [node] will be
79127910
/// omitted.
79137911
///
79147912
/// Passing [target] when applicable allows for detection of an annotation to
@@ -7935,8 +7933,8 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
79357933
];
79367934
}
79377935

7938-
/// Returns all [arguments] but kept in separate packets so they can be
7939-
/// further processed.
7936+
/// Returns all arguments from [node] but kept in separate packets so they can
7937+
/// be further processed.
79407938
///
79417939
/// Facilitates passing arguments in the calling convention used by runtime
79427940
/// helpers that check arguments.
@@ -7963,9 +7961,14 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
79637961
else
79647962
_visitExpression(arg),
79657963
];
7964+
// Named arguments with JS interop are only allowed in object literal
7965+
// constructors. Invocations to those are always transformed by
7966+
// `_emitObjectLiteral` and therefore we should never expect to see a JS
7967+
// interop invocation with named arguments here.
7968+
if (node.named.isNotEmpty) assert(!isJSInterop);
79667969
var namedArguments = node.named.isEmpty
79677970
? null
7968-
: [for (var arg in node.named) _emitNamedExpression(arg, isJSInterop)];
7971+
: [for (var arg in node.named) _emitNamedExpression(arg)];
79697972

79707973
return (
79717974
typeArguments: typeArguments,
@@ -7974,13 +7977,8 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
79747977
);
79757978
}
79767979

7977-
js_ast.Property _emitNamedExpression(
7978-
NamedExpression arg, [
7979-
bool isJsInterop = false,
7980-
]) {
7981-
var value = isJsInterop ? _assertInterop(arg.value) : arg.value;
7982-
return js_ast.Property(_propertyName(arg.name), _visitExpression(value));
7983-
}
7980+
js_ast.Property _emitNamedExpression(NamedExpression arg) =>
7981+
js_ast.Property(_propertyName(arg.name), _visitExpression(arg.value));
79847982

79857983
/// Emits code for the `JS(...)` macro.
79867984
js_ast.Node _emitInlineJSCode(StaticInvocation node) {
@@ -8206,7 +8204,15 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
82068204
var ctor = node.target;
82078205
var ctorClass = ctor.enclosingClass;
82088206
var args = node.arguments;
8209-
if (isJSAnonymousType(ctorClass)) return _emitObjectLiteral(args, ctor);
8207+
if (isJSAnonymousType(ctorClass)) {
8208+
var namedProperties = {
8209+
for (final named in node.arguments.named) named.name: named.value,
8210+
};
8211+
return _emitObjectLiteral(
8212+
namedProperties,
8213+
isDartJsInterop: hasDartJSInteropAnnotation(ctorClass),
8214+
);
8215+
}
82108216
// JS interop constructor calls do not provide an RTI at the call site.
82118217
var shouldProvideRti =
82128218
!isJSInteropMember(ctor) && _requiresRtiForInstantiation(ctorClass);
@@ -8294,7 +8300,15 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
82948300

82958301
js_ast.Expression _emitJSInteropNew(Member ctor, Arguments args) {
82968302
var ctorClass = ctor.enclosingClass!;
8297-
if (isJSAnonymousType(ctorClass)) return _emitObjectLiteral(args, ctor);
8303+
if (isJSAnonymousType(ctorClass)) {
8304+
var namedProperties = {
8305+
for (final named in args.named) named.name: named.value,
8306+
};
8307+
return _emitObjectLiteral(
8308+
namedProperties,
8309+
isDartJsInterop: hasDartJSInteropAnnotation(ctorClass),
8310+
);
8311+
}
82988312
// JS interop constructor calls do not require an RTI at the call site.
82998313
return js_ast.New(
83008314
_emitConstructorName(_coreTypes.nonNullableRawType(ctorClass), ctor),
@@ -8322,11 +8336,34 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
83228336
return InterfaceType(c, Nullability.nonNullable, typeArgs);
83238337
}
83248338

8325-
js_ast.Expression _emitObjectLiteral(Arguments node, Member ctor) {
8326-
var args = _emitArgumentList(node, types: false, target: ctor);
8327-
if (args.isEmpty) return js.call('{}');
8328-
assert(args.single is js_ast.ObjectInitializer);
8329-
return args.single;
8339+
/// Given a mapping [namedProperties] between names and their associated
8340+
/// expressions, returns an object literal with the same names mapped to the
8341+
/// transformed expressions.
8342+
///
8343+
/// This is used for object literal constructors in JS interop.
8344+
///
8345+
/// If [isDartJsInterop] is true, it implies that this is an object literal
8346+
/// constructor using `dart:js_interop`. If false, this method wraps the
8347+
/// expressions with an [_assertInterop] call.
8348+
js_ast.Expression _emitObjectLiteral(
8349+
Map<String, Expression> namedProperties, {
8350+
required bool isDartJsInterop,
8351+
}) {
8352+
if (namedProperties.isEmpty) return js.call('{}');
8353+
var properties = <js_ast.Property>[];
8354+
for (var name in namedProperties.keys) {
8355+
// `assertInterop` is not needed for `dart:js_interop`. It statically
8356+
// disallows `Function`s from being passed unless they were explicitly
8357+
// passed as an opaque reference, in which case, we shouldn't require the
8358+
// user to wrap them anyways.
8359+
var value = isDartJsInterop
8360+
? namedProperties[name]!
8361+
: _assertInterop(namedProperties[name]!);
8362+
properties.add(
8363+
js_ast.Property(_propertyName(name), _visitExpression(value)),
8364+
);
8365+
}
8366+
return js_ast.ObjectInitializer([...properties]);
83308367
}
83318368

83328369
@override

‎tests/lib/js/static_interop_test/external_dart_reference_test.dart‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,19 @@ external set _identity(JSFunction _);
3333
@JS()
3434
external T identity<T extends ExternalDartReference?>(T t);
3535

36-
extension type ObjectLiteral(JSObject _) {
36+
extension type ObjectLiteral._(JSObject _) {
37+
external ObjectLiteral({ExternalDartReference<Object> ref});
38+
external void operator []=(String key, ExternalDartReference<Object> value);
39+
}
40+
41+
@JS()
42+
@staticInterop
43+
@anonymous
44+
class AnonymousObjectLiteral {
45+
external factory AnonymousObjectLiteral({ExternalDartReference<Object> ref});
46+
}
47+
48+
extension on AnonymousObjectLiteral {
3749
external void operator []=(String key, ExternalDartReference<Object> value);
3850
}
3951

@@ -143,9 +155,17 @@ void generalTest() {
143155
);
144156

145157
// Functions should not trigger `assertInterop`.
146-
externalDartReference = () {}.toExternalReference;
147-
identity(EExternalDartReference(() {}.toExternalReference));
148-
ObjectLiteral(JSObject())['ref'] = externalDartReference;
158+
final externalDartRefFunction = () {}.toExternalReference;
159+
externalDartReference = externalDartRefFunction;
160+
identity(EExternalDartReference(externalDartRefFunction));
161+
// Test object literal constructors as well, as they are handled by the
162+
// compiler backends when compiling to JS.
163+
final objectLiteral = ObjectLiteral(ref: externalDartRefFunction);
164+
objectLiteral['ref'] = externalDartRefFunction;
165+
final anonymousObjectLiteral = AnonymousObjectLiteral(
166+
ref: externalDartRefFunction,
167+
);
168+
anonymousObjectLiteral['ref'] = externalDartRefFunction;
149169
}
150170

151171
// An example interface for a generic `WritableSignal` from

0 commit comments

Comments
 (0)