[js-interop] Fix function binding and avoid noSuchMethod when using map notation

[js-interop] Fix function binding and avoid noSuchMethod when using map notation

This change does two things:

(1) It avoids noSuchMethod and dartbug.com/9283 when map notation is
used.  I don't like losing the varargs-like behavior of NSM, so I'm
not entirely sure we should land this part.

(2) This also binds receivers on returned functions to make
a['foo']() and a.foo() equivalent.  JS has odd semantics around this.
In JS:

  a['foo']()

a is the receiver (i.e., the "this") in the function invocation.  But, in the following:

  var tmp = a['foo'];
  tmp();

it is not.  We don't distinguish between the two in js-interop.  This
change essentially forces the former semantics over the latter.

Note: kevmoo's pop-pop-win app in dartbug.com/9283 works with in
dart2js minified with these changes along with the source change in
comment #15.

Committed: https://github.com/dart-lang/js-interop/commit/e561ab6

[kevmoo-old, 2013-03-25 17:21:16]

On 2013/03/25 17:18:04, vsm wrote:

2 questions.

1) Do all tests pass w/ minify w/ this change? ...Assuming yes, would rather ask
than verify myself. :-)
2) Is this considered a work-around for 9283? Do we want a TODO to remove once
the SDK bug is fixed?

[vsm, 2013-03-25 17:32:00]

On 2013/03/25 17:21:16, kevmoo wrote:
> On 2013/03/25 17:18:04, vsm wrote:
> 
> 2 questions.
> 
> 1) Do all tests pass w/ minify w/ this change? ...Assuming yes, would rather
ask
> than verify myself. :-)

No.  If all the tests are rewritten to use map notation (similar to the two I
just added), they all pass.  I.e., use foo['bar'] instead of foo.bar in your
code.

> 2) Is this considered a work-around for 9283? Do we want a TODO to remove once
> the SDK bug is fixed?

Not sure yet to be honest.  The compiler team is out on holiday (easter in
Denmark).  Even when 9283 is fixed, it may be advantageous to avoid NSM for code
size.  We were using NSM instead of call to essentially get varargs which is
kind of odd usage.  I might look at Erik's trick as another workaround here.

I will add a comment though.

[alexandre.ardhuin, 2013-03-25 22:39:21]

LGTM for function binding.

For modifications on FunctionProxy, I'm like you Vijay - not really convinced. 
1. I don't think people will migrate their code base from "a.foo" to
"a.['foo']".
2. There's a temporary workaround : don't use "--minify" until 9283 has been
fixed.

That said, I did some tests with "--minify". I have try to find a workaround
based on
https://code.google.com/p/dart/codesearch#dart/trunk/dart/tests/language/no_s...
(pointed by Erik Corry) to keep noSuchMethod but without success.
However, with "--minify", I have noticed that all calls like "a[foo]()",
"a[foo](a1)" or "a[foo](a1, a2)" are received in noSuchMethod with a membername
like "call$0" "call$1" "call$2". Perhaps could we test this and keep
noSuchMethod :

  noSuchMethod(InvocationMirror invocation) {
    if (invocation.isMethod && invocation.memberName == 'call' 
        || invocation.memberName.startsWith(r"call$")) {
      var message = [_id, '', 'apply',
                     invocation.positionalArguments.map(_serialize).toList()];
      var result = _port.callSync(message);
      if (result[0] == 'throws') throw result[1];
      return _deserialize(result[1]);
    } else {
      return super.noSuchMethod(invocation);
    }
  }

What do you think ?

[vsm, 2013-04-07 03:53:46]

Please take another look.

I had to redo the function binding.  My initial solution was broken for
constructors (see the new test).

Erik's workaround on invocation essentially means losing varargs - you probably
ran into this as well Alexandre.  Instead, I've fixed Proxy's NSM to handle
function invocation.  FunctionProxy.call will handle a call with up to 6
arguments.  More arguments, and an NSM will be triggered.

Regarding the original bug, the compiler team is trying to find a good fix.  In
the meantime, this CL will avoid NSM if (a) you use map notation and (b) your
functions take 6 arguments or less.  I suspect this will be a good thing
regardless.  Note, under minified compilation, only the 'map notation' tests
pass.

On 2013/03/25 22:39:21, alexandre.ardhuin wrote:
> LGTM for function binding.
> 
> For modifications on FunctionProxy, I'm like you Vijay - not really convinced.

> 1. I don't think people will migrate their code base from "a.foo" to
> "a.['foo']".
> 2. There's a temporary workaround : don't use "--minify" until 9283 has been
> fixed.
> 
> That said, I did some tests with "--minify". I have try to find a workaround
> based on
>
https://code.google.com/p/dart/codesearch#dart/trunk/dart/tests/language/no_s...
> (pointed by Erik Corry) to keep noSuchMethod but without success.
> However, with "--minify", I have noticed that all calls like "a[foo]()",
> "a[foo](a1)" or "a[foo](a1, a2)" are received in noSuchMethod with a
membername
> like "call$0" "call$1" "call$2". Perhaps could we test this and keep
> noSuchMethod :
> 
>   noSuchMethod(InvocationMirror invocation) {
>     if (invocation.isMethod && invocation.memberName == 'call' 
>         || invocation.memberName.startsWith(r"call$")) {
>       var message = [_id, '', 'apply',
>                      invocation.positionalArguments.map(_serialize).toList()];
>       var result = _port.callSync(message);
>       if (result[0] == 'throws') throw result[1];
>       return _deserialize(result[1]);
>     } else {
>       return super.noSuchMethod(invocation);
>     }
>   }
> 
> What do you think ?

[kasperl, 2013-04-07 05:15:34]

I actually find using proxy['foo'] fairly attractive compared to using
proxy.foo. It's trivial to minify and you don't pay the price (code size and
performance) of using NSM.

[alexandre.ardhuin, 2013-04-07 21:20:20]

lgtm

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart
File lib/js.dart (right):

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode391
lib/js.dart:391: typeof(message.object) == 'function') {
Could typeof(message.object) be something else that function ?

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode...
lib/js.dart:1001: } else if (member == 'call') {
If what I have observed on minification of call is something that will not
change (ie. member looks like "call$1" "call$2"...), you can add the following
to the if.

member == 'call' || new RegExp(r"^call\$\d+$").hasMatch(member)

Thus even with minification of dart2js, the test "call JS function with
varargs." is passing.

[vsm, 2013-04-07 21:31:50]

Committed patchset #3 manually as re561ab6 (presubmit successful).

[alexandre.ardhuin, 2013-04-07 21:32:18]

On 2013/04/07 05:15:34, kasperl wrote:
> I actually find using proxy['foo'] fairly attractive compared to using
> proxy.foo. It's trivial to minify and you don't pay the price (code size and
> performance) of using NSM.

From a technical point of view, I agree : better code size and better
performance.
But from a user point of view, it's longer to write and less readable IMHO :
  js.context["person"]["father"]["name"]
instead of 
  js.context.person.father.name

In my js wrapper libraries, I will try to migrate to the proxy['foo'] notation.
For google_maps, I will have a lot of work. But it's for a good reason.

[vsm, 2013-04-07 21:37:36]

Thanks for the review!

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart
File lib/js.dart (right):

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode391
lib/js.dart:391: typeof(message.object) == 'function') {
It shouldn't.  I'll change this to assert that.

On 2013/04/07 21:20:20, alexandre.ardhuin wrote:
> Could typeof(message.object) be something else that function ?

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode...
lib/js.dart:1001: } else if (member == 'call') {
I'm uncomfortable making an assumptions on minified names based on current
behavior.  I would expect it to change.  Let's see where bug 9283 ends up. 

On 2013/04/07 21:20:20, alexandre.ardhuin wrote:
> If what I have observed on minification of call is something that will not
> change (ie. member looks like "call$1" "call$2"...), you can add the following
> to the if.
> 
> member == 'call' || new RegExp(r"^call\$\d+$").hasMatch(member)
> 
> Thus even with minification of dart2js, the test "call JS function with
> varargs." is passing.