dart2js: fix named arguments with Function.apply in the startup emitter.

dart2js: fix named arguments with Function.apply in the startup emitter.

This includes a full rewrite of the Function.apply in the runtime.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/0231f6ba39854a029b06257b7fb5aa5f12d7b0ed

[floitsch, 2015-07-22 15:52:07]

floitsch@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-07-23 19:43:42]

Any plans for unittests for the new emitter and each feature added so far?

One idea, would be to submit a few files of generated js code that illustrate
how each feature looks in the output. Even though it's not useful to maintain
those files by hand (I'd recommend autogenerating them and validating that they
are current), just seeing the generated code helps a lot in visualizing what the
emitter code is doing.

Then on any feature update we'd be able to see how the generated code is
affected by a change in the compiler (that could make code reviews easier to
follow too)

https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart
(right):

https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart:684: ///
If the method is a call-method, also returns the necessary properties
nit: call-method => closure-call-method

https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart:687:
Map<js.Expression, js.Expression> properties =
nit: var? 
(I prefer not to repeat the type when we can see it directly in the rhs)

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1160: * Implements
[Function.apply] for the "new" emitters.
"new" emitters => start_up and lazy emitters?

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1180: * JavaScript array with
the default values. Otherwise it is a
nit: Otherwise => Otherwise, when the function accepts optional named arguments,
 ...

(Because there are to `ifs` earlier, it wasn't clear whether you meant otherwise
about positional vs named, or about having vs not having optional arguments
altogether)

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1192: static
applyFunctionNewEmitter(Function function,
not sure what to name this now that there is no "new emitter".

Some ideas:
* applyFunction2
* applyFunctionForClosureCallMethod

or maybe rename the old to applyFunctionFullEmitter instead?

Another alternative: have more than one js_helper libraries, one with shared
code, and one per emitter?

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1252: var interceptor =
getInterceptor(function);
move this further down, closer to where we need it? (in case we reach the
functionNoSuchMethod case?)

[Siggi Cherem (dart-lang), 2015-07-23 19:44:31]

(but otherwise lgtm)

[floitsch, 2015-07-29 18:26:39]

On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> Any plans for unittests for the new emitter and each feature added so far?
> 
> One idea, would be to submit a few files of generated js code that illustrate
> how each feature looks in the output. Even though it's not useful to maintain
> those files by hand (I'd recommend autogenerating them and validating that
they
> are current), just seeing the generated code helps a lot in visualizing what
the
> emitter code is doing.
> 
> Then on any feature update we'd be able to see how the generated code is
> affected by a change in the compiler (that could make code reviews easier to
> follow too)

No. I don't have any plans to add more tests. Most of the features are tested by
functionality, and then we have a few tests that look specifically at the output
(like making sure that we do optimizations right, and don't emit too much code).
I could upload a small program with every CL (to aid visualizing the generated
code), but I wouldn't want to check that code in.
> 
>
https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
> File pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart
> (right):
> 
>
https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart:684: ///
> If the method is a call-method, also returns the necessary properties
> nit: call-method => closure-call-method
> 
>
https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart:687:
> Map<js.Expression, js.Expression> properties =
> nit: var? 
> (I prefer not to repeat the type when we can see it directly in the rhs)
> 
>
https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
> File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):
> 
>
https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
> sdk/lib/_internal/js_runtime/lib/js_helper.dart:1160: * Implements
> [Function.apply] for the "new" emitters.
> "new" emitters => start_up and lazy emitters?
> 
>
https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
> sdk/lib/_internal/js_runtime/lib/js_helper.dart:1180: * JavaScript array with
> the default values. Otherwise it is a
> nit: Otherwise => Otherwise, when the function accepts optional named
arguments,
>  ...
> 
> (Because there are to `ifs` earlier, it wasn't clear whether you meant
otherwise
> about positional vs named, or about having vs not having optional arguments
> altogether)
> 
>
https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
> sdk/lib/_internal/js_runtime/lib/js_helper.dart:1192: static
> applyFunctionNewEmitter(Function function,
> not sure what to name this now that there is no "new emitter".
> 
> Some ideas:
> * applyFunction2
> * applyFunctionForClosureCallMethod
> 
> or maybe rename the old to applyFunctionFullEmitter instead?
> 
> Another alternative: have more than one js_helper libraries, one with shared
> code, and one per emitter?
> 
>
https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
> sdk/lib/_internal/js_runtime/lib/js_helper.dart:1252: var interceptor =
> getInterceptor(function);
> move this further down, closer to where we need it? (in case we reach the
> functionNoSuchMethod case?)

[floitsch, 2015-07-29 18:26:46]

https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart
(right):

https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart:684: ///
If the method is a call-method, also returns the necessary properties
On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> nit: call-method => closure-call-method

Done.

https://codereview.chromium.org/1252533003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/startup_emitter/fragment_emitter.dart:687:
Map<js.Expression, js.Expression> properties =
On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> nit: var? 
> (I prefer not to repeat the type when we can see it directly in the rhs)

We always used the full type, but done.

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1160: * Implements
[Function.apply] for the "new" emitters.
On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> "new" emitters => start_up and lazy emitters?

Done.

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1180: * JavaScript array with
the default values. Otherwise it is a
On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> nit: Otherwise => Otherwise, when the function accepts optional named
arguments,
>  ...
> 
> (Because there are to `ifs` earlier, it wasn't clear whether you meant
otherwise
> about positional vs named, or about having vs not having optional arguments
> altogether)

Done.

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1192: static
applyFunctionNewEmitter(Function function,
On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> not sure what to name this now that there is no "new emitter".
> 
> Some ideas:
> * applyFunction2
> * applyFunctionForClosureCallMethod
> 
> or maybe rename the old to applyFunctionFullEmitter instead?
> 
> Another alternative: have more than one js_helper libraries, one with shared
> code, and one per emitter?

I ended up picking applyFunction2.
In the end it is a different way of doing applyFunction. We could try to encode
the differences in the name, but I'm not sure it helps that much...

https://codereview.chromium.org/1252533003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:1252: var interceptor =
getInterceptor(function);
On 2015/07/23 19:43:42, Siggi Cherem (dart-lang) wrote:
> move this further down, closer to where we need it? (in case we reach the
> functionNoSuchMethod case?)

Done.

[floitsch, 2015-08-03 16:06:36]

Committed patchset #3 (id:40001) manually as
0231f6ba39854a029b06257b7fb5aa5f12d7b0ed (presubmit successful).

[Siggi Cherem (dart-lang), 2015-08-04 02:20:21]

lgtm