Closed Bug 786135 Opened 12 years ago Closed 12 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(Keywords: addon-compat, dev-doc-complete, Whiteboard: [js:t])


(1 file)

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.
Whiteboard: [js:t]
This is now starting to be a web compat problem in the opposite direction.  See
I say we just do this.  Patch in a bit.
Assignee: general → jwalden+bmo
Attached patch PatchSplinter 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.
Attachment #707235 - Flags: review?(dmandelin)
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 {|.
Attachment #707235 - Flags: review?(dmandelin) → review+
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.
Target Milestone: --- → mozilla21
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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).
Summary: Consider making parseInt("042") === 42, now that other engines are moving that way → Make parseInt("042") === 42, now that other engines are moving that way
Thanks for the doc :-)
I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
You need to log in before you can comment on or make changes to this bug.