dart2js: Construct the entire output as a single AST before printing.

dart2js: Construct the entire output as a single AST before printing.

BUG=
R=floitsch@google.com, sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/03917ffe4d249b4cbd92d970ce859a0d53172f63

[herhut, 2015-05-19 13:11:41]

Patchset #1 (id:1) has been deleted

[herhut, 2015-05-19 13:12:00]

herhut@google.com changed reviewers:
+ floitsch@google.com, sra@google.com

[herhut, 2015-05-19 13:14:29]

@sra: PTAL the js_ast implementation. This should be a non-breaking change. 

@floitsch: PTAL the emitter part. Ideally, the new emitter should follow a
similar approach but I won't look at that for now.

Once this landed, I will separate building and printing into two steps and add
frequency analysis.

[floitsch, 2015-05-19 13:43:35]

LGTM.

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/runtime_types.dart:553: jsAst.Expression
getTypeRepresentationWithHashes(DartType type,
Name is bad now.

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:525: return
js.statement('${namer.currentIsolate}.# = #',
I don't like this.
It is true, that the isolateProperties object is, at this moment, stored in the
namer.currentIsolate variable. But since that's confusing we have a
"isolateProperties" variable that keeps track which JS variable contains the
object.
Now it looks as if we stored the initialValue in the currentIsolate (which is
not the case. We just use the currentIsolate's variable-name to hold the
isolateProperties object).

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:4627:
rti.getTypeRepresentationWithHashes(argument,
looks like it fits on one line.

https://codereview.chromium.org/1140703006/diff/20001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:1159: bool disableRenaming = false;
change to "enableRenaming".

[herhut, 2015-05-19 14:08:09]

Now just waiting for sra@ OK.

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/runtime_types.dart:553: jsAst.Expression
getTypeRepresentationWithHashes(DartType type,
On 2015/05/19 13:43:35, floitsch wrote:
> Name is bad now.

How about 'WithPlaceholders'? But in essence they are still hashes, just encoded
differently.

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:525: return
js.statement('${namer.currentIsolate}.# = #',
On 2015/05/19 13:43:35, floitsch wrote:
> I don't like this.
> It is true, that the isolateProperties object is, at this moment, stored in
the
> namer.currentIsolate variable. But since that's confusing we have a
> "isolateProperties" variable that keeps track which JS variable contains the
> object.
> Now it looks as if we stored the initialValue in the currentIsolate (which is
> not the case. We just use the currentIsolate's variable-name to hold the
> isolateProperties object).

But I can no longer do the magic "from here on we change what currentIsolate is"
trick, as the template will be instantiated in one go. Also, I find it even more
confusing to use a global to communicate some state change across many local
functions. Now it is at least clear where things are stored by this particular
piece of code.

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:4627:
rti.getTypeRepresentationWithHashes(argument,
On 2015/05/19 13:43:35, floitsch wrote:
> looks like it fits on one line.

Done.

https://codereview.chromium.org/1140703006/diff/20001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:1159: bool disableRenaming = false;
On 2015/05/19 13:43:35, floitsch wrote:
> change to "enableRenaming".

Done.

[floitsch, 2015-05-19 14:15:28]

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:525: return
js.statement('${namer.currentIsolate}.# = #',
On 2015/05/19 14:08:09, herhut wrote:
> On 2015/05/19 13:43:35, floitsch wrote:
> > I don't like this.
> > It is true, that the isolateProperties object is, at this moment, stored in
> the
> > namer.currentIsolate variable. But since that's confusing we have a
> > "isolateProperties" variable that keeps track which JS variable contains the
> > object.
> > Now it looks as if we stored the initialValue in the currentIsolate (which
is
> > not the case. We just use the currentIsolate's variable-name to hold the
> > isolateProperties object).
> 
> But I can no longer do the magic "from here on we change what currentIsolate
is"
> trick, as the template will be instantiated in one go. Also, I find it even
more
> confusing to use a global to communicate some state change across many local
> functions. Now it is at least clear where things are stored by this particular
> piece of code.

No it's not clear. This code says: "store the initializer in the current
isolate". and it doesn't do that.
It stores it in the variable named "namer.currentIsolate" which holds the
isolate-properties object. When looking at the code that's completely
non-obvious.

If you want to avoid isolateProperties, then create a new variable:
"currentIsolateOrIsolateProperties" and explain in the comment that the JS
variable is used for both.

[floitsch, 2015-05-19 14:16:50]

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:525: return
js.statement('${namer.currentIsolate}.# = #',
On 2015/05/19 14:15:28, floitsch wrote:
> On 2015/05/19 14:08:09, herhut wrote:
> > On 2015/05/19 13:43:35, floitsch wrote:
> > > I don't like this.
> > > It is true, that the isolateProperties object is, at this moment, stored
in
> > the
> > > namer.currentIsolate variable. But since that's confusing we have a
> > > "isolateProperties" variable that keeps track which JS variable contains
the
> > > object.
> > > Now it looks as if we stored the initialValue in the currentIsolate (which
> is
> > > not the case. We just use the currentIsolate's variable-name to hold the
> > > isolateProperties object).
> > 
> > But I can no longer do the magic "from here on we change what currentIsolate
> is"
> > trick, as the template will be instantiated in one go. Also, I find it even
> more
> > confusing to use a global to communicate some state change across many local
> > functions. Now it is at least clear where things are stored by this
particular
> > piece of code.
> 
> No it's not clear. This code says: "store the initializer in the current
> isolate". and it doesn't do that.
> It stores it in the variable named "namer.currentIsolate" which holds the
> isolate-properties object. When looking at the code that's completely
> non-obvious.
> 
> If you want to avoid isolateProperties, then create a new variable:
> "currentIsolateOrIsolateProperties" and explain in the comment that the JS
> variable is used for both.

Also: it doesn't matter when things are instantiated. You are doing a textual
string-replacement here. So the "at this time during the compilation..." would
still work if you create the templates in order.
(Not that they need to, but the "instantiated at one time" doesn't matter).

[sra1, 2015-05-19 20:56:55]

lgtm with fixes to call 'new Template(...)' or a new constructor.

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
File pkg/js_ast/lib/src/template.dart (right):

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:58: {this.isExpression: true, this.forceCopy:
false}) {
Assert here?

assert(this.isExpression ? ast is Expression : ast is Statement);

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:62: Template.withExpressionResult(this.ast,
{hasPlaceholders : false})
I think we should keep  'withExpressionResult' for creating a 'constant'
template.
Either use new Template() or a new constructor.

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:74: Template.withStatementResult(this.ast,
{hasPlaceholders : false})
Is this used?

[herhut, 2015-05-20 08:09:16]

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
File pkg/js_ast/lib/src/template.dart (right):

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:58: {this.isExpression: true, this.forceCopy:
false}) {
On 2015/05/19 20:56:55, sra1 wrote:
> Assert here?
> 
> assert(this.isExpression ? ast is Expression : ast is Statement);

Done.

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:62: Template.withExpressionResult(this.ast,
{hasPlaceholders : false})
On 2015/05/19 20:56:54, sra1 wrote:
> I think we should keep  'withExpressionResult' for creating a 'constant'
> template.
> Either use new Template() or a new constructor.

Done.

https://codereview.chromium.org/1140703006/diff/40001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:74: Template.withStatementResult(this.ast,
{hasPlaceholders : false})
On 2015/05/19 20:56:55, sra1 wrote:
> Is this used?

Removed.

[herhut, 2015-05-20 08:11:56]

On 2015/05/19 14:16:50, floitsch wrote:
>
https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
> File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):
> 
>
https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:525: return
> js.statement('${namer.currentIsolate}.# = #',
> On 2015/05/19 14:15:28, floitsch wrote:
> > On 2015/05/19 14:08:09, herhut wrote:
> > > On 2015/05/19 13:43:35, floitsch wrote:
> > > > I don't like this.
> > > > It is true, that the isolateProperties object is, at this moment, stored
> in
> > > the
> > > > namer.currentIsolate variable. But since that's confusing we have a
> > > > "isolateProperties" variable that keeps track which JS variable contains
> the
> > > > object.
> > > > Now it looks as if we stored the initialValue in the currentIsolate
(which
> > is
> > > > not the case. We just use the currentIsolate's variable-name to hold the
> > > > isolateProperties object).
> > > 
> > > But I can no longer do the magic "from here on we change what
currentIsolate
> > is"
> > > trick, as the template will be instantiated in one go. Also, I find it
even
> > more
> > > confusing to use a global to communicate some state change across many
local
> > > functions. Now it is at least clear where things are stored by this
> particular
> > > piece of code.
> > 
> > No it's not clear. This code says: "store the initializer in the current
> > isolate". and it doesn't do that.
> > It stores it in the variable named "namer.currentIsolate" which holds the
> > isolate-properties object. When looking at the code that's completely
> > non-obvious.
> > 
> > If you want to avoid isolateProperties, then create a new variable:
> > "currentIsolateOrIsolateProperties" and explain in the comment that the JS
> > variable is used for both.
> 
> Also: it doesn't matter when things are instantiated. You are doing a textual
> string-replacement here. So the "at this time during the compilation..." would
> still work if you create the templates in order.
> (Not that they need to, but the "instantiated at one time" doesn't matter).

Correct but then I would have to again first precompute the parts in a specific
order. I just don't like this order constraint. It is very confusing if you do
not know what it does. The only reason to use that variable is to shorten the
generated code. Using a long variable defeats that purpose. I will use a longer
fieldname to make it clear.

[herhut, 2015-05-20 10:31:21]

On 2015/05/20 08:11:56, herhut wrote:
> On 2015/05/19 14:16:50, floitsch wrote:
> >
>
https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
> > File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):
> > 
> >
>
https://codereview.chromium.org/1140703006/diff/20001/pkg/compiler/lib/src/js...
> > pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:525: return
> > js.statement('${namer.currentIsolate}.# = #',
> > On 2015/05/19 14:15:28, floitsch wrote:
> > > On 2015/05/19 14:08:09, herhut wrote:
> > > > On 2015/05/19 13:43:35, floitsch wrote:
> > > > > I don't like this.
> > > > > It is true, that the isolateProperties object is, at this moment,
stored
> > in
> > > > the
> > > > > namer.currentIsolate variable. But since that's confusing we have a
> > > > > "isolateProperties" variable that keeps track which JS variable
contains
> > the
> > > > > object.
> > > > > Now it looks as if we stored the initialValue in the currentIsolate
> (which
> > > is
> > > > > not the case. We just use the currentIsolate's variable-name to hold
the
> > > > > isolateProperties object).
> > > > 
> > > > But I can no longer do the magic "from here on we change what
> currentIsolate
> > > is"
> > > > trick, as the template will be instantiated in one go. Also, I find it
> even
> > > more
> > > > confusing to use a global to communicate some state change across many
> local
> > > > functions. Now it is at least clear where things are stored by this
> > particular
> > > > piece of code.
> > > 
> > > No it's not clear. This code says: "store the initializer in the current
> > > isolate". and it doesn't do that.
> > > It stores it in the variable named "namer.currentIsolate" which holds the
> > > isolate-properties object. When looking at the code that's completely
> > > non-obvious.
> > > 
> > > If you want to avoid isolateProperties, then create a new variable:
> > > "currentIsolateOrIsolateProperties" and explain in the comment that the JS
> > > variable is used for both.
> > 
> > Also: it doesn't matter when things are instantiated. You are doing a
textual
> > string-replacement here. So the "at this time during the compilation..."
would
> > still work if you create the templates in order.
> > (Not that they need to, but the "instantiated at one time" doesn't matter).
> 
> Correct but then I would have to again first precompute the parts in a
specific
> order. I just don't like this order constraint. It is very confusing if you do
> not know what it does. The only reason to use that variable is to shorten the
> generated code. Using a long variable defeats that purpose. I will use a
longer
> fieldname to make it clear.

I have decided in the end to reduce the scope where the magic $ exists and add 
a comment in the one place where it is used.

[herhut, 2015-05-20 10:31:50]

Committed patchset #3 (id:60001) manually as
03917ffe4d249b4cbd92d970ce859a0d53172f63 (presubmit successful).