569464 - parsing LET expressions with COMMAs in the body

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Dimitris Vardoulakis]

The spidermonkey repl gives the error

SyntaxError: missing ; before statement:

for the expression

let (x=2, y=3) x=3, y=13

which should parse correctly, with "x=3, y=13" as the body of the let.

Comment 1

[Dave Herman [:dherman]]

> let (x=2, y=3) x=3, y=13
> 
> which should parse correctly

This is accurate...

> with "x=3, y=13" as the body of the let.

...but this is not. IIRC, the body of a let expression is an AssignmentExpression (a sub-grammar of Expression), so the body of the let should be |x=3| and the entire expression should (I believe) parse as a two-element list expression with the elements:

    0: let(x=2, y=3) x=3
    1: y=13

See also: http://wiki.ecmascript.org/doku.php?id=proposals:block_expressions#let_expression

Dave

Comment 2

[Brendan Eich [:brendan]]

What's more, Waldemar pointed out a reduce-reduce conflict in let expressions as implemented, which arises because we allow a let expression to be the entire expression in an expression statement:

js> function f(x) x
js> let (a=1) a ? f : x++(42);
typein:2: SyntaxError: missing ; before statement:
typein:2: let (a=1) a ? f : x++(42);
typein:2: ...................^

Per the grammar, we should parse the ?: expression as an assignment expression that is the let expression's body, then evaluate the let expression to f in this case, then call f on 42 returning 42.

This is a dumb bug, fixable by doing the obvious thing we did for function expressions: forbid the expression form at front of a statement.

Anyway, this bug is just plain invalid. We used AssignmentExpression for let expression because you want to write let expressions as actual (non-terminal) arguments in a call expression.

/be

Comment 3

[Dave Herman [:dherman]]

> Anyway, this bug is just plain invalid. We used AssignmentExpression for let
> expression because you want to write let expressions as actual (non-terminal)
> arguments in a call expression.

That doesn't make this bug invalid. It's an AssignmentExpression, but that just means it parses the part outside the comma as part of a ListExpression containing a LetExpression. The shell does allow ListExpressions, so the example should still parse, just not as Dimitris predicted.

For example, it parses just fine as a nested expression:

js> (let (x=2, y=3) x=3, y=13)
13
js> y
13

And yet:

js> let (x=2, y=3) x=3, y=13
typein:7: SyntaxError: missing ; before statement:
typein:7: let (x=2, y=3) x=3, y=13
typein:7: .................^

AFAICT, this still looks like a bug, at least in the shell top-level, if not inside the parser itself.

Dave

Comment 4

[Brendan Eich [:brendan]]

Oops, this is the very same bug Waldemar reported. What's wrong with me? Thanks, Dave. Anyway, there's a soundness problem with our grammar.

I do not think the fix is to parse a let expression at the start of a statement, however. So be prepared for all statements beginning with 'let' followed by '(' to require a braced body (let block, not let expression).

/be

Comment 5

[Brendan Eich [:brendan]]

Taking to make reparations (if breaking the reporter's wanted use-case can be called a reparation!).

/be

Comment 6

[Brendan Eich [:brendan]]

Cc'ing Blake, my co-conspirator in implementing ES4's let forms for JS1.7.

/be

Comment 7

[Brendan Eich [:brendan]]

dherman, cdleary: this bug is free for stealing. Any interest (or: help)?

/be

Comment 8

[Dave Herman [:dherman]]

Check my understanding: your proposed fix is that an ExpressionStatement has the restriction:

    lookahead ∉ { "{", "function", "let" }

Is that right?

Dave

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Check my understanding: your proposed fix is that an ExpressionStatement has
> the restriction:
> 
>     lookahead ∉ { "{", "function", "let" }
> 
> Is that right?

Right -- doubtful anything in Firefox or other XUL apps/add-ons uses a let expression (at all, let alone as the whole of an expression statement) but best to tryserver or build your own with the patch that does this, and test.

/be

Comment 10

[Dave Herman [:dherman]]

I got this-- it was easy. I'll post the patch tomorrow.

Dave

Comment 11

[Dave Herman [:dherman]]

Ah, I'll also have to fix the decompiler to parenthesize let-expressions in statement position.

Dave

Comment 12

[Dave Herman [:dherman]]

Suggestion from Jesse on #jsapi just now:

Jesse: dherman: it might also be good to add a test to ensure that
Jesse: for each (e in (let(x) x*x for each (x in [1, 2])))  print(e)
Jesse: gives NaN,NaN
Jesse: in other words, it's parsed as
Jesse: for each (e in ((let(x) x*x) for each (x in [1, 2])))  print(e)
Jesse: rather than
Jesse: for each (e in (let(x) (x*x for each (x in [1, 2]))))  print(e)

I'll include this in the patch.

Dave

Comment 13

[Dave Herman [:dherman]]

Attachment: modifies parser/decompiler to disallow let-expression-statements

Passes all SpiderMonkey tests, but I'm currently pushing to tryserver to make sure this doesn't cause any problems with any browser JS.

Dave

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 473920 [details] [diff] [review]
modifies parser/decompiler to disallow let-expression-statements

>                     if (*rval != '\0' && (rval[0] != '/' || rval[1] != '*')) {
>                         js_printf(jp,
>                                   (*rval == '{' ||
>                                    (strncmp(rval, js_function_str, 8) == 0 &&
>-                                    rval[8] == ' '))
>+                                    rval[8] == ' ') ||
>+                                   (strncmp(rval, "let (", 5) == 0))

Don't overparenthesize == against || (the new strncmp call).

Do overparenthesize && against || or certain GCC vintages will warn.

You can pull the right-hand side of && up to be on the same line, which helps readability.

r=me with these fixed.

/be

Comment 15

[Dave Herman [:dherman]]

So far TryServer has found a few places where |let| expression statements are used, all of them in tests. I missed a few trace-tests, which were easy to fix. There's also

    /content/xbl/test/test_bug389322.xhtml

which is also easy to fix, but outside the SpiderMonkey jurisdiction. Should I go ahead and fix it in this patch?

(TryServer's still running, so if it comes up with more failures I'll report back.)

Dave

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> There's also
> 
>     /content/xbl/test/test_bug389322.xhtml
> 
> which is also easy to fix, but outside the SpiderMonkey jurisdiction. Should I
> go ahead and fix it in this patch?

Yes.

/be

Comment 17

[Dave Herman [:dherman]]

Attachment: updates 4 trace-tests and an xbl test

Comment 18

[Dave Herman [:dherman]]

Attachment: fixed another test case with a let-expression-statement

Found one more browser test library that was using a let-expression-statement and fixed it. (It was hard to find because for some reason, instead of causing tests to fail fast with an error, they were just becoming unresponsive.)

At this point, the only oranges I'm seeing seem to be random and known, so I think it's probably ready to push to tracemonkey.

Dave

Comment 19

[Brendan Eich [:brendan]]

Dave raised the idea on IRC of relaxing our let statement (let block) syntax to make the braces optional, while still forbidding an expression statement from starting with 'let'. This would be grammatically sound and backward compatible.

As discretion is the better part of valor, and after Dave's painful try-al by orange fire (sorry), I'm in favor.

/be

Comment 20

[Dave Herman [:dherman]]

> Dave raised the idea on IRC of relaxing our let statement (let block) syntax to
> make the braces optional, while still forbidding an expression statement from
> starting with 'let'. This would be grammatically sound and backward compatible.

I originally thought what you meant by this was:

    LetStatement ::= "let" LetHead Statement

but from our IRC conversation this afternoon that sounds like it's not what you meant. Can you clarify? Did you mean:

    LetStatement ::= ...
                  |  "let" LetHead BlockStatement
                  |  "let" LetHead AssignmentExpression ";"

I'm not sure this would actually change the behavior of the parser.

Something else, then?

Dave

Comment 21

[Dave Herman [:dherman]]

>     LetStatement ::= ...
>                   |  "let" LetHead BlockStatement
>                   |  "let" LetHead AssignmentExpression ";"

Edit: scratch the ellipsis there; I meant:

    LetStatement ::= "let" LetHead BlockStatement
                  |  "let" LetHead AssignmentExpresson ";"

Dave

Comment 22

[Brendan Eich [:brendan]]

It certainly would change the parser and grammar to have 'let' illegal at start of expression statements, but (for backward compatibility, including syntax error compatibility) also support

Statement ::= ... | "let" LetHead AssignmentExpression ";"

The key fix is to prevent PrimaryExpression from producing "let" LetHead AssignmentExpression. The backward compatibility afforded by the other "let" statement is that it looks like an expression statement.

But you can't try to use the left prefix that looks like a let expression as the callee in a CallExpression (see comment 2).

/be

Comment 23

[Dave Herman [:dherman]]

I have just about convinced myself that this is a wild goose chase.

The ambiguity (or at least, an ambiguity) in the original grammar is for input of the form:

    let <LetHead> <MemberExpr> <Arguments> ;

in a statement context. This can be parsed either as:

(a)             ExprStmt
                 /     \
           LetExpr      ";"
         /    |    \
    "let" <LetHead> CallExpr
                     /      \
          <MemberExpr>      <Arguments>

or

(b)                 ExprStmt
                     /    \
               CallExpr    ";"
                /    \
           LetExpr   <Arguments>
         /    |    \
    "let" <LetHead> <MemberExpr>

Waldemar's example demonstrates the ambiguity because the sentence

    a ? f : x++(42)

can't be parsed as an AssignmentExpression; it gets stuck at the "(" token:

              AssignExpr
               CondExpr
             /    |     \
    LogORExpr AssignExpr AssignExpr
        .         .       CondExpr
        .         .          .
       "a"       "f"         .
                          PostfixExpr
                             /    \
                         LHSExpr  "++"
                            .
                            .
                        Identifier
                           "x"

So to parse the sentence

    let (a=1) a ? f : x++(42);

as a Statement in the ES4 or JS 1.7 grammar, we would have to resolve in favor of (b) and treat |a ? f : x++| as the body of the LetExpression. If we try to use (a), we treat |a ? f : x++(42)| as the body of the let and get stuck at the "(" token.

Currently, our code resolves the ambiguity in favor of (a), entering a LetExpression production when it sees a "let" token in statement position. (For some reason it points to the "++" token in the error message, but stepping through the code shows that it really is at the "(" token.)

Brendan: you suggested changing the grammar to resolve the ambiguity by disallowing "let" to begin an ExpressionStatement, and adding a production

    LetStatement ::= ... | "let" LetHead AssignmentExpression ";"

This resolves the ambiguity the same way we currently do, i.e., in favor of (a). In particular, Waldemar's example would continue to fail to parse at the same point. IOW, I still don't see any way that your suggestion changes the behavior of SpiderMonkey. Resolving the ambiguity in the grammar is fine, but if I'm not mistaken, it doesn't change the implementation.

Dave

Comment 24

[Brendan Eich [:brendan]]

We talked about this yesterday and I suggested banning 'let' from the start of an expression statement only under "use strict", since the issue is future-proofing, not trying to fix the low-priority grammar bug.

/be

Comment 25

[Dave Herman [:dherman]]

Yeah, I'm on board for that. Will submit a patch ASAP.

Dave

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #23)
> For some reason it points to the "++" token in the error message, but stepping
> through the code shows that it really is at the "(" token.

The bug is in MatchOrInsertSemicolon -- it reports an error based on a peeked-at token, so the error reporting code blames the current token, the ++ in this case. MatchOrInsertSemicolon should getToken. How about a ride-along fix for this here?

/be

Comment 27

[Dave Herman [:dherman]]

> The bug is in MatchOrInsertSemicolon -- it reports an error based on a
> peeked-at token, so the error reporting code blames the current token, the ++
> in this case. MatchOrInsertSemicolon should getToken. How about a ride-along
> fix for this here?

Sure, easy enough. No test case, though, because the SyntaxError doesn't include column numbers.

Dave

Comment 28

[Dave Herman [:dherman]]

Attachment: bans let-expression-statements in strict mode

Implements the strict-mode restriction. Also fixes the minor MatchOrInsertSemicolon error-reporting bug.

Dave

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 475253 [details] [diff] [review]
bans let-expression-statements in strict mode

>@@ -332,8 +332,9 @@ MSG_DEF(JSMSG_BAD_PROXY_FIX,          24
> MSG_DEF(JSMSG_INVALID_EVAL_SCOPE_ARG, 250, 0, JSEXN_EVALERR, "invalid eval scope argument")
> MSG_DEF(JSMSG_ACCESSOR_WRONG_ARGS,    251, 3, JSEXN_SYNTAXERR, "{0} functions must have {1} argument{2}")
> MSG_DEF(JSMSG_THROW_TYPE_ERROR,       252, 0, JSEXN_TYPEERR, "'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them")
> MSG_DEF(JSMSG_BAD_TOISOSTRING_PROP,   253, 0, JSEXN_TYPEERR, "toISOString property is not callable")
> MSG_DEF(JSMSG_BAD_PARSE_NODE,         254, 0, JSEXN_INTERNALERR, "bad parse node")
> MSG_DEF(JSMSG_NOT_EXPECTED_TYPE,      255, 3, JSEXN_TYPEERR, "{0}: expected {1}, got {2}")
> MSG_DEF(JSMSG_CALLER_IS_STRICT,       256, 0, JSEXN_TYPEERR, "access to strict mode caller function is censored")
> MSG_DEF(JSMSG_NEED_DEBUG_MODE,        257, 0, JSEXN_ERR, "function can be called only in debug mode")
>+MSG_DEF(JSMSG_STRICT_CODE_LET_EXPR_STMT, 258, 0, JSEXN_ERR, "strict mode code may not contain unparenthesized let expression statements")

Lose _CODE to make it fit better? I see one STRICT_CODE_ precedent, hmph. Your call.

/be

Comment 30

[Dave Herman [:dherman]]

http://hg.mozilla.org/tracemonkey/rev/b77f5038c42f

Comment 31

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b77f5038c42f