[pkg:js] Enable modular compilation for exports

Requires making caches use references instead of TreeNodes, and
classes are revisited if needed due to deserializing another module.
Extensions are stored in a library to extensions map so they can be
invalidated easier. Modular tests are added.

Change-Id: Ic33e1190f02f201591616d988de6cc6c8ddad89d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/263540
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Riley Porter <rileyporter@google.com>

pkg/_js_interop_checks/lib/js_interop_checks.dart

@@ -233,6 +233,7 @@ class JsInteropChecks extends RecursiveVisitor {
       _libraryIsGlobalNamespace = true;
     }
     super.visitLibrary(lib);
+    exportChecker.visitLibrary(lib);
     _libraryIsGlobalNamespace = false;
     _libraryHasJSAnnotation = false;
     _libraryExtensionsIndex = null;
@@ -375,12 +376,6 @@ class JsInteropChecks extends RecursiveVisitor {
     }
   }
 
-  @override
-  void visitExtension(Extension extension) {
-    exportChecker.visitExtension(extension);
-    super.visitExtension(extension);
-  }
-
   /// Reports an error if [functionNode] has named parameters.
   void _checkNoNamedParameters(FunctionNode functionNode) {
     // ignore: unnecessary_null_comparison

pkg/_js_interop_checks/lib/src/js_interop.dart

@@ -67,13 +67,21 @@ List<String> getNativeNames(Annotatable a) {
 /// If [a] has a `@JSExport('...')` annotation, returns the value inside the
 /// parentheses.
 ///
-/// If the class does not have a `@JSExport()` annotation, returns an empty
-/// String. Note that a value is guaranteed to exist.
+/// If there is no value or the class does not have a `@JSExport()` annotation,
+/// returns an empty String.
 String getJSExportName(Annotatable a) {
   String jsExportValue = '';
   for (var annotation in a.annotations) {
     if (_isJSExportAnnotation(annotation)) {
-      return _stringAnnotationValues(annotation)[0];
+      var jsExportValues = _stringAnnotationValues(annotation);
+      // TODO(srujzs): Theoretically, this should never be empty as there is a
+      // default empty value. However, in the modular tests, dart2js modular
+      // analysis does not see the default value, and reports this as empty in
+      // some cases. We should investigate why and fix it, but for now, we just
+      // manually provide the default value.
+      if (jsExportValues.isNotEmpty) {
+        jsExportValue = jsExportValues[0];
+      }
     }
   }
   return jsExportValue;

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

@@ -38,11 +38,14 @@ class _GetSet {
 
 class ExportChecker {
   final DiagnosticReporter<Message, LocatedMessage> _diagnosticReporter;
-  final Map<Class, Map<String, Set<Member>>> exportClassToMemberMap = {};
-  final Map<Class, _ExportStatus> exportStatus = {};
+  final Map<Reference, Map<String, Set<Member>>> exportClassToMemberMap = {};
+  final Map<Reference, _ExportStatus> exportStatus = {};
   final Class _objectClass;
-  final Map<Class, Map<String, Member>> _overrideMap = {};
-  final Map<Reference, Set<Extension>> staticInteropClassesWithExtensions = {};
+  final Map<Reference, Map<String, Member>> _overrideMap = {};
+  // Store map of libraries to @staticInterop extensions, so that we can compute
+  // the class to extension map later. Prefer to do it this way so that modular
+  // compilation can invalidate recompiled extensions.
+  static final Map<Reference, Set<Extension>> libraryExtensionMap = {};
 
   ExportChecker(this._diagnosticReporter, this._objectClass);
 
@@ -84,12 +87,12 @@ class ExportChecker {
   /// Note that we use a map from the unique name (with setter renaming) to
   /// avoid duplicate checks on classes, and to store the overrides.
   void _collectOverrides(Class cls) {
-    if (_overrideMap.containsKey(cls)) return;
+    if (_overrideMap.containsKey(cls.reference)) return;
     Map<String, Member> memberMap;
     var superclass = cls.superclass;
     if (superclass != null && superclass != _objectClass) {
       _collectOverrides(superclass);
-      memberMap = Map.from(_overrideMap[superclass]!);
+      memberMap = Map.from(_overrideMap[superclass.reference]!);
     } else {
       memberMap = {};
     }
@@ -109,7 +112,7 @@ class ExportChecker {
         memberMap[memberName] = member;
       }
     }
-    _overrideMap[cls] = memberMap;
+    _overrideMap[cls.reference] = memberMap;
   }
 
   /// Determine if [cls] is exportable, and if so, compute the export members.
@@ -127,8 +130,9 @@ class ExportChecker {
     // If the class doesn't have the annotation or if the class wasn't marked
     // when we visited the members and checked their annotations, there's
     // nothing to do for this class.
-    if (!classHasJSExport && exportStatus[cls] != _ExportStatus.EXPORTABLE) {
-      exportStatus[cls] = _ExportStatus.NON_EXPORTABLE;
+    if (!classHasJSExport &&
+        exportStatus[cls.reference] != _ExportStatus.EXPORTABLE) {
+      exportStatus[cls.reference] = _ExportStatus.NON_EXPORTABLE;
       return;
     }
 
@@ -139,17 +143,18 @@ class ExportChecker {
           cls.fileOffset,
           cls.name.length,
           cls.location?.file);
-      exportStatus[cls] = _ExportStatus.EXPORT_ERROR;
+      exportStatus[cls.reference] = _ExportStatus.EXPORT_ERROR;
     }
 
     _collectOverrides(cls);
 
-    var allExportableMembers = _overrideMap[cls]!.values.where((member) =>
-        // Only members that qualify are those that are exportable, and either
-        // their class has the annotation or they have it themselves.
-        member.exportable &&
-        (js_interop.hasJSExportAnnotation(member) ||
-            js_interop.hasJSExportAnnotation(member.enclosingClass!)));
+    var allExportableMembers = _overrideMap[cls.reference]!.values.where(
+        (member) =>
+            // Only members that qualify are those that are exportable, and
+            // either their class has the annotation or they have it themselves.
+            member.exportable &&
+            (js_interop.hasJSExportAnnotation(member) ||
+                js_interop.hasJSExportAnnotation(member.enclosingClass!)));
     var exports = <String, Set<Member>>{};
 
     // Store the exportable members.
@@ -199,7 +204,7 @@ class ExportChecker {
           cls.fileOffset,
           cls.name.length,
           cls.location?.file);
-      exportStatus[cls] = _ExportStatus.EXPORT_ERROR;
+      exportStatus[cls.reference] = _ExportStatus.EXPORT_ERROR;
     }
 
     if (exports.isEmpty) {
@@ -208,11 +213,11 @@ class ExportChecker {
           cls.fileOffset,
           cls.name.length,
           cls.location?.file);
-      exportStatus[cls] = _ExportStatus.EXPORT_ERROR;
+      exportStatus[cls.reference] = _ExportStatus.EXPORT_ERROR;
     }
 
-    exportClassToMemberMap[cls] = exports;
-    exportStatus[cls] ??= _ExportStatus.EXPORTABLE;
+    exportClassToMemberMap[cls.reference] = exports;
+    exportStatus[cls.reference] ??= _ExportStatus.EXPORTABLE;
   }
 
   /// Check that the [member] can be exportable if it has an annotation, and if
@@ -228,23 +233,26 @@ class ExportChecker {
             member.fileOffset,
             member.name.text.length,
             member.location?.file);
-        if (cls != null) exportStatus[cls] = _ExportStatus.EXPORT_ERROR;
+        if (cls != null) {
+          exportStatus[cls.reference] = _ExportStatus.EXPORT_ERROR;
+        }
       } else {
         // Mark as exportable so we know that the class has an exportable member
         // when we process the class later.
-        if (cls != null) exportStatus[cls] = _ExportStatus.EXPORTABLE;
+        if (cls != null) exportStatus[cls.reference] = _ExportStatus.EXPORTABLE;
       }
     }
   }
 
-  /// Store the [extension] if the on-type is a `@staticInterop` class.
-  void visitExtension(Extension extension) {
-    var onType = extension.onType;
-    if (onType is InterfaceType &&
-        js_interop.hasStaticInteropAnnotation(onType.classNode)) {
-      staticInteropClassesWithExtensions
-          .putIfAbsent(onType.className, () => {})
-          .add(extension);
+  void visitLibrary(Library library) {
+    for (var extension in library.extensions) {
+      var onType = extension.onType;
+      if (onType is InterfaceType &&
+          js_interop.hasStaticInteropAnnotation(onType.classNode)) {
+        libraryExtensionMap
+            .putIfAbsent(library.reference, () => {})
+            .add(extension);
+      }
     }
   }
 }
@@ -258,6 +266,8 @@ class StaticInteropMockValidator {
   final Map<Class, Map<String, Set<ExtensionMemberDescriptor>>>
       _staticInteropExportNameToDescriptorMap = {};
   final TypeEnvironment _typeEnvironment;
+  late final Map<Reference, Set<Extension>>
+      _staticInteropClassesWithExtensions = _computeStaticInteropExtensionMap();
   StaticInteropMockValidator(
       this._diagnosticReporter, this._exportChecker, this._typeEnvironment);
 
@@ -283,9 +293,9 @@ class StaticInteropMockValidator {
   bool validateCreateStaticInteropMock(
       StaticInvocation node, Class staticInteropClass, Class dartClass) {
     var conformanceError = false;
-    var exportNameToDescriptors = _computeImplementableExtensionMembers(
-        staticInteropClass, _exportChecker.staticInteropClassesWithExtensions);
-    var exportMap = _exportChecker.exportClassToMemberMap[dartClass]!;
+    var exportNameToDescriptors =
+        _computeImplementableExtensionMembers(staticInteropClass);
+    var exportMap = _exportChecker.exportClassToMemberMap[dartClass.reference]!;
 
     for (var exportName in exportNameToDescriptors.keys) {
       var descriptors = exportNameToDescriptors[exportName]!;
@@ -445,19 +455,45 @@ class StaticInteropMockValidator {
     return true;
   }
 
+  /// Compute a mapping between all the @staticInterop classes and their
+  /// extensions.
+  ///
+  /// We do this here instead of in the export checker for two reasons:
+  /// 1. Modular compilation may invalidate extensions, so we need some way to
+  /// get rid of old extensions.
+  /// 2. The work to do this is only done when you use the
+  /// `createStaticInteropMock` API, leaving unrelated libraries alone.
+  ///
+  /// TODO(srujzs): This does not take into account any scoping. This might mean
+  /// that if another library defines an extension on the @staticInterop class
+  /// that is outside of the scope of the current library, this API will report
+  /// an error. Considering this API should primarily be used in tests, such a
+  /// compilation will be unlikely, but we should revisit this.
+  Map<Reference, Set<Extension>> _computeStaticInteropExtensionMap() {
+    // Process the stored libaries, and create a mapping between @staticInterop
+    // classes and their extensions.
+    var staticInteropClassesWithExtensions = <Reference, Set<Extension>>{};
+    for (var library in ExportChecker.libraryExtensionMap.keys) {
+      for (var extension in ExportChecker.libraryExtensionMap[library]!) {
+        var onType = extension.onType as InterfaceType;
+        staticInteropClassesWithExtensions
+            .putIfAbsent(onType.className, () => {})
+            .add(extension);
+      }
+    }
+    return staticInteropClassesWithExtensions;
+  }
+
   /// Returns a map between all the implementable external extension member
   /// names and the descriptors that have that name for [staticInteropClass].
   ///
-  /// [staticInteropClassesWithExtensions] is a map between all the
-  /// `@staticInterop` classes and their extensions.
-  ///
   /// Also computes a mapping between descriptors and their name for error
   /// reporting.
   Map<String, Set<ExtensionMemberDescriptor>>
-      _computeImplementableExtensionMembers(Class staticInteropClass,
-          Map<Reference, Set<Extension>> staticInteropClassesWithExtensions) {
+      _computeImplementableExtensionMembers(Class staticInteropClass) {
     assert(js_interop.hasStaticInteropAnnotation(staticInteropClass));
 
+    // Get the cached result if we've already processed this class.
     var exportNameToDescriptors =
         _staticInteropExportNameToDescriptorMap[staticInteropClass];
     if (exportNameToDescriptors != null) {
@@ -471,7 +507,7 @@ class StaticInteropMockValidator {
     // the supertypes.
     void getAllDescriptors(Class cls) {
       if (classes.add(cls)) {
-        var extensions = staticInteropClassesWithExtensions[cls.reference];
+        var extensions = _staticInteropClassesWithExtensions[cls.reference];
         if (extensions != null) {
           for (var extension in extensions) {
             for (var descriptor in extension.members) {
@@ -611,7 +647,16 @@ class StaticInteropMockCreator extends Transformer {
           node.location?.file);
       return false;
     }
-    var exportStatus = _exportChecker.exportStatus[dartClass]!;
+    if (!_exportChecker.exportStatus.containsKey(dartClass.reference)) {
+      // This occurs when we deserialize previously compiled modules. Those
+      // modules may contain export classes, so we need to revisit the classes
+      // in those previously compiled modules if they are used.
+      dartClass.procedures
+          .forEach((member) => _exportChecker.visitMember(member));
+      dartClass.fields.forEach((member) => _exportChecker.visitMember(member));
+      _exportChecker.visitClass(dartClass);
+    }
+    var exportStatus = _exportChecker.exportStatus[dartClass.reference];
     if (exportStatus == _ExportStatus.NON_EXPORTABLE) {
       _diagnosticReporter.report(
           templateJsInteropExportClassNotMarkedExportable
@@ -641,7 +686,8 @@ class StaticInteropMockCreator extends Transformer {
   /// and returns it.
   TreeNode _createExport(StaticInvocation node, InterfaceType dartType,
       [DartType? returnType, Expression? proto]) {
-    var exportMap = _exportChecker.exportClassToMemberMap[dartType.classNode]!;
+    var exportMap =
+        _exportChecker.exportClassToMemberMap[dartType.classNode.reference]!;
 
     var block = <Statement>[];
     returnType ??= _typeEnvironment.coreTypes.objectNonNullableRawType;

tests/modular/export_and_mock/export.dart

@@ -0,0 +1,24 @@
+// Copyright (c) 2022, 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.
+
+library export;
+
+import 'package:js/js.dart';
+
+import 'mock.dart';
+
+@JSExport()
+class DartClass {
+  String field = '';
+  final String finalField = '';
+  String get getSet => '';
+  set getSet(String val) {}
+  @JSExport('method')
+  String renamedMethod() => '';
+}
+
+extension on StaticInterop {
+  // Different implementation for same field.
+  external bool field;
+}

tests/modular/export_and_mock/main.dart

@@ -0,0 +1,13 @@
+// Copyright (c) 2022, 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.
+
+import 'dart:js_util';
+
+import 'export.dart';
+import 'mock.dart';
+
+void main() {
+  createDartExport<DartClass>(DartClass());
+  createStaticInteropMock<StaticInterop, DartClass>(DartClass());
+}

tests/modular/export_and_mock/mock.dart

@@ -0,0 +1,19 @@
+// Copyright (c) 2022, 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.
+
+library mock;
+
+import 'package:js/js.dart';
+
+@JS()
+@staticInterop
+class StaticInterop {}
+
+extension on StaticInterop {
+  external String field;
+  external final String finalField;
+  external String get getSet;
+  external set getSet(String val);
+  external String method();
+}

tests/modular/export_and_mock/modules.yaml

@@ -0,0 +1,13 @@
+# Copyright (c) 2022, 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.
+#
+# Test that modular compilation still works with `createDartExport` and
+# `createStaticInteropMock`.
+dependencies:
+  main: [export, mock]
+  export: [js, mock]
+  mock: js
+  js: meta
+packages:
+  js: ../../../pkg/js/lib