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.

Sign up for GitHub

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

Jump to bottom

Inconsistent formatting behavior when BinaryInteger cannot be converted into Int64 #186

Open
joey-gm opened this issue Jun 25, 2023 · 2 comments
Open

Inconsistent formatting behavior when BinaryInteger cannot be converted into Int64 #186

joey-gm opened this issue Jun 25, 2023 · 2 comments

Comments

@joey-gm
Copy link
Contributor

joey-gm commented Jun 25, 2023

ICUNumberFormatter doesn't support UInt64 number formatting. Integers not representable by Int64 are clamped as per documentation.

Under current implementation, however, if a BinaryInteger cannot be converted into Int64, IntegerFormatStyle will first attempt to convert the integer into Decimal (represented by Double) before falling back to clamping. This may result in inconsistent formatting behavior.

One edge case is 9223372036854775808 (Int64.max + 1). This integer can be converted to a floating-point value (9.223372036854776E+18), which will then be formatted into "9" instead of the the clamped number of 9,223,372,036,854,775,807 (Int64.max).

Should we drop the Decimal conversion in IntegerFormatStyle?

  • IntegerFormatStyle: Lines 221-222
  • IntegerFormatStyle.Percent: Lines 253-254
  • IntegerFormatStyle.Currency: Lines 285-286
  • AttributedString: Lines 512-513

Personally, I would prefer to drop the Int64 clamping as well and fall back to the Swift Standard Library's LosslessStringConvertible: String(value). This will allow us to preserve the correct numeric value albeit incorrect format. Ideally, we should have a proper implementation for UInt64 number formatting.

https://github.com/apple/swift-foundation/blob/fa61cd8aeb316727f9c7d740caa675e8c6f75abf/Sources/FoundationInternationalization/Formatting/Number/IntegerFormatStyle.swift#L211-L225

https://github.com/apple/swift-foundation/blob/fa61cd8aeb316727f9c7d740caa675e8c6f75abf/Sources/FoundationInternationalization/Formatting/Number/IntegerFormatStyle.swift#L253-L254

https://github.com/apple/swift-foundation/blob/fa61cd8aeb316727f9c7d740caa675e8c6f75abf/Sources/FoundationInternationalization/Formatting/Number/IntegerFormatStyle.swift#L285-L286

https://github.com/apple/swift-foundation/blob/fa61cd8aeb316727f9c7d740caa675e8c6f75abf/Sources/FoundationInternationalization/Formatting/Number/IntegerFormatStyle.swift#L512-L513
@jmschonfeld
Copy link
Contributor

I believe we added the Decimal conversion a little while back to support formatting values between Int64.max and UInt64.max so that numbers like UInt.max (equivalent to UInt64.max on some machines) were formatted correctly. We actually have some tests to ensure that these numbers are formatted correctly:

https://github.com/apple/swift-foundation/blob/b1f7b252df0ddbb7fa857284b1d1584940b6d477/Tests/FoundationInternationalizationTests/Formatting/NumberFormatStyleTests.swift#L1869-L1872

However, those tests are guarded by an #if FOUNDATION_FRAMEWORK because we haven't ported Decimal. Are you seeing the behavior incorrect for Int64.max + 1 somewhere? In practice I don't believe the conversion to decimal here actually produces a formatted "9" and the assertions in the above code pass, but @itingliu you might have more context here.

@itingliu
Copy link
Contributor

itingliu commented Nov 7, 2023

Is this resolved by #262?

oscbyspro added a commit to oscbyspro/swift-foundation that referenced this issue Dec 11, 2023
@oscbyspro
Removed conversions to Decimal in each integer format style (swiftlan…
220493f 
…g#186).

BinaryInteger's numeric string representation supersedes Decimal in the following cases:

1. IntegerFormatStyle.
2. integerFormatStyle.Attributed.
3. IntegerFormatStyle.Currency.
4. IntegerFormatStyle.Percent.
@oscbyspro oscbyspro mentioned this issue Dec 13, 2023
Merged
itingliu pushed a commit that referenced this issue Dec 21, 2023
@oscbyspro
Improved big-number formatting (#346)
c8b6d20 
* Improved integer and floating point `formatted()` methods.

- [IntegerFormatStyle] I removed the trapping conversion to `Int`. IntegerFormatStyle can format big integers since (#262).
- [FloatingPointFormatStyle] I removed the rounding conversion to `Double`. `formatted()` now does whatever `format(_:)` does.
- [Decimal.FormatStyle] N/A (there were no conversions here).

* Reenabled "Decimal Tests" in NumberFormatStyleTests.swift.

- IntegerFormatStyle big integer tests succeed.
- IntegerFormatStyle.Attributed big integer tests fail (output is clamped to `Int64`).

* Fixes IntegerFormatStyle.Attributed.

- Added a numeric string representation case to ICUNumberFormatter.Value.
- IntegerFormatStyle.Attributed now uses the above instead of `Int64(clamping:)`.

* Removed conversions to Decimal in each integer format style (#186).

BinaryInteger's numeric string representation supersedes Decimal in the following cases:

1. IntegerFormatStyle.
2. integerFormatStyle.Attributed.
3. IntegerFormatStyle.Currency.
4. IntegerFormatStyle.Percent.

* Check whether numeric string is zero using Double.

The numeric string format permits redundant zeros (like `+00.00`).

* Removed `isZero` and `doubleValue` from `ICUNumberFormatter.Value`.

Both `isZero` and `doubleValue` were used in `ByteCountFormatStyle`. These values are now taken from `FormatInput` (`Int64`) instead. Removing them from `ICUNumberFormatter.Value` makes it easier to accommodate non-numeric payloads such as strings, which can be used to format arbitrary precision numbers.

* Added `_format(_:doubleValue:)` to `ByteCountFormatStyle`.

Here's an internal method simliar to the `_format(_:)` method removed earlier because  wants its method back! I removed the first method to accommodate `ICUNumberFormatter.Value` cases that cannot implement `doubleValue`. The new method parameterizes the conversion instead, so you can call it whenever conversions to `Double` are possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jmschonfeld @itingliu @joey-gm