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
: 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)
: Jason Orendorff [:jorendorff]
: 869089 (view as bug list)
Depends on: 836061
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-01-27 19:50:31 PST
This is now starting to be a web compat problem in the opposite direction.  See
Comment 2 User image 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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-28 12:18:28 PST
Created attachment 707235 [details] [diff] [review]

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.
Comment 4 User image David Mandelin [:dmandelin] 2013-01-28 12:50:39 PST
Comment on attachment 707235 [details] [diff] [review]

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 User image 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) {
    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:

that I could push this:

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 User image Ryan VanderMeulen [:RyanVM] 2013-01-29 06:50:11 PST
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-29 22:59:49 PST
I updated 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 User image Jesse Ruderman 2013-02-19 00:50:12 PST
I added this change to
Comment 9 User image David Bruant 2013-02-20 01:24:22 PST
Thanks for the doc :-)
Comment 10 User image 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.
Comment 11 User image 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.