Take inheritance into account when computing the elements accessible by mirrors.

sdk/lib/_internal/compiler/implementation/enqueue.dart

Index: sdk/lib/_internal/compiler/implementation/enqueue.dart
diff --git a/sdk/lib/_internal/compiler/implementation/enqueue.dart b/sdk/lib/_internal/compiler/implementation/enqueue.dart
index 4ce3d5c93c3f325271104ded7e08941fb0829eba..8c34277e3aa891797e14455561cc840abda14fac 100644
--- a/sdk/lib/_internal/compiler/implementation/enqueue.dart
+++ b/sdk/lib/_internal/compiler/implementation/enqueue.dart
@@ -73,13 +73,16 @@ abstract class Enqueuer {
   final Map<String, Link<Element>> instanceFunctionsByName
       = new Map<String, Link<Element>>();
   final Set<ClassElement> seenClasses = new Set<ClassElement>();
+  Set<ClassElement> recentClasses = new Setlet<ClassElement>();
   final Universe universe = new Universe();
 
+  static final TRACE_MIRROR_ENQUEUING = false;

[floitsch, 2014/06/27 08:59:15]

should we start making these things const X.fromEnvironment?

for example:
 ... = const bool.fromEnvironment("TRACE_MIRROR_ENQUEUING");

[herhut, 2014/06/27 12:34:35]

I did not know it was that easy to read an environment variable.

Done.

+
   bool queueIsClosed = false;
   EnqueueTask task;
   native.NativeEnqueuer nativeEnqueuer;  // Set by EnqueueTask
 
-  bool hasEnqueuedEverything = false;
+  bool hasEnqueuedReflectiveElements = false;
   bool hasEnqueuedReflectiveStaticFields = false;
 
   Enqueuer(this.name, this.compiler, this.itemCompilationContextCreator);
@@ -108,7 +111,8 @@ abstract class Enqueuer {
    */
   void internalAddToWorkList(Element element);
 
-  void registerInstantiatedType(InterfaceType type, Registry registry) {
+  void registerInstantiatedType(InterfaceType type, Registry registry,
+                                {mirrorUsage: false}) {

[karlklose, 2014/06/27 07:45:57]

Only use registerInstantiatedType for types that are instantiated (via new) in
the source code.

If we really need to track the classes used from the mirrors, we should have a
separate set and include the classes in this set in
CodeEmitterTask.computeNeededClasses.

[herhut, 2014/06/27 12:34:35]

This only replicated what the original implementation did. I will leave it like
that in this CL with a TODO and refactor in a follow up.

     task.measure(() {
       ClassElement cls = type.element;
       registry.registerDependency(cls);
@@ -119,16 +123,20 @@ abstract class Enqueuer {
           // classes; a native abstract class may have non-abstract subclasses
           // not declared to the program.  Instances of these classes are
           // indistinguishable from the abstract class.
-          || cls.isNative) {
+          || cls.isNative
+          // Likewise, if this registration comes from the mirror system,
+          // all bets are off.
+          || mirrorUsage) {
         universe.instantiatedClasses.add(cls);
       }
       onRegisterInstantiatedClass(cls);
     });
   }
 
-  void registerInstantiatedClass(ClassElement cls, Registry registry) {
+  void registerInstantiatedClass(ClassElement cls, Registry registry,
+                                 {mirrorUsage: false}) {
     cls.ensureResolved(compiler);
-    registerInstantiatedType(cls.rawType, registry);
+    registerInstantiatedType(cls.rawType, registry, mirrorUsage: mirrorUsage);
   }
 
   bool checkNoEnqueuedInvokedInstanceMethods() {
@@ -260,6 +268,7 @@ abstract class Enqueuer {
         if (seenClasses.contains(cls)) return;
 
         seenClasses.add(cls);
+        recentClasses.add(cls);
         cls.ensureResolved(compiler);
         cls.implementation.forEachMember(processInstantiatedClassMember);
         if (isResolutionQueue) {
@@ -308,48 +317,130 @@ abstract class Enqueuer {
     });
   }
 
-  void pretendElementWasUsed(Element element, Registry registry) {
-    if (!compiler.backend.isNeededForReflection(element)) return;
-    if (Elements.isUnresolved(element)) {
-      // Ignore.
-    } else if (element.isSynthesized
-               && element.library.isPlatformLibrary) {
-      // TODO(ahe): Work-around for http://dartbug.com/11205.
-    } else if (element.isConstructor) {
-      ClassElement cls = element.declaration.enclosingClass;
-      registerInstantiatedType(cls.rawType, registry);
-      registerStaticUse(element.declaration);
-    } else if (element.isClass) {
-      ClassElement cls = element.declaration;
-      registerInstantiatedClass(cls, registry);
-      // Make sure that even abstract classes are considered instantiated.
-      universe.instantiatedClasses.add(cls);
-    } else if (element.impliesType) {
-      // Don't enqueue typedefs, and type variables.
-    } else if (Elements.isStaticOrTopLevel(element)) {
-      registerStaticUse(element.declaration);
-    } else if (element.isInstanceMember) {
-      Selector selector = new Selector.fromElement(element, compiler);
-      registerSelectorUse(selector);
-      if (element.isField) {
-        Selector selector =
-            new Selector.setter(element.name, element.library);
-        registerInvokedSetter(selector);
+  /**
+   * Decides whether an element should be included to satisfy requirements
+   * of the mirror system.
+   *
+   * The actual implementation depends on the current compiler phase.
+   */
+  bool includeElementDueToMirrors(bool includeEnclosing, Element element);

[Johnni Winther, 2014/06/27 07:43:01]

Rename to [shouldIncludeElementDueToMirror] to indicate that this is a question
and not a command.

[Johnni Winther, 2014/06/27 07:43:01]

Make [includeEnclosing] a named parameter.

[herhut, 2014/06/27 12:34:36]

Done.

[herhut, 2014/06/27 12:34:36]

Done.

+
+  void logEnqueueReflectiveAction(action, [msg = ""]) {
+    if (TRACE_MIRROR_ENQUEUING) {
+      print("MIRROR_ENQUEUE (${isResolutionQueue ? "R" : "B"}): $action $msg");

[karlklose, 2014/06/27 07:45:57]

'R' -> 'resolution'?

What is 'B'?

[herhut, 2014/06/27 12:34:36]

Backend. Probably C for codegen is better. I have changed it.

+    }
+  }
+
+  void enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {

[Johnni Winther, 2014/06/27 07:43:00]

Add comment. I guess it is a helper method for [enqueueReflectiveElements].

[Johnni Winther, 2014/06/27 07:43:01]

Make [add] named.

[karlklose, 2014/06/27 07:45:57]

Could you add a comment about the semantics of 'add'?

[floitsch, 2014/06/27 08:59:15]

nit: s/add/forceAdd/

I'm guessing that's the meaning.

Since it's not completely clear, maybe document what "add" does.

[herhut, 2014/06/27 12:34:36]

Done.

[herhut, 2014/06/27 12:34:36]

Done.

[herhut, 2014/06/27 12:34:36]

Done.

[herhut, 2014/06/27 12:34:37]

Done.

+    if (includeElementDueToMirrors(add, ctor)) {
+      logEnqueueReflectiveAction(ctor);
+      ClassElement cls = ctor.declaration.enclosingClass;
+      registerInstantiatedType(cls.rawType, compiler.mirrorDependencies);

[Johnni Winther, 2014/06/27 07:43:01]

Shouldn't this call have `mirrorUsage: true` ?

[karlklose, 2014/06/27 07:45:57]

I am not sure we should register this type here.

[herhut, 2014/06/27 12:34:35]

It will not have an effect currently but it is more appropriate to set it to
true.

Done.

[herhut, 2014/06/27 12:34:36]

This implements what was done before. I will redo the tracking in a follow up
CL.

+      registerStaticUse(ctor.declaration);
+    }
+  }
+
+  void enqueueReflectiveMember(Element element, bool add) {

[Johnni Winther, 2014/06/27 07:43:00]

Make [add] named.

[Johnni Winther, 2014/06/27 07:43:01]

Add comment.

[herhut, 2014/06/27 12:34:35]

Done.

[herhut, 2014/06/27 12:34:36]

Done.

+    add = includeElementDueToMirrors(add, element);

[karlklose, 2014/06/27 07:45:57]

Inline call to includeElementsDueToMirrors? (Also below)

[herhut, 2014/06/27 12:34:36]

Below I have to pass the result on to the processing of members. 

Done.

+    if (add && !element.impliesType) {

[floitsch, 2014/06/27 08:59:15]

Keep comment exlpaining what "impliesType" means.
(The old code had the comment "Don't enqueue typedefs, and type variables")

Btw: is this true? When I iterate over a library I would like to see the
typedef.

[herhut, 2014/06/27 12:34:36]

Done.

+      logEnqueueReflectiveAction(element);
+      if (Elements.isStaticOrTopLevel(element)) {
+        registerStaticUse(element.declaration);
+      } else if (element.isInstanceMember) {
+        // We need to enqueue all members matching this one in subclasses, as
+        // well.
+        // TODO(herhut): Use TypedSelector.subtype for enqueueing
+        Selector selector = new Selector.fromElement(element, compiler);
+        registerSelectorUse(selector);
+        if (element.isField) {
+          Selector selector =
+              new Selector.setter(element.name, element.library);
+          registerInvokedSetter(selector);
+        }
       }
     }
   }
 
-  void enqueueEverything() {
-    if (hasEnqueuedEverything) return;
-    compiler.log('Enqueuing everything');
-    task.ensureAllElementsByName();
-    for (Link link in task.allElementsByName.values) {
-      for (Element element in link) {
-        pretendElementWasUsed(element, compiler.mirrorDependencies);
+  void enqueueReflectiveElementsInClass(ClassElement cls,

[Johnni Winther, 2014/06/27 07:43:00]

Add comment.

[herhut, 2014/06/27 12:34:36]

Done.

+                                        Iterable<ClassElement> recents,
+                                        bool add) {

[Johnni Winther, 2014/06/27 07:43:00]

Make [add] named.

[herhut, 2014/06/27 12:34:37]

Done.

+    if (cls.library.isInternalLibrary) return;
+    if (cls.isInjected) return;

[Johnni Winther, 2014/06/27 07:43:00]

Merge conditions.

[herhut, 2014/06/27 12:34:36]

Done.

+    add = includeElementDueToMirrors(add, cls);
+    if (add) {
+      logEnqueueReflectiveAction(cls, "register");
+      ClassElement decl = cls.declaration;
+      registerInstantiatedClass(decl, compiler.mirrorDependencies,
+          mirrorUsage: true);
+    }
+    // If the class is never instantiated, we know nothing of it can possibly
+    // be reflected upon.
+    // TODO(herhut): Add a warning if a mirrors annotation cannot hit.
+    if (recents.contains(cls.declaration)) {
+      logEnqueueReflectiveAction(cls, "members");
+      cls.constructors.forEach((Element element) {
+        enqueueReflectiveConstructor(element, add);
+      });
+      cls.forEachClassMember((Member member) {
+        enqueueReflectiveMember(member.element, add);
+      });
+    }
+  }
+
+  void enqueueReflectiveSpecialClasses() {

[Johnni Winther, 2014/06/27 07:43:00]

Add comment.

[herhut, 2014/06/27 12:34:36]

Done.

+    // [Closure] is treated specially as it is the superclass of all closures.
+    // Although it is in an internal library, we mark it as reflectable. Note
+    // that none of its methods are reflectable, unless reflectable by
+    // inheritance.
+    ClassElement closureClass = compiler.closureClass;
+    if (compiler.backend.referencedFromMirrorSystem(closureClass)) {
+      logEnqueueReflectiveAction(closureClass);
+      registerInstantiatedClass(closureClass, compiler.mirrorDependencies,
+          mirrorUsage: true);
+    }
+  }
+
+  void enqueueReflectiveElementsInLibrary(LibraryElement lib,

[Johnni Winther, 2014/06/27 07:43:00]

Add comment.

[herhut, 2014/06/27 12:34:36]

Done.

+                                          Iterable<ClassElement> recents,
+                                          bool add) {
+    add = includeElementDueToMirrors(add, lib);
+    lib.forEachLocalMember((Element member) {
+      if (member.isClass) {
+        enqueueReflectiveElementsInClass(member, recents, add);
+      } else {
+        enqueueReflectiveMember(member, add);
+      }
+    });
+  }
+
+  /// Enqueue all elements that are matched by the mirrors used
+  /// annotation or, in lack thereof, all elements.
+  void enqueueReflectiveElements(Iterable<ClassElement> recents) {
+    if (!hasEnqueuedReflectiveElements) {
+      logEnqueueReflectiveAction("!START enqueueAll");
+      // First round of enqueuing, visit everything that is visible to
+      // also pick up static top levels, etc.
+      // Also, during the first round, consider all classes that have been seen
+      // as recently seen, as a couple of rounds of resolution might have run

[floitsch, 2014/06/27 08:59:15]

maybe?

... as treeshaking might only be disabled at a later point (forcing us to
enqueue everything), when several rounds of resolution had happened.

[herhut, 2014/06/27 12:34:35]

Rephrased:

Also, during the first round, consider all classes that have been seen 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. 

+      // before treeshaking is disabled and we thus enqueue everything.
+      recents = seenClasses.toSet();
+      compiler.log('Enqueuing everything');
+      for (LibraryElement lib in compiler.libraries.values) {
+        enqueueReflectiveElementsInLibrary(lib, recents, false);
       }
+      enqueueReflectiveSpecialClasses();
+      hasEnqueuedReflectiveElements = true;
+      hasEnqueuedReflectiveStaticFields = true;
+      logEnqueueReflectiveAction("!DONE enqueueAll");
+    } else if (recents.isNotEmpty) {
+      // Keep looking at new classes until fixpoint is reached.
+      logEnqueueReflectiveAction("!START enqueueRecents");
+      recents.forEach((ClassElement cls) {
+        enqueueReflectiveElementsInClass(cls, recents,
+            includeElementDueToMirrors(false, cls.library));
+      });
+      logEnqueueReflectiveAction("!DONE enqueueRecents");
     }
-    hasEnqueuedEverything = true;
-    hasEnqueuedReflectiveStaticFields = true;
   }
 
   /// Enqueue the static fields that have been marked as used by reflective
@@ -358,7 +449,7 @@ abstract class Enqueuer {
     if (hasEnqueuedReflectiveStaticFields) return;
     hasEnqueuedReflectiveStaticFields = true;
     for (Element element in elements) {
-      pretendElementWasUsed(element, compiler.mirrorDependencies);
+      enqueueReflectiveMember(element, true);
     }
   }
 
@@ -529,21 +620,30 @@ abstract class Enqueuer {
   }
 
   void registerClosure(Element element, Registry registry) {

[Johnni Winther, 2014/06/27 07:43:00]

Change [Element] to [FunctionElement].

[herhut, 2014/06/27 12:34:36]

Done.

+    universe.allClosures.add(element);
     registerIfGeneric(element, registry);
   }
 
   void forEach(f(WorkItem work)) {
     do {
-      while (!queue.isEmpty) {
+      while (queue.isNotEmpty) {
         // TODO(johnniwinther): Find an optimal process order.
         f(queue.removeLast());
       }
-      onQueueEmpty();
-    } while (!queue.isEmpty);
+      List recents = recentClasses.toList(growable: false);
+      recentClasses.clear();
+      if (!onQueueEmpty(recents)) recentClasses.addAll(recents);
+    } while (queue.isNotEmpty || recentClasses.isNotEmpty);
   }
 
-  void onQueueEmpty() {
-    compiler.backend.onQueueEmpty(this);
+  /// [onQueueEmpty] is called whenever the queue is drained. [recentClasses]
+  /// contains the set of all classes seen for the first time since
+  /// [onQueueEmpty] was called last. A return value of [true] indicates that
+  /// the [recentClasses] have been processed and may be cleared. If [false] is
+  /// returned, [onQueueEmpty] will be called once the queue is empty again (or
+  /// still empty) and [recentClasses] will be a superset of the current value.
+  bool onQueueEmpty(Iterable<ClassElement> recentClasses) {
+    return compiler.backend.onQueueEmpty(this, recentClasses);
   }
 
   void logSummary(log(message)) {
@@ -603,6 +703,18 @@ class ResolutionEnqueuer extends Enqueuer {
     return false;
   }
 
+  /**
+   * Decides whether an element should be included to satisfy requirements
+   * of the mirror system.
+   *
+   * During resoltion, we have to resort to matching elements against the

[Johnni Winther, 2014/06/27 07:43:01]

'resoltion' -> 'resolution'.

[karlklose, 2014/06/27 07:45:57]

'resoltion' -> 'resolution'.

[herhut, 2014/06/27 12:34:36]

Done.

[herhut, 2014/06/27 12:34:37]

Done.

+   * [MirrorsUsed] pattern, as we do not have a complete picture of the world,
+   * yet.
+   */
+  bool includeElementDueToMirrors(bool includeEnclosing, Element element) {

[karlklose, 2014/06/27 07:45:57]

Does it make sense to move functionality like this to the MirrorUsageAnalyzer or
create a class to handle these?

[herhut, 2014/06/27 12:34:36]

Yes, this is also on the list of further refactoring.

+    return includeEnclosing || compiler.backend.requiredByMirrorSystem(element);
+  }
+
   void internalAddToWorkList(Element element) {
     assert(invariant(element, element is AnalyzableElement,
         message: 'Element $element is not analyzable.'));
@@ -689,9 +801,9 @@ class ResolutionEnqueuer extends Enqueuer {
     deferredTaskQueue.add(new DeferredTask(element, action));
   }
 
-  void onQueueEmpty() {
+  bool onQueueEmpty(Iterable<ClassElement> recentClasses) {
     emptyDeferredTaskQueue();
-    super.onQueueEmpty();
+    return super.onQueueEmpty(recentClasses);
   }
 
   void emptyDeferredTaskQueue() {
@@ -727,6 +839,17 @@ class CodegenEnqueuer extends Enqueuer {
   bool isProcessed(Element member) =>
       member.isAbstract || generatedCode.containsKey(member);
 
+  /**
+   * Decides whether an element should be included to satisfy requirements
+   * of the mirror system.
+   *
+   * For code generation, we rely on the precomputed set of elements that takes
+   * subtyping constraints into account.
+   */
+  bool includeElementDueToMirrors(bool includeEnclosing, Element element) {
+    return compiler.backend.isAccessibleByReflection(element);
+  }
+
   void internalAddToWorkList(Element element) {
     if (compiler.hasIncrementalSupport) {
       newlyEnqueuedElements.add(element);