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.

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

Parsing 0.0 (decimal) different in 13.0.2 #2768

Closed
marcovisserFurore opened this issue Dec 6, 2022 · 7 comments · Fixed by #2769
Closed

Parsing 0.0 (decimal) different in 13.0.2 #2768

marcovisserFurore opened this issue Dec 6, 2022 · 7 comments · Fixed by #2769

Comments

@marcovisserFurore
Copy link

After using Newtonsoft.Json version 13.0.2, I encountered an unwanted behavior.

Source/destination types

        [TestMethod]
        public void ParseJsonDecimal()
        {
            var json = @"{ ""property"": 0.0}";

            var reader = new JsonTextReader(new StringReader(json))
            {
                FloatParseHandling = FloatParseHandling.Decimal
            };

            while (reader.Read())
            {
                if (reader.TokenType == JsonToken.Float)
                {
                    if (reader.Value is decimal d)
                    {
                        d.ToString(NumberFormatInfo.InvariantInfo).Should().Be("0.0");

                    }
                    else throw new AssertFailedException("Value is not decimal");
                }
            }
        }

Expected behavior

I would expect that the decimal 0.0 contains still the trailing zero after parsing.

Actual behavior

Decimal looses precision after parsing. The trailing zero (0.0) is gone.

Steps to reproduce

See above unit test.

@elgonzo
Copy link

elgonzo commented Dec 6, 2022

Nice catch. The behavior seemed to have changed with 13.0.2.
In 13.0.1, the trailing zeroes are still preserved.

The cause of the issue is in commit 13717cf#diff-188cdcd35a8220c2477c864944a945fa67539c0beac51d3909424ff07dea7fe4 based on this pull request #2639

Specifically, the BoxedPrimitives.Get(decimal) method:

internal static object Get(decimal value) => value == decimal.Zero ? DecimalZero : value;
private static readonly object DecimalZero = decimal.Zero;

For any numerically zero value with trailing zeroes like 0.0, this method unfortunately returns decimal.zero which is just 0 without any trailing zeros.

@JamesNK
Copy link
Owner

JamesNK commented Dec 6, 2022

Should fix this.

Is there a way to detect whether a decimal has trailing zeroes without ToString? If there is then decimals with trailing zeroes can skip the cache. If there isn't then remove caching of decimals altogether.

@elgonzo
Copy link

elgonzo commented Dec 7, 2022

I know of two ways. However, both are available only beginning with .NET 7.

The following expressions yield false for a zero value with trailing decimal zeros, and true for a zero value without trailing decimal zeros (not sure which of these two would perform better, though):

Using decimal.IsCanonical(decimal): decimal.IsCanonical(value) && value == decimal.Zero

or using decimal.Scale: value.Scale == 0 && value == decimal.Zero

Dotnetfiddle showing both decimal.IsCanonical and decimal.Scale in action: https://dotnetfiddle.net/qa2XD1

@JamesNK
Copy link
Owner

JamesNK commented Dec 7, 2022

I think decimal.GetBits, then checking the flags value in the span, should work in .NET 6 - https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Decimal.cs,602

@JamesNK
Copy link
Owner

JamesNK commented Dec 7, 2022

PR: #2769

@jahmai-ca
Copy link

Is this related / will the PR address negative zero (-0.0) is now being (de)serialized as 0 instead of -0?

@JamesNK
Copy link
Owner

JamesNK commented Dec 15, 2022

Is this related / will the PR address negative zero (-0.0) is now being (de)serialized as 0 instead of -0?

It's related but existing PR doesn't fix it. New PR for negative doubles: #2777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants