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

Printf fixes around handling of -0.0 (negative zero) #18147

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Printf fixes around handling of -0.0 (negative zero) #18147

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
21a6776
Add failing test case demonstrating sprintf negative zero bug #15557
jwosty Dec 13, 2024
541dad2
Fix negative zero behavior with printf "+" flag
jwosty Dec 13, 2024
cad21b4
Add printf tests for infinity and nan
jwosty Dec 14, 2024
74a0c91
Refactor
jwosty Dec 14, 2024
5a5e37e
Fix printf sign handling for -0 decimal and work around a .NET bug
jwosty Dec 14, 2024
f33ccc7
Work around another corner case consequence of .NET negative decimal …
jwosty Dec 14, 2024
fcd573c
Add test cases
jwosty Dec 14, 2024
d8485cb
Add release notes
jwosty Dec 14, 2024
274544c
Correct wrong issue link
jwosty Dec 15, 2024
06fc2ab
Change test case expected outputs in light of reviews
jwosty Dec 16, 2024
68f5af1
Comment out some test cases for now, and add some others
jwosty Dec 16, 2024
9c035f3
Fix sign handling to not be a breaking change with respect to sprintf…
jwosty Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/release-notes/.FSharp.Core/9.0.200.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

* Fix exception on Post after MailboxProcessor was disposed ([Issue #17849](https://github.com/dotnet/fsharp/issues/17849), [PR #17922](https://github.com/dotnet/fsharp/pull/17922))
* Fix missing null annotation in Async.SwitchToContext ([Issue #18055](https://github.com/dotnet/fsharp/issues/18055), [PR #18059](https://github.com/dotnet/fsharp/pull/18059))
* Fix printf handling of -0.0 (negative zero) values for float, float32, and decimal values ([Issue #15557](https://github.com/dotnet/fsharp/issues/15557) and [Issue #15558](https://github.com/dotnet/fsharp/issues/15558), [PR #18147](https://github.com/dotnet/fsharp/pull/18147))

### Added

Expand Down
40 changes: 34 additions & 6 deletions src/FSharp.Core/printf.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,28 @@ module internal PrintfImpl =

/// Contains functions to handle left/right and no justification case for numbers
module GenericNumber =

let isPositive (n: obj) =

let inline doubleIsPositive (n: double) =
Copy link
Member

@tannergooding tannergooding Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you just use double.IsPositive(n), or does F# need to differ?

.NET Framework notably has different behavior than .NET (Core), which is also intentional as .NET Framework has a much higher backwards compatibility bar

n >= 0.0
// Ensure -0.0 is treated as negative (see https://github.com/dotnet/fsharp/issues/15557)
// and use bitwise comparison because floating point comparison treats +0.0 as equal to -0.0
&& (BitConverter.DoubleToInt64Bits n <> BitConverter.DoubleToInt64Bits -0.0)
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to do the right thing for NaN? What is the behavior F# expects to have there, particularly with the existance of both +NaN and -NaN at the bitwise level?


let inline singleIsPositive (n: single) =
doubleIsPositive (float n)
Copy link
Member

Choose a reason for hiding this comment

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

Related to the question of NaN, this has a chance of causing normalization and doing the "wrong thing". It should likely be explicitly handled instead, potentially using single.IsPositive


let decimalSignBit (n: decimal) =
// Unfortunately it's impossible to avoid this array allocation without either targeting .NET 5+ or relying on knowledge about decimal's internal representation
Copy link
Member

Choose a reason for hiding this comment

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

decimal's representation is notably fixed and while it technically isn't strictly documented, it isn't really "internal".

decimal is a C# primitive type that must strictly be 16-bytes in size (a hard assumption exists here), but also is an interchange type for the Win32 DECIMAL type and it's layout must be ABI compatible with this: https://learn.microsoft.com/en-us/windows/win32/api/wtypes/ns-wtypes-decimal-r1

Copy link
Member

@tannergooding tannergooding Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could likely explicitly document this if really needed, but it's generally safe to depend on.

There is a decimal.IsPositive API or you could notably also use NativePtr.stackalloc to avoid the allocation if the library was multitargeted

Copy link
Member

Choose a reason for hiding this comment

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

The library is netstandard2.0/2.1, which I think rules out the IsPositive/IsNegative methods. And ._flags field itself is private, I am not sure there is a safe way to read it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@T-Gro we could write an equivalent struct type and use magic like Unsafe.As to bitwise cast it, but that feels evil to me

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I would vote against doing it that way

let bits = Decimal.GetBits n
bits[3] >>> 31

let inline decimalIsNegativeZero (n: decimal) =
decimalSignBit n <> 0
Copy link
Member

Choose a reason for hiding this comment

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

This is notably checking IsNegative, not IsNegativeZero and would also return true for -1.0m, as an example.


let inline decimalIsPositive (n: decimal) =
Copy link
Member

@tannergooding tannergooding Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's much cheaper to just check value._flags >= 0: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Decimal.cs,1443, which is what the official decimal.IsPositive API does

Copy link
Member

Choose a reason for hiding this comment

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

The same as above, this has to build with netstandard2.0.

n > 0.0M || (n = 0.0M && decimalSignBit n = 0)

let isPositive (n: obj) =
match n with
| :? int8 as n -> n >= 0y
| :? uint8 -> true
Expand All @@ -671,9 +691,9 @@ module internal PrintfImpl =
| :? uint64 -> true
| :? nativeint as n -> n >= 0n
| :? unativeint -> true
| :? single as n -> n >= 0.0f
| :? double as n -> n >= 0.0
| :? decimal as n -> n >= 0.0M
| :? single as n -> singleIsPositive n
| :? double as n -> doubleIsPositive n
| :? decimal as n -> decimalIsPositive n
| _ -> failwith "isPositive: unreachable"

/// handles right justification when pad char = '0'
Expand Down Expand Up @@ -852,11 +872,19 @@ module internal PrintfImpl =

module FloatAndDecimal =

let fixupDecimalSign (n: decimal) (nStr: string) =
Copy link
Member

Choose a reason for hiding this comment

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

decimal not including the negative sign for 0 is an intentional choice for this type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vzarytovskii we should probably just follow the same behavior as .NET here then, right? i.e. make -0 decimals format the same as +0 decimals

Copy link
Member

Choose a reason for hiding this comment

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

I would ping @T-Gro for that, let the team decide what's desired and whether suggestion is needed.

Copy link
Member

Choose a reason for hiding this comment

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

It is a change we could take - but it should be documented on the .NET breaking changes and happen on a major version update.

// Forward-compatible workaround for a .NET bug which causes -0.0m (negative zero) to be missing its sign (see: https://github.com/dotnet/runtime/issues/110712)
// This also affects numbers which round/truncate to negative zero (i.e. very small negative numbers)
if (n = 0.0m && not (nStr.StartsWith "-") && GenericNumber.decimalIsNegativeZero n) || (n < 0.0m && not (nStr.StartsWith "-")) then
"-" + nStr
else
nStr

let rec toFormattedString fmt (v: obj) =
match v with
| :? single as n -> n.ToString(fmt, CultureInfo.InvariantCulture)
| :? double as n -> n.ToString(fmt, CultureInfo.InvariantCulture)
| :? decimal as n -> n.ToString(fmt, CultureInfo.InvariantCulture)
| :? decimal as n -> n.ToString(fmt, CultureInfo.InvariantCulture) |> fixupDecimalSign n
| _ -> failwith "toFormattedString: unreachable"

let isNumber (x: obj) =
Expand Down
88 changes: 88 additions & 0 deletions tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,94 @@ type PrintfTests() =
Assert.AreEqual(" 7B", sprintf "%*X" 8 123 )
Assert.AreEqual("7B ", sprintf "%-*X" 8 123 )

// test cases for https://github.com/dotnet/fsharp/issues/15557
[<Fact>]
member this.``sign flag - positive and negative one``() =
test "%f" +1.0 "1.000000"
test "%f" -1.0 "-1.000000"
test "%+f" +1.0 "+1.000000"
test "%+f" -1.0 "-1.000000"

test "%f" +1.0f "1.000000"
test "%f" -1.0f "-1.000000"
test "%+f" +1.0f "+1.000000"
test "%+f" -1.0f "-1.000000"

test "%f" +1.0M "1.000000"
test "%f" -1.0M "-1.000000"
test "%+f" +1.0M "+1.000000"
test "%+f" -1.0M "-1.000000"

[<Fact>]
member this.``sign flag - positive and negative zero``() =
test "%f" +0.0 "0.000000"
test "%f" -0.0 "-0.000000"
test "%f" -0.0000001 "-0.000000"
test "%+f" +0.0 "+0.000000"
test "%+f" -0.0 "-0.000000"
test "%+f" -0.0000001 "-0.000000"

test "%f" +0.0f "0.000000"
test "%f" -0.0f "-0.000000"
test "%f" -0.0000001f "-0.000000"
test "%+f" +0.0f "+0.000000"
test "%+f" -0.0f "-0.000000"
test "%+f" -0.0000001f "-0.000000"

test "%f" +0.0M "0.000000"
test "%f" -0.0M "-0.000000"
test "%f" -0.0000001M "-0.000000"
test "%+f" +0.0M "+0.000000"
test "%+f" -0.0M "-0.000000"
test "%+f" -0.0000001M "-0.000000"

[<Fact>]
member this.``sign flag - infinity``() =
test "%f" +infinity "Infinity"
test "%f" -infinity "-Infinity"
test "%+f" +infinity "Infinity"
test "%+f" -infinity "-Infinity"

test "%f" +infinityf "Infinity"
test "%f" -infinityf "-Infinity"
test "%+f" +infinityf "Infinity"
test "%+f" -infinityf "-Infinity"

[<Fact>]
member this.``sign flag - NaN``() =
test "%f" +nan "NaN"
test "%f" -nan "NaN"
test "%+f" +nan "NaN"
test "%+f" -nan "NaN"

test "%f" +nanf "NaN"
test "%f" -nanf "NaN"
test "%+f" +nanf "NaN"
test "%+f" -nanf "NaN"

// test cases for https://github.com/dotnet/fsharp/issues/15558 (same root cause as #15557; listing for completeness)
[<Fact>]
member this.``zero padding - positive and negative one`` () =
test "%010.3f" +1.0 "000001.000"
test "%010.3f" -1.0 "-00001.000"

test "%010.3f" +1.0f "000001.000"
test "%010.3f" -1.0f "-00001.000"

test "%010.3f" +1.0M "000001.000"
test "%010.3f" -1.0M "-00001.000"

[<Fact>]
member this.``zero padding - positive and negative zero`` () =
test "%010.3f" +0.0 "000000.000"
test "%010.3f" -0.0 "-00000.000"

test "%010.3f" +0.0f "000000.000"
test "%010.3f" -0.0f "-00000.000"

test "%010.3f" +0.0M "000000.000"
test "%010.3f" -0.0M "-00000.000"

[<Fact>]
member _.``union case formatting`` () =
Assert.AreEqual("CaseOne", sprintf "%A" CaseOne)
Expand Down
Loading