Creates `reflectors`, as a meta-meta feature that enables dynamic selection of a "mirror system".

Creates `reflectors`, as a meta-meta feature enabling dynamic selection of the "mirror system".

When a library is expected to be used in a context where some useful
reflectors have already been created, it is difficult for that library
to find the reflectors, but it is very easy to return the set of all
reflectors from a static method in `Reflectable`. As a result, such a
library can now offer services using reflectable based reflection,
without requiring an `init` method to learn about which reflectors
there are, and without creating its own reflector (which may cause a
lot of information to be duplicated).

R=sigurdm@google.com

Committed: https://github.com/dart-lang/reflectable/commit/23de9d2f6b6fe65dd74ab833c11351ea5bc0362e

[eernst, 2015-10-06 16:01:31]

eernst@google.com changed reviewers:
+ sigurdm@google.com

[eernst, 2015-10-06 16:01:31]

Adds a `reflectors` static method that provides access to all reflectors in the
current program.

[sigurdm, 2015-10-07 08:31:59]

(As discussed offline) I am not so happy about exposing the set of Reflectors -
I feel that using this in most practical  situations will lead to very bad
structure of the program, and make it hard to understand what is going on, and
how much reflection data is generated/needed.

Also ideally this could be achieved with what we already have something like:

@GlobalQuantifyCapability(Reflectable, MetaReflector)
import "package:reflectable/reflectable.dart";

class MetaReflector extends Reflectable {
  const MetaReflector() : super(
      const InstanceInvokeCapability("reflect"),
      subtypeQuantifyCapability);
}

And use `metaReflector.annotatedClasses`

Not sure how to get to the instances though.

[eernst, 2015-10-08 18:27:49]

PTAL,

lots of code & debugging was needed in order to support libraries in a more
general way than that which has been tested so far; `reflectors` still present,
but I think we were in near-agreement about doing that.

[sigurdm, 2015-10-09 07:52:04]

I think "near-agreeing" is a bit of spin :)
I give a LGTM - but still I only foresee problems with providing the raw
`reflectors` set.
Like I said yesterday I prefer the encapsulation of a function (getInstance,
perhaps with a better name), so the user cannot iterate over all values, but is
able to get an instance from a classMirror - I can see some valid use-cases for
that.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/reflect...
File reflectable/lib/reflectable.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/reflect...
reflectable/lib/reflectable.dart:112: static Set<Reflectable> get reflectors =>
implementation.reflectors;
I still think it would be better to give:

static Reflector getInstance(ClassMirror reflectorClass);

So people are not tempted to browse through the values.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/enc...
File reflectable/lib/src/encoding_constants.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/enc...
reflectable/lib/src/encoding_constants.dart:19: const clazz = 7; // Cannot use
keyword `class`.
I think the convention is to write klass. Personally I don't care.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/ref...
File reflectable/lib/src/reflectable_transformer_based.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/ref...
reflectable/lib/src/reflectable_transformer_based.dart:1254: print("libraries:
$this"); // DEBUG
Debug print

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:998: }));
A newline here would look better.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:1366: String name =
setterName.substring(setterName.length - 1) == "="
Can we now get a setterName not ending in '='? When is that?

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:2261: String name = subClass
== null
Could be
```
String name = subClass ?? (type.isMixinApplication ? type.name : null);
```

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:3090:
Iterable<TopLevelVariableElement> _extractDeclaredVariables(
Give these functions a (short) comment

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:3387: bool get isEnum =>
mixin.isEnum || superclass.isEnum;
As I understand it you cannot mix an enum in or subclass it, so I think we can
just return `false`.

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_domain.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_domain.dart:5: // File being transformed
by the reflectable transformer.
Could be doc-comment of the library annotation.

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_test.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_test.dart:29: Map<String,
Iterable<Reflectable>> scopeMap = <String, Iterable<Reflectable>>{
While this is a perfectly valid approach, I still think this mapping could be
more elegantly defined by having the scopes represented as Reflectors. 
Then adding a reflector to a scope would amount to annotating it with the
scope-reflector (perhaps via GlobalQuantifyCapability).

Each of the scope-reflectors is then annotated by scope-meta-reflector and the
mapping can be constructed as:

```
Map<Reflectable, Iterable<Reflectable>> scopeMap = {};
scopeMetaReflector.annotatedClasses.forEach(
    (ClassMirror scopeReflectorMirror) {
  Reflectable scopeReflector =
      Reflectable.getInstance(scopeReflectorMirror);
  scopeMap[scopeReflector] = 
      scopeReflector.annotatedClasses.map(Reflectable.getInstance);
});
```

Then any package can add reflectors to any scope that it knows about, and the
mapping is automatically updated. (in your case the mapping needs to be in the
end-users control, otherwise it cannot be updated).

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_user.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_user.dart:61: doit() {
what is 'it'? maybe call it: 'runTests'

[eernst, 2015-10-09 10:09:05]

Review response.

Note that some redundant elements were removed from `_[gs]ettersOfLibrary`, some
functions were renamed for consistency. PTAL.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/reflect...
File reflectable/lib/reflectable.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/reflect...
reflectable/lib/reflectable.dart:112: static Set<Reflectable> get reflectors =>
implementation.reflectors;
On 2015/10/09 07:52:04, sigurdm wrote:
> I still think it would be better to give:
> 
> static Reflector getInstance(ClassMirror reflectorClass);
> 
> So people are not tempted to browse through the values.

Introduced a `getInstance` static method and adjusted the examples to use that
instead.

`reflectors` is no more available.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/enc...
File reflectable/lib/src/encoding_constants.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/enc...
reflectable/lib/src/encoding_constants.dart:19: const clazz = 7; // Cannot use
keyword `class`.
On 2015/10/09 07:52:04, sigurdm wrote:
> I think the convention is to write klass. Personally I don't care.

Check
http://stackoverflow.com/questions/2529974/why-do-java-programmers-like-to-na...

Haven't changed it.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/ref...
File reflectable/lib/src/reflectable_transformer_based.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/ref...
reflectable/lib/src/reflectable_transformer_based.dart:1254: print("libraries:
$this"); // DEBUG
On 2015/10/09 07:52:04, sigurdm wrote:
> Debug print

Done.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:998: }));
On 2015/10/09 07:52:04, sigurdm wrote:
> A newline here would look better.

Not sure what you mean?

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:1366: String name =
setterName.substring(setterName.length - 1) == "="
On 2015/10/09 07:52:04, sigurdm wrote:
> Can we now get a setterName not ending in '='? When is that?

I found `someName = value` in the generated code along with `someNam = value`
(where `someNam` does not exist separately, it's simply `someName` incorrectly
cut down one char), so clearly the method is being called with both "the setter
name with '=' and without". This change fixed that, but I have still seen it so
there must be another location doing the same thing.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:2261: String name = subClass
== null
On 2015/10/09 07:52:04, sigurdm wrote:
> Could be
> ```
> String name = subClass ?? (type.isMixinApplication ? type.name : null);
> ```

It's a bit more tricky than that: If `subClass == null` then the result of the
whole expression must be `null`, but the expression you mention would not have
that property.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:3090:
Iterable<TopLevelVariableElement> _extractDeclaredVariables(
On 2015/10/09 07:52:04, sigurdm wrote:
> Give these functions a (short) comment

Done.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:3387: bool get isEnum =>
mixin.isEnum || superclass.isEnum;
On 2015/10/09 07:52:04, sigurdm wrote:
> As I understand it you cannot mix an enum in or subclass it, so I think we can
> just return `false`.

Yep, the spec confirms that. Removed the comment, now returns false.

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_domain.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_domain.dart:5: // File being transformed
by the reflectable transformer.
On 2015/10/09 07:52:04, sigurdm wrote:
> Could be doc-comment of the library annotation.

Ah, didn't think about that! Done.

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_test.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_test.dart:29: Map<String,
Iterable<Reflectable>> scopeMap = <String, Iterable<Reflectable>>{
On 2015/10/09 07:52:04, sigurdm wrote:
> While this is a perfectly valid approach, I still think this mapping could be
> more elegantly defined by having the scopes represented as Reflectors. 
> Then adding a reflector to a scope would amount to annotating it with the
> scope-reflector (perhaps via GlobalQuantifyCapability).
> 
> Each of the scope-reflectors is then annotated by scope-meta-reflector and the
> mapping can be constructed as:
> 
> ```
> Map<Reflectable, Iterable<Reflectable>> scopeMap = {};
> scopeMetaReflector.annotatedClasses.forEach(
>     (ClassMirror scopeReflectorMirror) {
>   Reflectable scopeReflector =
>       Reflectable.getInstance(scopeReflectorMirror);
>   scopeMap[scopeReflector] = 
>       scopeReflector.annotatedClasses.map(Reflectable.getInstance);
> });
> ```
> 
> Then any package can add reflectors to any scope that it knows about, and the
> mapping is automatically updated. (in your case the mapping needs to be in the
> end-users control, otherwise it cannot be updated).

Why don't you write that as an extra test case? ;)

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_user.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_user.dart:61: doit() {
On 2015/10/09 07:52:04, sigurdm wrote:
> what is 'it'? maybe call it: 'runTests'

Done.

[sigurdm, 2015-10-09 10:47:20]

LGTM

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:998: }));
On 2015/10/09 10:09:05, eernst wrote:
> On 2015/10/09 07:52:04, sigurdm wrote:
> > A newline here would look better.
> 
> Not sure what you mean?
Initialization of settersCode and variableIndices are separated by a newline.
You could separate gettersCode from settersCode likewise.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:1366: String name =
setterName.substring(setterName.length - 1) == "="
On 2015/10/09 10:09:05, eernst wrote:
> On 2015/10/09 07:52:04, sigurdm wrote:
> > Can we now get a setterName not ending in '='? When is that?
> 
> I found `someName = value` in the generated code along with `someNam = value`
> (where `someNam` does not exist separately, it's simply `someName` incorrectly
> cut down one char), so clearly the method is being called with both "the
setter
> name with '=' and without". This change fixed that, but I have still seen it
so
> there must be another location doing the same thing.

I think we should track down where that happens. Maybe change the assert to a
hard fail.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:2261: String name = subClass
== null
On 2015/10/09 10:09:04, eernst wrote:
> On 2015/10/09 07:52:04, sigurdm wrote:
> > Could be
> > ```
> > String name = subClass ?? (type.isMixinApplication ? type.name : null);
> > ```
> 
> It's a bit more tricky than that: If `subClass == null` then the result of the
> whole expression must be `null`, but the expression you mention would not have
> that property. 
Oops - you're right!

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
File test_reflectable/test/meta_reflectors_test.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/test_reflectable/test/m...
test_reflectable/test/meta_reflectors_test.dart:29: Map<String,
Iterable<Reflectable>> scopeMap = <String, Iterable<Reflectable>>{
On 2015/10/09 10:09:05, eernst wrote:
> On 2015/10/09 07:52:04, sigurdm wrote:
> > While this is a perfectly valid approach, I still think this mapping could
be
> > more elegantly defined by having the scopes represented as Reflectors. 
> > Then adding a reflector to a scope would amount to annotating it with the
> > scope-reflector (perhaps via GlobalQuantifyCapability).
> > 
> > Each of the scope-reflectors is then annotated by scope-meta-reflector and
the
> > mapping can be constructed as:
> > 
> > ```
> > Map<Reflectable, Iterable<Reflectable>> scopeMap = {};
> > scopeMetaReflector.annotatedClasses.forEach(
> >     (ClassMirror scopeReflectorMirror) {
> >   Reflectable scopeReflector =
> >       Reflectable.getInstance(scopeReflectorMirror);
> >   scopeMap[scopeReflector] = 
> >       scopeReflector.annotatedClasses.map(Reflectable.getInstance);
> > });
> > ```
> > 
> > Then any package can add reflectors to any scope that it knows about, and
the
> > mapping is automatically updated. (in your case the mapping needs to be in
the
> > end-users control, otherwise it cannot be updated).
> 
> Why don't you write that as an extra test case? ;)

Good idea!

https://codereview.chromium.org/1391543003/diff/40001/reflectable/lib/reflect...
File reflectable/lib/reflectable.dart (right):

https://codereview.chromium.org/1391543003/diff/40001/reflectable/lib/reflect...
reflectable/lib/reflectable.dart:114: /// there is such an instance which is
being used as a reflector. If
Add new paragraph after first sentence.

[eernst, 2015-10-09 11:24:14]

Review response. Landing now.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:998: }));
On 2015/10/09 10:47:20, sigurdm wrote:
> On 2015/10/09 10:09:05, eernst wrote:
> > On 2015/10/09 07:52:04, sigurdm wrote:
> > > A newline here would look better.
> > 
> > Not sure what you mean?
> Initialization of settersCode and variableIndices are separated by a newline.
> You could separate gettersCode from settersCode likewise.

Done.

https://codereview.chromium.org/1391543003/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:1366: String name =
setterName.substring(setterName.length - 1) == "="
On 2015/10/09 10:47:20, sigurdm wrote:
> On 2015/10/09 10:09:05, eernst wrote:
> > On 2015/10/09 07:52:04, sigurdm wrote:
> > > Can we now get a setterName not ending in '='? When is that?
> > 
> > I found `someName = value` in the generated code along with `someNam =
value`
> > (where `someNam` does not exist separately, it's simply `someName`
incorrectly
> > cut down one char), so clearly the method is being called with both "the
> setter
> > name with '=' and without". This change fixed that, but I have still seen it
> so
> > there must be another location doing the same thing.
> 
> I think we should track down where that happens. Maybe change the assert to a
> hard fail.

Did that, and it doesn't fail now. I think this may have happened because I
removed the `variables` from the iteration that finds features in libraries (we
used to have duplicates because of that). Changed the function back to the
original form using `assert`.

https://codereview.chromium.org/1391543003/diff/40001/reflectable/lib/reflect...
File reflectable/lib/reflectable.dart (right):

https://codereview.chromium.org/1391543003/diff/40001/reflectable/lib/reflect...
reflectable/lib/reflectable.dart:114: /// there is such an instance which is
being used as a reflector. If
On 2015/10/09 10:47:20, sigurdm wrote:
> Add new paragraph after first sentence.

Done.

[eernst, 2015-10-09 11:24:38]

Committed patchset #4 (id:60001) manually as
23de9d2f6b6fe65dd74ab833c11351ea5bc0362e (presubmit successful).