runtime: double.ToString("R") not working for certain number on x64
Extract this issue from https://github.com/dotnet/coreclr/issues/10651#issuecomment-315667429
Generally speaking, double.ToString("R") tries to:
- Convert the double to string in 15 digits precision.
- Convert the string back to double and compare to the original double. If they are the same, we return the converted string whose precision is 15.
- Otherwise, convert the double to string in 17 digits precision.
This introduced a bug and also listed as a known issue on MSDN (See “Notes to Callers”): https://stackoverflow.com/questions/24299692/why-is-a-round-trip-conversion-via-a-string-not-safe-for-a-double https://msdn.microsoft.com/en-us/library/kfsatb94(v=vs.110).aspx
To reproduce this issue, simplely try following code:
using System;
namespace lab
{
class Program
{
static void Main(string[] args)
{
double d1 = 0.84551240822557006;
string s = d1.ToString("R");
double d2 = double.Parse(s);
Console.WriteLine(d1 == d2);
}
}
}
The output is False which we expect True. The reason is d1.ToString(“R”) wrongly chose the result in precision of 15, which is 0.84551240822557. If we choose result in precision of 17, which is 0.84551240822557006, we can convert it back to double accurately.
The proposal is just convert the double to string in 17 digits precision directly. We can give it a try but it may be a breaking change with compat issue as @jkotas said here: https://github.com/dotnet/coreclr/issues/10651#issuecomment-315839827
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Reactions: 2
- Comments: 20 (19 by maintainers)
Now that dotnet/coreclr#22040 is merged, for .NET Core 3.0, ToString(), ToString(“G”), ToString(“R”), double.ToString(“G17”), and float.ToString(“G9”) should now always produce a string that will roundtrip to the original value…
I am closing this as https://github.com/dotnet/coreclr/issues/3313 is tracking the addition of an explicit testcase for
0.84551240822557006.Agree.
We add quirk switches to .NET Core re-actively, only once we get feedback that they are really needed for something important. We do not add them proactively like in .NET Framework just because of they might be needed in theory.
I think the general point is that serialization can occur anywhere, and incorrect serialization is bad.
We are already in a world where CoreFX is doing the wrong thing, which means we are broken for converting to/from anything that isn’t also doing the same broken behavior. As an example, serializing from C# and then deserializing that value in C++ (or vice versa) can wind up with different results (and may not be round-trippable).
My preference is to ensure we are IEEE compliant here, and actually round-trippable with anything else that is IEEE compliant (when using the “round-trippable” specifier).
I don’t think this would break Newtonsoft.Json, it would just change the value of serialized doubles. Of course that could break people who depend on the current serialized value.
Only action in Newtonsoft.Json would likely be fixing some unit tests where the serialized double value has changed.
Tagging all the people who were involved in https://github.com/dotnet/coreclr/pull/13140, plus a couple others. @eerhardt, @jkotas, @danmosemsft, @tarekgh, @AlexGhiondea, @gafter, @joperezr, @JamesNK, @terrajobst
As indicated by this issue, the current handling of “R” is incorrect and results in non-roundtrippable values for certain scenarios. The current handling is:
This also causes a fairly decent perf hit, as compared to just converting to G17 directly.
The people who are using “R” today, are likely doing so for serialization purposes (they want to be able to round-trip the result) and they would be broken for any of the values that don’t currently round-trip successfully. There are also likely some repos (like Newtonsoft.JSON, but possibly also CoreFX, CoreCLR, and Roslyn) which are reliant on the current behavior. Some examples would be in tests where they are directly comparing the “expected” vs “actual” string produced (rather than parsing the strings back to floating-point values and validating that those are equal).
My opinion is that we should take this breaking change, as it results in more correct/compliant behavior; However, I believe this is a large enough breaking change that we should also provide a “quirk” switch and ensure that the change is properly documented/announced to the public (with the reasoning of why the change is being made).
I believe for net core we can go ahead and take the breaking change there without any quirks. for the desktop, if we need to apply the fix there we have to introduce it with a quirk.
The second option listed by @tannergooding is possible too but I prefer to fix “R” to do the right thing as introducing a new format specifier will make “R” useless anyway.
CC @AlexGhiondea
IMO, it is worth doing one of the following: