Unexpected lexing: "2for" is accepted in array comprehension

RESOLVED FIXED in mozilla1.9.2

Status

()

--
minor
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jruderman, Assigned: brendan)

Tracking

({testcase})

Trunk
mozilla1.9.2
testcase
Points:
---
Bug Flags:
wanted1.9.2 ?
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
js> [2for (x in [3])] 
[2]

I did not expect this to be accepted.

Comment 1

9 years ago
Seems more like a parse than a lex error, e.g. this yields 7:

[7  /* comment */ for (x in [3])]
(Assignee)

Comment 2

9 years ago
(In reply to comment #1)
> Seems more like a parse than a lex error, e.g. this yields 7:
> 
> [7  /* comment */ for (x in [3])]

No bug there -- comprehension expression is a constant.

Good point though, this is a parser bug.

/be
Summary: Unexpected lexing: "2for" is accepted in array comprehension → Unexpected parsing: "2for" is accepted in array comprehension
(Assignee)

Comment 3

9 years ago
Created attachment 407705 [details] [diff] [review]
fix
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #407705 - Flags: review?(jorendorff)
(Assignee)

Comment 4

9 years ago
Safe fix, want to get past it.

/be
Flags: wanted1.9.2?
(Reporter)

Comment 5

9 years ago
Why is this the parser's responsibility rather than the lexer's?  What badness would result from fixing this in the lexer?

FWIW, similar things happen with other keywords:

js> (0in [7])  
true

js> (0instanceof Object) 
false
(Assignee)

Comment 6

9 years ago
The lexer could treat any non-punctuator, non-whitespace char abutting a number literal as an error, but only the parser can judge context:

js> 1for (i in this);
typein:1: SyntaxError: missing ; before statement:
typein:1: 1for (i in this);
typein:1: ^

Would it be better to have the lexer always say "syntax error" instead?

What do other browsers do?

/be
(Assignee)

Comment 7

9 years ago
Jesse's summary was correct. We're in violation of ECMA-262 7.8.3, specifically

The source character immediately following a NumericLiteral must not be an IdentifierStart or DecimalDigit.

NOTE For example:

3in

is an error and not the two input elements 3 and in.

/be
(Assignee)

Updated

9 years ago
Blocks: 445494
OS: Mac OS X → All
Hardware: x86 → All
Summary: Unexpected parsing: "2for" is accepted in array comprehension → Unexpected lexing: "2for" is accepted in array comprehension
Target Milestone: --- → mozilla1.9.2
(Reporter)

Comment 8

9 years ago
Opera has the same bug as Firefox.  Safari correctly says "Parse error".
(Reporter)

Comment 9

9 years ago
Filed a bug against Opera: DSK-268647.
(Assignee)

Comment 10

9 years ago
Created attachment 407717 [details] [diff] [review]
fix, plus the ==/!= operators for JSToken{Pos,Ptr}

>14-year-old bug. Jesse++.

/be
Attachment #407705 - Attachment is obsolete: true
Attachment #407717 - Flags: review?(jorendorff)
Attachment #407705 - Flags: review?(jorendorff)
(Assignee)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Comment 11

9 years ago
(In reply to comment #7)
> Jesse's summary was correct. We're in violation of ECMA-262 7.8.3, specifically
> 
> The source character immediately following a NumericLiteral must not be an
> IdentifierStart or DecimalDigit.

I don't see how the grammar allows DecimalDigit to immediately follow a NumericLiteral. Anyone? Seems like a micro-bug in ECMA-262.

/be
(Reporter)

Comment 12

9 years ago
Internet Explorer 8 has the same bug.
(In reply to comment #11)
> I don't see how the grammar allows DecimalDigit to immediately follow a
> NumericLiteral. Anyone? Seems like a micro-bug in ECMA-262.

0_ in strict mode (or in engines without octal literal support) where _ is DecimalDigit, at the level of lexing.  Past that, I also see nothing in grammar productions for the possibility.
Comment on attachment 407717 [details] [diff] [review]
fix, plus the ==/!= operators for JSToken{Pos,Ptr}

This is missing a test. r=me with that.
Attachment #407717 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 15

9 years ago
Created attachment 407760 [details] [diff] [review]
with test (in js/src/tests/ecma_3/LexicalConventions/7.8.3-01.js)

Bob, could you bless the test? It stringifies either the buggy true (boolean) result of eval("0in [1]") or the correct SyntaxError exception object and compares to a reference stringified exception. Is there a better way? Thanks,

/be
Attachment #407717 - Attachment is obsolete: true
Attachment #407760 - Flags: review?(bclary)

Comment 16

9 years ago
Comment on attachment 407760 [details] [diff] [review]
with test (in js/src/tests/ecma_3/LexicalConventions/7.8.3-01.js)


>+++ b/js/src/tests/ecma_3/LexicalConventions/7.8.3-01.js
>@@ -0,0 +1,23 @@
>+/*
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/licenses/publicdomain/
>+ */
>+

Great. I need to update the templates.

>+var BUGNUMBER = 523401;
>+var summary = 'identifier starts immediately after numeric literal';
>+var expect = "SyntaxError: identifier starts immediately after numeric literal";
>+var actual;
>+
>+printBugNumber(BUGNUMBER);
>+printStatus(summary);
>+
>+try {
>+    eval("actual = 0in [1]");
>+} catch (e) {
>+    actual = e;
>+}
>+actual = '' + actual;
>+
>+reportCompare(actual, expect, summary);

should be reportCompare(expect, actual, summary);

>+
>+printStatus("All tests passed!");

Not necessary but fine with me.

r+ with the reportCompare fix. Thanks!
Attachment #407760 - Flags: review?(bclary) → review+
(Assignee)

Comment 17

9 years ago
I found a few more reportCompare(actual, expect, ...) calls -- will fix.

This bug is an ES3 conformance issue. ES1 didn't have 'in' or 'instanceof' -- so 10 years old, not 14. Jesse++ still ;-).

/be
(Assignee)

Comment 18

9 years ago
http://hg.mozilla.org/tracemonkey/rev/9682779cae73

/be
Whiteboard: fixed-in-tracemonkey

Comment 19

9 years ago
don't forget to set in-testsuite+ when you check in a test. Muchos Gracias.
Flags: in-testsuite? → in-testsuite+

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9682779cae73
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.