786135 - Make parseInt("042") === 42, now that other engines are moving that way

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

In bug 577536 we made parseInt with leading zero (not followed by "x" or "X") parse as decimal for strict mode code, to conform to ES5 in that case; non-strict code would still have had it parse as octal.  Then in bug 583925 we reverted that change and just made parseInt of "0<not x or X>..." parse as octal as a deliberate spec violation, because we realized caller-dependent behavior was crazy, and we weren't sure the web could take full spec compliance.

Fast forward a bit, tho, and (according to bug 510415 comment 7) JSC implements ES5-compliant decimal parsing (and it's in Safari 6), v8 also implements it (to be shipped in Cr23, fix landed three weeks ago), and IE implements decimal parsing in standards mode.  So decimal may be both spec-compliant and more web-compatible than we suspected.

Given all the other browsers are moving toward the spec, we should reconsider the decision to deliberately violate ES5 here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is now starting to be a web compat problem in the opposite direction.  See http://stackoverflow.com/questions/14542377/timestamp-conversion-difference-between-chrome-and-firefox

Comment 2

[Jeff Walden [:Waldo]]

I say we just do this.  Patch in a bit.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

This is running through try now, with the only difference being that this patch moves the test from js1_8_5/extensions to ecma_5/Global, now that it's testing for specified behavior.

https://tbpl.mozilla.org/?tree=Try&rev=5952dcbb7165

Comment 4

[David Mandelin [:dmandelin]]

Comment on attachment 707235 [details] [diff] [review]
Patch

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

::: js/src/jsnum.cpp
@@ +343,5 @@
>          if (!ToInt32(cx, args[1], &radix))
>              return false;
> +        if (radix == 0) {
> +            radix = 10;
> +        } else {

Style nit: this one would look nicer as |} else if {|.

Comment 5

[Jeff Walden [:Waldo]]

For what it's worth, I don't see how I can make the style nit work without violating the no-else-after-return rule.  The structure with else-if would be this:

  if (radix == 0) {
    radix = 10;
  } else if (radix < 2 || radix > 36) {
    args.rval().setDouble(js_NaN);
    return true;
  } else  if (radix != 16) {
    stripPrefix = false;
  }

...because you can't have fallthrough in the |radix == 0| case to setting |stripPrefix| to false:

  assertEq(parseInt("0x15"), 21);

Also, the structure in the patch better parallels the spec algorithm, which alternates on radix == 0 and radix != 0.

After a few hiccups of fixing tests expecting the old behavior (and one place that had |parseInt("0755")| to work around lack of octal numbers -- I don't remember if this was in strict mode code, but I assume so -- I finally got a try run that went far enough green:

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

that I could push this:

https://hg.mozilla.org/integration/mozilla-inbound/rev/67f7ebdea2fe

Keep an eye out for site bustage, although honestly, I'm not sure there's really a good thing to do here, if people are starting to assume the new behavior is correct while the old behavior lingers in older stuff.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67f7ebdea2fe

Comment 7

[Jeff Walden [:Waldo]]

I updated https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/parseInt to more carefully talk about this edge case.  This also definitely needs to go into New in Firefox 21 documentation, tho, so setting dev-doc-needed as well (and addon-compat too).

Comment 8

[Jesse Ruderman]

I added this change to https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers.

Comment 9

[David Bruant]

Thanks for the doc :-)

Comment 10

[Kohei Yoshino]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21