Add support for closure calls through getters.

Add support for closure calls through getters.

Support reflect(o).invoke(#bar) where bar is a getter.
library.getField(#foo) where `foo` is a method.

BUG= http://dartbug.com/15138
R=karlklose@google.com

Committed: https://code.google.com/p/dart/source/detail?r=34555
Reverted: https://code.google.com/p/dart/source/detail?r=34556

Committed: https://code.google.com/p/dart/source/detail?r=34779
Reverted: https://code.google.com/p/dart/source/detail?r=34782

Committed: https://code.google.com/p/dart/source/detail?r=34818
Reverted: https://code.google.com/p/dart/source/detail?r=34820

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

[floitsch, 2014-03-12 23:07:01]

This CL is not yet cleaned up.

@Ryan: wanted to show you the test I added. (In case you are already fixing it).

[rmacnak, 2014-03-12 23:33:57]

On 2014/03/12 23:07:01, floitsch wrote:
> This CL is not yet cleaned up.
> 
> @Ryan: wanted to show you the test I added. (In case you are already fixing
it).

Locally I just copied the whole thing and swapped getters with variables.

Good point testing them when they haven't been accessed at the base level, I'll
do that as well.

[floitsch, 2014-03-19 22:55:18]

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:837: assert(namedArguments.isNotEmpty);
Code is unchanged. See patch-set 6 for an indented version where the diff works
better.

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:924: var cache = _classInvocationCache;
Code is mostly equivalent to code from old:895-918.

Instead of keeping the invocation (old:898,912) we have to recreate it in the
new code (new:976). This only happens the first time, since the cacheEntry is
later cached.

[karlklose, 2014-03-21 09:15:53]

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:348: String getter = JS("",
"#['\$getter']", methodMirror._jsFunction);
How about using a raw string instead of escaping.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:347: assert
(methodMirror.isRegularMethod);
No space between 'assert' and '('.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:921: Symbol name, int type, String
reflectiveName,
Start the parameter list on previous line.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1054: var result =
Why this change?

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1782: bool _staticFieldExists(Symbol
fieldName) {
Group this with _instanceFieldExists?

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1810: if (method != null &&
method.isStatic) {
This part needs a comment. Which situation is this handling?

[ahe, 2014-03-21 09:25:26]

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:348: String getter = JS("",
"#['\$getter']", methodMirror._jsFunction);
On 2014/03/21 09:15:54, karlklose wrote:
> How about using a raw string instead of escaping.

FWIW, I've used \$ in a number of places to remind myself to replace them later
with something that can be minified.

[floitsch, 2014-03-21 10:38:25]

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/195793013/diff/120001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:348: String getter = JS("",
"#['\$getter']", methodMirror._jsFunction);
On 2014/03/21 09:25:26, ahe wrote:
> On 2014/03/21 09:15:54, karlklose wrote:
> > How about using a raw string instead of escaping.
> 
> FWIW, I've used \$ in a number of places to remind myself to replace them
later
> with something that can be minified.

Will keep Peter's convention.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:347: assert
(methodMirror.isRegularMethod);
On 2014/03/21 09:15:54, karlklose wrote:
> No space between 'assert' and '('.

Done.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:921: Symbol name, int type, String
reflectiveName,
On 2014/03/21 09:15:54, karlklose wrote:
> Start the parameter list on previous line.

Done.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1054: var result =
On 2014/03/21 09:15:54, karlklose wrote:
> Why this change?

The comment applies for the invocation.
The signature of "_invoke" has changed and doesn't need the name anymore.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1782: bool _staticFieldExists(Symbol
fieldName) {
On 2014/03/21 09:15:54, karlklose wrote:
> Group this with _instanceFieldExists?

They are not in the same class.

https://codereview.chromium.org/195793013/diff/140001/sdk/lib/_internal/lib/j...
sdk/lib/_internal/lib/js_mirrors.dart:1810: if (method != null &&
method.isStatic) {
On 2014/03/21 09:15:54, karlklose wrote:
> This part needs a comment. Which situation is this handling?

Done.

[floitsch, 2014-03-27 12:27:59]

ping.

[karlklose, 2014-03-31 14:16:21]

LGTM.

[floitsch, 2014-03-31 14:22:51]

Committed patchset #9 manually as r34555 (presubmit successful).

[floitsch, 2014-03-31 17:35:10]

Had to revert. 3 tests failing:
dart2js_extra/mirrors_test
pkg/observe/test/path_observer_test
lib/mirrors/invoke_closurization_test/none (in checked mode)

[floitsch, 2014-04-03 20:10:24]

PTAL.
I fixed two issues (that were discovered by existing tests).

1. the NoSuchMethod exception should have a "fieldName=" when it tried to invoke
a setter. (and not just "fieldName".
Currently the test is hidden in a package, so I will add another one tomorrow.

2. When invoking a method (through mirrors) and the signature doesn't match we
must invoke the noSuchMethod of it. However, my patch added a getter
(closurizing the method), and we invoked the closurized method instead. Normally
that wouldn't change much (except for the name reported by noSuchMethod), but if
the object actually had a noSuchMethod we were not calling that one anymore.

[floitsch, 2014-04-03 20:11:20]

On 2014/04/03 20:10:24, floitsch wrote:
> PTAL.
> I fixed two issues (that were discovered by existing tests).
> 
> 1. the NoSuchMethod exception should have a "fieldName=" when it tried to
invoke
> a setter. (and not just "fieldName".
> Currently the test is hidden in a package, so I will add another one tomorrow.
> 
> 2. When invoking a method (through mirrors) and the signature doesn't match we
> must invoke the noSuchMethod of it. However, my patch added a getter
> (closurizing the method), and we invoked the closurized method instead.
Normally
> that wouldn't change much (except for the name reported by noSuchMethod), but
if
> the object actually had a noSuchMethod we were not calling that one anymore.

Never mind. Just hit more errors. Will ping again when it's ready.

[floitsch, 2014-04-04 21:13:32]

PTAL.
tests are passing now.

Was just a bad merge, and the 'isStub' was broken.

I also added a test for NSM-errors.

[karlklose, 2014-04-07 09:38:36]

Still LGTM.

[floitsch, 2014-04-07 15:02:22]

Committed patchset #15 manually as r34779 (presubmit successful).

[floitsch, 2014-04-07 19:19:32]

I had to revert.
PTAL if you want. Only minor fixes. Will commit tomorrow.

[floitsch, 2014-04-08 11:27:11]

Committed patchset #17 manually as r34818 (presubmit successful).

[floitsch, 2014-04-08 13:27:17]

Had to revert, but fixes are again minor.

A few other minor changes (status file change wasn't triggering on Dartium, and
a bad type annotation).

Resubmitting.

[floitsch, 2014-04-08 13:27:57]

Committed patchset #19 manually as r34823 (presubmit successful).

[karlklose, 2014-04-10 08:52:59]

LGTM.