-
Notifications
You must be signed in to change notification settings - Fork 790
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
Printf fixes around handling of -0.0 (negative zero) #18147
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes required
|
src/FSharp.Core/printf.fs
Outdated
@@ -852,11 +872,19 @@ module internal PrintfImpl = | |||
|
|||
module FloatAndDecimal = | |||
|
|||
let fixupDecimalSign (n: decimal) (nStr: string) = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/FSharp.Core/printf.fs
Outdated
|
||
let isPositive (n: obj) = | ||
|
||
let inline doubleIsPositive (n: double) = |
There was a problem hiding this comment.
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
src/FSharp.Core/printf.fs
Outdated
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) |
There was a problem hiding this comment.
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?
src/FSharp.Core/printf.fs
Outdated
&& (BitConverter.DoubleToInt64Bits n <> BitConverter.DoubleToInt64Bits -0.0) | ||
|
||
let inline singleIsPositive (n: single) = | ||
doubleIsPositive (float n) |
There was a problem hiding this comment.
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
src/FSharp.Core/printf.fs
Outdated
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
src/FSharp.Core/printf.fs
Outdated
bits[3] >>> 31 | ||
|
||
let inline decimalIsNegativeZero (n: decimal) = | ||
decimalSignBit n <> 0 |
There was a problem hiding this comment.
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.
src/FSharp.Core/printf.fs
Outdated
let inline decimalIsNegativeZero (n: decimal) = | ||
decimalSignBit n <> 0 | ||
|
||
let inline decimalIsPositive (n: decimal) = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
OK so after reading some of the review comments here, I went and checked the output of the currently released sprintf against both .NET 9 and .NET framework. The code: printfn "-- floats --"
printfn "%%f +0.0f => %f" +0.0f
printfn "%%f -0.0f => %f" -0.0f
printfn "%%+f +0.0f => %+f" +0.0f
printfn "%%+f -0.0f => %+f" -0.0f
printfn "%%010.3f +0.0f => %010.3f" +0.0f
printfn "%%010.3f -0.0f => %010.3f" -0.0f
printfn "-- decimals --"
printfn "%%f +0.0m => %f" +0.0m
printfn "%%f -0.0m => %f" -0.0m
printfn "%%+f +0.0m => %+f" +0.0m
printfn "%%+f -0.0m => %+f" -0.0m
printfn "%%010.3f +0.0m => %010.3f" +0.0m
printfn "%%010.3f -0.0m => %010.3f" -0.0m Output for .NET 9:
Output for .NET 4.8:
As you can see, neither #15557 nor #15558 are present in the .NET framework run (which makes some sense given @tannergooding's comments). Furthermore, the .NET framework version always treats (decimal and float) negative zero as positive zero for formatting purposes (which I didn't realize before since I didn't think to also test against Framework). Therefore I will amend this PR to preserve the .NET framework behavior in both cases. I personally don't particularly care how Additionally, it looks like |
Actually there is still one scenario that I'd argue is a bug under .NET framework, too: sprintf "%+f" -0.0000001 Under .NET framework this prints "0.000000" which is definitely a bug ( Thoughts? |
.NET Framework has many bugs that will never be fixed due to its much stronger back-compat bar. Other bugs around IEEE 754 floating-point include things like Typically this much stronger backwards compatibility bar means that the behavior should be preserved "as is" on .NET Framework and fixes should only be taken on .NET [Core] where relevant. F# could decide differently, but that itself comes with its own risks and potential for new problems due to it differing from what users may expect and differing from what they'd experience using the regular BCL APIs.
This assumption is already broken (on .NET Framework) by negative zero. Negative zero itself exists due to there being negative non-zero results which round towards zero due to the precision limitations of the underlying format, for scientific and other mathematical domains this information is often relevant and so is pertinent to display and preserve (which is different from If you're printing with a limited number of digits, then today this functions (in both F# and the BCL) by functionally rounding to that many digits; thus if you print to 2 fractional digits then Edit: Noting that "rounding to that many digits" is meant to account for the exact underlying represented value, not strictly the literal the user visualizes, thus |
I should correct myself; I meant it's that
Again, let me refine that statement:
EDIT: That being said, given this comment:
If we were to decide in FSharp.Core to fix the bug by making let determineSign n = if n >= 0.0 then "+" else "-" to something like: let determineSign n nDecimalPlaces = if (roundTo n nDecimalPlaces) >= 0.0 then "+" else "-" My feeling was that there would still be room for some floating-point weirdness, but perhaps I was wrong; would that actually be a reasonable approach to take? |
Description
Fixes #15557 and #15558
Checklist
Test cases added
Release notes entry updated: