Implements support for reflection on parameters.

Implements support for reflection on parameters.

During the analysis phase needed in order to see where I could store the
parameter related information, I changed many declarations from public
to private, and removed many unused classes in mirrors_unimpl.dart. Also
renamed `_reflectable` to `_reflector` a few places, for consistency.

Added `parameterMirrors` to `ReflectorData`, passed that information all
the way through to the *_reflection_data.dart files, and used that to
implement `ParameterMirrorImpl`. Extracted `VariableMirrorBase` from
`VariableMirrorImpl` as a new superclass containing the shared material
(which also means that we can have `ClassMirror` as the return type of
`MethodMirror`s `owner` and `MethodMirror` for `ParameterMirror`s `owner`,
which would otherwise mess up the return type variance of `owner`).

Strengthened the implementation in 'reflectable_implementation.dart'
such that `declarations` can only be invoked when a
`declarationsCapability` is present, as intended according to the design
document. Enforcing the same discipline in the transformer and hence
in generated code.

Added a mirror implementation for the type `dynamic` (presumably, we cannot
expect to avoid that type, and it is not possible to request support for
it using existing capabilities, and it's kind of silly to create a new
capability for a couple of odd-balls like `dynamic` (maybe `void` and
`Null` will be included for the same reason).

Decided that the set of type mirrors included should be extended will
all reachable parameter types: If we have `typeCapability` and
`declarationsCapability` then we can get `declarations` and for the
methods among them `parameters`. We should then also be able to get
a mirror for the types of those parameters, even in the case where no
other reason justifies that those `int` types should be included (e.g.,
if you have `int foo(String s)..' then `int` and `String` mirrors will
be available. This may be expensive, but requiring that all those argument
types are requsted otherwise would be very tedious. Programmers can avoid
it by omitting `typeCapability`!

Parameters whose type is a function type are not covered by this CL.

Lots of tests had to be adjusted because they violated the capability
semantics: When a test needs to call `declarations` it must also have
`declarationCapability`.

Finally, unfortunately, a number of diff chunks were created by
dartfmt, because it makes different decisions today than it did last
time those files were edited.

R=floitsch@google.com

Committed: https://github.com/dart-lang/reflectable/commit/2696c189b2b4cbcbf92a608b682761954c89424c

[eernst, 2015-08-13 15:17:40]

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

[eernst, 2015-08-13 15:17:40]

In response to the priorities indicated by Jacob Macdonald, support for
reflection on parameters was pushed to the front of the queue. This CL ignores
genericity, but otherwise it does most of what is required for this area.

[jakemac, 2015-08-13 16:07:01]

On 2015/08/13 15:17:40, eernst wrote:
> In response to the priorities indicated by Jacob Macdonald, support for
> reflection on parameters was pushed to the front of the queue. This CL ignores
> genericity, but otherwise it does most of what is required for this area.

Polymer doesn't actually need to reflect on the types of parameters of functions
(today at least), but instead but the types of the fields. I couldn't tell for
sure whether this cl adds that or not (the tests are all testing parameter
types, not field types).

[floitsch, 2015-08-13 17:33:36]

LGTM.

I don't like the private classes. In almost all cases one can just move them
into a different library.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
File reflectable/lib/src/mirrors_unimpl.dart (right):

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:114: _dataCache = data[_reflector];
Why is this lazy? It's just a lookup into a map. That shouldn't be a problem
when the constructing a mirror-object.

If it doesn't need to be lazy, you can remove the class again.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:208: // test that compares owners in
pre/post-transform code.
What's missing to write the test?

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:210: variableMirror._ownerIndex,
variableMirror._reflector, [], null, -1)],
long line.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:567: bool get isPrivate => 0 !=
_descriptor & constants.privateAttribute;
parenthesis and put 0 to the right.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:573: bool get isFinal => 0 !=
_descriptor & constants.finalAttribute;
ditto. (and probably other locations).

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:664: "Attempting to get a class mirror
for an un-marked class (type of $_name)");
long line.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/transfo...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/transfo...
reflectable/lib/src/transformer_implementation.dart:58: bool _contains(T t) =>
_map.containsKey(t);
I don't think it's worth having this redirection just for two uses.

[eernst, 2015-08-14 12:05:34]

Review response. Will land now.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
File reflectable/lib/src/mirrors_unimpl.dart (right):

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:114: _dataCache = data[_reflector];
On 2015/08/13 17:33:36, floitsch wrote:
> Why is this lazy? It's just a lookup into a map. That shouldn't be a problem
> when the constructing a mirror-object.
> 
> If it doesn't need to be lazy, you can remove the class again.

We have done that for a while, I just created the class and used it as a mixin
in order to avoid the code duplication. It is concerned with all lookups into
`data`, which means that an application doing a lot of reflection work could
perform that map lookup many times. Whether it is a useful type of caching is
hard to tell. Added a TODO indicating that we should turn it on and off and see
the effect when we have some more substantial pieces of software using it.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:210: variableMirror._ownerIndex,
variableMirror._reflector, [], null, -1)],
On 2015/08/13 17:33:36, floitsch wrote:
> long line.

Fixed. Btw, doing the usual 'Reformat with Dart style' in IntelliJ creates a 162
line diff (apparently, we hadn't done that to this particular file since the
last update to dartfmt).

Sigurd and I have agreed to isolate this kind of noise in each CL in the sense
that we will generally create a first patch set which is the result of running
dartfmt on all source files at first. This means that the first patch set is
pure auto-reformatting, and then reviewers can quickly verify what is going on
in patch 1 and then check out the diff starting from 2 in order to see the
topic-related updates.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:567: bool get isPrivate => 0 !=
_descriptor & constants.privateAttribute;
On 2015/08/13 17:33:36, floitsch wrote:
> parenthesis and put 0 to the right.

Done.

Btw, that's a very old C-ish habit: `if (NULL = p)..` is a buggy version of `if
(p == NULL)..` just like `if (p = NULL)..`, but the latter is accepted by the
compiler, so the ordering "<constant> <relation> <variable>" is considered a
good habit. But we don't actually need that. ;)

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:573: bool get isFinal => 0 !=
_descriptor & constants.finalAttribute;
On 2015/08/13 17:33:36, floitsch wrote:
> ditto. (and probably other locations).

Done.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:664: "Attempting to get a class mirror
for an un-marked class (type of $_name)");
On 2015/08/13 17:33:36, floitsch wrote:
> long line.

Funny, 'Reformat with Dart style' does not output many long lines (we used to
have to search for long lines after every 'Reformat..'), but this one persists.
Done.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/transfo...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/transfo...
reflectable/lib/src/transformer_implementation.dart:58: bool _contains(T t) =>
_map.containsKey(t);
On 2015/08/13 17:33:36, floitsch wrote:
> I don't think it's worth having this redirection just for two uses.

The reason why I created the abstraction is that it seems OK for a local
expression to access `_map`, but it is less obvious that usages outside the
class should do such a thing (after all, building the `Enumerator` behavior on a
`Map` is just one possible implementation). So I created `_contains` when the
need arose outside `class Enumerator`, and then it seemed most reasonable to use
it also inside the class.

Hence, I haven't deleted it yet.

[eernst, 2015-08-14 19:26:21]

A number of adjustments were required in order to merge with the latest `git cl
land` to github. Because of this, I did not yet land it after all, PTAL.

[eernst, 2015-08-17 14:07:39]

Landing now, further review comments will be taken into account in the next CL.

[eernst, 2015-08-17 14:08:25]

Committed patchset #3 (id:40001) manually as
2696c189b2b4cbcbf92a608b682761954c89424c (presubmit successful).

[floitsch, 2015-08-17 14:32:23]

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
File reflectable/lib/src/mirrors_unimpl.dart (right):

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:114: _dataCache = data[_reflector];
On 2015/08/14 12:05:34, eernst wrote:
> On 2015/08/13 17:33:36, floitsch wrote:
> > Why is this lazy? It's just a lookup into a map. That shouldn't be a problem
> > when the constructing a mirror-object.
> > 
> > If it doesn't need to be lazy, you can remove the class again.
> 
> We have done that for a while, I just created the class and used it as a mixin
> in order to avoid the code duplication. It is concerned with all lookups into
> `data`, which means that an application doing a lot of reflection work could
> perform that map lookup many times. Whether it is a useful type of caching is
> hard to tell. Added a TODO indicating that we should turn it on and off and
see
> the effect when we have some more substantial pieces of software using it.

Why is it not filled at construction?

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:208: // test that compares owners in
pre/post-transform code.
On 2015/08/13 17:33:36, floitsch wrote:
> What's missing to write the test?

?

[eernst, 2015-08-19 13:41:56]

Followup on old issues, with refs to new CLs.

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
File reflectable/lib/src/mirrors_unimpl.dart (right):

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:114: _dataCache = data[_reflector];
On 2015/08/17 14:32:23, floitsch wrote:
> On 2015/08/14 12:05:34, eernst wrote:
> > On 2015/08/13 17:33:36, floitsch wrote:
> > > Why is this lazy? It's just a lookup into a map. That shouldn't be a
problem
> > > when the constructing a mirror-object.
> > > 
> > > If it doesn't need to be lazy, you can remove the class again.
> > 
> > We have done that for a while, I just created the class and used it as a
mixin
> > in order to avoid the code duplication. It is concerned with all lookups
into
> > `data`, which means that an application doing a lot of reflection work could
> > perform that map lookup many times. Whether it is a useful type of caching
is
> > hard to tell. Added a TODO indicating that we should turn it on and off and
> see
> > the effect when we have some more substantial pieces of software using it.
> 
> Why is it not filled at construction?

Could do that, check https://codereview.chromium.org/1284423004

[Since I wrote the above, that experiment showed that it was not easy to make
such a change, so we have abandoned that CL.]

https://codereview.chromium.org/1289933004/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:208: // test that compares owners in
pre/post-transform code.
On 2015/08/17 14:32:23, floitsch wrote:
> On 2015/08/13 17:33:36, floitsch wrote:
> > What's missing to write the test?
> 
> ?

Oops, forgot that. Turned out to require more refactorings than I had expected.
Followup in https://codereview.chromium.org/1297503006.