Closed Bug 957513 Opened 10 years ago Closed 10 years ago

`DecimalIntegerLiteral` can never be `0` directly followed by `8` or `9`

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox32 --- wontfix
firefox33 + wontfix
firefox34 + wontfix

People

(Reporter: mathias, Assigned: sankha)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature, site-compat, Whiteboard: [js:p2][DocArea=JS])

Attachments

(2 files, 6 obsolete files)

The following JavaScript program should throw an error:

    08

As per the spec[1], `DecimalIntegerLiteral` can never be `0` directly followed by another decimal digit, although Chrome/Opera, PrestOpera, and Firefox do support it.

Note that https://people.mozilla.org/~jorendorff/es6-draft.html#sec-additional-syntax-numeric-literals does not apply here because `8` is not an `OctalDigit`.

[1] https://people.mozilla.org/~jorendorff/es6-draft.html#sec-literals-numeric-literals
Summary: `DecimalIntegerLiteral` can never be `0` directly followed by another decimal digit → `DecimalIntegerLiteral` can never be `0` directly followed by `8` or `9`
Blocks: es6
Attached patch patch v1 (obsolete) — Splinter Review
I just changed the ReportWarning to instead throw a syntax error while parsing. Not sure if this will be enough.
Attachment #8366749 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8366749 [details] [diff] [review]
patch v1

Review of attachment 8366749 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review...

This needs a test, of course, but it should be pretty easy:

    for (var code of ["08", "09", "01238", "01239", "08e+1"]) {
        assertThrowsInstanceOf(() => Function(code), SyntaxError);
        assertThrowsInstanceOf(() => eval(code), SyntaxError);
    }

    var ok = [0.8, 0.08, 0e8];  // these work fine of course

::: js/src/frontend/TokenStream.cpp
@@ +1394,5 @@
>                  // Octal integer literals are not permitted in strict mode code.
>                  if (!reportStrictModeError(JSMSG_DEPRECATED_OCTAL))
>                      goto error;
>  
> +                // Outside strict mode, we do not permit 08 and 09 and throw a

// Even in sloppy mode, 08 or 09 is a syntax error.

@@ +1400,2 @@
>                  if (c >= '8') {
> +                    if (!reportError(JSMSG_BAD_OCTAL, c == '8' ? "08" : "09")) {

Incidentally --- this gives somewhat strange error messages, like:

  js> 012348
  typein:2:0 SyntaxError: 08 is not a legal ECMA-262 octal constant

Could you please improve the error message while we're here?

"numbers starting with 0 followed by a digit are octals and can't contain %s"

and change the string to just be (c == '8' ? "8" : "9") without the 0.
Attachment #8366749 - Flags: feedback?(jwalden+bmo) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
Assignee: nobody → sankha93
Attachment #8366749 - Attachment is obsolete: true
Attachment #8367179 - Flags: review?(jorendorff)
Comment on attachment 8367179 [details] [diff] [review]
patch v2

Review of attachment 8367179 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, Sankha! Good work.

r=me with these nits fixed.

::: js/src/frontend/TokenStream.cpp
@@ +1392,5 @@
>              numStart = userbuf.addressOfNextRawChar() - 1;  // one past the '0'
>              while (JS7_ISDEC(c)) {
>                  // Octal integer literals are not permitted in strict mode code.
>                  if (!reportStrictModeError(JSMSG_DEPRECATED_OCTAL))
>                      goto error;

Actually ... why is this warning inside the loop? Please put it before the loop, while you're here.

Goofy preexisting code. :-P

@@ +1395,5 @@
>                  if (!reportStrictModeError(JSMSG_DEPRECATED_OCTAL))
>                      goto error;
>  
> +                // We do not permit 08 and 09 and throw a syntax error if it is
> +                // used.

Please use the exact wording

// Even in sloppy mode, 08 or 09 is a syntax error.

@@ +1402,2 @@
>                          goto error;
>                      }

Drop the braces here, please.
Attachment #8367179 - Flags: review?(jorendorff) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8367179 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8367486 [details] [diff] [review]
patch v3

Review of attachment 8367486 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, one more thing. After making this change, please mark the new patch "r+" and set "checkin-needed". Thanks!

::: js/src/frontend/TokenStream.cpp
@@ +1402,1 @@
>                          goto error;

This can no longer return true. Please drop the `if` and just say

    reportError(JSMSG_BAD_OCTAL, c == '8' ? "8" : "9");
    goto error;

like the other reportError call sites.
Attachment #8367486 - Flags: review+
Removing the checkin-needed flag now.
Keywords: checkin-needed
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #8367486 - Attachment is obsolete: true
Attachment #8367568 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Looks like this makes some js/tests fail:

    ecma/LexicalConventions/7.7.3-2.js
    js1_5/LexicalConventions/lexical-001.js
Sankha: do you have time to review why the LexicalConventions tests fail with your DecimalIntegerLiteral patch?
Flags: needinfo?(sankha93)
Keywords: feature
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [js:p2]
Yes, I'll do it shortly and attach a new patch.
Flags: needinfo?(sankha93)
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #8367568 - Attachment is obsolete: true
Attachment #8409318 - Flags: review?(jorendorff)
Comment on attachment 8409318 [details] [diff] [review]
patch v5

Review of attachment 8409318 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. A few nits and we can finally land this! Thanks, Sankha!

::: js/src/jit-test/tests/parser/parseOctal.js
@@ +4,5 @@
> +    assertThrowsInstanceOf(() => Function(code), SyntaxError);
> +    assertThrowsInstanceOf(() => eval(code), SyntaxError);
> +}
> +
> +var ok = [0.8, 0.08, 0e8];

Please add 1e08 and 1e+08.

::: js/src/tests/ecma/LexicalConventions/7.7.3-2.js
@@ +25,5 @@
>     Author:             christine@netscape.com
>     Date:               15 june 1998
>  
> +   This behaviour has been changed in ECMAScript 6 spec. A numeric literal starting
> +   with 0 will be considered an octal number and cannot contain 8 or 9.

Please just delete this file instead.

::: js/src/tests/js1_5/LexicalConventions/lexical-001.js
@@ +21,5 @@
>   *digit. We forgive this and assume he intended a decimal. If the
>   *JavaScript "strict" option is set though, we will give a warning.
> + *
> + *This behaviour has been changed in ECMAScript 6 spec. A numeric literal starting
> + *with 0 will be considered an octal number and cannot contain 8 or 9.

Instead of adding lines, just delete lines 12-22.
Attachment #8409318 - Flags: review?(jorendorff)
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #8409318 - Attachment is obsolete: true
Attachment #8412456 - Flags: review?(jorendorff)
Attachment #8412456 - Flags: review?(jorendorff) → review+
OK. I think this should be checked in after the branch for FF31 (Monday) since it's technically a breaking change. So setting ni?me to remind me to come back to it next week.
Flags: needinfo?(jorendorff)
Flags: needinfo?(jorendorff)
Keywords: checkin-needed
Asking review from Victor since this touches a devtools test.
Attachment #8412456 - Attachment is obsolete: true
Attachment #8418028 - Flags: review?(vporof)
Flags: needinfo?(sankha93)
Attachment #8418028 - Flags: review?(vporof) → review+
Keywords: checkin-needed
Is there a recent Try run for the latest patch?
At the moment, my laptop doesn't have internet access. So I'll push it to Try in the evening maybe. Cancelling checkin-needed until then.
Keywords: checkin-needed
Thanks for the Try run. In the future, try to be more aware of which tests you run, though. "All" runs consume a LOT of resources :)
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
https://hg.mozilla.org/mozilla-central/rev/586ec2ee45d7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Flags: needinfo?(jorendorff)
Depends on: 1018053
Blocks: 1022254
No longer blocks: 1022254
Depends on: 1022254
Depends on: 1025107
While this is a perfectly valid error to throw, it apparently breaks banks. (bug 1025107) Throwing a warning and automatically falling back to handling the literal as decimal rather than octal would possibly be preferable (at least for the time being).
I wonder if we should take this out of Aurora again. I'm not necessarily saying that we should give up on, it, but maybe it makes sense to keep it in Nightly for now?
Flags: needinfo?(jorendorff)
I asked Jason the same question in bug 1025107 comment 2. I suggested we should hold the JS error in Nightly and Aurora, but log a console warning in Beta and Release for a couple releases.
Sigh. Let's back it out. Till, can you take?
Flags: needinfo?(till)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout on mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/301e62e44f18

(Re-closing as the patch is still landed on mozilla-central. The issue will make itself known if it's enough of a problem that we have to back it out completely, so we don't need to track it with an open bug.)
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: needinfo?(till)
Flags: needinfo?(jorendorff)
Resolution: --- → FIXED
Till backed this patch out of Aurora 32, but let's track this for 33. If we can't get evangelism bugs like bug 1025107 or bug 1022254 fixed soon, we may need to back this out of 33, too.
hi,

So where are we on this for 33 ?
(In reply to Anshu Prateek from comment #33)
> So where are we on this for 33 ?

We haven't received any additional feedback about sites being broken, if that's what you mean. We might back it out on Aurora again after the next uplift, but no decision on that has been made and no new information is available.
Tracking for now based on Comment 31
Whiteboard: [js:p2] → [js:p2][DocArea=JS]
Please back this out of current Aurora 33 at minimum. There's report of this breaking another major bank website in bug 1025107 comment 16. Honestly, I think attempting to properly enforce this is a lost cause and it should be backed out completely. :(
Flags: needinfo?(till)
jorendorff: can you please back out this bug (or find someone to) from Aurora 33 and Nightly 34? Till is on PTO until August 13 and this bug is breaking more bank websites.
Flags: needinfo?(till) → needinfo?(jorendorff)
this looks like it breaks sugarcrm as well
I'm doing this today.
I said that and then there was some sort of infrastructure disaster, but I'll try again tonight.
Flags: needinfo?(jorendorff)
Approval Request Comment
[Feature/regressing bug #]: This patch backs out an earlier patch in this bug. The earlier patch is a serious problem, we need to back it out.
[User impact if declined]: Some banking web sites are broken!
[Describe test coverage new/current, TBPL]: This is just a backout. All pre-existing tests are restored. Also it landed on mozilla-central without incident.
[Risks and why]: No risk I can think of.
[String/UUID change made/needed]: None.
Attachment #8470905 - Flags: approval-mozilla-aurora?
"wontfix" here means we have backed out the patch and don't plan to land it again.

Fx33 is still "affected", meaning it contains the bad patch.
Attachment #8470905 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f170b174bc25

Reopening the bug for now, not sure if it should be WONTFIX or not.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla32 → ---
till's backout of June 16 is already in Aurora, nothing more is needed here.

-> WONTFIX because we cannot make this change to DecimalIntegerLiteral stick. TC39 should give up and specify it. I've raised it. It might happen.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → WONTFIX
(In reply to Florian Scholz [:fscholz] (elchi3) from comment #46)
> I added a small note here
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Lexical_grammar#Numeric_literals and referred to this bug for more details.

Correct me if I'm wrong, but something like 0444 will still be parsed as octal. That note isn't sufficient as-is then, as the decimal example 0123456789 will parse as decimal, yet 01234567 will parse as octal, different from 1234567. Both the decimal and octal sections need to note this behavior, as it's really confusing to learn (and should've never been in the spec in the first place). :/
Flags: needinfo?(fscholz)
Thanks, indeed a bit confusing :/ I updated the sections. Any review or further clarification on that wiki page is welcome.
Flags: needinfo?(fscholz)
(In reply to Florian Scholz [:fscholz] (elchi3) from comment #48)
> Thanks, indeed a bit confusing :/ I updated the sections. Any review or
> further clarification on that wiki page is welcome.

There are some other issues here and in another page that need addressing. I've emailed you with specifics.
You need to log in before you can comment on or make changes to this bug.