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

sdk/lib/_internal/compiler/implementation/js_backend/backend.dart

Index: sdk/lib/_internal/compiler/implementation/js_backend/backend.dart
diff --git a/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart b/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart
index 6722f4657cc36e859ad1c47cc6bd0f89d8a32da1..4c38f9a2a4bd2d9ceed19629674432fa28cfd89b 100644
--- a/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart
+++ b/sdk/lib/_internal/compiler/implementation/js_backend/backend.dart
@@ -265,9 +265,6 @@ class JavaScriptBackend extends Backend {
   /// these constants must be registered.
   final List<Dependency> metadataConstants = <Dependency>[];
 
-  /// List of symbols that the user has requested for reflection.
-  final Set<String> symbolsUsed = new Set<String>();
-
   /// List of elements that the user has requested for reflection.
   final Set<Element> targetsUsed = new Set<Element>();
 
@@ -275,10 +272,20 @@ class JavaScriptBackend extends Backend {
   /// element must be retained.
   final Set<Element> metaTargetsUsed = new Set<Element>();
 
+  /// Set of methods that are needed by reflection. Computed using
+  /// [computeMembersNeededForReflection] on first use.
+  Iterable<Element> _membersNeededForReflection = null;
+  Iterable<Element> get membersNeededForReflection {
+    assert(_membersNeededForReflection != null);
+    return _membersNeededForReflection;
+  }
+
+  /// List of symbols that the user has requested for reflection.
+  final Set<String> symbolsUsed = new Set<String>();
+
   /// List of elements that the backend may use.
   final Set<Element> helpersUsed = new Set<Element>();
 
-
   /// Set of typedefs that are used as type literals.
   final Set<TypedefElement> typedefTypeLiterals = new Set<TypedefElement>();
 
@@ -353,7 +360,7 @@ class JavaScriptBackend extends Backend {
       }
     }
 
-    return isNeededForReflection(element.declaration);
+    return isAccessibleByReflection(element.declaration);
   }
 
   bool canBeUsedForGlobalOptimizations(Element element) {
@@ -774,7 +781,10 @@ class JavaScriptBackend extends Backend {
     registerCheckedModeHelpers(registry);
   }
 
-  onResolutionComplete() => rti.computeClassesNeedingRti();
+  onResolutionComplete() {
+    computeMembersNeededForReflection();
+    rti.computeClassesNeedingRti();
+  }
 
   void onStringInterpolation(Registry registry) {
     assert(registry.isForResolution);
@@ -1574,10 +1584,14 @@ class JavaScriptBackend extends Backend {
   }
 
   /// Should [element] (a getter) be retained for reflection?
-  bool shouldRetainGetter(Element element) => isNeededForReflection(element);
+  bool shouldRetainGetter(Element element) {
+    return isAccessibleByReflection(element);
+  }
 
   /// Should [element] (a setter) be retained for reflection?
-  bool shouldRetainSetter(Element element) => isNeededForReflection(element);
+  bool shouldRetainSetter(Element element) {
+    return isAccessibleByReflection(element);
+  }
 
   /// Should [name] be retained for reflection?
   bool shouldRetainName(String name) {
@@ -1590,7 +1604,7 @@ class JavaScriptBackend extends Backend {
 
   bool retainMetadataOf(Element element) {
     if (mustRetainMetadata) hasRetainedMetadata = true;
-    if (mustRetainMetadata && isNeededForReflection(element)) {
+    if (mustRetainMetadata && referencedFromMirrorSystem(element)) {
       for (MetadataAnnotation metadata in element.metadata) {
         metadata.ensureResolved(compiler);
         Constant constant = constants.getConstantForMetadata(metadata);
@@ -1775,54 +1789,51 @@ class JavaScriptBackend extends Backend {
    * system.
    */
   bool isAccessibleByReflection(Element element) {
-    // TODO(ahe): This isn't sufficient: simply importing dart:mirrors
-    // causes hasInsufficientMirrorsUsed to become true.
-    if (hasInsufficientMirrorsUsed) return true;
-    return isNeededForReflection(element);
+    if (element.isClass) {
+      element = getDartClass(element);
+    }
+    // We have to treat closure classes specially here, as they only come into
+    // existence after [membersNeededForReflection] has been computed.
+    if (element is SynthesizedCallMethodElementX) {
+      SynthesizedCallMethodElementX closure = element;
+      element = closure.expression;

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

This could also be computed as closure.closureClass.methodElement which also
works for ClosureFieldElement which might need handling here.

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

I think we have decided not to expose those fields. So they will be included as
they are referenced from the [call] member but won't be reflectable.

+    } else if (element is ClosureClassElement) {
+      ClosureClassElement closure = element;
+      element = closure.methodElement;
+    }
+    return membersNeededForReflection.contains(element);
   }
 
   /**
-   * Returns `true` if the emitter must emit the element even though there
-   * is no direct use in the program, but because the reflective system may
-   * need to access it.
+   * Returns true if the element has to be resolved due to a mirrorsUsed
+   * annotation. If we have insufficient mirrors used annotations, we only
+   * keep additonal elements if treeshaking has been disabled.
    */
-  bool isNeededForReflection(Element element) {
-    if (!isTreeShakingDisabled) return false;
-    element = getDartClass(element);
-    if (hasInsufficientMirrorsUsed) return isTreeShakingDisabled;
-    /// Record the name of [element] in [symbolsUsed]. Return true for
-    /// convenience.
-    bool registerNameOf(Element element) {
-      symbolsUsed.add(element.name);
-      if (element.isConstructor) {
-        symbolsUsed.add(element.enclosingClass.name);
-      }
-      return true;
-    }
-
-    Element enclosing = element.enclosingElement;
-    if (enclosing != null && isNeededForReflection(enclosing)) {
-      return registerNameOf(element);
-    }
-
-    if (isNeededThroughMetaTarget(element)) {
-      return registerNameOf(element);
-    }
+  bool requiredByMirrorSystem(Element element) {
+    return hasInsufficientMirrorsUsed && isTreeShakingDisabled ||
+           matchesMirrorsMetaTarget(element) ||
+           targetsUsed.contains(element);
+  }
 
-    if (!targetsUsed.isEmpty && targetsUsed.contains(element)) {
-      return registerNameOf(element);
-    }
+  /**
+   * Returns true of the element matches a mirrorsUsed annotation. If

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

'of' -> 'if'.

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

Done.

+   * we have insufficient mirrorsUsed information, this returns true for
+   * all elements, as they might all be potentially referenced.
+   */
+  bool referencedFromMirrorSystem(Element element, [recursive = true]) {
+    Element enclosing = recursive ? element.enclosingElement : null;
 
-    // TODO(kasperl): Consider caching this information. It is consulted
-    // multiple times because of the way we deal with the enclosing element.
-    return false;
+    return hasInsufficientMirrorsUsed ||
+           matchesMirrorsMetaTarget(element) ||
+           targetsUsed.contains(element) ||
+           (enclosing != null && referencedFromMirrorSystem(enclosing));
   }
 
   /**
    * Returns `true` if the element is needed because it has an annotation
    * of a type that is used as a meta target for reflection.
    */
-  bool isNeededThroughMetaTarget(Element element) {
+  bool matchesMirrorsMetaTarget(Element element) {
     if (metaTargetsUsed.isEmpty) return false;
     for (Link link = element.metadata; !link.isEmpty; link = link.tail) {
       MetadataAnnotation metadata = link.head;
@@ -1838,6 +1849,128 @@ class JavaScriptBackend extends Backend {
     return false;
   }
 
+  /**
+   * Visits all classes and computes whether its members are needed for
+   * reflection.
+   *
+   * We have to precompute this set as we cannot easily answer the need for
+   * reflection locally when looking at the member: We lack the information by
+   * which classes a member is inherited. Called after resolution is complete.
+   *
+   * We filter out private libraries here, as their elements should not
+   * be visible by reflection unless some other interfaces makes them
+   * accessible.
+   */
+  computeMembersNeededForReflection() {
+    if (_membersNeededForReflection != null) return;
+    if (compiler.mirrorsLibrary == null) {
+      _membersNeededForReflection = const [];
+    }
+    // Compute a mapping from class to the closures it contains, so we
+    // can include include the correct ones when including the class.

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

'include include' -> 'include'.

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

Done.

+    Map<ClassElement, List<Element>> closureMap =
+        new Map<ClassElement, List<Element>>();
+    for (Element clos in compiler.resolverWorld.allClosures) {

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

'Element clos' -> 'FunctionElement closure'.

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

Done.

+      closureMap.putIfAbsent(clos.enclosingClass, () => []).add(clos);
+    }
+    Set<Element> reflectableMembers = new Set<Element>();
+    ResolutionEnqueuer resolution = compiler.enqueuer.resolution;
+    for (ClassElement cls in resolution.universe.instantiatedClasses) {
+      // Do not process internal classes.
+      if (cls.library.isInternalLibrary) continue;
+      if (cls.isInjected) continue;

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

Merge conditions.

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

Done.

+      if (referencedFromMirrorSystem(cls)) {
+        Set<Name> memberNames = new LinkedHashSet<Name>(

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

Consider moving this logic to its own method.

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

It has side effects on reflectableMembers and I'd rather have it all in one
place.

+            equals: (Name a, Name b) => a.isSimilarTo(b),
+            hashCode: (Name a) => a.similarHashCode);
+        // 1) the class (should be life)

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

'life' -> 'live' here and more below.

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

'life' -> 'live'. (Also below.)

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

Done.

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

Done.

+        assert(invariant(cls, resolution.isLive(cls)));
+        reflectableMembers.add(cls);
+        // 2) its constructors (if life)
+        cls.constructors.forEach((Element constructor) {

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

How about:
  cls.constructors
      .where(resolution.isLive)
      .forEach(reflectableMembers.add);
?

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

constructors is a Link<Element> and has no filter :(

+          if (resolution.isLive(constructor)) {
+            reflectableMembers.add(constructor);
+          }
+        });
+        // 3) all members, including fields via getter/setters (if life)
+        cls.forEachClassMember((Member member) {
+          if (resolution.isLive(member.element)) {
+            memberNames.add(member.name);
+            reflectableMembers.add(member.element);
+          }
+        });
+        // 4) all overriding members of subclasses/subtypes (should be life)
+        if (compiler.world.hasAnySubtype(cls)) {
+          for (ClassElement subcls in compiler.world.subtypesOf(cls)) {
+            subcls.forEachClassMember((Member member) {
+              if (memberNames.contains(member.name)) {
+                assert(invariant(member.element,
+                    resolution.isLive(member.element)));
+                reflectableMembers.add(member.element);
+              }
+            });
+          }
+        }
+        // 5) all its closures
+        List<Element> closures = closureMap[cls];
+        if (closures != null) {
+          reflectableMembers.addAll(closures);
+        }
+      } else {
+        // check members themselves
+        cls.constructors.forEach((ConstructorElement element) {

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

Could you share the code of this closure with the two closures below?

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

I would need an adapter anyway from member to member.element, so this won't save
much code but readability would suffer.

+          if (!compiler.enqueuer.resolution.isLive(element)) return;

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

[cls] should be live if the constructor is.

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

Yes, but [cls] is not reflectable. This handles the case where a constructor is
reflectable but the class itself is not. Not sure this can currently be
expressed but just to be on the safe side.

+          if (referencedFromMirrorSystem(element, false)) {
+            reflectableMembers.add(element);
+          }
+        });
+        cls.forEachClassMember((Member member) {
+          if (!compiler.enqueuer.resolution.isLive(member.element)) return;
+          if (referencedFromMirrorSystem(member.element, false)) {
+            reflectableMembers.add(member.element);
+          }
+        });
+        // Also add in closures
+        List<Element> closures = closureMap[cls];
+        if (closures != null) {
+          for (Element closure in closures) {
+            if (referencedFromMirrorSystem(closure.enclosingMember, false)) {
+              reflectableMembers.add(closure);
+            }
+          }
+        }
+      }
+    }
+    // We also need top-level non-class elements like static functions and
+    // global fields. We use the resolution queue to decide which elements are
+    // part of the live world.
+    for (LibraryElement lib in compiler.libraries.values) {
+      if (lib.isInternalLibrary) continue;
+      lib.forEachLocalMember((Element member) {
+        if (!member.isClass &&
+            compiler.enqueuer.resolution.isLive(member) &&
+            referencedFromMirrorSystem(member)) {
+          reflectableMembers.add(member);
+        }
+      });
+    }
+    // Lastly, we add in all the closures found if they match.
+    for (Element clos in compiler.resolverWorld.allClosures) {

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

'Element clos' -> 'FunctionElement closure'.

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

Done.

+      if (clos.enclosingMember == null ||
+          referencedFromMirrorSystem(clos.enclosingMember)) {
+        reflectableMembers.add(clos);
+      }
+    }
+    // And their superclasses.
+    reflectableMembers.add(compiler.closureClass);

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

Only if 'allClosures' is non-empty?

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

The [Closure] class could still be part of a MirrorsUsed annotation and thus
someone might want to reflect on it. So both conditions need to be checked.

+    reflectableMembers.add(compiler.boundClosureClass);

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

Only if we have tear-offs?

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

See above.

+    // Register all symbols of reflectable elements
+    for (Element element in reflectableMembers) {
+      symbolsUsed.add(element.name);
+    }
+    _membersNeededForReflection = reflectableMembers;
+  }
+
   jsAst.Call generateIsJsIndexableCall(jsAst.Expression use1,
                                        jsAst.Expression use2) {
     String dispatchPropertyName = 'init.dispatchPropertyName';
@@ -1900,18 +2033,18 @@ class JavaScriptBackend extends Backend {
   }
 
   /// Called when [enqueuer] is empty, but before it is closed.
-  void onQueueEmpty(Enqueuer enqueuer) {
+  bool onQueueEmpty(Enqueuer enqueuer, Iterable<ClassElement> recentClasses) {
     // Add elements referenced only via custom elements.  Return early if any
     // elements are added to avoid counting the elements as due to mirrors.
     customElementsAnalysis.onQueueEmpty(enqueuer);
-    if (!enqueuer.queueIsEmpty) return;
+    if (!enqueuer.queueIsEmpty) return false;
 
     if (!enqueuer.isResolutionQueue && preMirrorsMethodCount == 0) {
       preMirrorsMethodCount = generatedCode.length;
     }
 
     if (isTreeShakingDisabled) {
-      enqueuer.enqueueEverything();
+      enqueuer.enqueueReflectiveElements(recentClasses);
     } else if (!targetsUsed.isEmpty && enqueuer.isResolutionQueue) {
       // Add all static elements (not classes) that have been requested for
       // reflection. If there is no mirror-usage these are probably not
@@ -1931,6 +2064,7 @@ class JavaScriptBackend extends Backend {
       }
       metadataConstants.clear();
     }
+    return true;
   }
 
   void onElementResolved(Element element, TreeElements elements) {