Build finishClasses from JS AST nodes. (issue 12208067)

[a...@google.com]

Reviewers: ngeoffray,

Message:
Stephen, Erik: FYI, no need to have three people reviewing this change.

Description:
Build finishClasses from JS AST nodes.


Please review this at https://codereview.chromium.org/12208067/

SVN Base: https://dart.googlecode.com/svn/branches/bleeding_edge

Affected files:
M dart/sdk/lib/_internal/compiler/implementation/js/nodes.dart
M dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
M
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart

[a...@google.com]

I cannot create a completely empty diff between the generated out of this
version and the original version. However, I have made another CL (which I
won't
submit) which shows the minor tweaks necessary to get an empty diff. See:
https://codereview.chromium.org/12212064/

https://codereview.chromium.org/12208067/

[a...@google.com]

Not much, but this saves 913 bytes on swarm in minified mode.

My plan is to get the entire "init" function converted, as I need to change
it a
lot to implement deferred/lazy loading, and I'm actually having a really
hard
time reading the "init" function as it isn't correctly indented in the
output.
So I figured rather than fixing that by adding more bytes (added
whitespace), I
should make the function smaller in minified mode :-)


https://codereview.chromium.org/12208067/

[Kasper Lund]

I like.

[s...@google.com]

I'm finding the bigger functions a little too hard to read.

One of the problems is that, with .addTo(), the construction has shifted
from
begin functional to being operational. Another problem is that the code is
really big. 5x explosion on a 3 line function is much more manageable that
5x
on a 20 line function.

Florian's idea was that we parse the canned JS to generate the AST.
I think this CL exposes the limits of a library of AST construction
combinators,
so I'm in favor of a parsing approach since the bigger functions will be
more
maintainable.

Starting on a JS parser sooner has some advantages.

With a little modification to parse placeholders we could parse the JS(...)
code
and solve the problem we have when substituting expressions of unknown
precedence. We have mostly been very lucky here.

Parsing with placeholders also opens the way for templated generation in
some
places where we generate code by hand. I think we would want named
placeholders
(#FOO) for bigger templates rather than the positional JS-# placeholders,
and
some splicing (#..ARGS) to make that work.

The parser does not have to cover the whole language, e.g. we can omit
parsing
'with', always use // style comments, always use \u escapes for quotes in
strings, limit the syntax of numbers, etc. We can add statements like
switch
and do-while as we need them. Only a subset of the language is on the
critical
path.


https://codereview.chromium.org/12208067/

[Florian Loitsch]

https://code.google.com/p/jsparser/

It hasn't been updated for a long time, but I guess that it's still relatively easy to adapt.

The current AST initially came from there.

--
Give a man a fire and he's warm for the whole day,
but set fire to him and he's warm for the rest of his life. - Terry Pratchett

[Peter Ahé]

I'm not terribly excited about adding a JavaScript parser to dart2js.

The direction I'd like to take is to write the "init" function in
Dart, and I'll look into that after M3.

As for the problem with JS, my preference would be to add a few
different variants to express precedence.

Generally, I'd like to have as little JS as possible. One reason for
this is that Dart is supposed to be easier to optimize than
JavaScript. Another reason is that the mirror system will eventually
need access to most of this code, so you dynamically can build new
classes and isolates.

Cheers,
Peter

On Thu, Feb 7, 2013 at 11:51 PM, <s...@google.com> wrote:

[Florian Loitsch]

On Fri, Feb 8, 2013 at 2:05 PM, Peter Ahé <a...@google.com> wrote:

I'm not terribly excited about adding a JavaScript parser to dart2js.

The direction I'd like to take is to write the "init" function in
Dart, and I'll look into that after M3.

I strongly advise against running any kind of Dart code before the Isolate is set up. 

As for the problem with JS, my preference would be to add a few
different variants to express precedence.

Generally, I'd like to have as little JS as possible. One reason for
this is that Dart is supposed to be easier to optimize than
JavaScript.

Anything dart2js can produce, we can write ourselves, too.

[a...@google.com]

I have fully converted "init" to by built from JS.

There are many open questions about what approach we should take in the
future,
but I would really like to have this change landed so I can make progress on
loading classes lazily.

Cheers,
Peter

https://codereview.chromium.org/12208067/

[a...@google.com]

PTAL, I hope it is more readable now.

Cheers,
Peter


https://codereview.chromium.org/12208067/

[s...@google.com]

On 2013/02/11 19:32:41, ahe wrote:
> PTAL, I hope it is more readable now.

> Cheers,
> Peter

I'm still having a hard time reading this.
Can we VC to discuss?

https://codereview.chromium.org/12208067/

[ngeo...@google.com]

I like the new builder. jsAst and js are a bit annoying, maybe use jst and
jsb,
but I understand this is not great either.

About the changes from JS blobs -> AST, not sure what's our best option. We
will
probably also have some small JS blobs in a new (third) revision of the
isolate
library. The first isolate library had quite a lot of JS, and I doubt we can
maintain such JS if it's written in JS ASTs.

But for now, I'm OK with this change.


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

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode203
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:203:
// var needsAccessor = (lastCharCode & $SUFFIX_MASK) >=
$FIRST_SUFFIX_CODE;
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode250
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:250:
// prototype["set\$" + accessorName] = new Function("v", setterString);
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode538
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:538:
js['newPrototype'][js['member']].assign(js['prototype'][js['member']])
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart#newcode78
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:78:
jsAst.Expression buildConstructor(String mangledName, List<String>
fieldNames) {
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart#newcode79
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:79:
return new jsAst.NamedFunction(
Why don't you use JsBuilder here?

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart#newcode158
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:158:
List<jsAst.Parameter> stubParameters) {
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart#newcode182
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:182:
[js.use(name), new jsAst.LiteralNumber('$arity')]))));
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart#newcode426
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:426:
new jsAst.Call(new jsAst.Fun([], new jsAst.Block(statements)), [])),
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
File dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart#newcode2154
dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2154:
push(new js.Binary(cmp, typeOf, js.js.string(typeName)));
wat? Maybe there's something nice we can write here and below.

https://codereview.chromium.org/12208067/

[ngeo...@google.com]

LGTM

https://codereview.chromium.org/12208067/

[erik...@google.com]

I think it's OK with the readability. It feels quite a lot lighter than
adding
a JS parser to the program.

LGTM

https://codereview.chromium.org/12208067/

[floi...@google.com]

DBC.


https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(left):

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#oldcode308
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:308:
'''/* Opera does not support 'getOwnPropertyNames'. Therefore we use
That should be fixed with today's announcement ;)

Seriously, though: we should a TODO(issue-number) to remove work-arounds
for Opera.
There is another work-around at line 249. That one could actually
already been obsolete (since they fixed it more than 6 months ago).

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

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode508
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:508:
js['prototype']['__proto__'].assign(js['superConstructor']['prototype']),
80chars.

https://codereview.chromium.org/12208067/

[a...@google.com]

I found a few problems during testing.

Nicolas, could you take another look? Please ignore patch sets 5 and 6.
What is
interesting to you are the changes since patch set 4.

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

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode203
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:203:
// var needsAccessor = (lastCharCode & $SUFFIX_MASK) >=
$FIRST_SUFFIX_CODE;

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode250
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:250:
// prototype["set\$" + accessorName] = new Function("v", setterString);

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#newcode538
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:538:
js['newPrototype'][js['member']].assign(js['prototype'][js['member']])

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart#newcode78
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:78:
jsAst.Expression buildConstructor(String mangledName, List<String>
fieldNames) {

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

I'm not changing this file anymore.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart#newcode79
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:79:
return new jsAst.NamedFunction(

On 2013/02/12 08:42:41, ngeoffray wrote:
> Why don't you use JsBuilder here?

Ditto.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart#newcode158
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:158:
List<jsAst.Parameter> stubParameters) {

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart#newcode182
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:182:
[js.use(name), new jsAst.LiteralNumber('$arity')]))));

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart#newcode426
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:426:
new jsAst.Call(new jsAst.Fun([], new jsAst.Block(statements)), [])),

On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
File dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart#newcode2154
dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2154:
push(new js.Binary(cmp, typeOf, js.js.string(typeName)));

On 2013/02/12 08:42:41, ngeoffray wrote:
> wat? Maybe there's something nice we can write here and below.

Done.

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(left):

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart#oldcode308
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:308:
'''/* Opera does not support 'getOwnPropertyNames'. Therefore we use

On 2013/02/13 18:53:14, floitsch wrote:
> That should be fixed with today's announcement ;)

> Seriously, though: we should a TODO(issue-number) to remove
work-arounds for
> Opera.
> There is another work-around at line 249. That one could actually
already been
> obsolete (since they fixed it more than 6 months ago).

Good point, I'll add todos and file bugs.

https://codereview.chromium.org/12208067/

[ngeo...@google.com]

Still LGTM

https://codereview.chromium.org/12208067/