From 21a6776ac5b782615aecead4453480be0619e21e Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Fri, 13 Dec 2024 15:23:39 -0600 Subject: [PATCH 01/12] Add failing test case demonstrating sprintf negative zero bug #15557 --- .../FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index 187fa62c60e..622e5370cbc 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -75,6 +75,16 @@ type PrintfTests() = Assert.AreEqual(" 7B", sprintf "%*X" 8 123 ) Assert.AreEqual("7B ", sprintf "%-*X" 8 123 ) + + [] + member this.``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" + [] member _.``union case formatting`` () = Assert.AreEqual("CaseOne", sprintf "%A" CaseOne) From 541dad294a0e3e804864400a118014e3ba50e14f Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Fri, 13 Dec 2024 16:41:04 -0600 Subject: [PATCH 02/12] Fix negative zero behavior with printf "+" flag --- src/FSharp.Core/printf.fs | 14 ++++++++++---- .../Microsoft.FSharp.Core/PrintfTests.fs | 19 +++++++++++++------ 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/FSharp.Core/printf.fs b/src/FSharp.Core/printf.fs index b2b42f23370..6a337a7b816 100644 --- a/src/FSharp.Core/printf.fs +++ b/src/FSharp.Core/printf.fs @@ -658,8 +658,14 @@ module internal PrintfImpl = /// Contains functions to handle left/right and no justification case for numbers module GenericNumber = - - let isPositive (n: obj) = + + let inline singleIsNotNegativeZero (n: single) = + BitConverter.DoubleToInt64Bits (float n) <> BitConverter.DoubleToInt64Bits -0.0 + + let inline doubleIsNotNegativeZero (n: double) = + BitConverter.DoubleToInt64Bits n <> BitConverter.DoubleToInt64Bits -0.0 + + let isPositive (n: obj) = match n with | :? int8 as n -> n >= 0y | :? uint8 -> true @@ -671,8 +677,8 @@ module internal PrintfImpl = | :? uint64 -> true | :? nativeint as n -> n >= 0n | :? unativeint -> true - | :? single as n -> n >= 0.0f - | :? double as n -> n >= 0.0 + | :? single as n -> n >= 0.0f && singleIsNotNegativeZero n + | :? double as n -> n >= 0.0 && doubleIsNotNegativeZero n | :? decimal as n -> n >= 0.0M | _ -> failwith "isPositive: unreachable" diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index 622e5370cbc..2fa590e23ec 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -78,12 +78,19 @@ type PrintfTests() = [] member this.``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.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" [] member _.``union case formatting`` () = From cad21b4abd2e520894f8f343b8479e45ecb36ed1 Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sat, 14 Dec 2024 12:33:56 -0600 Subject: [PATCH 03/12] Add printf tests for infinity and nan --- src/FSharp.Core/printf.fs | 6 +-- .../Microsoft.FSharp.Core/PrintfTests.fs | 38 ++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/FSharp.Core/printf.fs b/src/FSharp.Core/printf.fs index 6a337a7b816..b05d7c3f6e7 100644 --- a/src/FSharp.Core/printf.fs +++ b/src/FSharp.Core/printf.fs @@ -659,12 +659,12 @@ module internal PrintfImpl = /// Contains functions to handle left/right and no justification case for numbers module GenericNumber = - let inline singleIsNotNegativeZero (n: single) = - BitConverter.DoubleToInt64Bits (float n) <> BitConverter.DoubleToInt64Bits -0.0 - let inline doubleIsNotNegativeZero (n: double) = BitConverter.DoubleToInt64Bits n <> BitConverter.DoubleToInt64Bits -0.0 + let inline singleIsNotNegativeZero (n: single) = + doubleIsNotNegativeZero (float n) + let isPositive (n: obj) = match n with | :? int8 as n -> n >= 0y diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index 2fa590e23ec..eb78d529199 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -75,9 +75,21 @@ type PrintfTests() = Assert.AreEqual(" 7B", sprintf "%*X" 8 123 ) Assert.AreEqual("7B ", sprintf "%-*X" 8 123 ) + + [] + 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" + [] - member this.``positive and negative zero``() = + 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" @@ -91,7 +103,31 @@ type PrintfTests() = test "%+f" +0.0f "+0.000000" test "%+f" -0.0f "-0.000000" test "%+f" -0.0000001f "-0.000000" + + [] + 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" + + [] + 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" + [] member _.``union case formatting`` () = Assert.AreEqual("CaseOne", sprintf "%A" CaseOne) From 74a0c912f850f737572ea9ab59b836708442e46a Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sat, 14 Dec 2024 12:48:11 -0600 Subject: [PATCH 04/12] Refactor --- src/FSharp.Core/printf.fs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/FSharp.Core/printf.fs b/src/FSharp.Core/printf.fs index b05d7c3f6e7..e86b6871f96 100644 --- a/src/FSharp.Core/printf.fs +++ b/src/FSharp.Core/printf.fs @@ -659,11 +659,14 @@ module internal PrintfImpl = /// Contains functions to handle left/right and no justification case for numbers module GenericNumber = - let inline doubleIsNotNegativeZero (n: double) = - BitConverter.DoubleToInt64Bits n <> BitConverter.DoubleToInt64Bits -0.0 + let inline doubleIsPositive (n: double) = + 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) - let inline singleIsNotNegativeZero (n: single) = - doubleIsNotNegativeZero (float n) + let inline singleIsPositive (n: single) = + doubleIsPositive (float n) let isPositive (n: obj) = match n with @@ -677,8 +680,8 @@ module internal PrintfImpl = | :? uint64 -> true | :? nativeint as n -> n >= 0n | :? unativeint -> true - | :? single as n -> n >= 0.0f && singleIsNotNegativeZero n - | :? double as n -> n >= 0.0 && doubleIsNotNegativeZero n + | :? single as n -> singleIsPositive n + | :? double as n -> doubleIsPositive n | :? decimal as n -> n >= 0.0M | _ -> failwith "isPositive: unreachable" From 5a5e37e9b503aa07ebc7eddbbd1f03ad6de36a6d Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sat, 14 Dec 2024 15:47:50 -0600 Subject: [PATCH 05/12] Fix printf sign handling for -0 decimal and work around a .NET bug --- src/FSharp.Core/printf.fs | 22 +++++++++++++++++-- .../Microsoft.FSharp.Core/PrintfTests.fs | 14 +++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/FSharp.Core/printf.fs b/src/FSharp.Core/printf.fs index e86b6871f96..541aa1c2dc2 100644 --- a/src/FSharp.Core/printf.fs +++ b/src/FSharp.Core/printf.fs @@ -668,6 +668,17 @@ module internal PrintfImpl = let inline singleIsPositive (n: single) = doubleIsPositive (float n) + 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 + let bits = Decimal.GetBits n + bits[3] >>> 31 + + let inline decimalIsNegativeZero (n: decimal) = + decimalSignBit n <> 0 + + let inline decimalIsPositive (n: decimal) = + n > 0.0M || (n = 0.0M && decimalSignBit n = 0) + let isPositive (n: obj) = match n with | :? int8 as n -> n >= 0y @@ -682,7 +693,7 @@ module internal PrintfImpl = | :? unativeint -> true | :? single as n -> singleIsPositive n | :? double as n -> doubleIsPositive n - | :? decimal as n -> n >= 0.0M + | :? decimal as n -> decimalIsPositive n | _ -> failwith "isPositive: unreachable" /// handles right justification when pad char = '0' @@ -861,11 +872,18 @@ module internal PrintfImpl = module FloatAndDecimal = + let fixupDecimalSign (n: decimal) (nStr: string) = + // 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) + if n = 0.0m && not (nStr.StartsWith "-") && GenericNumber.decimalIsNegativeZero n 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) = diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index eb78d529199..fc7bfae125a 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -87,6 +87,11 @@ type PrintfTests() = 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" [] member this.``sign flag - positive and negative zero``() = @@ -103,7 +108,14 @@ type PrintfTests() = 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" + [] member this.``sign flag - infinity``() = test "%f" +infinity "Infinity" From f33ccc737d48216e0a903d2e3f4abe6b71727f15 Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sat, 14 Dec 2024 16:14:41 -0600 Subject: [PATCH 06/12] Work around another corner case consequence of .NET negative decimal formatting bug --- src/FSharp.Core/printf.fs | 3 ++- .../FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/FSharp.Core/printf.fs b/src/FSharp.Core/printf.fs index 541aa1c2dc2..15b98c52dd2 100644 --- a/src/FSharp.Core/printf.fs +++ b/src/FSharp.Core/printf.fs @@ -874,7 +874,8 @@ module internal PrintfImpl = let fixupDecimalSign (n: decimal) (nStr: string) = // 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) - if n = 0.0m && not (nStr.StartsWith "-") && GenericNumber.decimalIsNegativeZero n then + // 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 diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index fc7bfae125a..da1ece3f0c0 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -111,7 +111,7 @@ type PrintfTests() = test "%f" +0.0M "0.000000" test "%f" -0.0M "-0.000000" - test "%f" -0.0000001M "-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" From fcd573cbad18d490de5efdb4d89f8f72ca98247c Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sat, 14 Dec 2024 16:16:34 -0600 Subject: [PATCH 07/12] Add test cases --- .../Microsoft.FSharp.Core/PrintfTests.fs | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index da1ece3f0c0..5632eb78e4f 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -75,7 +75,7 @@ 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 [] member this.``sign flag - positive and negative one``() = test "%f" +1.0 "1.000000" @@ -111,7 +111,7 @@ type PrintfTests() = test "%f" +0.0M "0.000000" test "%f" -0.0M "-0.000000" - test "%f" -0.0000001M "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" @@ -140,6 +140,29 @@ type PrintfTests() = test "%+f" +nanf "NaN" test "%+f" -nanf "NaN" + // test cases for https://github.com/dotnet/runtime/issues/110712 (same root cause as #15557; listing for completeness) + [] + 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" + + [] + 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" + [] member _.``union case formatting`` () = Assert.AreEqual("CaseOne", sprintf "%A" CaseOne) From d8485cbacecff0607efbf7a0947555e6f61386de Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sat, 14 Dec 2024 16:26:39 -0600 Subject: [PATCH 08/12] Add release notes --- docs/release-notes/.FSharp.Core/9.0.200.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/release-notes/.FSharp.Core/9.0.200.md b/docs/release-notes/.FSharp.Core/9.0.200.md index fdcc65f5537..be09a375126 100644 --- a/docs/release-notes/.FSharp.Core/9.0.200.md +++ b/docs/release-notes/.FSharp.Core/9.0.200.md @@ -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 From 274544c8d24b7ba5816091d56a7949dc7f96e52d Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Sun, 15 Dec 2024 11:36:36 -0600 Subject: [PATCH 09/12] Correct wrong issue link --- .../FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index 5632eb78e4f..a0d10bb4f9e 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -140,7 +140,7 @@ type PrintfTests() = test "%+f" +nanf "NaN" test "%+f" -nanf "NaN" - // test cases for https://github.com/dotnet/runtime/issues/110712 (same root cause as #15557; listing for completeness) + // test cases for https://github.com/dotnet/fsharp/issues/15558 (same root cause as #15557; listing for completeness) [] member this.``zero padding - positive and negative one`` () = test "%010.3f" +1.0 "000001.000" From 06fc2abf559d42cae8ecbfa4b3a7fc12037c297d Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Mon, 16 Dec 2024 11:28:39 -0600 Subject: [PATCH 10/12] Change test case expected outputs in light of reviews --- .../Microsoft.FSharp.Core/PrintfTests.fs | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index a0d10bb4f9e..6fd148aef7e 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -96,25 +96,28 @@ type PrintfTests() = [] 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.0000001 "0.000000" test "%+f" +0.0 "+0.000000" - test "%+f" -0.0 "-0.000000" - test "%+f" -0.0000001 "-0.000000" + test "%+f" -0.0 "+0.000000" + // TODO: should this output -0.000000 or +0.000000? See https://github.com/dotnet/fsharp/pull/18147#issuecomment-2546220183 + 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.0000001f "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" + // see previous comment + 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.0000001M "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" + // see previous comment + test "%+f" -0.0000001M "+0.000000" [] member this.``sign flag - infinity``() = @@ -155,13 +158,13 @@ type PrintfTests() = [] 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.0 "000000.000" test "%010.3f" +0.0f "000000.000" - test "%010.3f" -0.0f "-00000.000" + test "%010.3f" -0.0f "000000.000" test "%010.3f" +0.0M "000000.000" - test "%010.3f" -0.0M "-00000.000" + test "%010.3f" -0.0M "000000.000" [] member _.``union case formatting`` () = From 68f5af10133d489e538211f2c79967db618947d0 Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Mon, 16 Dec 2024 12:59:25 -0600 Subject: [PATCH 11/12] Comment out some test cases for now, and add some others --- .../Microsoft.FSharp.Core/PrintfTests.fs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index 6fd148aef7e..d7e5b388844 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -101,7 +101,7 @@ type PrintfTests() = test "%+f" +0.0 "+0.000000" test "%+f" -0.0 "+0.000000" // TODO: should this output -0.000000 or +0.000000? See https://github.com/dotnet/fsharp/pull/18147#issuecomment-2546220183 - test "%+f" -0.0000001 "+0.000000" + // test "%+f" -0.0000001 "+0.000000" test "%f" +0.0f "0.000000" test "%f" -0.0f "0.000000" @@ -109,7 +109,7 @@ type PrintfTests() = test "%+f" +0.0f "+0.000000" test "%+f" -0.0f "+0.000000" // see previous comment - test "%+f" -0.0000001f "+0.000000" + // test "%+f" -0.0000001f "+0.000000" test "%f" +0.0M "0.000000" test "%f" -0.0M "0.000000" @@ -117,7 +117,7 @@ type PrintfTests() = test "%+f" +0.0M "+0.000000" test "%+f" -0.0M "+0.000000" // see previous comment - test "%+f" -0.0000001M "+0.000000" + // test "%+f" -0.0000001M "+0.000000" [] member this.``sign flag - infinity``() = @@ -157,14 +157,14 @@ type PrintfTests() = [] member this.``zero padding - positive and negative zero`` () = - test "%010.3f" +0.0 "000000.000" - test "%010.3f" -0.0 "000000.000" + test "%010.3f" +0.0 "000000.000" + test "%010.3f" -0.0 "000000.000" - test "%010.3f" +0.0f "000000.000" - test "%010.3f" -0.0f "000000.000" + test "%010.3f" +0.0f "000000.000" + test "%010.3f" -0.0f "000000.000" - test "%010.3f" +0.0M "000000.000" - test "%010.3f" -0.0M "000000.000" + test "%010.3f" +0.0M "000000.000" + test "%010.3f" -0.0M "000000.000" [] member _.``union case formatting`` () = From 9c035f3c9f45ec8a0b51a4717254d442152e23ab Mon Sep 17 00:00:00 2001 From: John Wostenberg Date: Mon, 16 Dec 2024 14:38:23 -0600 Subject: [PATCH 12/12] Fix sign handling to not be a breaking change with respect to sprintf behavior on .NET Framework --- src/FSharp.Core/printf.fs | 44 +++++++------------ .../Microsoft.FSharp.Core/PrintfTests.fs | 19 +++++--- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/src/FSharp.Core/printf.fs b/src/FSharp.Core/printf.fs index 15b98c52dd2..b5dd7486e95 100644 --- a/src/FSharp.Core/printf.fs +++ b/src/FSharp.Core/printf.fs @@ -659,25 +659,9 @@ module internal PrintfImpl = /// Contains functions to handle left/right and no justification case for numbers module GenericNumber = - let inline doubleIsPositive (n: double) = - 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) - - let inline singleIsPositive (n: single) = - doubleIsPositive (float n) - - 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 - let bits = Decimal.GetBits n - bits[3] >>> 31 - - let inline decimalIsNegativeZero (n: decimal) = - decimalSignBit n <> 0 - - let inline decimalIsPositive (n: decimal) = - n > 0.0M || (n = 0.0M && decimalSignBit n = 0) + let inline singleIsPositive n = n >= 0.0f + let inline doubleIsPositive n = n >= 0.0 + let inline decimalIsPositive n = n >= 0.0M let isPositive (n: obj) = match n with @@ -871,20 +855,24 @@ module internal PrintfImpl = | _ -> invalidArg (nameof spec) "Invalid integer format" module FloatAndDecimal = - - let fixupDecimalSign (n: decimal) (nStr: string) = - // 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 + + let fixupSign isPositive (nStr: string) = + // .NET Core and .NET Framework differ in how ToString and other formatting methods handle certain negative floating-point values (namely, -0.0 and values which round to -0.0 upon display). + // (see: https://devblogs.microsoft.com/dotnet/floating-point-parsing-and-formatting-improvements-in-net-core-3-0/) + // So in order for F#'s sprintf to behave consistently across platforms, we essentially "polyfill" (normalize) the output to ToString across the two runtimes. Specifically we do this by + // removing the '-' character in situations where the rest of the sprintf logic treats the number as positive, but .NET Core treats it as negative (i.e. -0.0, or -0.0000000001 when + // displaying with only a few decimal places) + // TODO: make this work for numbers like -0.0000000001 + if isPositive && nStr.StartsWith "-" then + nStr.Substring 1 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) |> fixupDecimalSign n + | :? single as n -> n.ToString(fmt, CultureInfo.InvariantCulture) |> fixupSign (GenericNumber.singleIsPositive n) + | :? double as n -> n.ToString(fmt, CultureInfo.InvariantCulture) |> fixupSign (GenericNumber.doubleIsPositive n) + | :? decimal as n -> n.ToString(fmt, CultureInfo.InvariantCulture) |> fixupSign (GenericNumber.decimalIsPositive n) | _ -> failwith "toFormattedString: unreachable" let isNumber (x: obj) = diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs index d7e5b388844..6ba6181588d 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs @@ -97,25 +97,30 @@ type PrintfTests() = 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" - // TODO: should this output -0.000000 or +0.000000? See https://github.com/dotnet/fsharp/pull/18147#issuecomment-2546220183 - // 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" - // see previous comment - // 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" + + [] + member this.``sign flag - very small positive and negative numbers``() = + test "%f" -0.0000001 "0.000000" + // TODO: should this output -0.000000 or +0.000000? See https://github.com/dotnet/fsharp/pull/18147#issuecomment-2546220183 + // test "%+f" -0.0000001 "+0.000000" + + test "%f" -0.0000001f "0.000000" + // see previous comment + // test "%+f" -0.0000001f "+0.000000" + + test "%f" -0.0000001M "0.000000" // see previous comment // test "%+f" -0.0000001M "+0.000000"