866369 - Incorrect rounding in Intl.NumberFormat

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Norbert Lindenberg]

ECMA-402 specifies the rounding behavior in Intl.NumberFormat.prototype.format to round towards the closest number that can be represented with the specified number of digits; in cases where two such numbers exist, it rounds away from zero. See the specifications of ToRawPrecision step 3.a and ToRawFixed step 2, both of which handle only the absolute value of the number to be formatted:
http://www.ecma-international.org/ecma-402/1.0/#ToRawPrecision
http://www.ecma-international.org/ecma-402/1.0/#ToRawFixed

The current implementation in SpiderMonkey doesn't handle this correctly - it doesn't always round away from zero:
js> 123.5.toLocaleString("en", {maximumFractionDigits: 0})
"124"
js> 124.5.toLocaleString("en", {maximumFractionDigits: 0}) 
"124"
js> 124.4.toLocaleString("en", {maximumFractionDigits: 0})    
"124"
js> 124.6.toLocaleString("en", {maximumFractionDigits: 0}) 
"125"
js> (-123.5).toLocaleString("en", {maximumFractionDigits: 0})
"-124"
js> (-124.5).toLocaleString("en", {maximumFractionDigits: 0}) 
"-124"

Comment 1

[Norbert Lindenberg]

Attachment: Test case for Test402 conformance test suite

This test is for the Test402 conformance test suite; it will also be pulled into js/src/tests.

Comment 2

[Norbert Lindenberg]

Attachment: Set ICU rounding mode

I neglected to set the rounding mode for ICU, and the default in ICU is to break ties towards the even number.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 742673 [details] [diff] [review]
Set ICU rounding mode

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

Huh.  I never really took the time to notice all the other number-formatting stuff in JS was round-away-from-zero, and not round-to-nearest-even as the language operations do.  Good to know!

I remember thinking about this issue existing when reviewing these patches initially, then not doing due diligence to verify.  :-\  Sorry about that!

I'll batch up these patch-lands with the Ugandan currency change, when the tree reopens.

Comment 4

[Jeff Walden [:Waldo]]

...or no, I'll just push the js/src change, and we'll pick up the other in a test402 update.  :-)  Skimming too hard, escaped me that was a change not to our tree.

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00275de9314a

Keeping open for the test402 update that'll pick this up...

Comment 6

[Norbert Lindenberg]

Attachment: Test case for jstests

The test case update for Test402, now pulled into jstests. Carrying r+jwalden.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75db6ad43aa1

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/00275de9314a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75db6ad43aa1

Comment 10

[Gordon P. Hemsley [:GPHemsley]]

Mass-moving existing Intl-related bugs to the new Core :: JavaScript: Internationalization API component.

If you think this bug has been moved in error, feel free to move it back to Core :: JavaScript Engine.

[Mass change filter: core-js-intl-api-move]