Add a small JS parser to ease the building of ASTs

Add a small JS parser to ease the building of ASTs


BUG=

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

[ahe, 2013-02-19 10:05:24]

I'm not sure about adding another parser. I would like to make sure that
everyone on the team is comfortable with this approach.

I'm concerned about the lack of direct tests of the parser. I think it would be
great to have unit tests that parse an expression and use js.prettyPrint to
verify that we get the desired behavior.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:932: class JsBuilder {
How about moving JsBuilder and MiniJsParser to as separate library?

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1086: class MiniJsParser
{
I would call this "JsExpressionParser" or something like that.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1088: : lastCategory =
NONE, lastToken = null, lastPosition = 0, position = 0 {
One line per initializer.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1174: } while
(src.codeUnitAt(position) != code);
This doesn't handle \ in strings.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1188: (cat == NUMERIC &&
newCat == DOT) ||      // eg. 3.1415
I think this will allow things like: 1.1.1.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1280: // without
brackets.
Should be "without parentheses". "brackets" is ambiguous, in the US it normally
refers to [].

[erikcorry, 2013-02-19 10:38:29]

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:932: class JsBuilder {
On 2013/02/19 10:05:25, ahe wrote:
> How about moving JsBuilder and MiniJsParser to as separate library?

I'd like to move them to a different file (a separate library feels like
overkill) but if I do that in this change the new stuff will be obscured in the
move, making code review very hard.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1174: } while
(src.codeUnitAt(position) != code);
On 2013/02/19 10:05:25, ahe wrote:
> This doesn't handle \ in strings.

That's right, that's noted in the description of the parser.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1188: (cat == NUMERIC &&
newCat == DOT) ||      // eg. 3.1415
On 2013/02/19 10:05:25, ahe wrote:
> I think this will allow things like: 1.1.1.

Yes, it will.  I think my preferred solution is "don't do that, then".

The parser can also parse 'made up' operators like *+ and +++.

[ahe, 2013-02-19 10:47:34]

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:932: class JsBuilder {
On 2013/02/19 10:38:29, erikcorry wrote:
> On 2013/02/19 10:05:25, ahe wrote:
> > How about moving JsBuilder and MiniJsParser to as separate library?
> 
> I'd like to move them to a different file (a separate library feels like
> overkill) but if I do that in this change the new stuff will be obscured in
the
> move, making code review very hard.

Good point.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1174: } while
(src.codeUnitAt(position) != code);
On 2013/02/19 10:38:29, erikcorry wrote:
> On 2013/02/19 10:05:25, ahe wrote:
> > This doesn't handle \ in strings.
> 
> That's right, that's noted in the description of the parser.

You could throw an exception if you see a backslash.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1188: (cat == NUMERIC &&
newCat == DOT) ||      // eg. 3.1415
On 2013/02/19 10:38:29, erikcorry wrote:
> On 2013/02/19 10:05:25, ahe wrote:
> > I think this will allow things like: 1.1.1.
> 
> Yes, it will.  I think my preferred solution is "don't do that, then".
> 
> The parser can also parse 'made up' operators like *+ and +++.

You could add code like this after line 1191:

if (cat == NUMERIC) double.parse(lastToken, (_) { throw new
MiniJsParserError(...); });
if (cat == SYMBOL) {
  // Refactor Printer.visitBinary, etc to check that symbol is a real operator.
}

In the long run, it helps everyone working on the compiler to get an error early
when using features that aren't supported.

[sra1, 2013-02-20 01:50:24]

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1076: /// * string,
boolean, null and numeric literals (no backslash escapes, no hex).
see comment below about backslashes.
I'd revise to say that backslash escapes work except for *the opening quote*,
but are not validated.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1086: class MiniJsParser
{
On 2013/02/19 10:05:25, ahe wrote:
> I would call this "JsExpressionParser" or something like that.

I think the urge to extend to statements will be irresistible, so keep the name.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1174: } while
(src.codeUnitAt(position) != code);
On 2013/02/19 10:47:34, ahe wrote:
> On 2013/02/19 10:38:29, erikcorry wrote:
> > On 2013/02/19 10:05:25, ahe wrote:
> > > This doesn't handle \ in strings.
> > 
> > That's right, that's noted in the description of the parser.
> 
> You could throw an exception if you see a backslash.

Noooo! It works pretty well (e.g. is actually used with string literals
containing \n newlines).  The LiteralString AST expects the string to be quoted
and have all escaping done.  The only thing that goes wrong with parsing is if
the string contains an escape of the opening quote.  Other escapes get copied to
the out.js without being processed, which is great for throughput :-)

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1188: (cat == NUMERIC &&
newCat == DOT) ||      // eg. 3.1415
On 2013/02/19 10:47:34, ahe wrote:
> On 2013/02/19 10:38:29, erikcorry wrote:
> > On 2013/02/19 10:05:25, ahe wrote:
> > > I think this will allow things like: 1.1.1.
> > 
> > Yes, it will.  I think my preferred solution is "don't do that, then".
> > 
> > The parser can also parse 'made up' operators like *+ and +++.
> 
> You could add code like this after line 1191:
> 
> if (cat == NUMERIC) double.parse(lastToken, (_) { throw new
> MiniJsParserError(...); });
> if (cat == SYMBOL) {
>   // Refactor Printer.visitBinary, etc to check that symbol is a real
operator.

We should avoid putting checks in the printer that prints 100x the trees that we
parse.  Perhaps it would be ok in debug mode.

> }
> 
> In the long run, it helps everyone working on the compiler to get an error
early
> when using features that aren't supported.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1309: return new
Prefix("!", expression);
Looks like this silently gets  "!a == true"   wrong.

Probably best to add a level between Binary and Call to put the prefix/ unary
operators:  typeof ! ~ - ++ -- + void delete.
We probably care about them in approximately that order.

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

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:199:
js['needsGetter'].def(needsGetterCode('lastCharCode')),
js['var needsGetter = (lastCharCode & 3) > 0'],

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:371: // var
fields = desc[''], supr;
Looking at the parser, you should be able to parse this monster directly:

js['var fields = desc[""], supr'];

[erikcorry, 2013-02-20 14:39:09]

New version PTAL.  Now supports new and typeof.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1188: (cat == NUMERIC &&
newCat == DOT) ||      // eg. 3.1415
On 2013/02/20 01:50:24, sra1 wrote:
> On 2013/02/19 10:47:34, ahe wrote:
> > On 2013/02/19 10:38:29, erikcorry wrote:
> > > On 2013/02/19 10:05:25, ahe wrote:
> > > > I think this will allow things like: 1.1.1.
> > > 
> > > Yes, it will.  I think my preferred solution is "don't do that, then".
> > > 
> > > The parser can also parse 'made up' operators like *+ and +++.
> > 
> > You could add code like this after line 1191:
> > 
> > if (cat == NUMERIC) double.parse(lastToken, (_) { throw new
> > MiniJsParserError(...); });
> > if (cat == SYMBOL) {
> >   // Refactor Printer.visitBinary, etc to check that symbol is a real
> operator.
> 
> We should avoid putting checks in the printer that prints 100x the trees that
we
> parse.  Perhaps it would be ok in debug mode.
> 
> > }
> > 
> > In the long run, it helps everyone working on the compiler to get an error
> early
> > when using features that aren't supported.
> 

The printer already performs this check (at least in host_checked mode where it
says "Forgot operator").  I added a check added to the parser in the interest of
failing early.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js/nodes.dart:1309: return new
Prefix("!", expression);
On 2013/02/20 01:50:24, sra1 wrote:
> Looks like this silently gets  "!a == true"   wrong.
> 
> Probably best to add a level between Binary and Call to put the prefix/ unary
> operators:  typeof ! ~ - ++ -- + void delete.
> We probably care about them in approximately that order.

Done.

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

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:199:
js['needsGetter'].def(needsGetterCode('lastCharCode')),
On 2013/02/20 01:50:24, sra1 wrote:
> js['var needsGetter = (lastCharCode & 3) > 0'],

Instead I restored the old version where there is a snippet of code for testing
the flags which is defined near where the flag constants are defined for easy
maintenance.

https://codereview.chromium.org/12276002/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:371: // var
fields = desc[''], supr;
On 2013/02/20 01:50:24, sra1 wrote:
> Looking at the parser, you should be able to parse this monster directly:
> 
> js['var fields = desc[""], supr'];

Done.

[ahe, 2013-02-20 14:46:16]

On Wed, Feb 20, 2013 at 3:33 PM, Erik Corry <erikcorry@google.com> wrote:
> Can we assume that people who do not complain are happy?  Otherwise it
> will take me a long time to collect all the LGTMs I need.

SGTM

> I was going to add them to the file where you put the new tests
> associated with https://codereview.chromium.org//12208067 but I just
> couldn't find it!

That's a good point, some of the methods I added with CL 12208067 deserved unit
tests. I'm better at thinking about this when I review code. My bad, and I
encourage my reviewers to help me remember unit tests.

[ahe, 2013-02-20 15:01:44]

LGTM!

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

https://codereview.chromium.org/12276002/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:342: js["var
fields = desc[''], supr"],
So much better!

[sra1, 2013-02-20 21:22:58]

lgtm

[erikcorry, 2013-02-21 08:45:25]

Committed patchset #8 manually as r18806 (presubmit successful).