Clarifies the status of implicit accessors; fixed several related bugs.

Clarified the status of implicit accessors; fixed several related bugs.

Added a test 'implicit_getter_setter_test.dart' which explores the
properties of implicit getters and setters for various kinds of
instance and static variable declarations.

Refactored the representation of mirrors in transformed code, in order
to enable unusual things for this purpose: The parameter of an implicit
setter has a special name, finding the owner of several of the relevant
mirrors used to involve some circularities (that we couldn't see how
to break with the old setup),

Deleted several declarations which are not used because of the
refactoring. Deleted code which used to create implicit accessor
mirrors (actually it was dead code, which was one of the bugs, but
now it's replaced by different code in a different location).

Implemented `staticMembers` on `ClassMirrorImpl` in order to support
static implicit accessors, and added various members earlier in the
pipeline to support it (`_staticMemberCache` and others).

Reclassified 'parameter_mirrors_test.dart' in .status: It is actually
blocked by the same issue #8 as some other tests (moving code for
the value of default arguments).

R=sigurdm@google.com

Committed: https://github.com/dart-lang/reflectable/commit/49c7a5647e460844a1407493573d8e24abd77616

[eernst, 2015-08-20 08:53:50]

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

[eernst, 2015-08-20 08:53:51]

Refactors representation of information about fields and associated implicit
accessors, such that we can support a mirror on the argument of an implicit
setter, etc.

[sigurdm, 2015-08-20 09:39:53]

LGTM

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

https://codereview.chromium.org/1297503006/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:547: _unsupported();
Add comment describing what causes -1.
Maybe unsupported could take a string describing what exactly was unsupported.

https://codereview.chromium.org/1297503006/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:656: String toString() =>
"ImplicitGetterMirror($simpleName)";
Would ImplicitGetterMirror($qualifiedName) not be more informative?

https://codereview.chromium.org/1297503006/diff/1/test_reflectable/.status
File test_reflectable/.status (right):

https://codereview.chromium.org/1297503006/diff/1/test_reflectable/.status#ne...
test_reflectable/.status:13: build/test/parameter_mirrors_test: CompileTimeError
# Issue #8
Maybe we should split this up into a test with the features that works, and one
only using the problematic feature in order to get higher coverage.

[eernst, 2015-08-20 10:43:25]

Will land. Refactoring of the test that fails because of moving code (`const
C()` not defined in one library, because it's called `const original.C()`) would
be a separate CL.

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

https://codereview.chromium.org/1297503006/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:656: String toString() =>
"ImplicitGetterMirror($simpleName)";
On 2015/08/20 09:39:52, sigurdm wrote:
> Would ImplicitGetterMirror($qualifiedName) not be more informative?

Changed this occurrence plus some others (including `MethodMirrorImpl`) to use
`qualifiedName`, such that they are all pushed up to the same level that we have
in `ClassMirrorImpl`. Also fixed the class name (such that we can actually see
`Impl` in all cases).

https://codereview.chromium.org/1297503006/diff/1/test_reflectable/.status
File test_reflectable/.status (right):

https://codereview.chromium.org/1297503006/diff/1/test_reflectable/.status#ne...
test_reflectable/.status:13: build/test/parameter_mirrors_test: CompileTimeError
# Issue #8
On 2015/08/20 09:39:52, sigurdm wrote:
> Maybe we should split this up into a test with the features that works, and
one
> only using the problematic feature in order to get higher coverage.

Yes. The problem emerged because I added code to include information about
parameters, and you added metadata to a parameter, independently, and then we
require moving code when those two get together.

So maybe we should just remove that `@C()` annotation on the parameter, and
create a new test that puts metadata annotations in many different locations,
and checks them?

[eernst, 2015-08-20 10:44:17]

Committed patchset #2 (id:20001) manually as
49c7a5647e460844a1407493573d8e24abd77616 (presubmit successful).

[sigurdm, 2015-08-20 10:46:34]

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

https://codereview.chromium.org/1297503006/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:547: _unsupported();
On 2015/08/20 09:39:52, sigurdm wrote:
> Add comment describing what causes -1.
> Maybe unsupported could take a string describing what exactly was unsupported.

Did you overlook this?

https://codereview.chromium.org/1297503006/diff/1/test_reflectable/.status
File test_reflectable/.status (right):

https://codereview.chromium.org/1297503006/diff/1/test_reflectable/.status#ne...
test_reflectable/.status:13: build/test/parameter_mirrors_test: CompileTimeError
# Issue #8
On 2015/08/20 10:43:25, eernst wrote:
> On 2015/08/20 09:39:52, sigurdm wrote:
> > Maybe we should split this up into a test with the features that works, and
> one
> > only using the problematic feature in order to get higher coverage.
> 
> Yes. The problem emerged because I added code to include information about
> parameters, and you added metadata to a parameter, independently, and then we
> require moving code when those two get together.
> 
> So maybe we should just remove that `@C()` annotation on the parameter, and
> create a new test that puts metadata annotations in many different locations,
> and checks them?

That would work too

[eernst, 2015-08-20 12:55:11]

Review response; future CL upcoming, containing things missing in here.

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

https://codereview.chromium.org/1297503006/diff/1/reflectable/lib/src/mirrors...
reflectable/lib/src/mirrors_unimpl.dart:547: _unsupported();
On 2015/08/20 10:46:34, sigurdm wrote:
> On 2015/08/20 09:39:52, sigurdm wrote:
> > Add comment describing what causes -1.
> > Maybe unsupported could take a string describing what exactly was
unsupported.
> 
> Did you overlook this?

Forgot it half-way through.

Looked into it again, added TODOs: (1) when that index is generated, it is a bug
if the corresponding class is not in `classes`, (2) at this point `-1` means
'without capability`, so we throw for that). Remaining issue: How do we
recognize that there is no support for function types, and how do we catch
`dynamic`? (3) Added that to the TODO, too.