Closed Bug 401466 Opened 12 years ago Closed 12 years ago

narcissus - fail to insert ; in expr --expr

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bc, Assigned: brendan)

Details

Attachments

(1 file, 1 obsolete file)

js> b=1
1
js> s = 'a = 4\n --b \n'
a = 4
 --b 

js> parse(s)
:2: Missing ; before statement
js> eval(s)
0
Flags: in-testsuite-
Flags: in-litmus-
Attached patch Fix (obsolete) — Splinter Review
Compare with http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsparse.c&rev=3.303&root=/cvsroot&mark=4331-4346#4331 .
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #286499 - Flags: review?(brendan)
I thought about that, but try s = "a = (3\n) --b\n" in both SpiderMonkey and Narcissus. More thought needed.

/be
Attached patch fixSplinter Review
Actually, SpiderMonkey has a bug here!

js> x=1, y=2, eval("(x\n)-- y")                                       
1

This is incorrect per ECMA-262 Edition 3 7.9.1:

* When, as the program is parsed from left to right, a token (called the offending token) is encountered that is not allowed by any production of the grammar, then a semicolon is automatically inserted before the offending token if one or more of the following conditions is true:

  1. The offending token is separated from the previous token by at least one LineTerminator.
  2. The offending token is }.

Witness without eval or unnecessary newline (which does not bring into play the restricted production language):

js> (x)-- y
typein:2: SyntaxError: missing ; before statement:
typein:2: (x)-- y
typein:2: ......^

I'll file another bug and patch SpiderMonkey in a minute.

/be
Assignee: mrbkap → brendan
Attachment #286499 - Attachment is obsolete: true
Attachment #287266 - Flags: review?(mrbkap)
Attachment #286499 - Flags: review?(brendan)
Priority: -- → P2
Hardware: PC → All
Comment on attachment 287266 [details] [diff] [review]
fix

I think it might make things nicer to add a currentToken to Tokenizer.
Attachment #287266 - Flags: review?(mrbkap) → review+
(In reply to comment #4)
> (From update of attachment 287266 [details] [diff] [review])
> I think it might make things nicer to add a currentToken to Tokenizer.

There is already a token getter for the current token, but this patch uses the previous token. Could add t.prevToken, but if this is the only use-case I would rather not.

Fixed on trunk:

js/narcissus/jsparse.js 1.26

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.