Last Comment Bug 786135 - Make parseInt("042") === 42, now that other engines are moving that way
: Make parseInt("042") === 42, now that other engines are moving that way
Status: RESOLVED FIXED
[js:t]
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 869089 (view as bug list)
Depends on: 836061
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-27 18:45 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2013-08-06 11:24 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (5.75 KB, patch)
2013-01-28 12:18 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-08-27 18:45:43 PDT
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 [:bz] 2013-01-27 19:50:31 PST
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] (remove +bmo to email) 2013-01-28 10:10:26 PST
I say we just do this.  Patch in a bit.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-28 12:18:28 PST
Created attachment 707235 [details] [diff] [review]
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] 2013-01-28 12:50:39 PST
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] (remove +bmo to email) 2013-01-28 21:43:38 PST
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] 2013-01-29 06:50:11 PST
https://hg.mozilla.org/mozilla-central/rev/67f7ebdea2fe
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-29 22:59:49 PST
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 2013-02-19 00:50:12 PST
I added this change to https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers.
Comment 9 David Bruant 2013-02-20 01:24:22 PST
Thanks for the doc :-)
Comment 10 Kohei Yoshino [:kohei] 2013-02-23 23:31:57 PST
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
Comment 11 Loic 2013-05-06 16:06:17 PDT
*** Bug 869089 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.