572014 - LETs and expression closures in the narcissus parser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dimitris Vardoulakis]

Attachment: diffs in jsdefs, jslex, jsparse

I attached a patch to the narcissus parser that handles some mozilla-specific JS extensions: let statements, let definitions, let expressions, let in the head of a forloop, expression closures. 

It doesn't handle let with destructuring. Also, it has a bug that's also found in the spidermonkey parser (Bug 569464).

Comment 1

[Dimitris Vardoulakis]

Attachment: new diffs in jsparse

Narcissus now parses all of jetpack without errors. I added for/each, yield and array comprehensions. I also improved the error reporting for for/in. Patrick, you can go ahead and review it now.

Comment 2

[Patrick Walton (:pcwalton)]

Attachment: dimvar's latest patch

Attaching dimvar's patch (this is his patch, not mine; I just rebased it to tracemonkey).

Comment 3

[Patrick Walton (:pcwalton)]

Review follows.

All the comments you added to Narcissus are much appreciated, especially if Narcissus is to be the basis for JS language prototyping in the future.

> +    //dimvar: when is the else branch taken?

I'm not sure myself; will need to ask Brendan why it's there. In any case this comment should probably be removed :)

> +    case LET:

Two spaces after the cases of a switch statement. In other words, half-indent. (Several of these in the patch.)

> +        if (n.value === "Let stm") return n;

Might want to introduce new constants (like LET_STATEMENT) for these, in the style of the existing code.

> +            else if (n2t !== IDENTIFIER) throw se;

Brace the body of the if statement since the others are braced. Also cuddle the elses, and put "throw se" on a separate line. There are several of these throughout the patch.

> +    }
> +    else /* Expression closures (1.8) */
> +        f.body = Expression(t, x, COMMA);

Cuddle and brace this one too.

+        while (ss[--i].type !== BLOCK) ; // a BLOCK *must* be found.

What happens if a block isn't found during this loop? Will this error out with a meaningful error?

+                        if (t.match(IDENTIFIER))
+                            if (t.token.value !== "each")
+                                throw t.newSyntaxError("Invalid comprehension");
+                            else
+                                n.foreach = true;

Brace the outer if to avoid the dangling else ambiguity.

Other than that, looks fine overall!

Comment 4

[Dimitris Vardoulakis]

Attachment: Patch including Patrick's suggestions

I did all changes Patrick suggested. 

Regarding the "BLOCK *must* be found" comment: 
There is no need to throw an error. The input program can't influence if a BLOCK will be found or not, it's always found because of how narcissus works. If a block isn't found this means a bug in narcissus.

Comment 5

[Dimitris Vardoulakis]

Attachment: fixed a couple errors

Added a comma in jsdefs.

Fixed the definition of Tokenizer which had a syntax error (creating by rebasing from the common-js version of narcissus?)

Parses jetpack w/out errors.

Comment 6

[Patrick Walton (:pcwalton)]

> +        if (n.type === LET_STM) {
> +            return n;
> +        } else if (n.type === LET_EXP) {// exps in stm context are semi nodes

"else" after "return" should be eliminated.

> +        for (i=0; i < n2.length; i++) n.varDecls.push(n2[i]);

I would make this two lines.

Other than that, LGTM. Feel free to push.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> > +        if (n.type === LET_STM) {
> > +            return n;
> > +        } else if (n.type === LET_EXP) {// exps in stm context are semi nodes
> 
> "else" after "return" should be eliminated.

And therefore the braces around the return go too.

> > +        for (i=0; i < n2.length; i++) n.varDecls.push(n2[i]);
> 
> I would make this two lines.

For debuggability. Also, space on both sides of binary operators including =.

Just a drive-by on the comments, I haven't read the patch yet. May if time allows, no worries. Patrick's r+ stands.

/be

Comment 8

[Dimitris Vardoulakis]

Did the changes and committed. Thanks for reviewing Patrick.

Comment 9

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/tracemonkey/rev/80c7df5c6bf0

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/80c7df5c6bf0