dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.17k stars 4.72k forks source link

NaN test fails on IL based serializer. #25561

Closed jiayi11 closed 4 years ago

jiayi11 commented 6 years ago

When a double or single property or field is attributed to say the default value is NaN, the IL and reflection based serializers still serialize the NaN value. It is expected that when a field or property has the same value as the attributed default value, that the value is not serialized.

This issue is caused by the fact that the expression double.NaN != double.NaN will return true. These serializers need to be modified to use the Equals method on double/single as that accounts for the NaN value.

tannergooding commented 6 years ago

@yujayee, for serialization, does the underlying bit representation of the given NaN matter (or is it at least explicitly documented not to)?

jiayi11 commented 6 years ago

@mconnew

mconnew commented 6 years ago

@tannergooding, I understand that there are many bit representations of NaN but the value NaN is treated more conceptually than as a literal single value in .Net. The change is to switch to using the Equals method which internally relies on double.IsNaN which accounts for the various possible representations.
I don't believe that we need to explicitly specify that the underlying bit representation doesn't matter because NaN is treated as concept throughout the framework. The documentation for double/single doesn't go into the bit level of implementation detail. For example double.IsNaN documentation says:

Returns a value that indicates whether the specified value is not a number (NaN).

Where NaN is linking to the documentation for NaN. In that documentation it says:

Represents a value that is not a number (NaN). This field is constant.

So here it's saying NaN is constant (single value) and that IsNaN evaluates whether a particular double is the NaN constant (implied by linking to the NaN constant documentation). There's some handwavyness around this. Basically, I don't think it's appropriate that the serializers document NaN behavior to greater detail than the double and single data types do. I also believe the planned behavior is what would be reasonably expected to happen when specifying NaN as the default value. There is no mechanism built in to .Net to differentiate between different NaN values, and additionally the XML spec simply says any not a number value should be expressed as that textual constant. If the XML spec ever gets updated to differentiate between different NaN values, e.g. to accomodate some of the new features in IEE 754-2008 such as qNaN, then we would still need .Net to be updated (and I suspect the actual specs would need to be revised) and at that point we would need to change behavior and would consider documenting about different NaN values. But until then, NaN conceptually represents a single idea undiscernible from any other instance of NaN.

jiayi11 commented 6 years ago

The test actually passed in reflection based serializer. Setting default value to NaN ends up calling the method IsDefaultValue(TypeMapping mapping, object o, object value, bool isNullable), it uses Equals() to compare the values which is totally correct. Only fix in IL serializer is needed.

jiayi11 commented 6 years ago

Close as won't fix for now.