Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Fix spec bugs in numberformat.html caused by Unified NumberFormat #572

Merged
merged 5 commits into from
Dec 23, 2021

Conversation

sffc
Copy link
Contributor

@sffc sffc commented May 6, 2021

I changed part of the ToRawPrecision algorithm in ES2019 as part of Unified NumberFormat. However, in my work on NFv3, I realized that one change I made is unsound. This is in a crucial function, ToRawPrecision, which is used for all significant digits rounding.

Problem explanation: In ToRawPrecision, the e value is supposed to represent the power of 10 of the resolved number after rounding. However, my change in ES2020 computed e based on the input value before rounding.

Example values of x, p, n, and e, with xFinal for reference:

Formula: xn × 10ep+1

x p n e xFinal
1.11 3 111 0 1.11
11.1 3 111 1 11.1
1.11 2 11 0 1.1
11.1 2 11 1 11
9.999 3 100 1 10.0
9.999 2 10 1 10
9.9 3 990 0 9.90
9.9 2 99 0 9.9

Practically, the ES2020 algorithm works in all cases except those where the number rounds up to the next magnitude. A spec-compliant ES2020 implementation would need to return "9.99" instead of "10.0" in the fifth row.

Please check my logic!

All browsers still implement it correctly, but I would like to restore the old algorithm as soon as possible. I consider this an editorial change because it is restoring correct behavior after a mess-up in ES2020. (2021-05-06: This is still normative.)

Can we sneak this into ES2021? (2021-05-06: No, don't push this into ES 2021.)

@sffc sffc changed the title Editorial: Restore ES2019 algorithm for ToRawPrecision Normative: Restore ES2019 algorithm for ToRawPrecision May 6, 2021
@sffc sffc changed the title Normative: Restore ES2019 algorithm for ToRawPrecision Normative: Fix spec bugs in numberformat.html caused by Unified NumberFormat May 12, 2021
@sffc
Copy link
Contributor Author

sffc commented May 12, 2021

I found another spec bug originating from Unified NumberFormat, which I added to this PR as 565a054.

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PartitionNumberPattern revert looks great, the ToRawPrecision change is a bit more complicated but since it is exactly the same as the previous line, it should be all good.

@ryzokuken
Copy link
Member

@gibson042 @leobalter could we have another review on this?

@sffc
Copy link
Contributor Author

sffc commented Jul 13, 2021

I would like to have this checked in. Can I have another review please?

@ryzokuken
Copy link
Member

spec/numberformat.html Show resolved Hide resolved
spec/numberformat.html Outdated Show resolved Hide resolved
sffc and others added 2 commits August 12, 2021 21:33
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@sffc sffc requested a review from gibson042 August 13, 2021 02:34
sffc added a commit to tc39/proposal-intl-numberformat-v3 that referenced this pull request Aug 13, 2021
@ryzokuken
Copy link
Member

@sffc do you think this is ready to merge?

@sffc
Copy link
Contributor Author

sffc commented Dec 23, 2021

Yes, if you all approve, this can be merged. Is there anything that needs to be done to fix the CI failure, or is that just flaky?

@ljharb
Copy link
Member

ljharb commented Dec 23, 2021

It's unlikely to be flaky; if you rebase it i suspect you'll see a real lint failure.

Update PR #572 with latest master
@sffc
Copy link
Contributor Author

sffc commented Dec 23, 2021

I merged in the latest master, and CI is green now. 😃

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks, will all the boxes checked, let's merge this!

@ryzokuken ryzokuken merged commit f0f66cf into master Dec 23, 2021
@ryzokuken ryzokuken deleted the es-2019-torawprecision branch December 23, 2021 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants