Mirrors overhaul.

Mirrors overhaul.

R=johnniwinther@google.com

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

[Johnni Winther, 2013-09-03 12:36:57]

LGTM

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:16: // Turn all getters into final fields?
Why?

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:79: * name is [libraryName].
Comment needs update.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:325: 
Extra line.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:434: *
Extra line.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:552: * If [:type.instanceLookup(name:] is a
regular method, the result of this
'(name' -> '(name)'.
'is a regular method' -> 'returns a regular method'?
What is 'type'?

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:583: * function.
'of a function is that function' -> 'of a method is the method itself'?

[Michael Lippautz (Google), 2013-09-03 17:24:50]

DBC

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (left):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:1187: class MirroredCompilationError extends
MirroredError {
Note: I suspect that removing this classes will break the vm mirrors. So this
needs to be in sync with a change there?

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:655: // TODO(ahe): Drop from release 1.0?
Or "This operation is optional and may return [:null:]."? :)

(Also sync instead of async?)

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:791: * An immutable map from names to mirrors
for all type variables for
comment out of sync

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:804: * An immutable map from names to mirrors
for all type arguments for
comment out of sync

[ahe, 2013-09-03 17:31:10]

Clarification: this is the proposed changes to the API.

First step is to ensure that we agree about the direction.

Then we'll figure out how to go about making it happen without breaking our
users too many times.

[rmacnak, 2013-09-03 18:42:37]

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (left):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:1159: class MirroredUncaughtExceptionError
extends MirroredError {
We will need this for cross-isolate reflection, but I'm okay with knocking it
out for now.

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:1187: class MirroredCompilationError extends
MirroredError {
We should keep this. Catching compilation errors is interesting for interactive
development. Dart2js need not do anything because either a method will be
missing from tree-shaking and a normal NoSuchMethodError results, or all the
relevant code will have been eagerly compiled.

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:786: /// Finds the instance member named
[name] declared or inherited in the
Does this include getters and setters? If so, how do we distinguish getters from
regular methods?

[ahe, 2013-09-03 18:47:20]

Thank you, Ryan and Michael for taking a look!

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (left):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:1187: class MirroredCompilationError extends
MirroredError {
On 2013/09/03 17:24:50, Michael Lippautz wrote:
> Note: I suspect that removing this classes will break the vm mirrors. So this
> needs to be in sync with a change there?

Yes, I think I would start by moving these classes into the VM patch file.

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:786: /// Finds the instance member named
[name] declared or inherited in the
On 2013/09/03 18:42:37, Ryan Macnak wrote:
> Does this include getters and setters?

Yes.

> If so, how do we distinguish getters from
> regular methods?

I probably don't understand your question, because the only answer I can think
of is:

ClassMirror c = ...;
var m = c.instanceLookup(#foo);
if (m is MethodMirror) {
  if (m.isGetter) print('foo is a getter');
  if (m.isRegularMethod) print('foo is regular method');
}

[rmacnak, 2013-09-03 19:44:26]

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:786: /// Finds the instance member named
[name] declared or inherited in the
On 2013/09/03 18:47:20, ahe wrote:
> On 2013/09/03 18:42:37, Ryan Macnak wrote:
> > Does this include getters and setters?
> 
> Yes.
> 
> > If so, how do we distinguish getters from
> > regular methods?
> 
> I probably don't understand your question, because the only answer I can think
> of is:
> 
> ClassMirror c = ...;
> var m = c.instanceLookup(#foo);
> if (m is MethodMirror) {
>   if (m.isGetter) print('foo is a getter');
>   if (m.isRegularMethod) print('foo is regular method');
> }

I was thinking something like this, but the override rules seem to forbid it.

import 'dart:mirrors';

class S {
  get foo => 'S-foo';
}
class C extends S {
  foo() => 'C-foo';
}
reflectClass(C)[#foo]

[ahe, 2013-09-03 19:52:36]

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/23455028/diff/1/dart/sdk/lib/mirrors/m...
dart/sdk/lib/mirrors/mirrors.dart:786: /// Finds the instance member named
[name] declared or inherited in the
On 2013/09/03 19:44:26, Ryan Macnak wrote:
> On 2013/09/03 18:47:20, ahe wrote:
> > On 2013/09/03 18:42:37, Ryan Macnak wrote:
> > > Does this include getters and setters?
> > 
> > Yes.
> > 
> > > If so, how do we distinguish getters from
> > > regular methods?
> > 
> > I probably don't understand your question, because the only answer I can
think
> > of is:
> > 
> > ClassMirror c = ...;
> > var m = c.instanceLookup(#foo);
> > if (m is MethodMirror) {
> >   if (m.isGetter) print('foo is a getter');
> >   if (m.isRegularMethod) print('foo is regular method');
> > }
> 
> I was thinking something like this, but the override rules seem to forbid it.
> 
> import 'dart:mirrors';
> 
> class S {
>   get foo => 'S-foo';
> }
> class C extends S {
>   foo() => 'C-foo';
> }
> reflectClass(C)[#foo]

Yes. We have tried very hard to ensure a single namespace, unlike Java, for
example, which has separate namespaces for methods and fields.

[Alan Knight, 2013-09-03 20:08:30]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:82: return libraries.values.singleWhere(
Might this be more useful if it used firstWhere so as to let you provide an
orElse?

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:801: List<TypeVariableMirror> get
typeVariables;
Would Iterable be better than List?

[ahe, 2013-09-03 20:14:30]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:82: return libraries.values.singleWhere(
On 2013/09/03 20:08:31, Alan Knight wrote:
> Might this be more useful if it used firstWhere so as to let you provide an
> orElse?

What I don't like about firstWhere is that it doesn't give me an error if the
library isn't unique.

We could have an optionally named parameter 'ifError' to this method.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:801: List<TypeVariableMirror> get
typeVariables;
On 2013/09/03 20:08:31, Alan Knight wrote:
> Would Iterable be better than List?

I think length and operator[] are important properties, so I think List is the
right interface.

[Alan Knight, 2013-09-03 20:30:55]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:82: return libraries.values.singleWhere(
On 2013/09/03 20:14:30, ahe wrote:
> On 2013/09/03 20:08:31, Alan Knight wrote:
> > Might this be more useful if it used firstWhere so as to let you provide an
> > orElse?
> 
> What I don't like about firstWhere is that it doesn't give me an error if the
> library isn't unique.
> 
> We could have an optionally named parameter 'ifError' to this method.

Does the error distinguish between the case of no match and multiple matches? It
seems awkward to have to get an error and then try to distinguish between the
two different problems. 

An advantage of firstWhere is that the implementation can stop looking once it
finds the first one, so for what I presume is the common case it's faster.

I can't help but note that if findInContext worked, quite a few cases where code
has to do this would both go away and get better results.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:801: List<TypeVariableMirror> get
typeVariables;
On 2013/09/03 20:14:30, ahe wrote:
> On 2013/09/03 20:08:31, Alan Knight wrote:
> > Would Iterable be better than List?
> 
> I think length and operator[] are important properties, so I think List is the
> right interface.

Important for what in this context? Are we expecting the indices in this to
correspond to something else? I'm just thinking that it might be useful for the
implementation to be able to generate these lazily, or otherwise optimize in
ways that might be trickier if it's a List.

[gbracha, 2013-09-17 18:27:52]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:677: Map<Symbol, DeclarationMirror> get
declarations;
See comments on declarations in ClassMirror below

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:762: Map<Symbol, DeclarationMirror> get
declarations;
I am slightly confused. I thought the plan was to add a declarations method with
named arguments that allowed us to filter the desired results (inherited or not,
getters, setters etc.) and we would keep the existing members until after that
was added. Then we would drop members as part of a big breaking change.

Also same applies above.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:784: Function operator [](Symbol name);
nits in wording:

Returns *a* closure ...


if m has ... with defaults ... *Or*

... if m has .. optional ...

[ahe, 2013-09-26 18:03:35]

A note from a chat with Gilad earlier today.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:788: DeclarationMirror instanceLookup(Symbol
name);
We also need a way to get a list or map of all instance members (including
inherited members).

Codename: "api".

Perhaps "instanceMembers".

This method returns getters, setters, and methods, not fields (but their implied
getters and setters).

This means that we need to say that the original declaration of an implicit
getter, for example, is its field.

[gbracha, 2013-09-26 19:50:35]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:763: 
In our latest discussion, we assume that all declarations, including
constructors and type variables, are included.  This also requires a rule in the
spec that forbids name conflicts between type variables and members and/or
constructors, which is desired anyway.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:785: 
Also, why is this restricted to static methods? What about getters and setters,
and instance members?

And what does it do if there is no member name [name]? Do we get null, or an
error of some sort?

[rmacnak, 2013-09-26 22:47:51]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:785: 
On 2013/09/26 19:50:35, gbracha wrote:
> Also, why is this restricted to static methods? What about getters and
setters,
> and instance members?

From a subscript operator on InstanceMirror?  Hoist this to ObjectMirror?

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:788: DeclarationMirror instanceLookup(Symbol
name);
On 2013/09/26 18:03:36, ahe wrote:
> We also need a way to get a list or map of all instance members (including
> inherited members).
> 
> Codename: "api".
> 
> Perhaps "instanceMembers".
> 
> This method returns getters, setters, and methods, not fields (but their
implied
> getters and setters).
> 
> This means that we need to say that the original declaration of an implicit
> getter, for example, is its field.

And perhaps add DeclarationMirror.isSynthetic

[gbracha, 2013-10-03 22:11:25]

I cannot quite approve this yet. Apart from the comments given by me below,
there are a few other comments by Ryan and Johhni that have not been addressed. 
My main issue is that I am opposed to the elimination of
MirroredCompilationError. It is not just a VM specific detail. It is part of the
API so people can see it and interact with it.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:79: * name is [libraryName].
On 2013/09/03 12:36:57, Johnni Winther wrote:
> Comment needs update.

Yes, can we please fix these issues in the CL so that it really reflects where
we want to be?

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:349: * mirror on a closure corresponding to
that method.
I'd tweak the wording a bit at some point, because "closure corresponding to a
method" is not very specific. We know we want the closurization per tha language
spec, and I'll eventually put in better wording, but this is ok for now.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:552: * If [:type.instanceLookup(name:] is a
regular method, the result of this
On 2013/09/03 12:36:57, Johnni Winther wrote:
> '(name' -> '(name)'.
> 'is a regular method' -> 'returns a regular method'?
> What is 'type'?

We have made some tweaks to this wording in other CLs. Minor, see

https://codereview.chromium.org/25741005/diff/6001/sdk/lib/mirrors/mirrors.dart

reproduced below

/**
  552    * Returns a closure for invoking the regular method named [name].
  553    *
  554    * If [:type.instanceLookup(name):] returns a regular method, the result
of
  555    * this method is a closure equivalent to:
  556    *
  557    *     (r1, .., rn, {p1: d1, ..., pk: dk}) {
  558    *       return this.invoke(name, [r1, .., rn], {#p1: p1,  .., #pk:
pk});
  559    *     }
  560    *
  561    * if m has required parameters r1, ..., rn, and named parameters p1,
..., pk
  562    * with defaults d1, ..., dk.
  563    *
  564    *     (r1, .., rn, [p1 = d1, …, pk = dk]) {
  565    *       return this.invoke(name, [r1, .., rn, p1, .., pk]);
  566    *     }
  567    *
  568    * if m has required parameters r1, ..., rn, and optional positional
  569    * parameters p1, ..., pk with defaults d1, ..., dk.
  570    */

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:655: // TODO(ahe): Drop from release 1.0?
On 2013/09/03 17:24:50, Michael Lippautz wrote:
> Or "This operation is optional and may return [:null:]."? :)
> 
> (Also sync instead of async?)

We have real demand for this. What are its prospects in dart2js?

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:674: * The members of a library are its
top-level classes, functions, variables,
also typedefs

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:763: 
On 2013/09/26 19:50:35, gbracha wrote:
> In our latest discussion, we assume that all declarations, including
> constructors and type variables, are included.  This also requires a rule in
the
> spec that forbids name conflicts between type variables and members and/or
> constructors, which is desired anyway.

We now decided not to include type variables, because we want to keep the getter
for typeVariables. It needs to be defined in TypeMirror so TypeDefMirror
supports it (because typedefs can be generic).

Can we get the wording updated to reflect these discussions?

[rmacnak, 2013-10-03 22:26:40]

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:655: // TODO(ahe): Drop from release 1.0?
On 2013/10/03 22:11:25, gbracha wrote:
> On 2013/09/03 17:24:50, Michael Lippautz wrote:
> > Or "This operation is optional and may return [:null:]."? :)
> > 
> > (Also sync instead of async?)
> 
> We have real demand for this. What are its prospects in dart2js?

This should be doable in dart2js because it compiles closure definitions like
mini classes wherein we can find the variables closed-over. (In NS2V8's
compilation strategy of using JS functions directly, implementing this would be
quite a challenge.) I'm not familiar with dart2js internals, but I would think
this is easier than generics.

[ahe, 2013-10-30 07:48:30]

Johnni, could you please take a look?

The intent is to remove stuff, we can add stuff back later if needed.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:79: * name is [libraryName].
On 2013/10/03 22:11:25, gbracha wrote:
> On 2013/09/03 12:36:57, Johnni Winther wrote:
> > Comment needs update.
> 
> Yes, can we please fix these issues in the CL so that it really reflects where
> we want to be?

Done.

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:349: * mirror on a closure corresponding to
that method.
On 2013/10/03 22:11:25, gbracha wrote:
> I'd tweak the wording a bit at some point, because "closure corresponding to a
> method" is not very specific. We know we want the closurization per tha
language
> spec, and I'll eventually put in better wording, but this is ok for now.

A kind soul fixed that for me in another CL :-)

https://codereview.chromium.org/23455028/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:552: * If [:type.instanceLookup(name:] is a
regular method, the result of this
On 2013/10/03 22:11:25, gbracha wrote:
> On 2013/09/03 12:36:57, Johnni Winther wrote:
> > '(name' -> '(name)'.
> > 'is a regular method' -> 'returns a regular method'?
> > What is 'type'?
> 
> We have made some tweaks to this wording in other CLs. Minor, see
> 
>
https://codereview.chromium.org/25741005/diff/6001/sdk/lib/mirrors/mirrors.dart
> 
> reproduced below
> 
> /**
>   552    * Returns a closure for invoking the regular method named [name].
>   553    *
>   554    * If [:type.instanceLookup(name):] returns a regular method, the
result
> of
>   555    * this method is a closure equivalent to:
>   556    *
>   557    *     (r1, .., rn, {p1: d1, ..., pk: dk}) {
>   558    *       return this.invoke(name, [r1, .., rn], {#p1: p1,  .., #pk:
> pk});
>   559    *     }
>   560    *
>   561    * if m has required parameters r1, ..., rn, and named parameters p1,
> ..., pk
>   562    * with defaults d1, ..., dk.
>   563    *
>   564    *     (r1, .., rn, [p1 = d1, …, pk = dk]) {
>   565    *       return this.invoke(name, [r1, .., rn, p1, .., pk]);
>   566    *     }
>   567    *
>   568    * if m has required parameters r1, ..., rn, and optional positional
>   569    * parameters p1, ..., pk with defaults d1, ..., dk.
>   570    */

I assume the other CL will land this change.

[Johnni Winther, 2013-10-30 08:02:25]

LGTM with comments.

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/_internal/li...
File dart/sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/_internal/li...
dart/sdk/lib/_internal/lib/js_mirrors.dart:79: Iterable<LibraryMirror>
findLibrary(Symbol libraryName) {
'Iterable<LibraryMirror>' -> 'LibraryMirror'.

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/mirrors/mirr...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:692: // TODO(ahe): Should include inherited
members?
Probably, since we now have [declarations] and [members] otherwise would be just
a subset of [declarations].

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:698: // DeclarationMirror
instanceLookup(Symbol name);
This is not needed if [members] include inherited members. It probably be
faster, though, if only access a few times on the class mirror.

[ahe, 2013-10-30 13:22:29]

Johnni, could you take another look.

I'm still working on removing "members".

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/mirrors/mirr...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/23455028/diff/82001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:692: // TODO(ahe): Should include inherited
members?
On 2013/10/30 08:02:26, Johnni Winther wrote:
> Probably, since we now have [declarations] and [members] otherwise would be
just
> a subset of [declarations].

This is a bad merge. It was replaced by declaration in the original CL.

[ahe, 2013-10-30 13:28:15]

Removed members, but still waiting for analyzer tests.

[Johnni Winther, 2013-10-30 13:47:48]

Still LGTM

[ahe, 2013-10-30 14:51:52]

Patchset #7 contains a few more tweaks. I'm submitting now.

[ahe, 2013-10-30 14:56:52]

Heads up: a few tweaks to:

dartdoc
pub
pkg/observe
pkg/path
pkg/polymer
pkg/serialization

[ahe, 2013-10-30 14:59:25]

Committed patchset #8 manually as r29551 (presubmit successful).

[Bob Nystrom, 2013-10-30 15:49:11]

path and pub changes LGTM. Thanks for the heads up!

- bob

[Alan Knight, 2013-10-30 16:27:39]

Serialization changes lgtm.

[rmacnak, 2013-10-30 16:55:37]

API, mirror tests and runtime LGTM

[Jennifer Messerly, 2013-10-30 19:50:43]

LGTM too :)