Make static tear-off closures a class, like instance tear-off closures.

Make static tear-off closures a class, like instance tear-off closures.

R=johnniwinther@google.com, sra@google.com

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

[ngeoffray, 2013-05-06 10:16:26]

@Kasper: there was a limitation on our tear-off closures of static methods.
Because we were putting the is$ properties on the JS method itself, and we are
now using getInterceptor for doing type checks, we had to do some special
handling for them.

By making closures uniform, we now can just return the object itself in
getInterceptor, and remove the JSFunction interceptor. An alternative would have
been to add the Object functions in each tear-off closure, but I like the
simplicity of having all closures be the same.

[sra1, 2013-05-07 00:26:13]

General comments

1. 'TearOff' seems to be a strange name.  It makes sense to maybe call
BoundClosures tear-offs, but these are constants and are not disconnected from
(or torn off from) anything.  They are constant closures of static methods or
functions.  Perhaps call them 'Function' or 'Closure'
  i.e.
  main$function  Function_main
  main$closure,  Closed_main      // The closure of main.

I suggest '$closure' in the emitted code and the term 'static closure' in the
generating code, e.g.

getStaticTearOffClosureName(element)
->
nameOfStaticClosureFor(element)      // or getNameOfStaticClosureFor if you
like.

(FWIW I would rename BoundClosure to BoundMethod.  The method has been bound to
a receiver to produce a closure.)

Looking at this code:

// Normal classes
$$....
// Bound closures
$$.TearOff_main = {"": "Closure;call$0", $name: "main$tearOff"};
// Static methods
$.main = function() {
  $.Swarm$(false).run$0();
};
// Constants
$.main$tearOff = new $.TearOff_main($.main);


2. Why is $name: "main$tearOff"?  "main" would be a more accurate name if it
gets printed in an error message.  It fact, it is necessary if the name is used
cross-isolate.

3. It takes a lot of ping-pong in the source to understand what is going on.
Is it possible to move the tearoff/closure to immediately after the functions
they close over?

4. $name should be a closure field.  It would be possible to share closures
(i.e. two static functions with identical types) if $name is field.  Add a TODO
for this.

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1821: final
Map<Element, Element> staticGetters = new Map<Element, Element>();
Is the key a FunctionElement?

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1823: void
emitStaticFunctionGetters(CodeBuffer eagerBuffer) {
Can we make constant functions be emitted instead of having this special
handling?

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1824:
staticGetters.forEach((Element element, Element closure) {
Please emit in source order, otherwise output order is unstable.

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1829:
buffer.write('$isolateProperties.$name = ');
Please construct an AST and prettyPrint that to get proper minification.

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1837:
compiler.codegenWorld.staticFunctionsNeedingGetter;
We have
  staticGetters.keys
and
  compiler.codegenWorld.staticFunctionsNeedingGetter

What is the difference?

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1869:
closureBuilder.addProperty(fieldName, js('"$tearOffName"'));
Make $name a field of the closure, initialized by the constructor call.

Add a TODO to synthesize common base classes for closures, bound closures and
static closures of indistinguishable type.

[ngeoffray, 2013-05-13 10:24:48]

Thanks Stephen, PTAL.

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1821: final
Map<Element, Element> staticGetters = new Map<Element, Element>();
On 2013/05/07 00:26:14, sra1 wrote:
> Is the key a FunctionElement?

Yes.

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1823: void
emitStaticFunctionGetters(CodeBuffer eagerBuffer) {
On 2013/05/07 00:26:14, sra1 wrote:
> Can we make constant functions be emitted instead of having this special
> handling?

Not sure I understand what you mean here.

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1824:
staticGetters.forEach((Element element, Element closure) {
On 2013/05/07 00:26:14, sra1 wrote:
> Please emit in source order, otherwise output order is unstable.

Done.

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1829:
buffer.write('$isolateProperties.$name = ');
On 2013/05/07 00:26:14, sra1 wrote:
> Please construct an AST and prettyPrint that to get proper minification.

Done.

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1837:
compiler.codegenWorld.staticFunctionsNeedingGetter;
On 2013/05/07 00:26:14, sra1 wrote:
> We have
>   staticGetters.keys
> and
>   compiler.codegenWorld.staticFunctionsNeedingGetter
> 
> What is the difference?

None.

https://codereview.chromium.org/14986002/diff/36/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1869:
closureBuilder.addProperty(fieldName, js('"$tearOffName"'));
On 2013/05/07 00:26:14, sra1 wrote:
> Make $name a field of the closure, initialized by the constructor call.
> 
> Add a TODO to synthesize common base classes for closures, bound closures and
> static closures of indistinguishable type.

Done.

[ngeoffray, 2013-05-13 10:26:47]

On 2013/05/07 00:26:13, sra1 wrote:
> General comments
> 
> 1. 'TearOff' seems to be a strange name.  It makes sense to maybe call
> BoundClosures tear-offs, but these are constants and are not disconnected from
> (or torn off from) anything.  They are constant closures of static methods or
> functions.  Perhaps call them 'Function' or 'Closure'
>   i.e.
>   main$function  Function_main
>   main$closure,  Closed_main      // The closure of main.
> 
> I suggest '$closure' in the emitted code and the term 'static closure' in the
> generating code, e.g.
> 
> getStaticTearOffClosureName(element)
> ->
> nameOfStaticClosureFor(element)      // or getNameOfStaticClosureFor if you
> like.
> 
> (FWIW I would rename BoundClosure to BoundMethod.  The method has been bound
to
> a receiver to produce a closure.)
> 
> Looking at this code:
> 
> // Normal classes
> $$....
> // Bound closures
> $$.TearOff_main = {"": "Closure;call$0", $name: "main$tearOff"};
> // Static methods
> $.main = function() {
>   $.Swarm$(false).run$0();
> };
> // Constants
> $.main$tearOff = new $.TearOff_main($.main);
> 
> 
> 2. Why is $name: "main$tearOff"?  "main" would be a more accurate name if it
> gets printed in an error message.  It fact, it is necessary if the name is
used
> cross-isolate.

It needs to be a different name than the actual method. spawnUri will do a
myClosure.call$1 call, and that is only on the created closure, not the static
method.

> 
> 3. It takes a lot of ping-pong in the source to understand what is going on.
> Is it possible to move the tearoff/closure to immediately after the functions
> they close over?

As discussed, we currently need to emit this way because of how we setup the '$'
object. More refactoring out of scope of this CL is needed.

> 
> 4. $name should be a closure field.  It would be possible to share closures
> (i.e. two static functions with identical types) if $name is field.  Add a
TODO
> for this.
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1821: final
> Map<Element, Element> staticGetters = new Map<Element, Element>();
> Is the key a FunctionElement?
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1823: void
> emitStaticFunctionGetters(CodeBuffer eagerBuffer) {
> Can we make constant functions be emitted instead of having this special
> handling?
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1824:
> staticGetters.forEach((Element element, Element closure) {
> Please emit in source order, otherwise output order is unstable.
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1829:
> buffer.write('$isolateProperties.$name = ');
> Please construct an AST and prettyPrint that to get proper minification.
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1837:
> compiler.codegenWorld.staticFunctionsNeedingGetter;
> We have
>   staticGetters.keys
> and
>   compiler.codegenWorld.staticFunctionsNeedingGetter
> 
> What is the difference?
> 
>
https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
> sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1869:
> closureBuilder.addProperty(fieldName, js('"$tearOffName"'));
> Make $name a field of the closure, initialized by the constructor call.
> 
> Add a TODO to synthesize common base classes for closures, bound closures and
> static closures of indistinguishable type.

[sra1, 2013-05-14 03:19:46]

lgtm

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1823: void
emitStaticFunctionGetters(CodeBuffer eagerBuffer) {
On 2013/05/13 10:24:48, ngeoffray wrote:
> On 2013/05/07 00:26:14, sra1 wrote:
> > Can we make constant functions be emitted instead of having this special
> > handling?
> 
> Not sure I understand what you mean here.

What I mean is that the whole life-cycle of a static method that escapes is very
similar to that of a Constant.  Perhaps there should be a subclass of Constant
for these closures.

[ngeoffray, 2013-05-14 08:28:45]

Thanks Stephen.

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/14986002/diff/36/sdk/lib/_internal/com...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1823: void
emitStaticFunctionGetters(CodeBuffer eagerBuffer) {
On 2013/05/14 03:19:46, sra1 wrote:
> On 2013/05/13 10:24:48, ngeoffray wrote:
> > On 2013/05/07 00:26:14, sra1 wrote:
> > > Can we make constant functions be emitted instead of having this special
> > > handling?
> > 
> > Not sure I understand what you mean here.
> 
> What I mean is that the whole life-cycle of a static method that escapes is
very
> similar to that of a Constant.  Perhaps there should be a subclass of Constant
> for these closures.

I see. So there is already a FunctionConstant to reference those static
closures. They however can't emit themselves the code for their initialization
because they need to know the closure class to instantiate.

[ngeoffray, 2013-05-14 09:41:08]

@Johnni: Could you please take a look at the changes in codegen.dart, line 5022?

[Johnni Winther, 2013-05-14 10:37:37]

lgtm

[ngeoffray, 2013-05-14 10:40:06]

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

[ahe, 2013-05-16 20:07:56]

https://codereview.chromium.org/14986002/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/14986002/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1836: 'new
$closureClass($isolateProperties.$staticName, "$closureName")');
This broke deferred loading (and try.dartlang.org).

You're emitting a constant here, but not using bufferForConstant.  This means
that $ isn't the correct object.  We can take a look at that tomorrow or later.

[sra1, 2013-05-16 20:28:33]

https://codereview.chromium.org/14986002/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/14986002/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1836: 'new
$closureClass($isolateProperties.$staticName, "$closureName")');
On 2013/05/16 20:07:56, ahe wrote:
> This broke deferred loading (and http://try.dartlang.org).
> 
> You're emitting a constant here, but not using bufferForConstant.  This means
> that $ isn't the correct object.  We can take a look at that tomorrow or
later.

This would 'just work' if static function closures were generated as a new kind
of constant, ClosureConstant, which works rather like a ConstructedConstant. 
The arguments would be the FunctionConstant referencing the raw static function
and the name string.
(It might even be reasonable to use ConstructedConstant directly).

[ahe, 2013-05-16 20:30:44]

https://codereview.chromium.org/14986002/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/14986002/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1836: 'new
$closureClass($isolateProperties.$staticName, "$closureName")');
On 2013/05/16 20:28:33, sra1 wrote:
> On 2013/05/16 20:07:56, ahe wrote:
> > This broke deferred loading (and http://try.dartlang.org).
> > 
> > You're emitting a constant here, but not using bufferForConstant.  This
means
> > that $ isn't the correct object.  We can take a look at that tomorrow or
> later.
> 
> This would 'just work' if static function closures were generated as a new
kind
> of constant, ClosureConstant, which works rather like a ConstructedConstant. 
> The arguments would be the FunctionConstant referencing the raw static
function
> and the name string.
> (It might even be reasonable to use ConstructedConstant directly).

I was thinking the same thing.  Well, up to the second comma.  Then you thought
ahead :-)