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

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

BUG= http://dartbug.com/19179
R=johnniwinther@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37868

[Johnni Winther, 2014-06-27 07:43:02]

LGTM

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:435: /// Returns true if
this element is covered by a mirrorsUsed annotation but
Add an empty line before doc comments.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:436: /// is ok to be
treeshaken away.
Add an example of how this is sound.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:440: bool
requiredByMirrorSystem(Element element) => false;
Give an example.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:451: /// all classes
added since the last call to onQueueEmpty. A return value
'added' -> 'added to the set of instantiation classes'?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:455: /// will be a
superset of the current value.
When is this needed?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:326: bool
includeElementDueToMirrors(bool includeEnclosing, Element element);
Make [includeEnclosing] a named parameter.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:326: bool
includeElementDueToMirrors(bool includeEnclosing, Element element);
Rename to [shouldIncludeElementDueToMirror] to indicate that this is a question
and not a command.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
Add comment. I guess it is a helper method for [enqueueReflectiveElements].

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
Make [add] named.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:338:
registerInstantiatedType(cls.rawType, compiler.mirrorDependencies);
Shouldn't this call have `mirrorUsage: true` ?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:343: void
enqueueReflectiveMember(Element element, bool add) {
Make [add] named.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:343: void
enqueueReflectiveMember(Element element, bool add) {
Add comment.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:364: void
enqueueReflectiveElementsInClass(ClassElement cls,
Add comment.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:366: bool add) {
Make [add] named.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:368: if (cls.isInjected)
return;
Merge conditions.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:390: void
enqueueReflectiveSpecialClasses() {
Add comment.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:403: void
enqueueReflectiveElementsInLibrary(LibraryElement lib,
Add comment.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:622: void
registerClosure(Element element, Registry registry) {
Change [Element] to [FunctionElement].

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:710: * During resoltion,
we have to resort to matching elements against the
'resoltion' -> 'resolution'.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1799: element
= closure.expression;
This could also be computed as closure.closureClass.methodElement which also
works for ClosureFieldElement which might need handling here.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1819: *
Returns true of the element matches a mirrorsUsed annotation. If
'of' -> 'if'.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1870: // can
include include the correct ones when including the class.
'include include' -> 'include'.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1873: for
(Element clos in compiler.resolverWorld.allClosures) {
'Element clos' -> 'FunctionElement closure'.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1881: if
(cls.isInjected) continue;
Merge conditions.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1886: // 1)
the class (should be life)
'life' -> 'live' here and more below.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1922: if
(!compiler.enqueuer.resolution.isLive(element)) return;
[cls] should be live if the constructor is.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1958: for
(Element clos in compiler.resolverWorld.allClosures) {
'Element clos' -> 'FunctionElement closure'.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1965:
reflectableMembers.add(compiler.closureClass);
Only if 'allClosures' is non-empty?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1966:
reflectableMembers.add(compiler.boundClosureClass);
Only if we have tear-offs?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:664:
(name == '' &&
Add parentheses around 'name == ... isAccess...(...)' and indent 'backend' under
name to make it clear that these go together.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:71: final
Set<Element> allClosures = new Set<Element>();
Set<Element> -> Set<FunctionElement>.

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
File tests/lib/mirrors/mirrors_used_inheritance_test.dart (right):

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
tests/lib/mirrors/mirrors_used_inheritance_test.dart:1: // Copyright (c) 2013,
the Dart project authors.  Please see the AUTHORS file
2013 -> 2014

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
tests/lib/mirrors/mirrors_used_inheritance_test.dart:5: // Test to make sure
that the names of classes that are marked with meta
Update comment.

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
tests/lib/mirrors/mirrors_used_inheritance_test.dart:67: var object =
objects[index];
[object] -> [reflected] to show that we access an instanceof Reflected.

[karlklose, 2014-06-27 07:45:58]

DBC:

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:115: {mirrorUsage:
false}) {
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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:330:
print("MIRROR_ENQUEUE (${isResolutionQueue ? "R" : "B"}): $action $msg");
'R' -> 'resolution'?

What is 'B'?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
Could you add a comment about the semantics of 'add'?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:338:
registerInstantiatedType(cls.rawType, compiler.mirrorDependencies);
I am not sure we should register this type here.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:344: add =
includeElementDueToMirrors(add, element);
Inline call to includeElementsDueToMirrors? (Also below)

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:710: * During resoltion,
we have to resort to matching elements against the
'resoltion' -> 'resolution'.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:714: bool
includeElementDueToMirrors(bool includeEnclosing, Element element) {
Does it make sense to move functionality like this to the MirrorUsageAnalyzer or
create a class to handle these?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1883:
Set<Name> memberNames = new LinkedHashSet<Name>(
Consider moving this logic to its own method.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1886: // 1)
the class (should be life)
'life' -> 'live'. (Also below.)

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1890:
cls.constructors.forEach((Element constructor) {
How about:
  cls.constructors
      .where(resolution.isLive)
      .forEach(reflectableMembers.add);
?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1921:
cls.constructors.forEach((ConstructorElement element) {
Could you share the code of this closure with the two closures below?

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(left):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:227:
reflectionMarker = '-';
Why is the marker not needed anymore?

[floitsch, 2014-06-27 08:59:16]

DBC.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:436: /// is ok to be
treeshaken away.
but might be ok to be treeshaken away.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:456: bool
onQueueEmpty(Enqueuer enqueuer, Iterable<ClassElement> recentClasses) {}
I don't like the name "onX" is something that allows me to register something
for when "X" happens.
I prefer notifyX.

(Note: this is not the only place, so don't necessarily worry for this CL).

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:79: static final
TRACE_MIRROR_ENQUEUING = false;
should we start making these things const X.fromEnvironment?

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
nit: s/add/forceAdd/

I'm guessing that's the meaning.

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:345: if (add &&
!element.impliesType) {
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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:424: // as recently seen,
as a couple of rounds of resolution might have run
maybe?

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

[karlklose, 2014-06-27 09:35:28]

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:456: bool
onQueueEmpty(Enqueuer enqueuer, Iterable<ClassElement> recentClasses) {}
For me onX is something that allows me to do something when "X" happens, so I
think it fits.

I would like to find a better name for onX and registerX, but I dislike notifyX
because it is the wrong verb: we do not want the receiver to notify the "X"
(think of notifyListeners).

[floitsch, 2014-06-27 12:05:38]

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:456: bool
onQueueEmpty(Enqueuer enqueuer, Iterable<ClassElement> recentClasses) {}
On 2014/06/27 09:35:28, karlklose wrote:
> For me onX is something that allows me to do something when "X" happens, so I
> think it fits.
That's not how it is used in most libraries. For example: domElement.onClick
> 
> I would like to find a better name for onX and registerX, but I dislike
notifyX
> because it is the wrong verb: we do not want the receiver to notify the "X"
> (think of notifyListeners).

[herhut, 2014-06-27 12:34:38]

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:435: /// Returns true if
this element is covered by a mirrorsUsed annotation but
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Add an empty line before doc comments.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:436: /// is ok to be
treeshaken away.
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Add an example of how this is sound.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:436: /// is ok to be
treeshaken away.
On 2014/06/27 08:59:15, floitsch wrote:
> but might be ok to be treeshaken away.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:440: bool
requiredByMirrorSystem(Element element) => false;
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Give an example.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:451: /// all classes
added since the last call to onQueueEmpty. A return value
On 2014/06/27 07:43:00, Johnni Winther wrote:
> 'added' -> 'added to the set of instantiation classes'?

seen for the first time by the enqueuer....

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:455: /// will be a
superset of the current value.
On 2014/06/27 07:43:00, Johnni Winther wrote:
> When is this needed?

This allows each step in onQueueEmpty to abort processing and return to
processing the queue before all recents have been processed. Used by the
CustomTags analysis.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:456: bool
onQueueEmpty(Enqueuer enqueuer, Iterable<ClassElement> recentClasses) {}
On 2014/06/27 12:05:38, floitsch wrote:
> On 2014/06/27 09:35:28, karlklose wrote:
> > For me onX is something that allows me to do something when "X" happens, so
I
> > think it fits.
> That's not how it is used in most libraries. For example: domElement.onClick
> > 
> > I would like to find a better name for onX and registerX, but I dislike
> notifyX
> > because it is the wrong verb: we do not want the receiver to notify the "X"
> > (think of notifyListeners).
> 

This needs a more global fix, so not in this CL...

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:79: static final
TRACE_MIRROR_ENQUEUING = false;
On 2014/06/27 08:59:15, floitsch wrote:
> should we start making these things const X.fromEnvironment?
> 
> for example:
>  ... = const bool.fromEnvironment("TRACE_MIRROR_ENQUEUING");

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

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:115: {mirrorUsage:
false}) {
On 2014/06/27 07:45:57, karlklose wrote:
> 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.

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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:326: bool
includeElementDueToMirrors(bool includeEnclosing, Element element);
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Rename to [shouldIncludeElementDueToMirror] to indicate that this is a
question
> and not a command.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:326: bool
includeElementDueToMirrors(bool includeEnclosing, Element element);
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Make [includeEnclosing] a named parameter.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:330:
print("MIRROR_ENQUEUE (${isResolutionQueue ? "R" : "B"}): $action $msg");
On 2014/06/27 07:45:57, karlklose wrote:
> 'R' -> 'resolution'?
> 
> What is 'B'?

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Make [add] named.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
On 2014/06/27 08:59:15, floitsch wrote:
> nit: s/add/forceAdd/
> 
> I'm guessing that's the meaning.
> 
> Since it's not completely clear, maybe document what "add" does.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Add comment. I guess it is a helper method for [enqueueReflectiveElements].

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:334: void
enqueueReflectiveConstructor(ConstructorElement ctor, bool add) {
On 2014/06/27 07:45:57, karlklose wrote:
> Could you add a comment about the semantics of 'add'?

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:338:
registerInstantiatedType(cls.rawType, compiler.mirrorDependencies);
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Shouldn't this call have `mirrorUsage: true` ?

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

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:338:
registerInstantiatedType(cls.rawType, compiler.mirrorDependencies);
On 2014/06/27 07:45:57, karlklose wrote:
> I am not sure we should register this type here.

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:343: void
enqueueReflectiveMember(Element element, bool add) {
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Make [add] named.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:343: void
enqueueReflectiveMember(Element element, bool add) {
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Add comment.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:344: add =
includeElementDueToMirrors(add, element);
On 2014/06/27 07:45:57, karlklose wrote:
> Inline call to includeElementsDueToMirrors? (Also below)

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

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:345: if (add &&
!element.impliesType) {
On 2014/06/27 08:59:15, floitsch wrote:
> 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.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:364: void
enqueueReflectiveElementsInClass(ClassElement cls,
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Add comment.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:366: bool add) {
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Make [add] named.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:368: if (cls.isInjected)
return;
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Merge conditions.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:390: void
enqueueReflectiveSpecialClasses() {
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Add comment.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:403: void
enqueueReflectiveElementsInLibrary(LibraryElement lib,
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Add comment.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:424: // as recently seen,
as a couple of rounds of resolution might have run
On 2014/06/27 08:59:15, floitsch wrote:
> maybe?
> 
> ... as treeshaking might only be disabled at a later point (forcing us to
> enqueue everything), when several rounds of resolution had happened.

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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:622: void
registerClosure(Element element, Registry registry) {
On 2014/06/27 07:43:00, Johnni Winther wrote:
> Change [Element] to [FunctionElement].

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:710: * During resoltion,
we have to resort to matching elements against the
On 2014/06/27 07:43:01, Johnni Winther wrote:
> 'resoltion' -> 'resolution'.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:710: * During resoltion,
we have to resort to matching elements against the
On 2014/06/27 07:45:57, karlklose wrote:
> 'resoltion' -> 'resolution'.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:714: bool
includeElementDueToMirrors(bool includeEnclosing, Element element) {
On 2014/06/27 07:45:57, karlklose wrote:
> Does it make sense to move functionality like this to the MirrorUsageAnalyzer
or
> create a class to handle these?

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1799: element
= closure.expression;
On 2014/06/27 07:43:01, Johnni Winther wrote:
> This could also be computed as closure.closureClass.methodElement which also
> works for ClosureFieldElement which might need handling here.

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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1819: *
Returns true of the element matches a mirrorsUsed annotation. If
On 2014/06/27 07:43:01, Johnni Winther wrote:
> 'of' -> 'if'.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1870: // can
include include the correct ones when including the class.
On 2014/06/27 07:43:01, Johnni Winther wrote:
> 'include include' -> 'include'.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1873: for
(Element clos in compiler.resolverWorld.allClosures) {
On 2014/06/27 07:43:01, Johnni Winther wrote:
> 'Element clos' -> 'FunctionElement closure'.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1881: if
(cls.isInjected) continue;
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Merge conditions.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1883:
Set<Name> memberNames = new LinkedHashSet<Name>(
On 2014/06/27 07:45:57, karlklose wrote:
> Consider moving this logic to its own method.

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1886: // 1)
the class (should be life)
On 2014/06/27 07:43:01, Johnni Winther wrote:
> 'life' -> 'live' here and more below.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1886: // 1)
the class (should be life)
On 2014/06/27 07:45:57, karlklose wrote:
> 'life' -> 'live'. (Also below.)

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1890:
cls.constructors.forEach((Element constructor) {
On 2014/06/27 07:45:57, karlklose wrote:
> How about:
>   cls.constructors
>       .where(resolution.isLive)
>       .forEach(reflectableMembers.add);
> ?

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1921:
cls.constructors.forEach((ConstructorElement element) {
On 2014/06/27 07:45:57, karlklose wrote:
> Could you share the code of this closure with the two closures below?

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

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1922: if
(!compiler.enqueuer.resolution.isLive(element)) return;
On 2014/06/27 07:43:01, Johnni Winther wrote:
> [cls] should be live if the constructor is.

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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1958: for
(Element clos in compiler.resolverWorld.allClosures) {
On 2014/06/27 07:43:01, Johnni Winther wrote:
> 'Element clos' -> 'FunctionElement closure'.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1965:
reflectableMembers.add(compiler.closureClass);
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Only if 'allClosures' is non-empty?

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.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1966:
reflectableMembers.add(compiler.boundClosureClass);
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Only if we have tear-offs?

See above.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(left):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:227:
reflectionMarker = '-';
On 2014/06/27 07:45:57, karlklose wrote:
> Why is the marker not needed anymore?

Not clear what this did before but the differentiation does not exist in the new
implementation anymore.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:664:
(name == '' &&
On 2014/06/27 07:43:01, Johnni Winther wrote:
> Add parentheses around 'name == ... isAccess...(...)' and indent 'backend'
under
> name to make it clear that these go together.

Done.

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:71: final
Set<Element> allClosures = new Set<Element>();
On 2014/06/27 07:43:02, Johnni Winther wrote:
> Set<Element> -> Set<FunctionElement>.

Done.

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
File tests/lib/mirrors/mirrors_used_inheritance_test.dart (right):

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
tests/lib/mirrors/mirrors_used_inheritance_test.dart:1: // Copyright (c) 2013,
the Dart project authors.  Please see the AUTHORS file
On 2014/06/27 07:43:02, Johnni Winther wrote:
> 2013 -> 2014

Done.

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
tests/lib/mirrors/mirrors_used_inheritance_test.dart:5: // Test to make sure
that the names of classes that are marked with meta
On 2014/06/27 07:43:02, Johnni Winther wrote:
> Update comment.

Done.

https://codereview.chromium.org/340783011/diff/60001/tests/lib/mirrors/mirror...
tests/lib/mirrors/mirrors_used_inheritance_test.dart:67: var object =
objects[index];
On 2014/06/27 07:43:02, Johnni Winther wrote:
> [object] -> [reflected] to show that we access an instanceof Reflected.

Actually, it is an instance of Subclass. I have renamed it to subclass to make
this clear. And added a comment :-)

[Johnni Winther, 2014-06-27 14:22:30]

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340783011/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:456: bool
onQueueEmpty(Enqueuer enqueuer, Iterable<ClassElement> recentClasses) {}
On 2014/06/27 12:05:38, floitsch wrote:
> On 2014/06/27 09:35:28, karlklose wrote:
> > For me onX is something that allows me to do something when "X" happens, so
I
> > think it fits.
> That's not how it is used in most libraries. For example: domElement.onClick
> > 
> > I would like to find a better name for onX and registerX, but I dislike
> notifyX
> > because it is the wrong verb: we do not want the receiver to notify the "X"
> > (think of notifyListeners).
> 

The `domElement.onClick` is a short-hand for getting of the "onClickStream"
property on which you register a listener. So the API has made
`domElement.onClick.listen(...)` the Dart-way of saying
"domElement.registerOnClickListener(...)". Thus 'onClick' is still the name of
the event that you want to provide a callback for.

In other words, I think the API naming convention has cluttered the terminology
of two abstractions, the observer and the observable, by giving the observable
the name for the observer.

Maybe we are left with names like 'callbackForOnQueueEmpty'.

[herhut, 2014-07-01 12:43:06]

Committed patchset #3 manually as r37868 (presubmit successful).