Fix deferred type literals and typedefs Closes https://github.com/dart-lang/sdk/issues/33890 Change-Id: I3a154d4de46747f3ad905de889f14188fdfff50a Reviewed-on: https://dart-review.googlesource.com/65442 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Sigmund Cherem <sigmund@google.com>
diff --git a/pkg/compiler/lib/src/deferred_load.dart b/pkg/compiler/lib/src/deferred_load.dart index 8b87744..e72eca9 100644 --- a/pkg/compiler/lib/src/deferred_load.dart +++ b/pkg/compiler/lib/src/deferred_load.dart
@@ -15,6 +15,7 @@ ConstantValue, ConstructedConstantValue, DeferredConstantValue, + TypeConstantValue, DeferredGlobalConstantValue, InstantiationConstantValue; import 'elements/types.dart'; @@ -193,6 +194,10 @@ Iterable<ImportEntity> memberImportsTo( MemberEntity element, LibraryEntity library); + /// Returns every [ImportEntity] that imports [element] into [library]. + Iterable<ImportEntity> typedefImportsTo( + TypedefEntity element, LibraryEntity library); + /// Collects all direct dependencies of [element]. /// /// The collected dependent elements and constants are are added to @@ -410,18 +415,9 @@ } } constant.getDependencies().forEach((ConstantValue dependency) { - if (dependency is DeferredConstantValue) { - /// New deferred-imports are only discovered when we are visiting the - /// main output unit (size == 0) or code reachable from a deferred - /// import (size == 1). After that, we are rediscovering the - /// same nodes we have already seen. - if (newSet.length <= 1) { - queue.addConstant( - dependency, importSets.singleton(dependency.import)); - } - } else { - _updateConstantRecursive(dependency, oldSet, newSet, queue); - } + // Constants are not allowed to refer to deferred constants, so + // no need to check for a deferred type literal here. + _updateConstantRecursive(dependency, oldSet, newSet, queue); }); } else { assert( @@ -505,16 +501,21 @@ // dependencies are already visited as dependencies of the enclosing member. } + /// Whether to enqueue a deferred dependency. + /// + /// Due to the nature of the algorithm, some dependencies may be visited more + /// than once. However, we know that new deferred-imports are only discovered + /// when we are visiting the main output unit (size == 0) or code reachable + /// from a deferred import (size == 1). After that, we are rediscovering the + /// same nodes we have already seen. + _shouldAddDeferredDependency(ImportSet newSet) => newSet.length <= 1; + void _processDependencies(LibraryEntity library, Dependencies dependencies, ImportSet oldSet, ImportSet newSet, WorkQueue queue) { for (ClassEntity cls in dependencies.classes) { Iterable<ImportEntity> imports = classImportsTo(cls, library); if (_isExplicitlyDeferred(imports)) { - /// New deferred-imports are only discovered when we are visiting the - /// main output unit (size == 0) or code reachable from a deferred - /// import (size == 1). After that, we are rediscovering the - /// same nodes we have already seen. - if (newSet.length <= 1) { + if (_shouldAddDeferredDependency(newSet)) { for (ImportEntity deferredImport in imports) { queue.addClass(cls, importSets.singleton(deferredImport)); } @@ -527,11 +528,7 @@ for (MemberEntity member in dependencies.members) { Iterable<ImportEntity> imports = memberImportsTo(member, library); if (_isExplicitlyDeferred(imports)) { - /// New deferred-imports are only discovered when we are visiting the - /// main output unit (size == 0) or code reachable from a deferred - /// import (size == 1). After that, we are rediscovering the - /// same nodes we have already seen. - if (newSet.length <= 1) { + if (_shouldAddDeferredDependency(newSet)) { for (ImportEntity deferredImport in imports) { queue.addMember(member, importSets.singleton(deferredImport)); } @@ -546,14 +543,36 @@ } for (ConstantValue dependency in dependencies.constants) { + // TODO(sigmund): either delete DeferredConstantValue or use it to + // represent deferred TypeConstantValues below. if (dependency is DeferredConstantValue) { - if (newSet.length <= 1) { + if (_shouldAddDeferredDependency(newSet)) { queue.addConstant( dependency, importSets.singleton(dependency.import)); } - } else { - _updateConstantRecursive(dependency, oldSet, newSet, queue); + continue; } + + if (dependency is TypeConstantValue) { + var type = dependency.representedType; + var imports = const <ImportEntity>[]; + if (type is InterfaceType) { + imports = classImportsTo(type.element, library); + } else if (type is TypedefType) { + imports = typedefImportsTo(type.element, library); + } + if (_isExplicitlyDeferred(imports)) { + if (_shouldAddDeferredDependency(newSet)) { + for (ImportEntity deferredImport in imports) { + queue.addConstant( + dependency, importSets.singleton(deferredImport)); + } + } + continue; + } + } + + _updateConstantRecursive(dependency, oldSet, newSet, queue); } }
diff --git a/pkg/compiler/lib/src/kernel/deferred_load.dart b/pkg/compiler/lib/src/kernel/deferred_load.dart index b312027..7b0a987 100644 --- a/pkg/compiler/lib/src/kernel/deferred_load.dart +++ b/pkg/compiler/lib/src/kernel/deferred_load.dart
@@ -48,6 +48,13 @@ } @override + Iterable<ImportEntity> typedefImportsTo( + TypedefEntity element, LibraryEntity library) { + ir.Typedef node = _elementMap.getTypedefNode(element); + return _findImportsTo(node, node.name, node.enclosingLibrary, library); + } + + @override Iterable<ImportEntity> memberImportsTo( Entity element, LibraryEntity library) { ir.Member node = _elementMap.getMemberDefinition(element).node;
diff --git a/pkg/compiler/lib/src/kernel/element_map.dart b/pkg/compiler/lib/src/kernel/element_map.dart index 7be52d1..23a83dd 100644 --- a/pkg/compiler/lib/src/kernel/element_map.dart +++ b/pkg/compiler/lib/src/kernel/element_map.dart
@@ -192,6 +192,9 @@ /// Returns the [ir.Library] corresponding to [library]. ir.Library getLibraryNode(LibraryEntity library); + /// Returns the node that defines [typedef]. + ir.Typedef getTypedefNode(covariant TypedefEntity typedef); + /// Returns the definition information for [member]. MemberDefinition getMemberDefinition(covariant MemberEntity member);
diff --git a/pkg/compiler/lib/src/kernel/element_map_impl.dart b/pkg/compiler/lib/src/kernel/element_map_impl.dart index cdbfe78..524c487 100644 --- a/pkg/compiler/lib/src/kernel/element_map_impl.dart +++ b/pkg/compiler/lib/src/kernel/element_map_impl.dart
@@ -844,6 +844,10 @@ return _classes.getData(cls).definition; } + ir.Typedef _getTypedefNode(covariant IndexedTypedef typedef) { + return _typedefs.getData(typedef).node; + } + @override ImportEntity getImport(ir.LibraryDependency node) { ir.Library library = node.parent; @@ -1439,6 +1443,11 @@ return _getClassDefinition(cls); } + @override + ir.Typedef getTypedefNode(TypedefEntity typedef) { + return _getTypedefNode(typedef); + } + /// Returns the element type of a async/sync*/async* function. @override DartType getFunctionAsyncOrSyncStarElementType(ir.FunctionNode functionNode) {
diff --git a/tests/compiler/dart2js_extra/deferred/type_literal_lib.dart b/tests/compiler/dart2js_extra/deferred/type_literal_lib.dart new file mode 100644 index 0000000..867c399 --- /dev/null +++ b/tests/compiler/dart2js_extra/deferred/type_literal_lib.dart
@@ -0,0 +1,7 @@ +// Copyright (c) 2018, 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. + +class A {} + +class B {}
diff --git a/tests/compiler/dart2js_extra/deferred/type_literal_test.dart b/tests/compiler/dart2js_extra/deferred/type_literal_test.dart new file mode 100644 index 0000000..d578d8d --- /dev/null +++ b/tests/compiler/dart2js_extra/deferred/type_literal_test.dart
@@ -0,0 +1,21 @@ +// Copyright (c) 2018, 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. + +// Regression test for Issue #33890. +// +// This fails if type literal constants are not deferred, but their RTI +// representation is. This test however will not detect if we accidentally start +// building the RTI representation in the main deferred unit (which is what was +// happening before the bug was introduced). + +import 'type_literal_lib.dart' deferred as a; +import 'package:expect/expect.dart'; + +main() async { + await a.loadLibrary(); + Expect.isFalse(confuse(a.A) == confuse(a.B)); +} + +@NoInline() +confuse(x) => x;
diff --git a/tests/compiler/dart2js_extra/deferred/typedef_lib.dart b/tests/compiler/dart2js_extra/deferred/typedef_lib.dart new file mode 100644 index 0000000..2bccab5 --- /dev/null +++ b/tests/compiler/dart2js_extra/deferred/typedef_lib.dart
@@ -0,0 +1,10 @@ +// Copyright (c) 2018, 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. + +class A {} + +class B {} + +typedef C(A a); +typedef D(B a);
diff --git a/tests/compiler/dart2js_extra/deferred/typedef_test.dart b/tests/compiler/dart2js_extra/deferred/typedef_test.dart new file mode 100644 index 0000000..1ad34a3 --- /dev/null +++ b/tests/compiler/dart2js_extra/deferred/typedef_test.dart
@@ -0,0 +1,23 @@ +// Copyright (c) 2018, 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. + +// Regression test for Issue #33890. +// +// This fails if type literal constants are not deferred, but their RTI +// representation is. This test however will not detect if we accidentally start +// building the RTI representation in the main deferred unit (which is what was +// happening before the bug was introduced). + +import 'typedef_lib.dart' deferred as a; +import 'package:expect/expect.dart'; + +main() async { + await a.loadLibrary(); + print(a.A); // unclear why without this the definition of A and B gets pulled + print(a.B); // into the main output unit. + Expect.isFalse(confuse(a.C) == confuse(a.D)); +} + +@NoInline() +confuse(x) => x;