Reland "[dart2wasm] Disallow use of old interop libraries"

Includes small wording modifications and extra allowlisting for benchmarks and Flutter engine tests. This reverts commit c97a17b2349bcbdadd34204a65a9e7c7c13def83. Change-Id: I2db391782c98c351d744ea7201c9421f13b97979 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/343689 Commit-Queue: Srujan Gaddam <srujzs@google.com> Reviewed-by: Sigmund Cherem <sigmund@google.com> Reviewed-by: Kevin Moore <kevmoo@google.com>
2024-10-01 17:02:10 +00:00 · 2024-01-02 21:14:50 +00:00 · 2024-01-02 21:14:50 +00:00 · 3a80ec26be
parent 821bdb6535
commit 3a80ec26be
8 changed files with 132 additions and 48 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -124,6 +124,15 @@

 [lints-3-0]: https://pub.dev/packages/lints/changelog#300

+
+#### Wasm compiler (dart2wasm)
+
+- **Breaking Change** [#54004][]: `dart:js_util`, `package:js`, and `dart:js`
+  are now disallowed from being imported when compiling with `dart2wasm`. Prefer
+  using `dart:js_interop` and `dart:js_interop_unsafe`.
+
+[#54004]: https://github.com/dart-lang/sdk/issues/54004
+
 #### Development JavaScript compiler (DDC)

 - Type arguments of `package:js` interop types are now printed as `any` instead
--- a/pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart
+++ b/pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart
@ -8570,6 +8570,38 @@ const MessageCode messageJsInteropDartJsInteropAnnotationForStaticInteropOnly =
        correctionMessage:
            r"""Try making this class an extension type or marking it as '@staticInterop'.""");

+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const Template<
+        Message Function(String name)>
+    templateJsInteropDisallowedInteropLibraryInDart2Wasm =
+    const Template<Message Function(String name)>(
+        "JsInteropDisallowedInteropLibraryInDart2Wasm",
+        problemMessageTemplate:
+            r"""JS interop library '#name' can't be imported when compiling to Wasm.""",
+        correctionMessageTemplate:
+            r"""Try using 'dart:js_interop' or 'dart:js_interop_unsafe' instead.""",
+        withArguments:
+            _withArgumentsJsInteropDisallowedInteropLibraryInDart2Wasm);
+
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const Code<Message Function(String name)>
+    codeJsInteropDisallowedInteropLibraryInDart2Wasm =
+    const Code<Message Function(String name)>(
+  "JsInteropDisallowedInteropLibraryInDart2Wasm",
+);
+
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+Message _withArgumentsJsInteropDisallowedInteropLibraryInDart2Wasm(
+    String name) {
+  if (name.isEmpty) throw 'No name provided';
+  name = demangleMixinApplicationName(name);
+  return new Message(codeJsInteropDisallowedInteropLibraryInDart2Wasm,
+      problemMessage:
+          """JS interop library '${name}' can't be imported when compiling to Wasm.""",
+      correctionMessage: """Try using 'dart:js_interop' or 'dart:js_interop_unsafe' instead.""",
+      arguments: {'name': name});
+}
+
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Code<Null> codeJsInteropEnclosingClassJSAnnotation =
    messageJsInteropEnclosingClassJSAnnotation;
@ -9329,35 +9361,6 @@ Message _withArgumentsJsInteropStaticInteropWithNonStaticSupertype(
      arguments: {'name': name, 'name2': name2});
 }

-// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
-const Template<Message Function(String name)>
-    templateJsInteropStrictModeForbiddenLibrary =
-    const Template<Message Function(String name)>(
-        "JsInteropStrictModeForbiddenLibrary",
-        problemMessageTemplate:
-            r"""Library '#name' is forbidden when strict mode is enabled.""",
-        correctionMessageTemplate:
-            r"""Remove the import of a forbidden library.""",
-        withArguments: _withArgumentsJsInteropStrictModeForbiddenLibrary);
-
-// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
-const Code<Message Function(String name)>
-    codeJsInteropStrictModeForbiddenLibrary =
-    const Code<Message Function(String name)>(
-  "JsInteropStrictModeForbiddenLibrary",
-);
-
-// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
-Message _withArgumentsJsInteropStrictModeForbiddenLibrary(String name) {
-  if (name.isEmpty) throw 'No name provided';
-  name = demangleMixinApplicationName(name);
-  return new Message(codeJsInteropStrictModeForbiddenLibrary,
-      problemMessage:
-          """Library '${name}' is forbidden when strict mode is enabled.""",
-      correctionMessage: """Remove the import of a forbidden library.""",
-      arguments: {'name': name});
-}
-
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Template<
    Message Function(String name)> templateLabelNotFound = const Template<
--- a/pkg/_js_interop_checks/lib/js_interop_checks.dart
+++ b/pkg/_js_interop_checks/lib/js_interop_checks.dart
@ -27,6 +27,7 @@ import 'package:_fe_analyzer_shared/src/messages/codes.dart'
        messageJsInteropStaticInteropParameterInitializersAreIgnored,
        messageJsInteropStaticInteropSyntheticConstructor,
        templateJsInteropDartClassExtendsJSClass,
+        templateJsInteropDisallowedInteropLibraryInDart2Wasm,
        templateJsInteropJSClassExtendsDartClass,
        templateJsInteropNonStaticWithStaticInteropSupertype,
        templateJsInteropStaticInteropNoJSAnnotation,
@ -36,8 +37,7 @@ import 'package:_fe_analyzer_shared/src/messages/codes.dart'
        templateJsInteropNativeClassInAnnotation,
        templateJsInteropStaticInteropTearOffsDisallowed,
        templateJsInteropStaticInteropTrustTypesUsageNotAllowed,
-        templateJsInteropStaticInteropTrustTypesUsedWithoutStaticInterop,
-        templateJsInteropStrictModeForbiddenLibrary;
+        templateJsInteropStaticInteropTrustTypesUsedWithoutStaticInterop;
 import 'package:_fe_analyzer_shared/src/messages/severity.dart' show Severity;
 import 'package:_js_interop_checks/src/transformations/export_checker.dart';
 import 'package:_js_interop_checks/src/transformations/js_util_optimizer.dart';
@ -71,7 +71,6 @@ class JsInteropChecks extends RecursiveVisitor {
  bool _classHasJSAnnotation = false;
  bool _classHasAnonymousAnnotation = false;
  bool _classHasStaticInteropAnnotation = false;
-  final _checkDisallowedInterop = false;
  bool _inTearoff = false;
  bool _libraryHasDartJSInteropAnnotation = false;
  bool _libraryHasJSAnnotation = false;
@ -92,11 +91,44 @@ class JsInteropChecks extends RecursiveVisitor {
    RegExp(r'(?<!generated_)tests/lib/js'),
  ];

-  /// Libraries that cannot be used when [_enforceStrictMode] is true.
-  static const _disallowedLibrariesInStrictMode = [
+  static final List<Pattern>
+      _allowedUseOfDart2WasmDisallowedInteropLibrariesTestPatterns = [
+    // Benchmarks.
+    RegExp(r'BigIntParsePrint/dart/native_version_javascript.dart'),
+    RegExp(r'JSInterop/dart/jsinterop_lib.dart'),
+    // Tests.
+    RegExp(r'(?<!generated_)tests/lib/js/export'),
+    // Negative lookahead to test the violation.
+    RegExp(
+        r'(?<!generated_)tests/lib/js/static_interop_test(?!/disallowed_interop_libraries_test.dart)'),
+    RegExp(r'(?<!generated_)tests/web/wasm'),
+    // Flutter tests.
+    RegExp(r'flutter/lib/web_ui/test'),
+  ];
+
+  // TODO(srujzs): Help migrate some of these away. Once we're done, we can
+  // remove `dart:*` interop libraries from the check as they can be moved out
+  // of `libraries.json`.
+  static const _allowedInteropLibrariesInDart2WasmPackages = [
+    // Both these packages re-export other interop libraries
+    'js',
+    'js_util',
+    // Flutter/benchmarks.
+    'flute',
+    'flutter',
+    'engine',
+    'ui',
+    // Non-SDK packages that have been migrated for the Wasm experiment but
+    // still have references to older interop libraries.
+    'package_info_plus',
+    'test',
+    'url_launcher_web',
+  ];
+
+  /// Interop libraries that cannot be used in dart2wasm.
+  static const _disallowedInteropLibrariesInDart2Wasm = [
    'package:js/js.dart',
    'package:js/js_util.dart',
-    'dart:html',
    'dart:js_util',
    'dart:js'
  ];
@ -285,10 +317,7 @@ class JsInteropChecks extends RecursiveVisitor {
        _libraryHasDartJSInteropAnnotation || hasJSInteropAnnotation(node);
    _libraryIsGlobalNamespace = _isLibraryGlobalNamespace(node);

-    // TODO(srujzs): Should we still keep around this check? Currently, it's
-    // unused since we allow the old interop on dart2wasm, but we should
-    // disallow them eventually.
-    if (_checkDisallowedInterop) _checkDisallowedLibrariesForDart2Wasm(node);
+    if (isDart2Wasm) _checkDisallowedLibrariesForDart2Wasm(node);

    super.visitLibrary(node);
    exportChecker.visitLibrary(node);
@ -504,12 +533,28 @@ class JsInteropChecks extends RecursiveVisitor {

  // JS interop library checks

+  /// Check that [node] doesn't depend on any disallowed interop libraries in
+  /// dart2wasm.
+  ///
+  /// We allowlist `dart:*` libraries, select packages, and test patterns.
  void _checkDisallowedLibrariesForDart2Wasm(Library node) {
+    final uri = node.importUri;
    for (final dependency in node.dependencies) {
      final dependencyUriString = dependency.targetLibrary.importUri.toString();
-      if (_disallowedLibrariesInStrictMode.contains(dependencyUriString)) {
+      if (_disallowedInteropLibrariesInDart2Wasm
+          .contains(dependencyUriString)) {
+        // TODO(srujzs): While we allow these imports for all `dart:*`
+        // libraries, we may want to restrict this further, as it may include
+        // `dart:ui`.
+        final allowedToImport = uri.isScheme('dart') ||
+            (uri.isScheme('package') &&
+                _allowedInteropLibrariesInDart2WasmPackages
+                    .any((pkg) => uri.pathSegments.first == pkg)) ||
+            _allowedUseOfDart2WasmDisallowedInteropLibrariesTestPatterns
+                .any((pattern) => uri.path.contains(pattern));
+        if (allowedToImport) return;
        _reporter.report(
-            templateJsInteropStrictModeForbiddenLibrary
+            templateJsInteropDisallowedInteropLibraryInDart2Wasm
                .withArguments(dependencyUriString),
            dependency.fileOffset,
            dependencyUriString.length,
@ -582,7 +627,7 @@ class JsInteropChecks extends RecursiveVisitor {
  /// a dart low level library, a foreign helper, a native test,
  /// or a from environment constructor.
  bool _isAllowedExternalUsage(Member member) {
-    Uri uri = member.enclosingLibrary.importUri;
+    final uri = member.enclosingLibrary.importUri;
    return uri.isScheme('dart') &&
            _pathsWithAllowedDartExternalUsage.contains(uri.path) ||
        _allowedNativeTestPatterns.any((pattern) => uri.path.contains(pattern));
--- a/pkg/front_end/messages.status
+++ b/pkg/front_end/messages.status
@ -601,6 +601,8 @@ JsInteropDartClassExtendsJSClass/analyzerCode: Fail # Web compiler specific
 JsInteropDartClassExtendsJSClass/example: Fail # Web compiler specific
 JsInteropDartJsInteropAnnotationForStaticInteropOnly/analyzerCode: Fail # Web compiler specific
 JsInteropDartJsInteropAnnotationForStaticInteropOnly/example: Fail # Web compiler specific
+JsInteropDisallowedInteropLibraryInDart2Wasm/analyzerCode: Fail # Web compiler specific
+JsInteropDisallowedInteropLibraryInDart2Wasm/example: Fail # Web compiler specific
 JsInteropEnclosingClassJSAnnotation/analyzerCode: Fail # Web compiler specific
 JsInteropEnclosingClassJSAnnotation/example: Fail # Web compiler specific
 JsInteropExportClassNotMarkedExportable/analyzerCode: Fail # Web compiler specific
@ -681,8 +683,6 @@ JsInteropStaticInteropWithInstanceMembers/analyzerCode: Fail # Web compiler spec
 JsInteropStaticInteropWithInstanceMembers/example: Fail # Web compiler specific
 JsInteropStaticInteropWithNonStaticSupertype/analyzerCode: Fail # Web compiler specific
 JsInteropStaticInteropWithNonStaticSupertype/example: Fail # Web compiler specific
-JsInteropStrictModeForbiddenLibrary/analyzerCode: Fail # Web compiler specific
-JsInteropStrictModeForbiddenLibrary/example: Fail # Web compiler specific
 LanguageVersionInvalidInDotPackages/analyzerCode: Fail
 LanguageVersionMismatchInPart/analyzerCode: Fail
 LanguageVersionMismatchInPart/part_wrapped_script: Fail # Part in (now) part.
--- a/pkg/front_end/messages.yaml
+++ b/pkg/front_end/messages.yaml
@ -5803,6 +5803,10 @@ JsInteropDartJsInteropAnnotationForStaticInteropOnly:
  problemMessage: "The '@JS' annotation from 'dart:js_interop' can only be used for static interop, either through extension types or '@staticInterop' classes."
  correctionMessage: "Try making this class an extension type or marking it as '@staticInterop'."

+JsInteropDisallowedInteropLibraryInDart2Wasm:
+  problemMessage: "JS interop library '#name' can't be imported when compiling to Wasm."
+  correctionMessage: "Try using 'dart:js_interop' or 'dart:js_interop_unsafe' instead."
+
 JsInteropEnclosingClassJSAnnotation:
  problemMessage: "Member has a JS interop annotation but the enclosing class does not."
  correctionMessage: "Try adding the annotation to the enclosing class."
@ -5968,10 +5972,6 @@ JsInteropStaticInteropWithNonStaticSupertype:
  problemMessage: "JS interop class '#name' has an `@staticInterop` annotation, but has supertype '#name2', which does not."
  correctionMessage: "Try marking the supertype as a static interop class using `@staticInterop`."

-JsInteropStrictModeForbiddenLibrary:
-  problemMessage: "Library '#name' is forbidden when strict mode is enabled."
-  correctionMessage: "Remove the import of a forbidden library."
-
 NonNullableInNullAware:
  problemMessage: "Operand of null-aware operation '#name' has type '#type' which excludes null."
  severity: WARNING
--- a/pkg/front_end/test/spell_checking_list_messages.txt
+++ b/pkg/front_end/test/spell_checking_list_messages.txt
@ -42,6 +42,7 @@ dart:_internal
 dart:ffi
 dart:html
 dart:js_interop
+dart:js_interop_unsafe
 dart_runner
 dartbug.com
 defaultasset
--- a/tests/lib/js/static_interop_test/disallowed_interop_libraries_test.dart
+++ b/tests/lib/js/static_interop_test/disallowed_interop_libraries_test.dart
@ -0,0 +1,23 @@
+// Copyright (c) 2023, 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.
+
+// Check that dart2wasm disallows these interop libraries from being imported.
+
+/**/ import 'dart:js';
+//   ^
+// [web] JS interop library 'dart:js' can't be imported when compiling to Wasm.
+
+/**/ import 'dart:js_util';
+//   ^
+// [web] JS interop library 'dart:js_util' can't be imported when compiling to Wasm.
+
+/**/ import 'package:js/js.dart';
+//   ^
+// [web] JS interop library 'package:js/js.dart' can't be imported when compiling to Wasm.
+
+/**/ import 'package:js/js_util.dart';
+//   ^
+// [web] JS interop library 'package:js/js_util.dart' can't be imported when compiling to Wasm.
+
+void main() {}
--- a/tests/lib/lib.status
+++ b/tests/lib/lib.status
@ -21,6 +21,9 @@ js/static_interop_test/top_level_member_annotation_static_test: SkipByDesign # S
 mirrors/*: SkipByDesign
 web/*: SkipByDesign

+[ $compiler != dart2wasm ]
+js/static_interop_test/disallowed_interop_libraries_test: SkipByDesign # Only checks on dart2wasm.
+
 [ $mode == product ]
 developer/timeline_test: Skip # Not supported
 isolate/issue_24243_parent_isolate_test: Skip # Requires checked mode