Runtime: A number of C# and VB compiler unit-tests utilizing decimal numbers fail due to a baseline difference when run against netcoreapp3.0

[AlekseyTs]

The baseline difference netcoreapp3.0 vs. Desktop and netcoreapp2.1

Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenIncrementTests.TestIncrementDecimal
http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Emit.UnitTests/CodeGen/CodeGenIncrementTests.cs,c05c79e995b122ba
[FAIL]
Roslyn.Test.Utilities.ExecutionException :
Execution failed for assembly '8f845bda-f520-4b93-ae9b-2a5ea14ea9c4, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
Expected:
0
-1
0
0
-1
0
-1
-1

  Actual:   -0
  -1
  -0
  -0
  -1
  0
  -1
  -1

Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenTests.DecimalBinaryOp_03
http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Emit.UnitTests/CodeGen/CodeGenTests.cs,95927685b4e96faf
[FAIL]
Roslyn.Test.Utilities.ExecutionException :
Execution failed for assembly '0bfb3eec-e2cd-4f49-a134-c1491f92e239, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
Expected:
1007
993
7000
142.85714285714285714285714286
6
-993
-1007
-7000
-142.85714285714285714285714286
-6
123.0012300
122.9987700
0.15129000000000
100000
0.0000000
12345678900000000.000000001235
12345678899999999.999999998765
15241577.6390794200000000
10000000729000059778004901.796
0.000000000983
246913578.1246913578
-0.1000000000
15241578765584515.651425087878
0.9999999991899999933660999449
123456789.0123456789

  Actual:   1007
  993
  7000
  142.85714285714285714285714286
  6
  -993
  -1007
  -7000
  -142.85714285714285714285714286
  -6
  123.0012300
  122.9987700
  0.15129000000000
  100000
  0.0000000
  12345678900000000.000000001235
  12345678899999999.999999998765
  15241577.6390794200000000
  10000000729000059778004901.796
  0.0000000009832122
  246913578.1246913578
  -0.1000000000
  15241578765584515.651425087878
  0.9999999991899999933660999449
  123456789.0123456789

Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenTests.DecimalLiteral_BreakingChange
http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Emit.UnitTests/CodeGen/CodeGenTests.cs,8ea05a57d23cfb1e
[FAIL]
Roslyn.Test.Utilities.ExecutionException :
Execution failed for assembly '3d19ca2c-395d-4c27-bd9b-f1c2776b3bfa, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
Expected: 0.0000000000000000000000000031
0.0000000000000000000000000030

  0.0000000000000000000000000001
  0.0000000000000000000000000000

  -0.0000000000000000000000000001
  0.0000000000000000000000000000

  0.1000000000000000000000000000
  0.1000000000000000000000000000
  Actual:   0.0000000000000000000000000030
  0.0000000000000000000000000030

  0.0000000000000000000000000000
  0.0000000000000000000000000001

  -0.0000000000000000000000000000
  -0.0000000000000000000000000001

  0.1000000000000000000000000001
  0.1000000000000000000000000001

Microsoft.CodeAnalysis.VisualBasic.UnitTests.CodeGenTests.DecimalLiteral_BreakingChange
http://source.roslyn.io/#Microsoft.CodeAnalysis.VisualBasic.Emit.UnitTests/CodeGen/CodeGenTests.vb,1b37e58ca2b1545f
[FAIL]
Roslyn.Test.Utilities.ExecutionException :
Execution failed for assembly 'b0b8850b-47a3-4732-9040-1a32ec3d7050, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
Expected:
0.0000000000000000000000000031
0.0000000000000000000000000030

  0.0000000000000000000000000001
  0.0000000000000000000000000000

  -0.0000000000000000000000000001
  0.0000000000000000000000000000

  0.1000000000000000000000000000

  Actual:   0.0000000000000000000000000030
  0.0000000000000000000000000030

  0.0000000000000000000000000000
  0.0000000000000000000000000001

  -0.0000000000000000000000000000
  -0.0000000000000000000000000001

  0.1000000000000000000000000001

Microsoft.CodeAnalysis.VisualBasic.UnitTests.CodeGenTests.PreserveZeroDigitsInDecimal
http://source.roslyn.io/#Microsoft.CodeAnalysis.VisualBasic.Emit.UnitTests/CodeGen/CodeGenTests.vb,a61e33b5b8173b1d
[FAIL]
Roslyn.Test.Utilities.ExecutionException :
Execution failed for assembly '8c24ee50-03cd-4a41-b992-8b542bd91b71, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
Expected: 0.0000 0.0000000
Actual: 0.0000 -0.0000000

[stephentoub]

cc: @pentp, @tannergooding

[tannergooding]

CC. @jkotas as well.

[tannergooding]

Going through these now, will update the below as I do.

The first issue Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenIncrementTests.TestIncrementDecimal is because we now print the negative sign when formatting -0
The compiler is doing things like incrementing -1 which was already returning -0.0, so this isn't a break in functionality.

The second issue Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenTests.DecimalBinaryOp_03 looks to be that we produce a more correct result.
The test expects 0.000000000983, but we return 0.0000000009832122. The latter is a more exact result.

The third issue Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenTests.DecimalLiteral_BreakingChange and fourth issue Microsoft.CodeAnalysis.VisualBasic.UnitTests.CodeGenTests.DecimalLiteral_BreakingChange looks to be a possible bug. It looks like we aren't taking the final one into account when determining the rounding direction. However, both entries should be rounding up to 0.0000000000000000000000000031, and so the test baseline would still be out of sync.

The fifth issue Microsoft.CodeAnalysis.VisualBasic.UnitTests.CodeGenTests.PreserveZeroDigitsInDecimal is because we now print the negative sign when formatting -0.0000000D.
The compiler was already parsing -0.0000000D to -0.0, so this isn't a break in functionality.

[pentp]

Is it actually correct and/or beneficial to format/parse decimal 0 with a negative sign flag literally as -0? Decimal math makes no distinction between -0 and 0 for any operation.

[tannergooding]

Decimal math makes no distinction between -0 and 0 for any operation.

That isn't quite true, various operations (such as multiplying) do take the sign of both inputs. For example, multiply a positive number by -0.0 returns -0.0. I'm not sure if it is actually useful in practice for the System.Decimal type (there are well-known scenarios for double/float and the IEEE decimal types, however).

[pentp]

It's more of a coincidence that the sign bit is propagated for -0 results, I think we should revert -0 decimal formatting back to just "0".

[tannergooding]

I've finished updating the notes above.

Two of the issues are related to us now printing the negative sign for -0.
One of the issues is that we now return a more exact result when dividing (@pentp, could you take a closer look here? I believe this was the code you had been touching)
The final issue is that we don't seem to be taking a trailing digit into account when rounding and needs to be fixed.

[tannergooding]

It's more of a coincidence that the sign bit is propagated for -0 results, I think we should revert -0 decimal formatting back to just "0".

I'm not so sure. For the most part, it follows the standard arithmetic logic, but it also matches what float/double do for most of the edge cases around -0. For example, It also happens for -1m + 1 (which turns into -0).

From what I recall, I had asked about System.Decimal when making the formatting fixes for double/float (for the IEEE compliance) and it was recommended that it be fixed for System.Decimal as well. I'm trying to see if I can find those comments...
Edit: Looks to be here: https://github.com/dotnet/coreclr/pull/21036#discussion_r234052512

[pentp]

The second issue is a result of dotnet/coreclr#20305 which fixed several long standing bugs in decimal remainder calculation.

The problem with decimal -0 is that it's somewhat arbitrary (it used to have no practical meaning, the only observable difference was that GetBits returned a different value).

-1 + +1 = -0
+1 + -1 = 0
+1 - +1 = 0
-1 - -1 = -0

My concern here is that if some financial calculations or something similar start arbitrarily printing -0.00 instead of 0.00, for example when summing up items 1 + 9 - 10 = 0, but just by changing the order to 1 - 10 + 9 = -0.

[tannergooding]

The problem with decimal -0 is that it's somewhat arbitrary (it used to have no practical meaning, the only observable difference was that GetBits returned a different value).

Right, but this is similar to binary -0 (which also, until recently, was not detectable outside using BitConverter and producing +/- Infinity). In most mathematical applications, it has no purpose. However, there are some areas of mathematics where it is important and it is an exposed concept in the IEEE decimal types.

My concern here is that if some financial calculations or something similar start arbitrarily printing -0.00 instead of 0.00, for example when summing up items 1 + 9 - 10 = 0, but just by changing the order to 1 - 10 + 9 = -0

Most of the same concerns exist for double/float and would exist for the IEEE decimal types if they were ever exposed. This at least makes it consistent (which was the reasoning behind making the change in the first place). Not saying that we couldn't or shouldn't revert it just for decimal, but there is more to consider.

[pentp]

But for IEEE types, -1 + 1 = 0 (not -0). A quick reading from Wikipedia indicates that regular addition/subtraction usually does not result in -0, but the rules are complicated.

I know that in the previous place I worked at we used decimal math a lot (probably more than half of all functions used decimals). For utilities consumption calculation, prediction, invoicing, accounting, etc. I'm pretty sure that if -0.00 started appearing in that app, people would be quite confused.

[jkotas]

I would trust @pentp judgement on the decimal -0 and revert the change. My 2 cents...

[tannergooding]

But for IEEE types, -1 + 1 = 0 (not -0).

Might that be a case where it is worth fixing the decimal logic so that it only computes -0 in the same cases as the IEEE types? For IEEE types, it is always +0, unless the rounding direction is roundTowardNegative; the exception being x + x and x - (-x) which retains the sign of x, even for zero.

Otherwise, the fix should be relatively straight forward and should just involve us normalizing decimal in RoundNumber (when it is called as part of NumberToString, or possibly earlier as part of DecimalToNumber).

For reference, the rules of the sign bit are defined in 6.3 The Sign bit. The basic summary is:

• The sign of a product or quotient is the exclusive OR of the operands signs
• The sign of a sum or difference differs from at most one of the signs
• The sign of a conversion, quantize, and round to integral operations is the sign of the first or only operand
• The sum rule listed above (always +0, except for...)
• Some specific rules for certain operations (such as sqrt, copysign, negate, etc).

[pentp]

I don't think it's worth changing decimal calculation rules just to accommodate -0. It has no practical value in the kind of calculations done with decimals (non-scientific, no subnormals, no infinities, just regular integers with a decimal point in between).

[tannergooding]

I should have the PR fixing two of the issues shortly:

• -0 will format as 0 for System.Decimal
• TryNumberToDecimal will be updated so that it considers parsed digits that are below 2^-28 as part of the non-zero tail

After which, the compiler will likely need to update its baselines for the second issue (so that, on .NET Core, it expects the more correct division result) and the third issue (we now consider all non-zero trailing digits, and so both results will round).

@pentp, does this sound correct to you?

There is also an additional issue not called out above. As part of the previous work, -0 now parses as -0, where previously -0 would parse as 0 and only -0.0 (or with any number of additional trailing zeros) would parse as -0.0 (with the appropriate number of trailing zeros). The compiler, since it handles the negative sign itself, looks to be special casing this and still returns 0 when it sees -0.

Should the above behavior be reverted such that -0 once again parses to 0? It might be worth noting decimal d = 0; d = -d results in -0, so if reverted; there is an inconsistency here.

[juliusfriedman]

I don't see why a concerned party is concerned... what's alarming to me here is that rather than discussion to allow for the sign to be omitted with a format option we are considering reverting for applications which relied on a behavior which is arguably incorrect to be the norm.

I would much rather see this not reverted and deal with the formatting issues just as we are with double.

[tannergooding]

@juliusfriedman, there can be no apps reliant on the behavior.

In all .NET Framework versions and all released .NET Core versions (so 2.2 and prior), System.Decimal formats negative zero as 0.

For .NET Core 3.0 (which hasn't released yet), System.Decimal, System.Double, and System.Single were all updated to format negative zero as -0. This is just reverting that change for System.Decimal to continue being the same behavior it has always had. System.Double and System.Single will continue having the new behavior because it makes us inline with the IEEE 754 specification for these types.

While I do believe there are use-cases for a decimal-based floating-point type that properly exposes negative zero, it sounds like System.Decimal (which is not an IEEE 754 type) is not the right place to do that. Instead, there should likely be a separate proposal suggesting we expose the IEEE decimal32, decimal64, and decimal128 types (possibly under System.Numerics).

[pentp]

After some more careful reading of the IEEE rules for signed zero, it looks like the only applicable rule to System.Decimal might be that, x - x= - 0 when rounding towards negative, but the addition/subtraction operations have no rounding options in the API. So that leaves only the rules concerning operations with a -0, but that requires one to already somehow have a negative zero in the first place...

[juliusfriedman]

Hence my point..

The practical application should be applied in the application, hiding the behavior especially in a type which utilizes such precision has no applicable applications and furthermore reduces the domain of applicability of the aforementioned.

In short, Signaling a value came from the negative domain is the ONLY thing which separated not having that information and since such as already conceded does not typically occur nor does provide any change in the underlying mathematics then what harm can come from allowing it?

Versus the latter where one is restricted from using said value but actually only on during round trips via a string in a predetermined format...

It's not useful until it is and if we don't have it then it can't be useful.

[tannergooding]

We should likely move further discussions about System.Decimal and -0 to a new thread, since this issue isn't specifically about that.

As I indicated above, I do believe there are uses-cases for a decimal-based floating-point type that properly exposes negative zero. However, based on the discussion above, and some further thought on it, we are likely better off by keeping System.Decimal as is (and abstracting the -0 concept away) since the existing type isn't really used in the domains where negative zero is useful and because it doesn't (and can't) expose some of the other concepts that make it actually usable for those domains (such as NaN and Infinity). Exposing a new type (likely the IEEE decimal types) which do take these concepts into account (and also have other benefits, like supporting a larger range of finite numbers, while taking the same amount of space), is likely the right way forward.

[juliusfriedman]

I would in closing say that it's not in use because it's extremely cumbersome to do so for when one also has to take into account documented behaviors including round trip.

It's for the same reason then it would not have as much use in double.

The benefit in having the sign in the string is not outweighed by the difference between the binary representations of such or the figure wouldn't exist in the first place.

Having a format option seems much more reasonable to me due to the same reasoning it was adopted for double, to allow the application to choose.

The only other option is reverting and then still having no other alternative to take advantage of the actual representation of the figure let alone how one distingues the multiple representations of the figure in the same type yet does not respect any variation of such during round trip.

Since the parsing need be adjusted for the correct precision anyhow it seems that we cope with such now especially since the value will continue to exist and propagate through use within this type and other types in the framework.

[pentp]

Converting to float/double from decimal preserves the sign even for -0. Maybe that should be normalized also instead?

[tannergooding]

CC. @AlekseyTs, @jaredpar

The first and fifth issues should now be fixed (no change required by the compiler)

• We had changed negative zero to include the sign when formatting. It was determined this should be reverted.

The third and fourth issues should now be fixed (possible change required by the compiler)

• We had updated the parsing code to take all trailing digits into account when rounding. When parsing we would stop processing digits in the buffer after we had exceeded the maximum scale that decimal supported. This would cause values to be incorrectly rounded as digits still in the buffer were not being counted towards the non-zero tail check. The logic was updated to take any digits still in the buffer into account when being checked.
• Given that we now take all trailing digits into account when rounding, the compiler will likely need to update its baselines for .NET Core.

The second issue is "by design" and will require the C# compiler to update its baselines for .NET Core

• A bug was fixed to ensure that decimal division would now return the correct result

[danmosemsft]

@tannergooding can this be closed? Not sure who owns the next action here

[danmosemsft]

Ping @tannergooding

[tannergooding]

Nothing more for us to do here, as far as I am aware.

The compiler will need to update its baselines for the remaining issues I called out above.