Implement method and variable reflection in dart:mirrors.

Implement method and variable reflection in dart:mirrors.
Committed: https://code.google.com/p/dart/source/detail?r=9493

[cshapiro, 2012-06-28 23:57:47]

lgtm with comments

http://codereview.chromium.org/10687004/diff/3002/lib/mirrors/mirrors.dart
File lib/mirrors/mirrors.dart (right):

http://codereview.chromium.org/10687004/diff/3002/lib/mirrors/mirrors.dart#ne...
lib/mirrors/mirrors.dart:50: * The isolate of origin for this [Mirror].
I am confused by the names.  The type is an IsolateMirror but the documentation
says isolate.  Did I miss something?

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2005: * \return If no error occurs, a list of strings
is returned.
I am confused about the conditions under which errors are returned.  Is any
object permitted as the target?  This applies to all of the new functions in
this header.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2018: *   Otherwise returns a function handle if the
function is found of
This does not read right specifically, "is found of Dart_Null()".  Perhaps you
meant something else?

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2030: * Returns the name for the provided function or
method.
Is there no possible error return?  Maybe provide a \return documentation string
here as well.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2035: * Determines whether a function or method is
declared abstract.
Since there cannot be an abstract function, perhaps the documentation string
should say something like "Returns true if the handle refers to an abstract
method" rather than teasing the existence of an abstract function.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2038: * \param is_static Returns whether the function
or method is declared abstract.
Same here, "returns true if 'function' is an abstract method".

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2046: * Determines whether a function or method is
declared static.
Declaring static seems confusing.  I understand there is a comment below that
tries to reconcile with reality, but similarly, outside of the embedding api
functions are static even though they are not declared so.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2060: * Determines whether a function or method is a
constructor.
Functions cannot be constructors.  What about factories?

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2101: DART_EXPORT Dart_Handle
Dart_GetVariableNames(Dart_Handle target);
Why not call this "Field" instead of "Variable"?

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2113: DART_EXPORT Dart_Handle
Dart_LookupVariable(Dart_Handle target,
Likewise.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2117: * Is this a variable declaration handle?
What is a variable in this context?  Is this a local variable or a class field
or something else?

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2122: * Returns the name for the provided variable.
Ditto.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2127: * Determines whether a variable is declared
static.
What kind of variable?  Local variable?

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2141: * Determines whether a variable is declared
final.
Ditto.

http://codereview.chromium.org/10687004/diff/3002/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2144: * \param is_static Returns whether the variable
is declared final.
You have the wrong name for the \param, it should be is_final.

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:289: const int kNumArgs = 8;
Can this be eliminated, maybe by something like this

Dart_Handle args[] = { ... , ... , ... };

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:342: const int kNumArgs = 11;
Same here, see above regarding the kNumArgs stuff.

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:382: const int kNumArgs = 4;
Same here too.

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:403: intptr_t len;
How do we know that len is always initialized?  Can we limit its scope?  Can we
exit early if "owner" is not a library?

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:407: names = Dart_LibraryGetClassNames(owner);
Since names here is not the same as names below, it would be nice if another
variable or another variable scope was used.

http://codereview.chromium.org/10687004/diff/3002/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:460: names = Dart_GetVariableNames(owner);
It looks like there are 3 different functions inside this one function.  Maybe
you can split this up into 3 separate functions for functions libraries and
variables.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:62: static RawString* StripName(Isolate* isolate,
const String& name) {
IdentifierPrettyName?  Better comment with examples.  There is a lot going on
inside of this function!

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2694: // Check for functions with the external
setter suffix '='.  Make
Totally optional, maybe make it clear that we are trying different cases. 
Having some repeated sentence structure in the comment will help that jump out.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2719: fprintf(stderr, "---> Couldn't find method
'%s' in class '%s'\n",
No arrows allowed!

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2727: // Check for functions with the external
setter suffix '='.  Make
Ditto.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2786: bool* is_abstract) {
Null check.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2793: *is_abstract = func.kind() ==
RawFunction::kAbstract;
Stick parens around the inner double equal.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2799: bool* is_static) {
Null check.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2812: bool* is_constructor) {
Null check.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2825: bool* is_getter) {
Null check.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2844: bool* is_setter) {
Null check.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2880: // Some special types like 'Dynamic' have a
null fields list.
Maybe fix this at the source if it is not too much trouble for this CL?

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2927: return Api::NewHandle(isolate,
cls.LookupField(var_name));
If you have a return, why not get rid of the else if?

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2941: return Api::ClassId(handle) == kField;
Maybe make sure this is safe?

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2958: bool* is_static) {
Do we normally do null check for out variables?  Add one?

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2971: bool* is_final) {
Null check for is_final too.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl_te...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:4805: "void _A() {}\n"
Make functions and variables lowercase?

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:4871: "var A;\n"
Same?

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:4900: Dart_Handle args[1];
Dart_Handle args[] = { list };

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:1934: // Check to see if munged_name is equal to bare_name
once the private
Maybe just make this an equality test and eliminate the disjunctions below. 
Like MatchesIgnoringPrivateKeys?  This is always guarded by an equals.

http://codereview.chromium.org/10687004/diff/3002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:1948: bool MatchesIgnorePrivate(const String& munged_name,
const String& bare_name) {
Ignoring?  Mangled?

[turnidge, 2012-07-09 23:45:17]

https://chromiumcodereview.appspot.com/10687004/diff/3002/lib/mirrors/mirrors...
File lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10687004/diff/3002/lib/mirrors/mirrors...
lib/mirrors/mirrors.dart:50: * The isolate of origin for this [Mirror].
On 2012/06/28 23:57:47, cshapiro wrote:
> I am confused by the names.  The type is an IsolateMirror but the
documentation
> says isolate.  Did I miss something?

Fixed the comment.  In an upcoming CL, this will be change from being an
IsolateMirror to a MirrorSystem or something like that.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2005: * \return If no error occurs, a list of strings
is returned.
On 2012/06/28 23:57:47, cshapiro wrote:
> I am confused about the conditions under which errors are returned.  Is any
> object permitted as the target?  This applies to all of the new functions in
> this header.

This comment would apply to all dart api functions, so I think it's a separate
change.  Considering....

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2018: *   Otherwise returns a function handle if the
function is found of
On 2012/06/28 23:57:47, cshapiro wrote:
> This does not read right specifically, "is found of Dart_Null()".  Perhaps you
> meant something else?

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2030: * Returns the name for the provided function or
method.
On 2012/06/28 23:57:47, cshapiro wrote:
> Is there no possible error return?  Maybe provide a \return documentation
string
> here as well.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2035: * Determines whether a function or method is
declared abstract.
On 2012/06/28 23:57:47, cshapiro wrote:
> Since there cannot be an abstract function, perhaps the documentation string
> should say something like "Returns true if the handle refers to an abstract
> method" rather than teasing the existence of an abstract function.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2038: * \param is_static Returns whether the function
or method is declared abstract.
On 2012/06/28 23:57:47, cshapiro wrote:
> Same here, "returns true if 'function' is an abstract method".

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2046: * Determines whether a function or method is
declared static.
On 2012/06/28 23:57:47, cshapiro wrote:
> Declaring static seems confusing.  I understand there is a comment below that
> tries to reconcile with reality, but similarly, outside of the embedding api
> functions are static even though they are not declared so.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2060: * Determines whether a function or method is a
constructor.
On 2012/06/28 23:57:47, cshapiro wrote:
> Functions cannot be constructors.  What about factories?

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2101: DART_EXPORT Dart_Handle
Dart_GetVariableNames(Dart_Handle target);
On 2012/06/28 23:57:47, cshapiro wrote:
> Why not call this "Field" instead of "Variable"?

The spec calls them variables instead of fields, so I decided to follow that.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2117: * Is this a variable declaration handle?
On 2012/06/28 23:57:47, cshapiro wrote:
> What is a variable in this context?  Is this a local variable or a class field
> or something else?

For now, a variable is a top-level variable or a "field" from a class.  The spec
calls these variables.  Local variables are not included.

Currently the only place a variable handle can come from is a call to
Dart_LookupVariable, which looks up a variable in a library or a class.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/include/dar...
runtime/include/dart_api.h:2144: * \param is_static Returns whether the variable
is declared final.
On 2012/06/28 23:57:47, cshapiro wrote:
> You have the wrong name for the \param, it should be is_final.

Done.

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

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:289: const int kNumArgs = 8;
On 2012/06/28 23:57:47, cshapiro wrote:
> Can this be eliminated, maybe by something like this
> 
> Dart_Handle args[] = { ... , ... , ... };

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:342: const int kNumArgs = 11;
On 2012/06/28 23:57:47, cshapiro wrote:
> Same here, see above regarding the kNumArgs stuff.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:382: const int kNumArgs = 4;
On 2012/06/28 23:57:47, cshapiro wrote:
> Same here too.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:403: intptr_t len;
On 2012/06/28 23:57:47, cshapiro wrote:
> How do we know that len is always initialized?  Can we limit its scope?  Can
we
> exit early if "owner" is not a library?

Taken care of by splitting out the 3 helper functions as suggested below.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:407: names = Dart_LibraryGetClassNames(owner);
On 2012/06/28 23:57:47, cshapiro wrote:
> Since names here is not the same as names below, it would be nice if another
> variable or another variable scope was used.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/lib/mirrors...
runtime/lib/mirrors.cc:460: names = Dart_GetVariableNames(owner);
On 2012/06/28 23:57:47, cshapiro wrote:
> It looks like there are 3 different functions inside this one function.  Maybe
> you can split this up into 3 separate functions for functions libraries and
> variables.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:62: static RawString* StripName(Isolate* isolate,
const String& name) {
On 2012/06/28 23:57:47, cshapiro wrote:
> IdentifierPrettyName?  Better comment with examples.  There is a lot going on
> inside of this function!

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2694: // Check for functions with the external
setter suffix '='.  Make
On 2012/06/28 23:57:47, cshapiro wrote:
> Totally optional, maybe make it clear that we are trying different cases. 
> Having some repeated sentence structure in the comment will help that jump
out.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2694: // Check for functions with the external
setter suffix '='.  Make
On 2012/06/28 23:57:47, cshapiro wrote:
> Totally optional, maybe make it clear that we are trying different cases. 
> Having some repeated sentence structure in the comment will help that jump
out.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2719: fprintf(stderr, "---> Couldn't find method
'%s' in class '%s'\n",
On 2012/06/28 23:57:47, cshapiro wrote:
> No arrows allowed!

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2727: // Check for functions with the external
setter suffix '='.  Make
On 2012/06/28 23:57:47, cshapiro wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2786: bool* is_abstract) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Null check.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2793: *is_abstract = func.kind() ==
RawFunction::kAbstract;
On 2012/06/28 23:57:47, cshapiro wrote:
> Stick parens around the inner double equal.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2799: bool* is_static) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Null check.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2812: bool* is_constructor) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Null check.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2825: bool* is_getter) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Null check.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2844: bool* is_setter) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Null check.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2880: // Some special types like 'Dynamic' have a
null fields list.
On 2012/06/28 23:57:47, cshapiro wrote:
> Maybe fix this at the source if it is not too much trouble for this CL?

Investigated.  Since Dynamic is allocated in the vm isolate and the empty array
is not, I will need to hold off on fixing this for now.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2927: return Api::NewHandle(isolate,
cls.LookupField(var_name));
On 2012/06/28 23:57:47, cshapiro wrote:
> If you have a return, why not get rid of the else if?

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2941: return Api::ClassId(handle) == kField;
On 2012/06/28 23:57:47, cshapiro wrote:
> Maybe make sure this is safe?

We do this a lot.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2958: bool* is_static) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Do we normally do null check for out variables?  Add one?

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:2971: bool* is_final) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Null check for is_final too.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/object.c...
runtime/vm/object.cc:1934: // Check to see if munged_name is equal to bare_name
once the private
On 2012/06/28 23:57:47, cshapiro wrote:
> Maybe just make this an equality test and eliminate the disjunctions below. 
> Like MatchesIgnoringPrivateKeys?  This is always guarded by an equals.

Done.

https://chromiumcodereview.appspot.com/10687004/diff/3002/runtime/vm/object.c...
runtime/vm/object.cc:1948: bool MatchesIgnorePrivate(const String& munged_name,
const String& bare_name) {
On 2012/06/28 23:57:47, cshapiro wrote:
> Ignoring?  Mangled?

Done.