Implement more of the mirrors library, primarily stuff to do with types.

Implement more of the mirrors library, primarily stuff to do with types.

Highlights:

- Implement FunctionClassMirror, used to reflect on the class of
  function objects.

- Implement TypeVariableMirror, used to reflect on type variables.

- Implement TypedefMirror, used to reflect on typedefs.

- Implement returnType for MethodMirror

- Implement type for VariableMirror/ParameterMirror

- Added supporting functions to the dart embedding api:
  - Dart_ClassIsTypedef
  - Dart_ClassGetTypedefReferent
  - Dart_ClassIsFunctionClass
  - Dart_ClassGetFunctionClassSignature
  - Dart_FunctionReturnType
  - Dart_FunctionParameterType
  - Dart_VariableType
  - Dart_GetTypeVariableNames
  - Dart_LookupTypeVariable
  - Dart_IsTypeVariable
  - Dart_TypeVariableOwner
  - Dart_TypeVariableUpperBound

Other stuff:

- Fixed bug in Function::NumberOfImplicitParameters for signature
  functions.

- Chnaged the default upper bound for type variables to Object from
  Dynamic.  Discussed this with Gilad and Regis.

- Fixed some smaller problems in mirrors_impl.dart.
Committed: https://code.google.com/p/dart/source/detail?r=12342

[gbracha, 2012-09-12 01:36:40]

I have some semantic concerns I mentioned in the comments: constrcutors are not
members, the return type of constructor is the class it is in, not Dynamic, and
the bound of a type variable is Object not Dynamic.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/isolate_mirror_local_test.dart (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_mirror_local_test.dart:245: func =
cls_mirror.members['MyClass'];
Is this supposed to find the constructor of MyClass? Constructors are not
members.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_mirror_local_test.dart:248: 
What does it mean for a constructor to return Dynamic? It returns the type of
the class it constructs (MyClass).

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_mirror_local_test.dart:278: 
Actually, the upper bound of a type variable defaults to Object.

[cshapiro, 2012-09-12 18:52:07]

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

https://chromiumcodereview.appspot.com/10916252/diff/1/lib/mirrors/mirrors.da...
lib/mirrors/mirrors.dart:405: * This map preserves the order of declaration of
the type variables.
I am not sure I understand the value of this comment.  Lists and maps are both
order preserving, so what is the point of mentioning it?  Also, the comment on
line 403 did not change, so I am having trouble with the big picture.

https://chromiumcodereview.appspot.com/10916252/diff/1/lib/mirrors/mirrors.da...
lib/mirrors/mirrors.dart:412: * This map preserves the order of declaration of
the type variables.
Same here as above.

https://chromiumcodereview.appspot.com/10916252/diff/1/lib/mirrors/mirrors.da...
lib/mirrors/mirrors.dart:493: * A mirror on the type that is the upper bound for
this type variable.
Maybe "of this type variable" instead of "for this..." ?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2047: * special kind of class handle.  These typedef
classes allow the
typedef classes or typedef class handles?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2049: * Dart_ClassGetTypedefReferent.
Wordy.

"Dart_ClassGetTypedefReferent returns the class handle referent."

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2053: DART_EXPORT bool
Dart_ClassIsTypedef(Dart_Handle handle);
Rename "handle" to "klass", "cls", or "clazz" ?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2059: * true.
Why?  I would expect this function to return an error if a precondition is
violated.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2068: DART_EXPORT bool
Dart_ClassIsFunctionType(Dart_Handle handle);
Rename "handle" to "klass", "cls", or "clazz" ?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2071: * Returns a signature function for a function
type.
signature function or function signature?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2191: * Returns a handle to the return type of a
function.
I am confused about the pervasive use of handle in this documentation.  The
handle seems implied.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:254: } else if (Dart_IsClass(target) ||
Dart_IsInterface(target)) {
why else if when you return on line 252?  That is kinda what happens on line
246.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:284: } else if (Dart_IsTypeVariable(target)) {
instead of else if, make this an ASSERT instead?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:361: Dart_Handle result;
move this down to line 372.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:376: for (int i = 0; i < len; i++) {
intptr_t ?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:565: Dart_Handle type = Dart_VariableType(var);
move this above the is_* stuff?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:81: if (mirror === null) {
You don't need === anymore for speed.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:347: if (resolved === null) {
ditto

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:349: "Mirror for type '$typeName' not
implemented");
"is not..." or "is unimplemented"

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:443: if (_methods === null) {
==

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:451: if (_constructors === null) {
==

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:459: if (_getters === null) {
more of the same, here and below

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:613: TypeMirror get upperBound() {
no () here and around here

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:910: : super('<TODO:unnamed>', null, type, false,
false) {}
Strange.  Why not leave the TODO comment?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:2487: // ASSERT(cls.IsDynamicClass() ||
cls.IsVoidClass());
This should have a TODO for the commented out code.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:2489: fprintf(stderr, "NO LIBRARY: %s\n",
cls.ToCString());
fprintf?  Why not use something in OS instead ?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:2576: // For now, the only typedefs are
non-canonical signature classes.
For now all typedefs are non-canonical signature classes.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl_test.cc:2397: EXPECT_VALID(lib);
Can we check something about the type of the object in the handle here? 
Something similar happens below for the cls things.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl_test.cc:4770: Dart_Handle lib =
TestCase::LoadTestScript(kScriptChars, NULL);
check lib

[turnidge, 2012-09-12 19:55:20]

Gilad -- ready for another look.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/isolate_mirror_local_test.dart (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_mirror_local_test.dart:245: func =
cls_mirror.members['MyClass'];
On 2012/09/12 01:36:40, gbracha wrote:
> Is this supposed to find the constructor of MyClass? Constructors are not
> members.

Ok, reworked the meanings of 'members', 'constructors', and 'methods' for
ClassMirrors to agree with the spec's notion of membership.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_mirror_local_test.dart:248: 
On 2012/09/12 01:36:40, gbracha wrote:
> What does it mean for a constructor to return Dynamic? It returns the type of
> the class it constructs (MyClass).

I patched up Dart_FunctionReturnType to say the return type for a constructor is
the type of the object being constructed.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/tests/vm/dart/...
runtime/tests/vm/dart/isolate_mirror_local_test.dart:278: 
On 2012/09/12 01:36:40, gbracha wrote:
> Actually, the upper bound of a type variable defaults to Object.

Added a fix in parser.cc for this.

[gbracha, 2012-09-12 20:14:35]

lgtm for the Dart code

[turnidge, 2012-09-12 21:39:30]

Carl -- ready for another look.

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

https://chromiumcodereview.appspot.com/10916252/diff/1/lib/mirrors/mirrors.da...
lib/mirrors/mirrors.dart:405: * This map preserves the order of declaration of
the type variables.
On 2012/09/12 18:52:07, cshapiro wrote:
> I am not sure I understand the value of this comment.  Lists and maps are both
> order preserving, so what is the point of mentioning it?  Also, the comment on
> line 403 did not change, so I am having trouble with the big picture.

Since maps are not by default ordered, this comment is required.  Another option
would be to have something like an OrderedMap, if that existed.

I've added a comment in mirrors.cc to actually use an ordered map, which I
current do not.

https://chromiumcodereview.appspot.com/10916252/diff/1/lib/mirrors/mirrors.da...
lib/mirrors/mirrors.dart:493: * A mirror on the type that is the upper bound for
this type variable.
On 2012/09/12 18:52:07, cshapiro wrote:
> Maybe "of this type variable" instead of "for this..." ?

Done.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2047: * special kind of class handle.  These typedef
classes allow the
On 2012/09/12 18:52:07, cshapiro wrote:
> typedef classes or typedef class handles?

Done.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2049: * Dart_ClassGetTypedefReferent.
On 2012/09/12 18:52:07, cshapiro wrote:
> Wordy.
> 
> "Dart_ClassGetTypedefReferent returns the class handle referent."

Made less wordy.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2059: * true.
On 2012/09/12 18:52:07, cshapiro wrote:
> Why?  I would expect this function to return an error if a precondition is
> violated.

Just so.  I'm telling them how to avoid getting an error.  Changed the wording
to be more explicit:

 * It is an error to call this function on a handle for which
 * Dart_ClassIsTypedef is not true.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2068: DART_EXPORT bool
Dart_ClassIsFunctionType(Dart_Handle handle);
On 2012/09/12 18:52:07, cshapiro wrote:
> Rename "handle" to "klass", "cls", or "clazz" ?

Done.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2071: * Returns a signature function for a function
type.
On 2012/09/12 18:52:07, cshapiro wrote:
> signature function or function signature?

Um, rewrote comment entirely:


/**
 * Returns a function handle representing the signature associated
 * with a function type.
 *
 * The return value is a function handle (See Dart_IsFunction, etc.).
 *
 * TODO(turnidge): Finish documentation.
 */

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2191: * Returns a handle to the return type of a
function.
On 2012/09/12 18:52:07, cshapiro wrote:
> I am confused about the pervasive use of handle in this documentation.  The
> handle seems implied.

Right.  I am torn about this.  I could start dropping the handles all over the
documentation which might make it all read easier.  I take it you think this is
a good idea?

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:254: } else if (Dart_IsClass(target) ||
Dart_IsInterface(target)) {
On 2012/09/12 18:52:07, cshapiro wrote:
> why else if when you return on line 252?  That is kinda what happens on line
> 246.

Got rid of all of the else-ifs.  Didn't add the assert.  See what you think.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:361: Dart_Handle result;
On 2012/09/12 18:52:07, cshapiro wrote:
> move this down to line 372.

Done.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:376: for (int i = 0; i < len; i++) {
On 2012/09/12 18:52:07, cshapiro wrote:
> intptr_t ?

Changed here and about 8 other places.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:565: Dart_Handle type = Dart_VariableType(var);
On 2012/09/12 18:52:07, cshapiro wrote:
> move this above the is_* stuff?

Rearranged.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:81: if (mirror === null) {
On 2012/09/12 18:52:07, cshapiro wrote:
> You don't need === anymore for speed.

Replaced all in this file.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:349: "Mirror for type '$typeName' not
implemented");
On 2012/09/12 18:52:07, cshapiro wrote:
> "is not..." or "is unimplemented"

Fixed here and elsewhere.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:613: TypeMirror get upperBound() {
On 2012/09/12 18:52:07, cshapiro wrote:
> no () here and around here

Done.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:910: : super('<TODO:unnamed>', null, type, false,
false) {}
On 2012/09/12 18:52:07, cshapiro wrote:
> Strange.  Why not leave the TODO comment?

Every class in this file has things that need to be fixed.  They are documented,
for the most part, by the fact that the getters throw exceptions.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:2576: // For now, the only typedefs are
non-canonical signature classes.
On 2012/09/12 18:52:07, cshapiro wrote:
> For now all typedefs are non-canonical signature classes.

Done.

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl_test.cc:2397: EXPECT_VALID(lib);
On 2012/09/12 18:52:07, cshapiro wrote:
> Can we check something about the type of the object in the handle here? 
> Something similar happens below for the cls things.

We do this all the time... discussed offline.  Will leave as is.

[cshapiro, 2012-09-13 04:29:58]

lgtm - some very minor stuff, dehandling comments might be best done in a
separate change, your call

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2191: * Returns a handle to the return type of a
function.
I think the handle is superfluous, after all what else could it be?  Also, the
inconsistent usage is a little bit confusing in the context of a reflection API
given the indirection inherent in many of these operations.

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors...
runtime/lib/mirrors.cc:973: return  Dart_New(cls, Dart_Null(), ARRAY_SIZE(args),
args);
There is an extra space between "return" and "Dart_New".

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors...
runtime/lib/mirrors_impl.dart:394: if (_owner !== null && _owner is! Mirror) {
This could be != instead of !==.  There are a few such instances here, above,
and below.

[turnidge, 2012-09-13 17:02:57]

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/10916252/diff/1/runtime/include/dart_a...
runtime/include/dart_api.h:2191: * Returns a handle to the return type of a
function.
On 2012/09/13 04:29:58, cshapiro wrote:
> I think the handle is superfluous, after all what else could it be?  Also, the
> inconsistent usage is a little bit confusing in the context of a reflection
API
> given the indirection inherent in many of these operations.

Fixed this use.  Will fix the more pervasive problem in a follow-up CL.

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors...
runtime/lib/mirrors.cc:973: return  Dart_New(cls, Dart_Null(), ARRAY_SIZE(args),
args);
On 2012/09/13 04:29:58, cshapiro wrote:
> There is an extra space between "return" and "Dart_New".

Done.

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/10916252/diff/4005/runtime/lib/mirrors...
runtime/lib/mirrors_impl.dart:394: if (_owner !== null && _owner is! Mirror) {
On 2012/09/13 04:29:58, cshapiro wrote:
> This could be != instead of !==.  There are a few such instances here, above,
> and below.

Fixed throughout.