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

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Mathias Bynens]

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

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

I just changed the ReportWarning to instead throw a syntax error while parsing. Not sure if this will be enough.

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 4

[Jason Orendorff [: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.

Comment 5

[Sankha Narayan Guria [:sankha]]

Attachment: patch v3

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Sankha Narayan Guria [:sankha]]

Removing the checkin-needed flag now.

Comment 8

[Sankha Narayan Guria [:sankha]]

Attachment: patch v4

Comment 9

[Jason Orendorff [:jorendorff]]

Looks like this makes some js/tests fail:

    ecma/LexicalConventions/7.7.3-2.js
    js1_5/LexicalConventions/lexical-001.js

Comment 10

[Chris Peterson [:cpeterson]]

Sankha: do you have time to review why the LexicalConventions tests fail with your DecimalIntegerLiteral patch?

Comment 11

[Sankha Narayan Guria [:sankha]]

Yes, I'll do it shortly and attach a new patch.

Comment 12

[Sankha Narayan Guria [:sankha]]

Attachment: patch v5

Comment 13

[Jason Orendorff [: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:             [e-mail]
>     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.

Comment 14

[Sankha Narayan Guria [:sankha]]

Attachment: patch v6

Comment 15

[Jason Orendorff [:jorendorff]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7217cdee50f

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/705313e71606 for mochitest-dt bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=38895022&tree=Mozilla-Inbound

Comment 18

[Sankha Narayan Guria [:sankha]]

Attachment: fixed devtools testcase

Asking review from Victor since this touches a devtools test.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is there a recent Try run for the latest patch?

Comment 20

[Sankha Narayan Guria [:sankha]]

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.

Comment 21

[Sankha Narayan Guria [:sankha]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=5e0a618df342

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/586ec2ee45d7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/586ec2ee45d7

Comment 25

[Dave Garrett]

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

Comment 26

[Mike Taylor [:miketaylr]]

Another broken (NSFW) site: https://github.com/webcompat/web-bugs/issues/76

Comment 27

[Till Schneidereit [:till]]

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?

Comment 28

[Chris Peterson [:cpeterson]]

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.

Comment 29

[Jason Orendorff [:jorendorff]]

Sigh. Let's back it out. Till, can you take?

Comment 30

[Till Schneidereit [:till]]

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

Comment 31

[Chris Peterson [:cpeterson]]

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.

Comment 32

[Kohei Yoshino]

Thanks Chris, I just tweeted
https://twitter.com/FxSiteCompat/status/479036565990043649

Comment 33

[Anshu Prateek]

hi,

So where are we on this for 33 ?

Comment 34

[Till Schneidereit [:till]]

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

Comment 35

[Benjamin Kerensa [:bkerensa]]

Tracking for now based on Comment 31

Comment 36

[Dave Garrett]

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

Comment 37

[Chris Peterson [:cpeterson]]

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.

Comment 38

[Russell Levy]

this looks like it breaks sugarcrm as well

Comment 39

[Jason Orendorff [:jorendorff]]

I'm doing this today.

Comment 40

[Jason Orendorff [:jorendorff]]

I said that and then there was some sort of infrastructure disaster, but I'll try again tonight.

Comment 41

[Jason Orendorff [:jorendorff]]

The backout landed over the weekend.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d13f2827dec1
https://hg.mozilla.org/mozilla-central/rev/d13f2827dec1

Comment 42

[Jason Orendorff [:jorendorff]]

Attachment: Back out changeset 586ec2ee45d7 (bug 957513) for breaking the web.

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.

Comment 43

[Jason Orendorff [:jorendorff]]

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

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f170b174bc25

Reopening the bug for now, not sure if it should be WONTFIX or not.

Comment 45

[Jason Orendorff [:jorendorff]]

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.

Comment 46

[Florian Scholz (Open Web Docs)]

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.

Comment 47

[Dave Garrett]

(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). :/

Comment 48

[Florian Scholz (Open Web Docs)]

Thanks, indeed a bit confusing :/ I updated the sections. Any review or further clarification on that wiki page is welcome.

Comment 49

[Dave Garrett]

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