From 911f3a8012004c33cd54800c80da27412c94d0da Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Tue, 28 Jun 2022 18:36:02 +0000 Subject: [PATCH] [pkg:js] Handle mutually recursive libraries Related issue: #49301 We have an invariant to make sure stubs aren't created across other libraries but really the invariant should test for other components, as it's valid to have two libraries call each other's factories. Similarly, stubs need to be added to the class before we clone the body, as the body may mutually recurse. Tests are changed to reflect this. Change-Id: I529b682f107a55791a52fa79f082cc8ef5fce1f3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249546 Reviewed-by: Riley Porter --- .../static_interop_class_eraser.dart | 34 +++++++++++-------- .../factory_stub_lib.dart | 20 +++++++++++ .../factory_stub_test.dart | 21 ++++-------- 3 files changed, 47 insertions(+), 28 deletions(-) create mode 100644 tests/web/native/static_interop_erasure/factory_stub_lib.dart diff --git a/pkg/_js_interop_checks/lib/src/transformations/static_interop_class_eraser.dart b/pkg/_js_interop_checks/lib/src/transformations/static_interop_class_eraser.dart index c98694ee831..163fe49d662 100644 --- a/pkg/_js_interop_checks/lib/src/transformations/static_interop_class_eraser.dart +++ b/pkg/_js_interop_checks/lib/src/transformations/static_interop_class_eraser.dart @@ -28,7 +28,7 @@ class StaticInteropClassEraser extends Transformer { final Class _javaScriptObject; final CloneVisitorNotMembers _cloner = CloneVisitorNotMembers(); late final _TypeSubstitutor _typeSubstitutor; - late Library currLibrary; + Component? currentComponent; StaticInteropClassEraser(CoreTypes coreTypes, {String libraryForJavaScriptObject = 'dart:_interceptors', @@ -54,21 +54,27 @@ class StaticInteropClassEraser extends Transformer { var stubs = factoryClass.procedures .where((procedure) => procedure.name.text == stubName); if (stubs.isEmpty) { - // We should only create the stub if we're processing the library in which - // the stub should exist. Any static invocation of the factory that - // doesn't exist in the same library as the factory should be processed - // after the library in which the factory exists. In modular compilation, - // the outline of that library should already contain the needed stub. - assert(factoryClass.enclosingLibrary == currLibrary); - // Note that the return type of the cloned function is transformed. - var functionNode = super - .visitFunctionNode(_cloner.cloneInContext(factoryTarget.function)) - as FunctionNode; + // We should only create the stub if we're processing the component in + // which the stub should exist. Any static invocation of the factory that + // doesn't exist in the same component as the factory should be processed + // after the component in which the factory exists. In modular + // compilation, the outline of that component should already contain the + // needed stub. + if (currentComponent != null) { + assert(factoryTarget.enclosingComponent == currentComponent); + } var staticMethod = Procedure( - Name(stubName), ProcedureKind.Method, functionNode, + Name(stubName), ProcedureKind.Method, FunctionNode(null), isStatic: true, fileUri: factoryTarget.fileUri) ..fileOffset = factoryTarget.fileOffset; factoryClass.addProcedure(staticMethod); + // Clone function node after processing the stub in case of mutually + // recursive factories. Note that the return type of the cloned function + // is transformed. + var functionNode = super + .visitFunctionNode(_cloner.cloneInContext(factoryTarget.function)) + as FunctionNode; + staticMethod.function = functionNode; return staticMethod; } else { assert(stubs.length == 1); @@ -78,7 +84,7 @@ class StaticInteropClassEraser extends Transformer { @override TreeNode visitLibrary(Library node) { - currLibrary = node; + currentComponent = node.enclosingComponent; return super.visitLibrary(node); } @@ -208,7 +214,7 @@ class StaticInteropStubCreator extends RecursiveVisitor { @override void visitLibrary(Library node) { - _eraser.currLibrary = node; + _eraser.currentComponent = node.enclosingComponent; super.visitLibrary(node); } diff --git a/tests/web/native/static_interop_erasure/factory_stub_lib.dart b/tests/web/native/static_interop_erasure/factory_stub_lib.dart new file mode 100644 index 00000000000..e7e3b85b6e0 --- /dev/null +++ b/tests/web/native/static_interop_erasure/factory_stub_lib.dart @@ -0,0 +1,20 @@ +// 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. + +@JS() +library factory_stub_lib; + +import 'package:js/js.dart'; + +import 'factory_stub_test.dart'; + +@JS('NativeClass') +@staticInterop +class StaticNativeClassCopy { + external StaticNativeClassCopy(); + factory StaticNativeClassCopy.nestedFactory() { + StaticNativeClass.nestedFactory(); + return StaticNativeClassCopy(); + } +} diff --git a/tests/web/native/static_interop_erasure/factory_stub_test.dart b/tests/web/native/static_interop_erasure/factory_stub_test.dart index 71fe51681dc..b71ef8e1e38 100644 --- a/tests/web/native/static_interop_erasure/factory_stub_test.dart +++ b/tests/web/native/static_interop_erasure/factory_stub_test.dart @@ -14,6 +14,7 @@ import 'package:js/js.dart'; import '../native_testing.dart'; import '../native_testing.dart' as native_testing; +import 'factory_stub_lib.dart'; NativeClass makeNativeClass() native; @@ -34,23 +35,15 @@ class StaticNativeClass { // This and `StaticNativeClassCopy.nestedFactory` exist to ensure that we // cover the case where invocations on factories are visible before their // declarations in the AST. This will test whether we correctly create the - // stub even if we haven't visited the declaration yet. - factory StaticNativeClass.nestedFactory() { - StaticNativeClassCopy.nestedFactory(); + // stub even if we haven't visited the declaration yet. It will also test the + // case where stubs need to be added before function bodies are visited so + // that mutually recursive factories can resolve. + factory StaticNativeClass.nestedFactory({bool callCopyFactory = false}) { + if (callCopyFactory) StaticNativeClassCopy.nestedFactory(); return StaticNativeClass(); } } -@JS('NativeClass') -@staticInterop -class StaticNativeClassCopy { - external StaticNativeClassCopy(); - factory StaticNativeClassCopy.nestedFactory() { - StaticNativeClass.simpleFactory(); - return StaticNativeClassCopy(); - } -} - void main() { nativeTesting(); native_testing.JS('', r''' @@ -69,5 +62,5 @@ void main() { StaticNativeClass staticNativeClass = StaticNativeClass.redirectingFactory(); staticNativeClass = StaticNativeClass.simpleFactory(); staticNativeClass = StaticNativeClass.factoryWithParam(staticNativeClass); - staticNativeClass = StaticNativeClass.nestedFactory(); + staticNativeClass = StaticNativeClass.nestedFactory(callCopyFactory: true); }