384758 - A statement can follow an expclo without an intervening semicolon

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

A statement can follow an expclo without an intervening semicolon.  I'm not sure if this is a bug, but it's weird to see two statements next to each other without a semicolon between them.

js> (function() { if(t) function x() foo() bar(); })
function () {
    if (t) {

        function x() foo();

    }
    bar();
}

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Release-noting this long-known bug for SM1.8 was embarrassing enough that I decided to look into it, and sure enough, it's pretty easy to fix.

I have not been in the parser much before.  This passes js/tests though.

Comment 2

[Igor Bukanov]

Comment on attachment 362116 [details] [diff] [review]
v1

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>+static JSBool
>+MatchSemiOrEndOfLine(JSContext *cx, JSTokenStream *ts, JSParseNode *pn)

Nit: a better name would be MatchOrInsertSemicolon as the code allows to skip the semicolon before the right curl.

>+{
>+    JSTokenType tt;
>+
>+    if (ON_CURRENT_LINE(ts, pn->pn_pos)) {

Nit: I do not know why currently Statement() uses this check. It seems its purpose is some subtle parameter passing to cover some corner cases. Ideally we should recover this knowledge ;), but at least not to spread this unknown, I suggest to remove the check here and write in Statement

if (!ON_CURRENT_LINE(ts, pn->pn_pos)))
     js_MatchToken(cx, ts, TOK_SEMI)
else if (!MatchOrInsertSemicolon) ...


>@@ -1313,20 +1335,23 @@ FunctionDef(JSContext *cx, JSTokenStream
>+    } else if (lambda == 0) {
>+        pn->pn_pos.end = CURRENT_TOKEN(ts).pos.end;

As a consequence of the above nit this line would no longer be necessary.

r+ with this fixed.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 362116 [details] [diff] [review]
v1

>@@ -1313,20 +1335,23 @@ FunctionDef(JSContext *cx, JSTokenStream
> #endif
>     pn->pn_pos.begin = CURRENT_TOKEN(ts).pos.begin;
> 
>     body = FunctionBody(cx, ts, &funtc);
>     if (!body)
>         return NULL;
> 
> #if JS_HAS_EXPR_CLOSURES
>-    if (tt == TOK_LC)
>+    if (tt == TOK_LC) {
>         MUST_MATCH_TOKEN(TOK_RC, JSMSG_CURLY_AFTER_BODY);
>-    else if (lambda == 0)
>-        js_MatchToken(cx, ts, TOK_SEMI);
>+    } else if (lambda == 0) {
>+        pn->pn_pos.end = CURRENT_TOKEN(ts).pos.end;
>+        if (!MatchSemiOrEndOfLine(cx, ts, pn))
>+            return NULL;
>+    }
> #else
>     MUST_MATCH_TOKEN(TOK_RC, JSMSG_CURLY_AFTER_BODY);
> #endif
>     pn->pn_pos.end = CURRENT_TOKEN(ts).pos.end;

pn->pn_pos.end is going to be set again here in the expression closure case. Move this up into the tt == TOK_LC case after the MUST_MATCH_TOKEN?

Igor's suggestions seem good too, just driving by. A bigger issue with all this is the likely requirement for parentheses around the expression closure body in a future ES spec. That will avoid this ASI issue and other grammatical problems.

/be

Comment 4

[Igor Bukanov]

To Brendan:

Do you know the need for that check in Statement:

if (ON_CURRENT_LINE(ts, pn->pn_pos))

at http://mxr.mozilla.org/mozilla-central/source/js/src/jsparse.cpp#3543 ?

Comment 5

[Brendan Eich [:brendan]]

Sure, I dimly recall... ;-) That's part of automatic semicolon insertion. Every case in the Statement switch that does not end in a right curly brace breaks to that if-then. See http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsparse.c&mark=3493-3505 and click the blame link.

/be

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Every
> case in the Statement switch that does not end in a right curly brace breaks to
> that if-then.

Yes, but why the automatic semicol insertion code does the check for ON_CURRENT_LINE(ts, pn->pn_pos)? For the cases where Statement does the break it seems the condition is always true since the code updates pn position before breaking. 

> See
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsparse.c&mark=3493-3505
> and click the blame link.

That lead to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsparse.c&rev=3.94 . But that just consolidated the checks for ON_CURRENT_LINE(ts, pn->pn_pos) in one place without making things clear.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> For the cases where Statement does the break
> it seems the condition is always true since the code updates pn position before
> breaking. 

It is interesting that the bug 469940 happened precisely due to this check. I.e. due to a missing update of pn->pn_pos.end that condition became false and code would insert the semicolon no matter what.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v2

I deleted the superfluous ON_CURRENT_LINE check. js/tests still pass.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v3

Sorry, I forgot to clean up a minor style issue.

Comment 10

[Igor Bukanov]

Comment on attachment 362206 [details] [diff] [review]
v3

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>-    (void) js_MatchToken(cx, ts, TOK_SEMI);
>+    if (!MatchOrInsertSemicolon(cx, ts))
>+        return NULL;
>     return pn;

Micro-nit: for dense code use return Match ? pn : NULL

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (In reply to comment #5)
> > Every
> > case in the Statement switch that does not end in a right curly brace breaks to
> > that if-then.
> 
> Yes, but why the automatic semicol insertion code does the check for
> ON_CURRENT_LINE(ts, pn->pn_pos)? For the cases where Statement does the break
> it seems the condition is always true since the code updates pn position before
> breaking. 

It looks like a bogus optimization from rev 3.2 (which is when fur landed a Netscape-private branch, but the code probably goes back to 1996 and late nights hacking), trying to avoid calling WellTerminated.

Good to get rid of this condition. Only thing it helped (indirectly, poorly) was to find failures to update pn->pn_pos.end.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 362206 [details] [diff] [review]
v3

WellTerminated makes a come-back :-P. New name is more descriptive of what it does. Thanks for fixing this.

Thinking about it more, I recall that ts->lineno could get ahead of pn_pos.end.lineno after some constructs, once upon a time, and that seemed like a good test to avoid calling WellTerminated. Or perhaps there were only bugs failing to update pn_pos.end.

Premature optimization is the root of all evil!

/be

Comment 13

[Brendan Eich [:brendan]]

We should take this, to avoid an ugly de-facto standard for wrongful ASI.

/be

Comment 14

[Jason Orendorff [:jorendorff]]

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

Comment 15

[Robert Sayre]

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

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/08894b2b76cb

Comment 17

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/080c323b3a39
/cvsroot/mozilla/js/tests/js1_8/regress/regress-384758.js,v  <--  regress-384758.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

verified 1.9.1, 1.9.2