[stable][dart2wasm] Use node's enclosing library annotation for lower…

…ings 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 Change-Id: I37661ed200c6db367e3f29f50b0877834f4c1639 Bug: #55359 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362190 Reviewed-by: Kevin Chisholm <[email protected]> Commit-Queue: Srujan Gaddam <[email protected]>
dart-lang · Apr 16, 2024 · 8ad4ca1 · 8ad4ca1
1 parent c09cb46
commit 8ad4ca1
Show file tree

Hide file tree

Showing 10 changed files with 403 additions and 261 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,13 @@
+## 3.3.4
+
+This is a patch release that:
+
+- Fixes an issue with JS interop in dart2wasm where JS interop methods that used
+  the enclosing library's `@JS` annotation were actually using the invocation's
+  enclosing library's `@JS` annotation. (issue [#55359])
+
+[#55359]: https://github.com/dart-lang/sdk/issues/55359
+
 ## 3.3.3 - 2024-03-27
 
 This is a patch release that:

diff --git a/pkg/dart2wasm/lib/js/interop_specializer.dart b/pkg/dart2wasm/lib/js/interop_specializer.dart
@@ -346,19 +346,11 @@ class InteropSpecializerFactory {
   final MethodCollector _methodCollector;
   final Map<Procedure, Map<int, Procedure>> _overloadedProcedures = {};
   final Map<Procedure, Map<String, Procedure>> _jsObjectLiteralMethods = {};
-  late String _libraryJSString;
   late final ExtensionIndex _extensionIndex;
 
   InteropSpecializerFactory(this._staticTypeContext, this._util,
       this._methodCollector, this._extensionIndex);
 
-  void enterLibrary(Library library) {
-    _libraryJSString = getJSName(library);
-    if (_libraryJSString.isNotEmpty) {
-      _libraryJSString = '$_libraryJSString.';
-    }
-  }
-
   String _getJSString(Annotatable a, String initial) {
     String selectorString = getJSName(a);
     if (selectorString.isEmpty) {
@@ -367,8 +359,13 @@ class InteropSpecializerFactory {
     return selectorString;
   }
 
-  String _getTopLevelJSString(Annotatable a, String initial) =>
-      '$_libraryJSString${_getJSString(a, initial)}';
+  String _getTopLevelJSString(
+      Annotatable a, String writtenName, Library enclosingLibrary) {
+    final name = _getJSString(a, writtenName);
+    final libraryName = getJSName(enclosingLibrary);
+    if (libraryName.isEmpty) return name;
+    return '$libraryName.$name';
+  }
 
   /// Get the `_Specializer` for the non-constructor [node] with its
   /// associated [jsString] name, and the [invocation] it's used in if this is
@@ -425,7 +422,8 @@ class InteropSpecializerFactory {
     if (node.enclosingClass != null &&
         hasJSInteropAnnotation(node.enclosingClass!)) {
       final cls = node.enclosingClass!;
-      final clsString = _getTopLevelJSString(cls, cls.name);
+      final clsString =
+          _getTopLevelJSString(cls, cls.name, cls.enclosingLibrary);
       if (node.isFactory) {
         return _getSpecializerForConstructor(
             hasAnonymousAnnotation(cls), node, clsString, invocation);
@@ -438,7 +436,8 @@ class InteropSpecializerFactory {
       final nodeDescriptor = _extensionIndex.getExtensionTypeDescriptor(node);
       if (nodeDescriptor != null) {
         final cls = _extensionIndex.getExtensionType(node)!;
-        final clsString = _getTopLevelJSString(cls, cls.name);
+        final clsString =
+            _getTopLevelJSString(cls, cls.name, node.enclosingLibrary);
         final kind = nodeDescriptor.kind;
         if ((kind == ExtensionTypeMemberKind.Constructor ||
             kind == ExtensionTypeMemberKind.Factory)) {
@@ -467,7 +466,9 @@ class InteropSpecializerFactory {
       }
     } else if (hasJSInteropAnnotation(node)) {
       return _getSpecializerForMember(
-          node, _getTopLevelJSString(node, node.name.text), invocation);
+          node,
+          _getTopLevelJSString(node, node.name.text, node.enclosingLibrary),
+          invocation);
     }
     return null;
   }

diff --git a/pkg/dart2wasm/lib/js/interop_transformer.dart b/pkg/dart2wasm/lib/js/interop_transformer.dart
@@ -55,7 +55,6 @@ class InteropTransformer extends Transformer {
 
   @override
   Library visitLibrary(Library lib) {
-    _interopSpecializerFactory.enterLibrary(lib);
     _methodCollector.enterLibrary(lib);
     _staticTypeContext.enterLibrary(lib);
     lib.transformChildren(this);

diff --git a/tests/lib/js/static_interop_test/extension_type/external_static_member_test.dart b/tests/lib/js/static_interop_test/extension_type/external_static_member_test.dart
@@ -2,25 +2,28 @@
 // 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.
 
-// SharedOptions=--enable-experiment=inline-class
+// TODO(srujzs): There's a decent amount of code duplication in this test. We
+// should combine this with
+// tests/lib/js/static_interop_test/external_static_member_lowerings_test.dart.
 
 @JS()
 library external_static_member_test;
 
 import 'dart:js_interop';
-import 'dart:js_util' as js_util;
+import 'dart:js_interop_unsafe';
 
 import 'package:expect/minitest.dart';
 
+import 'external_static_member_with_namespaces.dart' as namespace;
+
 @JS()
 external void eval(String code);
 
 @JS()
-extension type ExternalStatic._(JSObject obj) implements Object {
+extension type ExternalStatic._(JSObject obj) implements JSObject {
   external ExternalStatic();
   external factory ExternalStatic.factory();
   external ExternalStatic.multipleArgs(double a, String b);
-  external ExternalStatic.differentArgs(double a, [String b = '']);
   ExternalStatic.nonExternal() : this.obj = ExternalStatic() as JSObject;
 
   external static String field;
@@ -36,39 +39,20 @@ extension type ExternalStatic._(JSObject obj) implements Object {
   external static set renamedGetSet(String val);
 
   external static String method();
-  external static String differentArgsMethod(String a, [String b = '']);
   @JS('method')
   external static String renamedMethod();
 }
 
-void main() {
-  eval('''
-    globalThis.ExternalStatic = function ExternalStatic(a, b) {
-      var len = arguments.length;
-      this.a = len < 1 ? 0 : a;
-      this.b = len < 2 ? '' : b;
-    }
-    globalThis.ExternalStatic.method = function() {
-      return 'method';
-    }
-    globalThis.ExternalStatic.differentArgsMethod = function(a, b) {
-      return a + b;
-    }
-    globalThis.ExternalStatic.field = 'field';
-    globalThis.ExternalStatic.finalField = 'finalField';
-    globalThis.ExternalStatic.getSet = 'getSet';
-  ''');
-
+void testStaticMembers() {
   // Constructors.
   void testExternalConstructorCall(ExternalStatic externalStatic) {
-    expect(js_util.getProperty(externalStatic, 'a'), 0);
-    expect(js_util.getProperty(externalStatic, 'b'), '');
+    expect((externalStatic['a'] as JSNumber).toDartInt, 0);
+    expect((externalStatic['b'] as JSString).toDart, '');
   }
 
   testExternalConstructorCall(ExternalStatic());
   testExternalConstructorCall(ExternalStatic.factory());
   testExternalConstructorCall(ExternalStatic.multipleArgs(0, ''));
-  testExternalConstructorCall(ExternalStatic.differentArgs(0));
   testExternalConstructorCall(ExternalStatic.nonExternal());
 
   // Fields.
@@ -90,6 +74,69 @@ void main() {
 
   // Methods.
   expect(ExternalStatic.method(), 'method');
-  expect(ExternalStatic.differentArgsMethod('method'), 'methodundefined');
   expect(ExternalStatic.renamedMethod(), 'method');
 }
+
+void testNamespacedStaticMembers() {
+  // Constructors.
+  void testExternalConstructorCall(namespace.ExternalStatic externalStatic) {
+    expect((externalStatic['a'] as JSNumber).toDartInt, 0);
+    expect((externalStatic['b'] as JSString).toDart, '');
+  }
+
+  testExternalConstructorCall(namespace.ExternalStatic());
+  testExternalConstructorCall(namespace.ExternalStatic.factory());
+  testExternalConstructorCall(namespace.ExternalStatic.multipleArgs(0, ''));
+  testExternalConstructorCall(namespace.ExternalStatic.nonExternal());
+
+  // Fields.
+  expect(namespace.ExternalStatic.field, 'field');
+  namespace.ExternalStatic.field = 'modified';
+  expect(namespace.ExternalStatic.field, 'modified');
+  expect(namespace.ExternalStatic.renamedField, 'modified');
+  namespace.ExternalStatic.renamedField = 'renamedField';
+  expect(namespace.ExternalStatic.renamedField, 'renamedField');
+  expect(namespace.ExternalStatic.finalField, 'finalField');
+
+  // Getters and setters.
+  expect(namespace.ExternalStatic.getSet, 'getSet');
+  namespace.ExternalStatic.getSet = 'modified';
+  expect(namespace.ExternalStatic.getSet, 'modified');
+  expect(namespace.ExternalStatic.renamedGetSet, 'modified');
+  namespace.ExternalStatic.renamedGetSet = 'renamedGetSet';
+  expect(namespace.ExternalStatic.renamedGetSet, 'renamedGetSet');
+
+  // Methods.
+  expect(namespace.ExternalStatic.method(), 'method');
+  expect(namespace.ExternalStatic.renamedMethod(), 'method');
+}
+
+void main() {
+  eval('''
+    globalThis.ExternalStatic = function ExternalStatic(a, b) {
+      var len = arguments.length;
+      this.a = len < 1 ? 0 : a;
+      this.b = len < 2 ? '' : b;
+    }
+    globalThis.ExternalStatic.method = function() {
+      return 'method';
+    }
+    globalThis.ExternalStatic.field = 'field';
+    globalThis.ExternalStatic.finalField = 'finalField';
+    globalThis.ExternalStatic.getSet = 'getSet';
+  ''');
+  testStaticMembers();
+  eval('''
+    var library3 = {};
+    var library2 = {library3: library3};
+    var library1 = {library2: library2};
+    globalThis.library1 = library1;
+
+    library3.ExternalStatic = globalThis.ExternalStatic;
+    library3.ExternalStatic.field = 'field';
+    library3.ExternalStatic.finalField = 'finalField';
+    library3.ExternalStatic.getSet = 'getSet';
+    delete globalThis.ExternalStatic;
+  ''');
+  testNamespacedStaticMembers();
+}
diff --git a/tests/lib/js/static_interop_test/extension_type/external_static_member_with_namespaces.dart b/tests/lib/js/static_interop_test/extension_type/external_static_member_with_namespaces.dart
@@ -0,0 +1,35 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// 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.
+
+@JS('library1.library2')
+library external_static_member_with_namespaces_test;
+
+import 'dart:js_interop';
+
+@JS()
+external void eval(String code);
+
+@JS('library3.ExternalStatic')
+extension type ExternalStatic._(JSObject obj) implements JSObject {
+  external ExternalStatic();
+  external factory ExternalStatic.factory();
+  external ExternalStatic.multipleArgs(double a, String b);
+  ExternalStatic.nonExternal() : this.obj = ExternalStatic() as JSObject;
+
+  external static String field;
+  @JS('field')
+  external static String renamedField;
+  external static final String finalField;
+
+  external static String get getSet;
+  external static set getSet(String val);
+  @JS('getSet')
+  external static String get renamedGetSet;
+  @JS('getSet')
+  external static set renamedGetSet(String val);
+
+  external static String method();
+  @JS('method')
+  external static String renamedMethod();
+}