Implement DeclarationMirror.location for all but ParameterMirrors.

Implement DeclarationMirror.location for all but ParameterMirrors.

- Mark NativeFieldWrapper classes as synthetic.
- Mark ClassID fields as synthetic.

BUG=http://dartbug.com/22378
R=gbracha@google.com, hausner@google.com

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

[rmacnak, 2015-02-13 02:13:41]

rmacnak@google.com changed reviewers:
+ gbracha@google.com, hausner@google.com

[gbracha, 2015-02-13 03:00:57]

Tests lgtm as long as minor typos corrected.  For mirrors.dart, see proposed
spec refinements that correspond to these changes.

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

https://codereview.chromium.org/922023002/diff/20001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:293: */
So we probably should tighten the specification for this so that the location is
defined precisely. Something like (not sure if this is actually what we want):

If the reflectee is a variable, the returned location gives the position of the
variable name at its point of declaration.

If the reflectee is a library, class, typedef, function or type variable with
associated metadata, the returned location gives the position of the first
metadata declaration associated with the reflectee.

Otherwise:

If the reflectee is a library , the returned location gives the position of the
keyword 'library' at the reflectee's point of declaration.

If the reflectee is an abstract class, the returned location gives the position
of the keyword 'abstract' at the reflectee's point of declaration. Otherwise, f
the reflectee is a class, the returned location gives the position of the
keyword 'class' at the reflectee's point of declaration.

 If the reflectee is a typedef the returned location gives the position of the
of the keyword 'typedef' at the reflectee's point of declaration.

 If the reflectee is a function with a declared return type, the returned
location gives the position of the function's return type at the reflectee's
point of declaration. Otherwise. the returned location gives the position of the
function's name at the reflectee's point of declaration.

https://codereview.chromium.org/922023002/diff/20001/tests/lib/mirrors/class_...
File tests/lib/mirrors/class_mirror_location_other.dart (right):

https://codereview.chromium.org/922023002/diff/20001/tests/lib/mirrors/class_...
tests/lib/mirrors/class_mirror_location_other.dart:9: class
SpaceIdentedInOtherFile {}
Idented -> Indented

https://codereview.chromium.org/922023002/diff/20001/tests/lib/mirrors/class_...
tests/lib/mirrors/class_mirror_location_other.dart:11: class
TabIdentedInOtherFile {}
Ditto

https://codereview.chromium.org/922023002/diff/20001/tests/lib/mirrors/class_...
File tests/lib/mirrors/class_mirror_location_test.dart (right):

https://codereview.chromium.org/922023002/diff/20001/tests/lib/mirrors/class_...
tests/lib/mirrors/class_mirror_location_test.dart:55:
expectLocation(reflectClass(SpaceIdentedInOtherFile), otherSuffix, 9, 3);
Idented -> Indented

https://codereview.chromium.org/922023002/diff/20001/tests/lib/mirrors/class_...
tests/lib/mirrors/class_mirror_location_test.dart:56:
expectLocation(reflectClass(TabIdentedInOtherFile), otherSuffix, 11, 2);
Ditto.

[hausner, 2015-02-13 16:52:28]

LGTM with comments.

https://codereview.chromium.org/922023002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/922023002/diff/20001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:4405: if (is_patch_source() &&
I believe you don't have to validate whether the patch keyword is allowed at
this point. This has already been checked by the time you get here. I think you
can just skip/consume the word patch if you see it here.

https://codereview.chromium.org/922023002/diff/20001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:4507: ConsumeToken();  // Enum type name.
Don't you have to skip metadata here? I'd add a test case in one of the tests
that checks metadata parsing. Enums are probably not even covered by those
tests.

[rmacnak, 2015-02-13 20:44:38]

New patchsets have been uploaded after l-g-t-m from
gbracha@google.com,hausner@google.com

[rmacnak, 2015-02-13 20:46:34]

Updated how libraries are handle per proposed spec. PTAL.

https://codereview.chromium.org/922023002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/922023002/diff/20001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:4507: ConsumeToken();  // Enum type name.
On 2015/02/13 16:52:27, hausner wrote:
> Don't you have to skip metadata here? I'd add a test case in one of the tests
> that checks metadata parsing. Enums are probably not even covered by those
> tests.

Yikes, yes. I don't think enums are explicitly included in any mirror tests.

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

https://codereview.chromium.org/922023002/diff/20001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:293: */
On 2015/02/13 03:00:57, gbracha wrote:
> So we probably should tighten the specification for this so that the location
is
> defined precisely. Something like (not sure if this is actually what we want):
> 
> If the reflectee is a variable, the returned location gives the position of
the
> variable name at its point of declaration.
> 
> If the reflectee is a library, class, typedef, function or type variable with
> associated metadata, the returned location gives the position of the first
> metadata declaration associated with the reflectee.
> 
> Otherwise:
> 
> If the reflectee is a library , the returned location gives the position of
the
> keyword 'library' at the reflectee's point of declaration.
> 
> If the reflectee is an abstract class, the returned location gives the
position
> of the keyword 'abstract' at the reflectee's point of declaration. Otherwise,
f
> the reflectee is a class, the returned location gives the position of the
> keyword 'class' at the reflectee's point of declaration.
> 
>  If the reflectee is a typedef the returned location gives the position of the
> of the keyword 'typedef' at the reflectee's point of declaration.
> 
>  If the reflectee is a function with a declared return type, the returned
> location gives the position of the function's return type at the reflectee's
> point of declaration. Otherwise. the returned location gives the position of
the
> function's name at the reflectee's point of declaration.

A library might not have a 'library foo' declaration.

Should we be specifying this in terms of the productions in the spec's grammar?

[gbracha, 2015-02-13 20:58:27]

https://codereview.chromium.org/922023002/diff/40001/tests/lib/mirrors/other_...
File tests/lib/mirrors/other_declarations_location_test.dart (right):

https://codereview.chromium.org/922023002/diff/40001/tests/lib/mirrors/other_...
tests/lib/mirrors/other_declarations_location_test.dart:10: import
"library_with_annotated_declaration.dart";
Did I miss where these libs were defined?

[hausner, 2015-02-13 21:02:01]

lgtm

DBC

https://codereview.chromium.org/922023002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/922023002/diff/40001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5803: intptr_t Parser::GetLibraryDeclarationPosition() {
When you look at SkipMetadata, you'll see that this function is extremely simple
and probably should not be in the parser. Essentially, you just look at the
first token. If it is an @ or the token kLIBRARY, you return the position,
otherwise you return an invalid token position.

On the other hand, does this function really do what you thought it does?  Can a
script start with metadata followed by something other than the library keyword?
If so, you return a valid token position for a script that does not have a
library declaration.

https://codereview.chromium.org/922023002/diff/40001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5815: return Scanner::kNoSourcePos;
Scanner::kNoSourcePos is unfortunately 0, and a few lines above you can also
return 0 as a successful outcome of finding the a library declaration.

[rmacnak, 2015-02-13 22:57:12]

New patchsets have been uploaded after l-g-t-m from hausner@google.com

[rmacnak, 2015-02-13 22:57:23]

https://codereview.chromium.org/922023002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/922023002/diff/40001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5803: intptr_t Parser::GetLibraryDeclarationPosition() {
On 2015/02/13 21:02:01, hausner wrote:
> When you look at SkipMetadata, you'll see that this function is extremely
simple
> and probably should not be in the parser. Essentially, you just look at the
> first token. If it is an @ or the token kLIBRARY, you return the position,
> otherwise you return an invalid token position.

True, I'll do this in mirrors.cc instead.
 
> On the other hand, does this function really do what you thought it does?  Can
a
> script start with metadata followed by something other than the library
keyword?
> If so, you return a valid token position for a script that does not have a
> library declaration.

Unfortunately that can happen if a library has no library declaration and its
first declaration is annotated.  Added a TODO.

https://codereview.chromium.org/922023002/diff/40001/tests/lib/mirrors/other_...
File tests/lib/mirrors/other_declarations_location_test.dart (right):

https://codereview.chromium.org/922023002/diff/40001/tests/lib/mirrors/other_...
tests/lib/mirrors/other_declarations_location_test.dart:10: import
"library_with_annotated_declaration.dart";
On 2015/02/13 20:58:27, gbracha wrote:
> Did I miss where these libs were defined?

Forgot to tell source control about them. Uploaded.

[gbracha, 2015-02-13 23:16:36]

One last comment on the dart doc. Other than that LGTM on the etsts and spec.

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

https://codereview.chromium.org/922023002/diff/60001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:300: * the keyword 'library' at the reflectee's
point of declaration.
 "declaration." -> "declaration, if the reflectee is a named library, or the
first character of the first line in the compilation unit defining the reflectee
if the reflectee is anonymous

[rmacnak, 2015-02-13 23:29:09]

New patchsets have been uploaded after l-g-t-m from gbracha@google.com

[rmacnak, 2015-02-13 23:32:00]

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

https://codereview.chromium.org/922023002/diff/60001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:300: * the keyword 'library' at the reflectee's
point of declaration.
On 2015/02/13 23:16:36, gbracha wrote:
>  "declaration." -> "declaration, if the reflectee is a named library, or the
> first character of the first line in the compilation unit defining the
reflectee
> if the reflectee is anonymous

Done.

[rmacnak, 2015-02-13 23:32:37]

Committed patchset #5 (id:80001) manually as 43784 (presubmit successful).