Skip to content

Commit b11ab9f

Browse files
srujzsCommit Queue
authored andcommitted
[beta][dart2wasm] Use node's enclosing library annotation for lowerings
Closes #55359 The current library's annotation is used for the interop lowerings in dart2wasm. For most members, this is okay because we emit the equivalent JS code for the member when we visit the procedure and not when we visit the invocation. However, for methods, the invocation determines the resulting JS call due to the existence of optional parameters. In that case, if the invocation was not in the same library as the interop member declaration, it results in using the wrong library's annotation value. Adds tests for this case and does some cleanup of existing tests. Specifically: - Adds a consistent naming scheme for test libraries that are namespaced. - Adds code to delete the non-namespaced declarations so that the namespaced interop methods don't accidentally call those declarations. - Removes differentArgsMethod which was already tested in js_default_test. - Removes use of js_util in favor of js_interop_unsafe. Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/361241 Cherry-pick-request: #55430 Bug: #55359 Change-Id: Ibf7125fea6da7722549f3c87aafdf881132b104f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362195 Commit-Queue: Srujan Gaddam <srujzs@google.com> Reviewed-by: Kevin Chisholm <kevinjchisholm@google.com>
1 parent 749aa52 commit b11ab9f

10 files changed

+394
-362
lines changed

pkg/dart2wasm/lib/js/interop_specializer.dart

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -341,19 +341,11 @@ class InteropSpecializerFactory {
341341
final MethodCollector _methodCollector;
342342
final Map<Procedure, Map<int, Procedure>> _overloadedProcedures = {};
343343
final Map<Procedure, Map<String, Procedure>> _jsObjectLiteralMethods = {};
344-
late String _libraryJSString;
345344
late final ExtensionIndex _extensionIndex;
346345

347346
InteropSpecializerFactory(this._staticTypeContext, this._util,
348347
this._methodCollector, this._extensionIndex);
349348

350-
void enterLibrary(Library library) {
351-
_libraryJSString = getJSName(library);
352-
if (_libraryJSString.isNotEmpty) {
353-
_libraryJSString = '$_libraryJSString.';
354-
}
355-
}
356-
357349
String _getJSString(Annotatable a, String initial) {
358350
String selectorString = getJSName(a);
359351
if (selectorString.isEmpty) {
@@ -362,8 +354,13 @@ class InteropSpecializerFactory {
362354
return selectorString;
363355
}
364356

365-
String _getTopLevelJSString(Annotatable a, String initial) =>
366-
'$_libraryJSString${_getJSString(a, initial)}';
357+
String _getTopLevelJSString(
358+
Annotatable a, String writtenName, Library enclosingLibrary) {
359+
final name = _getJSString(a, writtenName);
360+
final libraryName = getJSName(enclosingLibrary);
361+
if (libraryName.isEmpty) return name;
362+
return '$libraryName.$name';
363+
}
367364

368365
/// Get the `_Specializer` for the non-constructor [node] with its
369366
/// associated [jsString] name, and the [invocation] it's used in if this is
@@ -420,7 +417,8 @@ class InteropSpecializerFactory {
420417
if (node.enclosingClass != null &&
421418
hasJSInteropAnnotation(node.enclosingClass!)) {
422419
final cls = node.enclosingClass!;
423-
final clsString = _getTopLevelJSString(cls, cls.name);
420+
final clsString =
421+
_getTopLevelJSString(cls, cls.name, cls.enclosingLibrary);
424422
if (node.isFactory) {
425423
return _getSpecializerForConstructor(
426424
hasAnonymousAnnotation(cls), node, clsString, invocation);
@@ -433,7 +431,8 @@ class InteropSpecializerFactory {
433431
final nodeDescriptor = _extensionIndex.getExtensionTypeDescriptor(node);
434432
if (nodeDescriptor != null) {
435433
final cls = _extensionIndex.getExtensionType(node)!;
436-
final clsString = _getTopLevelJSString(cls, cls.name);
434+
final clsString =
435+
_getTopLevelJSString(cls, cls.name, node.enclosingLibrary);
437436
final kind = nodeDescriptor.kind;
438437
if ((kind == ExtensionTypeMemberKind.Constructor ||
439438
kind == ExtensionTypeMemberKind.Factory)) {
@@ -462,7 +461,9 @@ class InteropSpecializerFactory {
462461
}
463462
} else if (hasJSInteropAnnotation(node)) {
464463
return _getSpecializerForMember(
465-
node, _getTopLevelJSString(node, node.name.text), invocation);
464+
node,
465+
_getTopLevelJSString(node, node.name.text, node.enclosingLibrary),
466+
invocation);
466467
}
467468
return null;
468469
}

pkg/dart2wasm/lib/js/interop_transformer.dart

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ class InteropTransformer extends Transformer {
5656

5757
@override
5858
Library visitLibrary(Library lib) {
59-
_interopSpecializerFactory.enterLibrary(lib);
6059
_methodCollector.enterLibrary(lib);
6160
_staticTypeContext.enterLibrary(lib);
6261
lib.transformChildren(this);

tests/lib/js/static_interop_test/extension_type/external_static_member_test.dart

Lines changed: 75 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,28 @@
22
// for details. All rights reserved. Use of this source code is governed by a
33
// BSD-style license that can be found in the LICENSE file.
44

5+
// TODO(srujzs): There's a decent amount of code duplication in this test. We
6+
// should combine this with
7+
// tests/lib/js/static_interop_test/external_static_member_lowerings_test.dart.
8+
59
@JS()
610
library external_static_member_test;
711

812
import 'dart:js_interop';
9-
import 'dart:js_util' as js_util;
13+
import 'dart:js_interop_unsafe';
1014

1115
import 'package:expect/minitest.dart';
1216

17+
import 'external_static_member_with_namespaces.dart' as namespace;
18+
1319
@JS()
1420
external void eval(String code);
1521

1622
@JS()
17-
extension type ExternalStatic._(JSObject obj) implements Object {
23+
extension type ExternalStatic._(JSObject obj) implements JSObject {
1824
external ExternalStatic();
1925
external factory ExternalStatic.factory();
2026
external ExternalStatic.multipleArgs(double a, String b);
21-
external ExternalStatic.differentArgs(double a, [String b = '']);
2227
ExternalStatic.nonExternal() : this.obj = ExternalStatic() as JSObject;
2328

2429
external static String field;
@@ -34,39 +39,20 @@ extension type ExternalStatic._(JSObject obj) implements Object {
3439
external static set renamedGetSet(String val);
3540

3641
external static String method();
37-
external static String differentArgsMethod(String a, [String b = '']);
3842
@JS('method')
3943
external static String renamedMethod();
4044
}
4145

42-
void main() {
43-
eval('''
44-
globalThis.ExternalStatic = function ExternalStatic(a, b) {
45-
var len = arguments.length;
46-
this.a = len < 1 ? 0 : a;
47-
this.b = len < 2 ? '' : b;
48-
}
49-
globalThis.ExternalStatic.method = function() {
50-
return 'method';
51-
}
52-
globalThis.ExternalStatic.differentArgsMethod = function(a, b) {
53-
return a + b;
54-
}
55-
globalThis.ExternalStatic.field = 'field';
56-
globalThis.ExternalStatic.finalField = 'finalField';
57-
globalThis.ExternalStatic.getSet = 'getSet';
58-
''');
59-
46+
void testStaticMembers() {
6047
// Constructors.
6148
void testExternalConstructorCall(ExternalStatic externalStatic) {
62-
expect(js_util.getProperty(externalStatic, 'a'), 0);
63-
expect(js_util.getProperty(externalStatic, 'b'), '');
49+
expect((externalStatic['a'] as JSNumber).toDartInt, 0);
50+
expect((externalStatic['b'] as JSString).toDart, '');
6451
}
6552

6653
testExternalConstructorCall(ExternalStatic());
6754
testExternalConstructorCall(ExternalStatic.factory());
6855
testExternalConstructorCall(ExternalStatic.multipleArgs(0, ''));
69-
testExternalConstructorCall(ExternalStatic.differentArgs(0));
7056
testExternalConstructorCall(ExternalStatic.nonExternal());
7157

7258
// Fields.
@@ -88,6 +74,69 @@ void main() {
8874

8975
// Methods.
9076
expect(ExternalStatic.method(), 'method');
91-
expect(ExternalStatic.differentArgsMethod('method'), 'methodundefined');
9277
expect(ExternalStatic.renamedMethod(), 'method');
9378
}
79+
80+
void testNamespacedStaticMembers() {
81+
// Constructors.
82+
void testExternalConstructorCall(namespace.ExternalStatic externalStatic) {
83+
expect((externalStatic['a'] as JSNumber).toDartInt, 0);
84+
expect((externalStatic['b'] as JSString).toDart, '');
85+
}
86+
87+
testExternalConstructorCall(namespace.ExternalStatic());
88+
testExternalConstructorCall(namespace.ExternalStatic.factory());
89+
testExternalConstructorCall(namespace.ExternalStatic.multipleArgs(0, ''));
90+
testExternalConstructorCall(namespace.ExternalStatic.nonExternal());
91+
92+
// Fields.
93+
expect(namespace.ExternalStatic.field, 'field');
94+
namespace.ExternalStatic.field = 'modified';
95+
expect(namespace.ExternalStatic.field, 'modified');
96+
expect(namespace.ExternalStatic.renamedField, 'modified');
97+
namespace.ExternalStatic.renamedField = 'renamedField';
98+
expect(namespace.ExternalStatic.renamedField, 'renamedField');
99+
expect(namespace.ExternalStatic.finalField, 'finalField');
100+
101+
// Getters and setters.
102+
expect(namespace.ExternalStatic.getSet, 'getSet');
103+
namespace.ExternalStatic.getSet = 'modified';
104+
expect(namespace.ExternalStatic.getSet, 'modified');
105+
expect(namespace.ExternalStatic.renamedGetSet, 'modified');
106+
namespace.ExternalStatic.renamedGetSet = 'renamedGetSet';
107+
expect(namespace.ExternalStatic.renamedGetSet, 'renamedGetSet');
108+
109+
// Methods.
110+
expect(namespace.ExternalStatic.method(), 'method');
111+
expect(namespace.ExternalStatic.renamedMethod(), 'method');
112+
}
113+
114+
void main() {
115+
eval('''
116+
globalThis.ExternalStatic = function ExternalStatic(a, b) {
117+
var len = arguments.length;
118+
this.a = len < 1 ? 0 : a;
119+
this.b = len < 2 ? '' : b;
120+
}
121+
globalThis.ExternalStatic.method = function() {
122+
return 'method';
123+
}
124+
globalThis.ExternalStatic.field = 'field';
125+
globalThis.ExternalStatic.finalField = 'finalField';
126+
globalThis.ExternalStatic.getSet = 'getSet';
127+
''');
128+
testStaticMembers();
129+
eval('''
130+
var library3 = {};
131+
var library2 = {library3: library3};
132+
var library1 = {library2: library2};
133+
globalThis.library1 = library1;
134+
135+
library3.ExternalStatic = globalThis.ExternalStatic;
136+
library3.ExternalStatic.field = 'field';
137+
library3.ExternalStatic.finalField = 'finalField';
138+
library3.ExternalStatic.getSet = 'getSet';
139+
delete globalThis.ExternalStatic;
140+
''');
141+
testNamespacedStaticMembers();
142+
}
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
@JS('library1.library2')
6+
library external_static_member_with_namespaces_test;
7+
8+
import 'dart:js_interop';
9+
10+
@JS()
11+
external void eval(String code);
12+
13+
@JS('library3.ExternalStatic')
14+
extension type ExternalStatic._(JSObject obj) implements JSObject {
15+
external ExternalStatic();
16+
external factory ExternalStatic.factory();
17+
external ExternalStatic.multipleArgs(double a, String b);
18+
ExternalStatic.nonExternal() : this.obj = ExternalStatic() as JSObject;
19+
20+
external static String field;
21+
@JS('field')
22+
external static String renamedField;
23+
external static final String finalField;
24+
25+
external static String get getSet;
26+
external static set getSet(String val);
27+
@JS('getSet')
28+
external static String get renamedGetSet;
29+
@JS('getSet')
30+
external static set renamedGetSet(String val);
31+
32+
external static String method();
33+
@JS('method')
34+
external static String renamedMethod();
35+
}

tests/lib/js/static_interop_test/extension_type/external_static_member_with_namespaces_test.dart

Lines changed: 0 additions & 102 deletions
This file was deleted.

0 commit comments

Comments
 (0)