The default bug view has changed. See this FAQ.

A statement can follow an expclo without an intervening semicolon

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: jorendorff)

Tracking

({testcase, verified1.9.1})

Trunk
x86
Mac OS X
testcase, verified1.9.1
Points:
---
Bug Flags:
wanted1.9.1 ?
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

10 years ago
Blocks: 380236
(Assignee)

Comment 1

8 years ago
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.
Assignee: general → jorendorff
Status: NEW → ASSIGNED
Attachment #362116 - Flags: review?(igor)

Comment 2

8 years ago
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.
Attachment #362116 - Flags: review?(igor)
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

8 years ago
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

Comment 6

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
Created attachment 362205 [details] [diff] [review]
v2

I deleted the superfluous ON_CURRENT_LINE check. js/tests still pass.
Attachment #362116 - Attachment is obsolete: true
Attachment #362205 - Flags: review?(igor)
(Assignee)

Comment 9

8 years ago
Created attachment 362206 [details] [diff] [review]
v3

Sorry, I forgot to clean up a minor style issue.
Attachment #362205 - Attachment is obsolete: true
Attachment #362206 - Flags: review?(igor)
Attachment #362205 - Flags: review?(igor)

Comment 10

8 years ago
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
Attachment #362206 - Flags: review?(igor) → review+
(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
Flags: wanted1.9.1?
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/tracemonkey/rev/e04b3e116486
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/e04b3e116486

Comment 16

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/08894b2b76cb
Keywords: fixed1.9.1

Comment 17

8 years ago
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
Flags: in-testsuite+

Comment 18

8 years ago
verified 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.