Last Comment Bug 384758 - A statement can follow an expclo without an intervening semicolon
: A statement can follow an expclo without an intervening semicolon
Status: VERIFIED FIXED
: testcase, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
Depends on:
Blocks: js1.8
  Show dependency treegraph
 
Reported: 2007-06-16 23:13 PDT by Jesse Ruderman
Modified: 2009-03-12 00:39 PDT (History)
9 users (show)
brendan: wanted1.9.1?
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.02 KB, patch)
2009-02-12 14:49 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (2.86 KB, patch)
2009-02-13 05:00 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v3 (2.75 KB, patch)
2009-02-13 05:03 PST, Jason Orendorff [:jorendorff]
igor: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-06-16 23:13:34 PDT
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] 2009-02-12 14:49:51 PST
Created attachment 362116 [details] [diff] [review]
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 2009-02-13 01:34:16 PST
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] 2009-02-13 01:56:35 PST
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 2009-02-13 02:06:40 PST
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] 2009-02-13 02:30:23 PST
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 2009-02-13 02:53:03 PST
(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 2009-02-13 03:10:53 PST
(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] 2009-02-13 05:00:03 PST
Created attachment 362205 [details] [diff] [review]
v2

I deleted the superfluous ON_CURRENT_LINE check. js/tests still pass.
Comment 9 Jason Orendorff [:jorendorff] 2009-02-13 05:03:49 PST
Created attachment 362206 [details] [diff] [review]
v3

Sorry, I forgot to clean up a minor style issue.
Comment 10 Igor Bukanov 2009-02-13 05:15:31 PST
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] 2009-02-13 09:44:30 PST
(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] 2009-02-13 09:50:09 PST
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] 2009-02-13 09:50:57 PST
We should take this, to avoid an ugly de-facto standard for wrongful ASI.

/be
Comment 14 Jason Orendorff [:jorendorff] 2009-02-16 14:51:38 PST
http://hg.mozilla.org/tracemonkey/rev/e04b3e116486
Comment 17 Bob Clary [:bc:] 2009-03-05 18:00:45 PST
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:] 2009-03-12 00:39:56 PDT
verified 1.9.1, 1.9.2

Note You need to log in before you can comment on or make changes to this bug.