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(); }

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 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 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

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 ?

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

(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.

(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.

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

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

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

(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 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

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

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

verified 1.9.1, 1.9.2