From 6efce3b4078bc08db7a0212f256bada8bd33bb4f Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Wed, 26 Oct 2022 16:34:38 +0000 Subject: [PATCH] [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 Reviewed-by: Riley Porter --- .../lib/js_interop_checks.dart | 7 +- .../lib/src/js_interop.dart | 14 +- .../static_interop_mock_creator.dart | 128 ++++++++++++------ tests/modular/export_and_mock/export.dart | 24 ++++ tests/modular/export_and_mock/main.dart | 13 ++ tests/modular/export_and_mock/mock.dart | 19 +++ tests/modular/export_and_mock/modules.yaml | 13 ++ 7 files changed, 168 insertions(+), 50 deletions(-) create mode 100644 tests/modular/export_and_mock/export.dart create mode 100644 tests/modular/export_and_mock/main.dart create mode 100644 tests/modular/export_and_mock/mock.dart create mode 100644 tests/modular/export_and_mock/modules.yaml diff --git a/pkg/_js_interop_checks/lib/js_interop_checks.dart b/pkg/_js_interop_checks/lib/js_interop_checks.dart index 21b0a994a90..cac8b428897 100644 --- a/pkg/_js_interop_checks/lib/js_interop_checks.dart +++ b/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 diff --git a/pkg/_js_interop_checks/lib/src/js_interop.dart b/pkg/_js_interop_checks/lib/src/js_interop.dart index 4f7259f8325..434079ae723 100644 --- a/pkg/_js_interop_checks/lib/src/js_interop.dart +++ b/pkg/_js_interop_checks/lib/src/js_interop.dart @@ -67,13 +67,21 @@ List 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; diff --git a/pkg/_js_interop_checks/lib/src/transformations/static_interop_mock_creator.dart b/pkg/_js_interop_checks/lib/src/transformations/static_interop_mock_creator.dart index e292ce99eb7..a2135df4ca2 100644 --- a/pkg/_js_interop_checks/lib/src/transformations/static_interop_mock_creator.dart +++ b/pkg/_js_interop_checks/lib/src/transformations/static_interop_mock_creator.dart @@ -38,11 +38,14 @@ class _GetSet { class ExportChecker { final DiagnosticReporter _diagnosticReporter; - final Map>> exportClassToMemberMap = {}; - final Map exportStatus = {}; + final Map>> exportClassToMemberMap = {}; + final Map exportStatus = {}; final Class _objectClass; - final Map> _overrideMap = {}; - final Map> staticInteropClassesWithExtensions = {}; + final Map> _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> 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 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 = >{}; // 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>> _staticInteropExportNameToDescriptorMap = {}; final TypeEnvironment _typeEnvironment; + late final Map> + _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> _computeStaticInteropExtensionMap() { + // Process the stored libaries, and create a mapping between @staticInterop + // classes and their extensions. + var staticInteropClassesWithExtensions = >{}; + 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> - _computeImplementableExtensionMembers(Class staticInteropClass, - Map> 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 = []; returnType ??= _typeEnvironment.coreTypes.objectNonNullableRawType; diff --git a/tests/modular/export_and_mock/export.dart b/tests/modular/export_and_mock/export.dart new file mode 100644 index 00000000000..b4d2d5c39f7 --- /dev/null +++ b/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; +} diff --git a/tests/modular/export_and_mock/main.dart b/tests/modular/export_and_mock/main.dart new file mode 100644 index 00000000000..c4a49ce3b01 --- /dev/null +++ b/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()); + createStaticInteropMock(DartClass()); +} diff --git a/tests/modular/export_and_mock/mock.dart b/tests/modular/export_and_mock/mock.dart new file mode 100644 index 00000000000..2c4dfaea9ba --- /dev/null +++ b/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(); +} diff --git a/tests/modular/export_and_mock/modules.yaml b/tests/modular/export_and_mock/modules.yaml new file mode 100644 index 00000000000..f793e915cf0 --- /dev/null +++ b/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