Fix Function.end_token_pos() and implement MethodMirror.source getter

Fix Function.end_token_pos() and implement MethodMirror.source getter.

Move .source getter from ClosureMirror to MethodMirror (as the TODO already suggested) and provide an implementation. 

BUG=
R=asiva@google.com, hausner@google.com

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

[Michael Lippautz (Google), 2013-08-19 21:28:14]

assert(), script.GetSnippet(), and script.GetTokenLocation() are not affected by
this change, since they're doing the right thing, i.e. assert(a==b /* comment
*/) may throw with a message "a==b /* comment */". Nobody else calls
GetSnippet() by now.

There are two cases for end tokens:

Case 1:
foo() {}

/* this */
// or this
@or this
bar() {}

Here we have the end token for foo() on the `}' and print this including the
last token. This is the general case. (Since we don't want the comments to end
up in the source of foo.)

Case 2:
a(() => null, null);
a(() => null);

End token for the passed closure is at semicolon/lparen, and since we cannot go
back that's just it. Printing this is a special case in the mirrors library.

The mirrors code could also go into a method of the internal Function object if
somebody else (some Stacktrace?) needs to get the underlying source.

[hausner, 2013-08-19 23:00:48]

Something is wrong with this change. All diffs appear empty when I click the
"View" link.

[Michael Lippautz (Google), 2013-08-19 23:52:36]

Strange... I reuploaded the exact same changes and now the diffs seem to work.

[gbracha, 2013-08-19 23:57:30]

Spec change is as intended.

[hausner, 2013-08-20 17:49:44]

See comments in parser.cc

As we discussed offline, you are correct that TokenPos()-1 is not guaranteed to
give you the previous token. Have you tried to redefine the end_token_pos of a
function to be exclusive (the first token past the function)? That might make
things simpler.

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

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2701: func.set_end_token_pos(TokenPos() - 1);
Read the closing brace after setting the end token pos, so you can avoid the - 1
(which is only accidentally correct as we discussed offline).

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2710: ParseNativeFunctionBlock(&params, func);
Can you refactor the code so that ParseNativeFunctionBlock does not consume the
semicolon? Then, set the end token pos to TokenPos() and read the semicolon
afterwards.

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2724: InvocationMirror::kMethod));
For completeness, you should also set the end token position here.

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2727: }
It would be nice to assert here that if the function already had an end token
position (set during top-level parsing), we are setting the same value.

[Michael Lippautz (Google), 2013-08-21 16:50:03]

Addressed comments and adjusted end tokens we get from diet parsing.

> As we discussed offline, you are correct that TokenPos()-1 is not guaranteed
to
> give you the previous token. Have you tried to redefine the end_token_pos of a
> function to be exclusive (the first token past the function)? That might make
> things simpler.

Yes, thought about it, but doesn't work in case (1) (see first message), as we
would print the comments attached to the next function (basically everything
commented until the next token). 

Another possibility:
We could modify/add GetSnippet() to take tokens instead of lines/cols. The
function would iterate from start to end token and keep buffering strings. We
could then, based on a parameter, include or exclude the last buffer. I feel
that this is less preferable though (iterating over tokens...). Opinions?

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

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2701: func.set_end_token_pos(TokenPos() - 1);
On 2013/08/20 17:49:44, hausner wrote:
> Read the closing brace after setting the end token pos, so you can avoid the -
1
> (which is only accidentally correct as we discussed offline).

Done.

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2710: ParseNativeFunctionBlock(&params, func);
On 2013/08/20 17:49:44, hausner wrote:
> Can you refactor the code so that ParseNativeFunctionBlock does not consume
the
> semicolon? Then, set the end token pos to TokenPos() and read the semicolon
> afterwards.

Done.

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2724: InvocationMirror::kMethod));
On 2013/08/20 17:49:44, hausner wrote:
> For completeness, you should also set the end token position here.

Done.

https://codereview.chromium.org/23038010/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2727: }
On 2013/08/20 17:49:44, hausner wrote:
> It would be nice to assert here that if the function already had an end token
> position (set during top-level parsing), we are setting the same value.

Added the assertion, which forced me to think about (and fix) the end positions
that we got from diet parsing :)

[hausner, 2013-08-21 20:32:24]

LGTM.

[siva, 2013-08-21 22:21:05]

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

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1297: // (3) "var foo = () => null": End token is `;',
but in this case  we are in
In your comment you mention end token is ';' but I don't see any ';'
character in that line.

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1299: if ((tkit.CurrentTokenKind() == Token::kCOMMA) || 
// (1)
what does "(1)" mean?

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

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1015: _source = _MethodMirror_source(_reflectee);
assert(_source != null);

[siva, 2013-08-21 22:31:54]

https://codereview.chromium.org/23038010/diff/14002/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/23038010/diff/14002/tests/lib/lib.status#newc...
tests/lib/lib.status:22: mirrors/method_mirror_source_test : Fail # Issue 6490
Will your test pass in the minified version of dart2dart?

[Michael Lippautz (Google), 2013-08-21 23:19:20]

Thanks!

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

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1297: // (3) "var foo = () => null": End token is `;',
but in this case  we are in
On 2013/08/21 22:21:05, siva wrote:
> In your comment you mention end token is ';' but I don't see any ';'
> character in that line.

Done.

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1299: if ((tkit.CurrentTokenKind() == Token::kCOMMA) || 
// (1)
On 2013/08/21 22:21:05, siva wrote:
> what does "(1)" mean?

I tried to refer to the comment above in the if statement, as the third case may
be non-obvious. Remove it?

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

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1015: _source = _MethodMirror_source(_reflectee);
On 2013/08/21 22:21:05, siva wrote:
> assert(_source != null);

Done.

https://codereview.chromium.org/23038010/diff/14002/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/23038010/diff/14002/tests/lib/lib.status#newc...
tests/lib/lib.status:22: mirrors/method_mirror_source_test : Fail # Issue 6490
On 2013/08/21 22:31:55, siva wrote:
> Will your test pass in the minified version of dart2dart?

All mirror tests have been excluded from dart2dart (line 71 in this file). See
https://code.google.com/p/dart/issues/detail?id=11511

[siva, 2013-08-26 02:31:27]

lgtm

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

https://codereview.chromium.org/23038010/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1299: if ((tkit.CurrentTokenKind() == Token::kCOMMA) || 
// (1)
Maybe replace with "foo() => null, bar);" case

On 2013/08/21 23:19:20, Michael Lippautz wrote:
> On 2013/08/21 22:21:05, siva wrote:
> > what does "(1)" mean?
> 
> I tried to refer to the comment above in the if statement, as the third case
may
> be non-obvious. Remove it?

[Michael Lippautz (Google), 2013-08-26 17:14:26]

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