Changes to mirrors in support of metadata access at runtime. Assumes VM changes in flight.…

Changes to mirrors in support of metadata access at runtime.

R=iposva@google.com

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

[gbracha, 2013-06-11 01:32:51]

This CL is just so this doesn't get missed if I get stuck in Jury duty for an
extended period. There are a few doc changes etc. that crept in as well that
probably should be elsewhere.

[ahe, 2013-06-11 08:48:10]

The description of this CL is: "Proposed changes to mirrors in support of
metadata access at runtime. Assumes VM changes in flight. I'm posting this now
because I will be out tomorrow due to jury duty. Untested in the least."

This is the message that will be added to the Subversion log, and it is not a
good fit to use words like "proposal" or "proposed".  Once this submitted, it is
no longer a proposal.  Words like that belong in the message that you send out
with the first review request.  Similarly, notes about timing, other
obligations, and the level of testing do not belong in the CL description.  At
the point the CL is submitted, it will be tested (because we don't submit
untested changes).

As far as I'm concerned, this change looks reasonable, but before you can submit
it, tests need to be written (or start passing, see
dart/tests/lib/mirrors/metadata_test.dart), and the CL description should be
updated (click on the "Edit Issue" link in the top-left corner, or follow this
link: https://codereview.chromium.org/16757007/edit).

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1287: Dart_Handle reflectee = UnwrapMirror(mirror);
What happens if mirror is not a mirror?

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:74: native 'LocalMirrorImpl_metadata';
I don't think you can wrap the native declaration in curly braces.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:171: int get hashCode => _reference.hashCode;
This is a bad hashCode.

It should be the hashCode from Object (identity hashCode).

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:174: return (runtimeType == m.runtimeType) &&
(_reference == m._reference);
I don't understand the need to check for runtimeType, and I don't understand why
you use a private field for comparison.

I think this should be:

bool operator== (other) {
  return other is InstanceMirror && identical(reflectee, other.reflectee);
}

And so forth for other subclasses of _LocalVMObjectMirrorImpl.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:617: return _metadata(this).map((o) =>
reflect(o));
This should be:

List<InstanceMirror> get metadata => _metadata(this).map(reflect).toList();

Notice that you need to call toList(), and there's no need to create an explicit
closure.

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

https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart#...
sdk/lib/mirrors/mirrors.dart:229: * If D is decorated with annotations A1, ...,
An
Add * around the greek.

https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart#...
sdk/lib/mirrors/mirrors.dart:493: [Map<Symbol,dynamic> namedArguments]);
Add space after comma.

[hausner, 2013-06-11 16:17:13]

It does not look like this code would compile/run, so I would not check it in.
It would be nice to have at least a minimal test to check the code when my
changes are in.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:74: native 'LocalMirrorImpl_metadata';
This isn't legal dart code.

[hausner, 2013-06-11 16:18:04]

It does not look like this code would compile/run, so I would not check it in.
It would be nice to have at least a minimal test to check the code when my
changes are in.

[ahe, 2013-06-11 16:21:22]

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:74: native 'LocalMirrorImpl_metadata';
On 2013/06/11 16:17:13, hausner wrote:
> This isn't legal dart code.

Neither is this:

List<InstanceMirror> _metadata(mirror) native 'LocalMirrorImpl_metadata';

What matters is what the VM accepts.

[gbracha, 2013-06-12 21:18:58]

On 2013/06/11 08:48:10, ahe wrote:
> The description of this CL is: "Proposed changes to mirrors in support of
> metadata access at runtime. Assumes VM changes in flight. I'm posting this now
> because I will be out tomorrow due to jury duty. Untested in the least."
> 
> This is the message that will be added to the Subversion log, and it is not a
> good fit to use words like "proposal" or "proposed".  Once this submitted, it
is
> no longer a proposal.  Words like that belong in the message that you send out
> with the first review request.  Similarly, notes about timing, other
> obligations, and the level of testing do not belong in the CL description.  At
> the point the CL is submitted, it will be tested (because we don't submit
> untested changes).
> 
> As far as I'm concerned, this change looks reasonable, but before you can
submit
> it, tests need to be written (or start passing, see
> dart/tests/lib/mirrors/metadata_test.dart), and the CL description should be
> updated (click on the "Edit Issue" link in the top-left corner, or follow this
> link: https://codereview.chromium.org/16757007/edit).
> 
> https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors.cc
> File runtime/lib/mirrors.cc (right):
> 
>
https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors.cc#newcod...
> runtime/lib/mirrors.cc:1287: Dart_Handle reflectee = UnwrapMirror(mirror);
> What happens if mirror is not a mirror?
> 
> https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart
> File runtime/lib/mirrors_impl.dart (right):
> 
>
https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
> runtime/lib/mirrors_impl.dart:74: native 'LocalMirrorImpl_metadata';
> I don't think you can wrap the native declaration in curly braces.
> 
>
https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
> runtime/lib/mirrors_impl.dart:171: int get hashCode => _reference.hashCode;
> This is a bad hashCode.
> 
> It should be the hashCode from Object (identity hashCode).
> 
>
https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
> runtime/lib/mirrors_impl.dart:174: return (runtimeType == m.runtimeType) &&
> (_reference == m._reference);
> I don't understand the need to check for runtimeType, and I don't understand
why
> you use a private field for comparison.
> 
> I think this should be:
> 
> bool operator== (other) {
>   return other is InstanceMirror && identical(reflectee, other.reflectee);
> }
> 
> And so forth for other subclasses of _LocalVMObjectMirrorImpl.
> 
>
https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
> runtime/lib/mirrors_impl.dart:617: return _metadata(this).map((o) =>
> reflect(o));
> This should be:
> 
> List<InstanceMirror> get metadata => _metadata(this).map(reflect).toList();
> 
> Notice that you need to call toList(), and there's no need to create an
explicit
> closure.
> 
> https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart
> File sdk/lib/mirrors/mirrors.dart (right):
> 
>
https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart#...
> sdk/lib/mirrors/mirrors.dart:229: * If D is decorated with annotations A1,
...,
> An
> Add * around the greek.
> 
>
https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart#...
> sdk/lib/mirrors/mirrors.dart:493: [Map<Symbol,dynamic> namedArguments]);
> Add space after comma.

CL description updated.

[gbracha, 2013-06-12 21:19:48]

On 2013/06/11 16:18:04, hausner wrote:
> It does not look like this code would compile/run, so I would not check it in.
> It would be nice to have at least a minimal test to check the code when my
> changes are in.

A metadata test exists already. Not sure if it is active.

[gbracha, 2013-06-12 21:21:17]

This builds and passes the tests. I'm not sure if the metadata test is run.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors.cc#newcod...
runtime/lib/mirrors.cc:1287: Dart_Handle reflectee = UnwrapMirror(mirror);
On 2013/06/11 08:48:10, ahe wrote:
> What happens if mirror is not a mirror?

The first argument is the receiver. The only way you would get a non-mirror here
is if one ties code in a non-mirror class to this native entrypoint. This is
true of  other parts of the VM mirror implementation as well.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:74: native 'LocalMirrorImpl_metadata';
On 2013/06/11 16:21:22, ahe wrote:
> On 2013/06/11 16:17:13, hausner wrote:
> > This isn't legal dart code.
> 
> Neither is this:
> 
> List<InstanceMirror> _metadata(mirror) native 'LocalMirrorImpl_metadata';
> 
> What matters is what the VM accepts.

Done.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:171: int get hashCode => _reference.hashCode;
On 2013/06/11 08:48:10, ahe wrote:
> This is a bad hashCode.
> 
> It should be the hashCode from Object (identity hashCode).

This really should be part of a different CL dealing with mirror equality. I'll
take it out of this CL.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:174: return (runtimeType == m.runtimeType) &&
(_reference == m._reference);
On 2013/06/11 08:48:10, ahe wrote:
> I don't understand the need to check for runtimeType, and I don't understand
why
> you use a private field for comparison.
> 
> I think this should be:
> 
> bool operator== (other) {
>   return other is InstanceMirror && identical(reflectee, other.reflectee);
> }
> 
> And so forth for other subclasses of _LocalVMObjectMirrorImpl.

Indeed, it probably is in the equality CL. This was placeholder that got in by
accident. Removing, to be added in a different CL.

https://codereview.chromium.org/16757007/diff/1/runtime/lib/mirrors_impl.dart...
runtime/lib/mirrors_impl.dart:617: return _metadata(this).map((o) =>
reflect(o));
On 2013/06/11 08:48:10, ahe wrote:
> This should be:
> 
> List<InstanceMirror> get metadata => _metadata(this).map(reflect).toList();
> 
> Notice that you need to call toList(), and there's no need to create an
explicit
> closure.

Done. In all 5 places.

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

https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart#...
sdk/lib/mirrors/mirrors.dart:229: * If D is decorated with annotations A1, ...,
An
On 2013/06/11 08:48:10, ahe wrote:
> Add * around the greek.

Done.

https://codereview.chromium.org/16757007/diff/1/sdk/lib/mirrors/mirrors.dart#...
sdk/lib/mirrors/mirrors.dart:493: [Map<Symbol,dynamic> namedArguments]);
On 2013/06/11 08:48:10, ahe wrote:
> Add space after comma.

Done.

[gbracha, 2013-06-14 01:30:23]

Here's a version that actually works, except for metadata on libraries, which
the VM does not yet support. It is anything but optimal (we could cache the
metadata once it is computed, or at least the reflectee). Also I suspect there
are better ways to achieve some of the things done here (note the hack in the
Dart code to force the owner to be computed).

[ahe, 2013-06-14 04:47:27]

Since we're sharing the test, you cannot just comment out the check of library
metadata. That would make the feature untested in dart2js. One possible solution
is to move the library metadata into another test. That is rather easy as the
new test can import the existing test. 

Also, I have a strong attachment to the print statement in the test. It serves a
number of purposes:

1. Prevent an optimizing compiler from throwing it all away.

2. Makes it really easy to confirm that the test is working correctly when
running from command-line. We're having serious issues with async tests not
doing what people think they should. I believe we would have less issues like
that if people used print more than they do today.

[Ivan Posva, 2013-06-14 15:16:48]

LGTM with comments addressed.

-Ivan

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:185: static Dart_Handle UnwrapMirror(Dart_Handle mirror);
Indentation. Here and many places below.

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1217: Dart_NativeArguments args) {
One line?

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:170: 
Extra line?

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:610: List<InstanceMirror> get metadata =>
_metadata(this).map(reflect).toList();
You might want to explain what you are using reflect for in this pattern.

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
File tests/lib/mirrors/metadata_test.dart (left):

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
tests/lib/mirrors/metadata_test.dart:42: print(metadata);
Please leave the print in.

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
File tests/lib/mirrors/metadata_test.dart (right):

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
tests/lib/mirrors/metadata_test.dart:54: /* 
checkMetadata(mirrors.findLibrary(const Symbol('test.metadata_test')).first,
I think the easiest way is to split this into separate tests and set the correct
expectations.

[gbracha, 2013-06-14 20:42:46]

Ok, all tests now passing. Will commit shortly. There is a new, separate library
metadata test that is now disabled.

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:185: static Dart_Handle UnwrapMirror(Dart_Handle mirror);
On 2013/06/14 15:16:48, Ivan Posva wrote:
> Indentation. Here and many places below.

Done.

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1217: Dart_NativeArguments args) {
On 2013/06/14 15:16:48, Ivan Posva wrote:
> One line?

Done.

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:170: 
On 2013/06/14 15:16:48, Ivan Posva wrote:
> Extra line?

Done.

https://codereview.chromium.org/16757007/diff/24001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:610: List<InstanceMirror> get metadata =>
_metadata(this).map(reflect).toList();
On 2013/06/14 15:16:48, Ivan Posva wrote:
> You might want to explain what you are using reflect for in this pattern.

Done.

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
File tests/lib/mirrors/metadata_test.dart (left):

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
tests/lib/mirrors/metadata_test.dart:42: print(metadata);
On 2013/06/14 15:16:48, Ivan Posva wrote:
> Please leave the print in.

Done.

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
File tests/lib/mirrors/metadata_test.dart (right):

https://codereview.chromium.org/16757007/diff/24001/tests/lib/mirrors/metadat...
tests/lib/mirrors/metadata_test.dart:54: /* 
checkMetadata(mirrors.findLibrary(const Symbol('test.metadata_test')).first,
On 2013/06/14 15:16:48, Ivan Posva wrote:
> I think the easiest way is to split this into separate tests and set the
correct
> expectations.

Done.

[gbracha, 2013-06-14 21:13:05]

Committed patchset #6 manually as r24050 (presubmit successful).