From 6980856181e8937e9137cbb922b59b52bd118be6 Mon Sep 17 00:00:00 2001 From: "johnniwinther@google.com" Date: Wed, 15 Oct 2014 10:30:07 +0000 Subject: [PATCH] Remove ResolutionEnqueuer.isLive BUG= R=floitsch@google.com Review URL: https://codereview.chromium.org//654903002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@41119 260f80e4-7a28-3924-810f-c04153c831b5 --- .../compiler/implementation/compiler.dart | 2 +- .../implementation/dart_backend/backend.dart | 2 +- .../implementation/deferred_load.dart | 2 +- .../compiler/implementation/enqueue.dart | 30 +++---- .../implementation/js_backend/backend.dart | 26 +++--- .../js_emitter/code_emitter_task.dart | 8 +- .../js_emitter/old_emitter/class_emitter.dart | 2 +- .../js_emitter/old_emitter/emitter.dart | 2 +- .../old_emitter/interceptor_emitter.dart | 3 +- .../js_emitter/old_emitter/nsm_emitter.dart | 2 +- .../implementation/resolution/members.dart | 3 + .../implementation/universe/universe.dart | 4 +- .../compiler/implementation/world.dart | 4 +- .../dart2js/analyze_unused_dart2js_test.dart | 2 +- .../compiler/dart2js/cpa_inference_test.dart | 5 +- .../dart2js/instantiated_classes_test.dart | 85 +++++++++++++++++++ 16 files changed, 133 insertions(+), 49 deletions(-) create mode 100644 tests/compiler/dart2js/instantiated_classes_test.dart diff --git a/sdk/lib/_internal/compiler/implementation/compiler.dart b/sdk/lib/_internal/compiler/implementation/compiler.dart index 9ae78138649..41fd4a50590 100644 --- a/sdk/lib/_internal/compiler/implementation/compiler.dart +++ b/sdk/lib/_internal/compiler/implementation/compiler.dart @@ -1885,7 +1885,7 @@ abstract class Compiler implements DiagnosticListener { void reportUnusedCode() { void checkLive(member) { if (member.isFunction) { - if (!enqueuer.resolution.isLive(member)) { + if (!enqueuer.resolution.hasBeenResolved(member)) { reportHint(member, MessageKind.UNUSED_METHOD, {'name': member.name}); } diff --git a/sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart b/sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart index e979a2b245e..3479b499f02 100644 --- a/sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart +++ b/sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart @@ -221,7 +221,7 @@ class DartBackend extends Backend { String assembledCode = outputter.assembleProgram( libraries: compiler.libraryLoader.libraries, - instantiatedClasses: compiler.resolverWorld.instantiatedClasses, + instantiatedClasses: compiler.resolverWorld.directlyInstantiatedClasses, resolvedElements: compiler.enqueuer.resolution.resolvedElements, usedTypeLiterals: usedTypeLiterals, postProcessElementAst: postProcessElementAst, diff --git a/sdk/lib/_internal/compiler/implementation/deferred_load.dart b/sdk/lib/_internal/compiler/implementation/deferred_load.dart index ec9edd1722f..ae262b7764f 100644 --- a/sdk/lib/_internal/compiler/implementation/deferred_load.dart +++ b/sdk/lib/_internal/compiler/implementation/deferred_load.dart @@ -296,7 +296,7 @@ class DeferredLoadTask extends CompilerTask { // to. Static members are not relevant, unless we are processing // extra dependencies due to mirrors. void addLiveInstanceMember(Element element) { - if (!compiler.enqueuer.resolution.isLive(element)) return; + if (!compiler.enqueuer.resolution.hasBeenResolved(element)) return; if (!isMirrorUsage && !element.isInstanceMember) return; collectDependencies(element.implementation); } diff --git a/sdk/lib/_internal/compiler/implementation/enqueue.dart b/sdk/lib/_internal/compiler/implementation/enqueue.dart index 8c101a4ed2a..a00c365e2d7 100644 --- a/sdk/lib/_internal/compiler/implementation/enqueue.dart +++ b/sdk/lib/_internal/compiler/implementation/enqueue.dart @@ -40,7 +40,7 @@ abstract class Enqueuer { = new Map>(); final Map> instanceFunctionsByName = new Map>(); - final Set _seenClasses = new Set(); + final Set _processedClasses = new Set(); Set recentClasses = new Setlet(); final Universe universe = new Universe(); @@ -91,7 +91,7 @@ abstract class Enqueuer { registry.registerDependency(cls); cls.ensureResolved(compiler); universe.registerTypeInstantiation(type, byMirrors: mirrorUsage); - onRegisterInstantiatedClass(cls); + processInstantiatedClass(cls); }); } @@ -105,7 +105,7 @@ abstract class Enqueuer { return filter.checkNoEnqueuedInvokedInstanceMethods(this); } - void processInstantiatedClass(ClassElement cls) { + void processInstantiatedClassMembers(ClassElement cls) { cls.implementation.forEachMember(processInstantiatedClassMember); } @@ -214,17 +214,17 @@ abstract class Enqueuer { void enableNoSuchMethod(Element element) {} void enableIsolateSupport() {} - void onRegisterInstantiatedClass(ClassElement cls) { + void processInstantiatedClass(ClassElement cls) { task.measure(() { - if (_seenClasses.contains(cls)) return; + if (_processedClasses.contains(cls)) return; // The class must be resolved to compute the set of all // supertypes. cls.ensureResolved(compiler); void processClass(ClassElement cls) { - if (_seenClasses.contains(cls)) return; + if (_processedClasses.contains(cls)) return; - _seenClasses.add(cls); + _processedClasses.add(cls); recentClasses.add(cls); cls.ensureResolved(compiler); cls.implementation.forEachMember(processInstantiatedClassMember); @@ -410,7 +410,7 @@ abstract class Enqueuer { // as recently seen, as we do not know how many rounds of resolution might // have run before tree shaking is disabled and thus everything is // enqueued. - recents = _seenClasses.toSet(); + recents = _processedClasses.toSet(); compiler.log('Enqueuing everything'); for (LibraryElement lib in compiler.libraryLoader.libraries) { enqueueReflectiveElementsInLibrary(lib, recents); @@ -639,7 +639,7 @@ abstract class Enqueuer { void forgetElement(Element element) { universe.forgetElement(element, compiler); - _seenClasses.remove(element); + _processedClasses.remove(element); } } @@ -682,13 +682,6 @@ class ResolutionEnqueuer extends Enqueuer { resolvedElements.add(element); } - /// Returns [:true:] if [element] has actually been used. - bool isLive(Element element) { - if (_seenClasses.contains(element)) return true; - if (hasBeenResolved(element)) return true; - return false; - } - /** * Decides whether an element should be included to satisfy requirements * of the mirror system. @@ -888,11 +881,12 @@ class QueueFilter { bool checkNoEnqueuedInvokedInstanceMethods(Enqueuer enqueuer) { enqueuer.task.measure(() { // Run through the classes and see if we need to compile methods. - for (ClassElement classElement in enqueuer.universe.instantiatedClasses) { + for (ClassElement classElement in + enqueuer.universe.directlyInstantiatedClasses) { for (ClassElement currentClass = classElement; currentClass != null; currentClass = currentClass.superclass) { - enqueuer.processInstantiatedClass(currentClass); + enqueuer.processInstantiatedClassMembers(currentClass); } } }); diff --git a/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart b/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart index b3dc856472e..6e567f577e8 100644 --- a/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart +++ b/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart @@ -1929,36 +1929,36 @@ class JavaScriptBackend extends Backend { bool foundClosure = false; Set reflectableMembers = new Set(); ResolutionEnqueuer resolution = compiler.enqueuer.resolution; - for (ClassElement cls in resolution.universe.instantiatedClasses) { + for (ClassElement cls in resolution.universe.directlyInstantiatedClasses) { // Do not process internal classes. if (cls.library.isInternalLibrary || cls.isInjected) continue; if (referencedFromMirrorSystem(cls)) { Set memberNames = new Set(); - // 1) the class (should be live) - assert(invariant(cls, resolution.isLive(cls))); + // 1) the class (should be resolved) + assert(invariant(cls, cls.isResolved)); reflectableMembers.add(cls); - // 2) its constructors (if live) + // 2) its constructors (if resolved) cls.constructors.forEach((Element constructor) { - if (resolution.isLive(constructor)) { + if (resolution.hasBeenResolved(constructor)) { reflectableMembers.add(constructor); } }); - // 3) all members, including fields via getter/setters (if live) + // 3) all members, including fields via getter/setters (if resolved) cls.forEachClassMember((Member member) { - if (resolution.isLive(member.element)) { + if (resolution.hasBeenResolved(member.element)) { memberNames.add(member.name); reflectableMembers.add(member.element); } }); - // 4) all overriding members of subclasses/subtypes (should be live) + // 4) all overriding members of subclasses/subtypes (should be resolved) if (compiler.world.hasAnySubtype(cls)) { for (ClassElement subcls in compiler.world.subtypesOf(cls)) { subcls.forEachClassMember((Member member) { if (memberNames.contains(member.name)) { // TODO(20993): find out why this assertion fails. // assert(invariant(member.element, - // resolution.isLive(member.element))); - if (resolution.isLive(member.element)) { + // resolution.hasBeenResolved(member.element))); + if (resolution.hasBeenResolved(member.element)) { reflectableMembers.add(member.element); } } @@ -1974,13 +1974,13 @@ class JavaScriptBackend extends Backend { } else { // check members themselves cls.constructors.forEach((ConstructorElement element) { - if (!compiler.enqueuer.resolution.isLive(element)) return; + if (!resolution.hasBeenResolved(element)) return; if (referencedFromMirrorSystem(element, false)) { reflectableMembers.add(element); } }); cls.forEachClassMember((Member member) { - if (!compiler.enqueuer.resolution.isLive(member.element)) return; + if (!resolution.hasBeenResolved(member.element)) return; if (referencedFromMirrorSystem(member.element, false)) { reflectableMembers.add(member.element); } @@ -2005,7 +2005,7 @@ class JavaScriptBackend extends Backend { if (lib.isInternalLibrary) continue; lib.forEachLocalMember((Element member) { if (!member.isClass && - compiler.enqueuer.resolution.isLive(member) && + resolution.hasBeenResolved(member) && referencedFromMirrorSystem(member)) { reflectableMembers.add(member); } diff --git a/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart b/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart index 874afa9d64d..a1bcf0553af 100644 --- a/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart +++ b/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart @@ -171,8 +171,8 @@ class CodeEmitterTask extends CompilerTask { // TODO(sigurdm): We should track those constants. constantUnit = compiler.deferredLoadTask.mainOutputUnit; } - outputConstantLists.putIfAbsent(constantUnit, () => new List()) - .add(constant); + outputConstantLists.putIfAbsent( + constantUnit, () => new List()).add(constant); } } @@ -186,8 +186,8 @@ class CodeEmitterTask extends CompilerTask { // Compute needed classes. Set instantiatedClasses = - compiler.codegenWorld.instantiatedClasses.where(computeClassFilter()) - .toSet(); + compiler.codegenWorld.directlyInstantiatedClasses + .where(computeClassFilter()).toSet(); void addClassWithSuperclasses(ClassElement cls) { neededClasses.add(cls); diff --git a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_emitter.dart b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_emitter.dart index f7c8a290017..dbd36111481 100644 --- a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_emitter.dart +++ b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/class_emitter.dart @@ -393,7 +393,7 @@ class ClassEmitter extends CodeEmitterHelper { // If the class is never instantiated we still need to set it up for // inheritance purposes, but we can simplify its JavaScript constructor. bool isInstantiated = - compiler.codegenWorld.instantiatedClasses.contains(element); + compiler.codegenWorld.directlyInstantiatedClasses.contains(element); void visitField(Element holder, VariableElement field) { assert(invariant(element, field.isDeclaration)); diff --git a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart index f98d119d6af..b7b5e736e51 100644 --- a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart +++ b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/emitter.dart @@ -1721,7 +1721,7 @@ class OldEmitter implements Emitter { // native elements. ClassElement cls = element.enclosingClassOrCompilationUnit.declaration; - if (compiler.codegenWorld.instantiatedClasses.contains(cls) + if (compiler.codegenWorld.directlyInstantiatedClasses.contains(cls) && !cls.isNative) { owner = cls; } diff --git a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/interceptor_emitter.dart b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/interceptor_emitter.dart index f643acfc640..e9370c76ffe 100644 --- a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/interceptor_emitter.dart +++ b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/interceptor_emitter.dart @@ -151,7 +151,8 @@ class InterceptorEmitter extends CodeEmitterHelper { } else { ClassElement jsUnknown = backend.jsUnknownJavaScriptObjectClass; - if (compiler.codegenWorld.instantiatedClasses.contains(jsUnknown)) { + if (compiler.codegenWorld + .directlyInstantiatedClasses.contains(jsUnknown)) { statements.add( js.statement('if (!(receiver instanceof #)) return #;', [namer.elementAccess(compiler.objectClass), diff --git a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/nsm_emitter.dart b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/nsm_emitter.dart index 06647e8f498..36351753178 100644 --- a/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/nsm_emitter.dart +++ b/sdk/lib/_internal/compiler/implementation/js_emitter/old_emitter/nsm_emitter.dart @@ -20,7 +20,7 @@ class NsmEmitter extends CodeEmitterHelper { void emitNoSuchMethodHandlers(AddPropertyFunction addProperty) { // Do not generate no such method handlers if there is no class. - if (compiler.codegenWorld.instantiatedClasses.isEmpty) return; + if (compiler.codegenWorld.directlyInstantiatedClasses.isEmpty) return; String noSuchMethodName = namer.publicInstanceMethodNameByArity( Compiler.NO_SUCH_METHOD, Compiler.NO_SUCH_METHOD_ARG_COUNT); diff --git a/sdk/lib/_internal/compiler/implementation/resolution/members.dart b/sdk/lib/_internal/compiler/implementation/resolution/members.dart index 75e1c980b66..4c5a6a9d91e 100644 --- a/sdk/lib/_internal/compiler/implementation/resolution/members.dart +++ b/sdk/lib/_internal/compiler/implementation/resolution/members.dart @@ -3200,6 +3200,7 @@ class ResolverVisitor extends MappingVisitor { }); registry.registerStaticUse(redirectionTarget); + // TODO(johnniwinther): Register the effective target type instead. registry.registerInstantiatedClass( redirectionTarget.enclosingClass.declaration); if (isSymbolConstructor) { @@ -3297,6 +3298,8 @@ class ResolverVisitor extends MappingVisitor { compiler.reportError(node.send.selector, MessageKind.TYPE_VARIABLE_IN_CONSTANT); } + // TODO(johniwinther): Avoid registration of `type` in face of redirecting + // factory constructors. registry.registerInstantiatedType(type); if (constructor.isFactoryConstructor && !type.typeArguments.isEmpty) { registry.registerFactoryWithTypeArguments(); diff --git a/sdk/lib/_internal/compiler/implementation/universe/universe.dart b/sdk/lib/_internal/compiler/implementation/universe/universe.dart index 6827a0dee00..e5e3915d851 100644 --- a/sdk/lib/_internal/compiler/implementation/universe/universe.dart +++ b/sdk/lib/_internal/compiler/implementation/universe/universe.dart @@ -99,7 +99,7 @@ class Universe { /// constructor that has been called directly and not only through a /// super-call. // TODO(johnniwinther): Improve semantic precision. - Iterable get instantiatedClasses { + Iterable get directlyInstantiatedClasses { return _directlyInstantiatedClasses; } @@ -113,7 +113,7 @@ class Universe { /// All directly instantiated types, that is, the types of the directly /// instantiated classes. /// - /// See [instantiatedClasses]. + /// See [directlyInstantiatedClasses]. // TODO(johnniwinther): Improve semantic precision. Iterable get instantiatedTypes => _instantiatedTypes; diff --git a/sdk/lib/_internal/compiler/implementation/world.dart b/sdk/lib/_internal/compiler/implementation/world.dart index c6246459fdd..b10e3081438 100644 --- a/sdk/lib/_internal/compiler/implementation/world.dart +++ b/sdk/lib/_internal/compiler/implementation/world.dart @@ -148,7 +148,7 @@ class World implements ClassWorld { /// Returns `true` if [cls] is instantiated. bool isInstantiated(ClassElement cls) { - return compiler.enqueuer.resolution.isLive(cls); + return compiler.resolverWorld.isInstantiated(cls); } /// Returns an iterable over the live classes that extend [cls] including @@ -385,7 +385,7 @@ class World implements ClassWorld { // classes: if the superclass of these classes require RTI, then // they also need RTI, so that a constructor passes the type // variables to the super constructor. - compiler.resolverWorld.allInstantiatedClasses.forEach(addSubtypes); + compiler.resolverWorld.directlyInstantiatedClasses.forEach(addSubtypes); } void registerMixinUse(MixinApplicationElement mixinApplication, diff --git a/tests/compiler/dart2js/analyze_unused_dart2js_test.dart b/tests/compiler/dart2js/analyze_unused_dart2js_test.dart index e7ff9c33bf8..e2867769ac2 100644 --- a/tests/compiler/dart2js/analyze_unused_dart2js_test.dart +++ b/tests/compiler/dart2js/analyze_unused_dart2js_test.dart @@ -45,7 +45,7 @@ bool checkResults(Compiler compiler, CollectingDiagnosticHandler handler) { 'sdk/lib/_internal/compiler/implementation/helpers/helpers.dart'); void checkLive(member) { if (member.isFunction) { - if (compiler.enqueuer.resolution.isLive(member)) { + if (compiler.enqueuer.resolution.hasBeenResolved(member)) { compiler.reportHint(member, MessageKind.GENERIC, {'text': "Helper function in production code '$member'."}); } diff --git a/tests/compiler/dart2js/cpa_inference_test.dart b/tests/compiler/dart2js/cpa_inference_test.dart index 7eacb8fa4a7..d636c05f1f9 100644 --- a/tests/compiler/dart2js/cpa_inference_test.dart +++ b/tests/compiler/dart2js/cpa_inference_test.dart @@ -1459,8 +1459,9 @@ testJsCall() { final expectedType = [result.bool]; result.checkNodeHasType('h', expectedType); result.checkNodeHasType('hNull', maybe(expectedType)); - final expectedIType = [result.base('A'), result.base('B'), - result.base('BB'), result.base('C'), + final expectedIType = [result.base('B'), + result.base('BB'), + result.base('C'), result.base('D')]; result.checkNodeHasType('i', expectedIType); result.checkNodeHasType('iNull', maybe(expectedIType)); diff --git a/tests/compiler/dart2js/instantiated_classes_test.dart b/tests/compiler/dart2js/instantiated_classes_test.dart new file mode 100644 index 00000000000..00fd80146ab --- /dev/null +++ b/tests/compiler/dart2js/instantiated_classes_test.dart @@ -0,0 +1,85 @@ +// Copyright (c) 2014, 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 instantiated_classes_test; + +import 'dart:async'; +import 'package:expect/expect.dart'; +import 'package:async_helper/async_helper.dart'; +import 'package:compiler/implementation/elements/elements.dart' + show ClassElement; +import 'type_test_helper.dart'; + +void main() { + asyncTest(() => Future.forEach([ + () => test("class Class {}", ["Class"]), + () => test("""abstract class A {} + class Class extends A {}""", + ["Class"]), + () => test("""class A {} + class Class extends A {}""", + ["Class"]), + () => test("""class A {} + class B {} + class Class extends A {}""", + ["Class"]), + () => test("""class A {} + class Class implements A {}""", + ["Class"]), + () => test("""class A {} + class Class extends Object with A {}""", + ["Class"]), + () => test("""class A {} + class B {} + class Class extends Object with B implements A {}""", + ["Class"]), + + () => test("""class A {} + class Class {}""", + ["Class", "A"], ["Class", "A"]), + () => test("""class A {} + class Class extends A {}""", + ["Class", "A"], ["Class", "A"]), + () => test("""class A {} + class Class implements A {}""", + ["Class", "A"], ["Class", "A"]), + () => test("""class A {} + class B extends A {} + class Class extends B {}""", + ["Class", "A"], ["Class", "A"]), + () => test("""class A {} + class B {} + class Class extends B with A {}""", + ["Class", "A"], ["Class", "A"]), + + // TODO(johnniwinther): Avoid registration of `Class` as instantiated. + () => test("""class A {} + class Class implements A { + factory Class() = A; + }""", + ["Class", "A"], ["Class"]), + ], (f) => f())); +} + +Future test(String source, List directlyInstantiatedClasses, + [List newClasses = const ["Class"]]) { + StringBuffer mainSource = new StringBuffer(); + mainSource.write('main() {\n'); + for (String newClass in newClasses) { + mainSource.write(' new $newClass();\n'); + } + mainSource.write('}'); + return TypeEnvironment.create(source, + mainSource: mainSource.toString(), + useMockCompiler: true).then((env) { + Iterable expectedClasses = + directlyInstantiatedClasses.map(env.getElement); + Iterable actualClasses = + env.compiler.resolverWorld.directlyInstantiatedClasses.where( + (c) => c.library == env.compiler.mainApp); + Expect.setEquals(expectedClasses, actualClasses); + }); +} + +