Constant evaluation related to Float-> Int conversion are somehow broken in Roslyn

[deniszykov]

Description

Prior to unknown version of CS compiler this expression evaluates to 24:

const byte myConst = unchecked((byte)(float)-1000);
> myConst 
0

Now it evaluates to 0.

Mono/old .NET Framework compiler evaluates it to 24.
If you do this conversion in runtime like this:

var x = (float)-1000;
> (byte)x
24

You get a right result. So something is broken in constant evaluation for Roslyn compiler.

Configuration

• .NET Framework version: 4.0.30319.42000
• dotnet version -> SDK Version: 5.0.302
• csc -version -> 3.10.0-4.21329.37 (246ce64)
• Windows 10 x64

Regression?

Yes, but I don't know which version did broke it.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[SingleAccretion]

I suspect this is expected behavior.

Cast from -1000f to byte is an overflowing one, and results of such casts are undefined, both per the runtime and the language specification.

Quite a while ago, Roslyn was changed to always return 0 for such casts, to aid in build reproducibility (x86/x64/ARM/ARM64 instructions that were used for these casts can and did return different values for the same input). Fundamentally, we cannot make the compiler and the runtime agree on the results without inserting range checks for every float/double -> integer cast (see also dotnet/runtime#47478).

I can also say that the results you see for this cast at runtime is because of the fact that the Jit compiler decomposes CAST(byte <- float) to CAST(byte <- CAST(int <- float)). It makes the result "defined" for a certain range of values, but you should not rely on that.

[deniszykov]

Quite a while ago, Roslyn was changed to always return 0 for such casts, to aid in build reproducibility (x86/x64/ARM/ARM64 instructions that were used for these casts can and did return different values for the same input).

I understand that undefined behavior will give different results on different platforms. Anyway, I expected that no one would break the legacy behavior of compiler. At least I expected a compiller warning in this case.

In my case only tests have broken, someone might have a working code broken.

[CyrusNajmabadi]

At least I expected a compiller warning in this case.

We could consider this as part of a warning wave. What do you think @jaredpar ? We were willing to change behavior (Which caused a runtime break), so i think adding a source time warning seems fine.

[tannergooding]

Just to clarify, the behavior was already "broken". If you ran the compiler on a given platform, the result was dependent on what the underlying hardware did which could (and did) differ between x64 and Arm64. It might also have already differed between x86/x64 using SSE2 and the legacy x87 FPU unit or other scenarios.

[jaredpar]

We could consider this as part of a warning wave. What do you think @jaredpar ?

My instinct is no here for a couple of reasons. As @tannergooding points out this was always a broken scenario and didn't have deterministic output. The other reason is that we've actually changed floating point behavior a number of times (at both the compiler and runtime level) over the lifetime of Roslyn. In this one particular case yes we could issue a warning wave but in the general case we cannot. This particular change doesn't feel compelling enough to be the one case we add a warning wave for.

[tmat]

It doesn't seem like this is specific to Expression Evaluation, but also occurs when compiling code in cs file. Moving to the compiler.

[CyrusNajmabadi]

In this one particular case yes we could issue a warning wave but in the general case we cannot.

I guess i'm not asking about the general case :) I'm asking about this specific case. If the user is generally just going to be broken in some fashion (i mean, we're detecting this and clearly just collapsing to 0), we might as well warn right? Or are you saying that this shouldn't even be a warning wave, it should be a pure warning?

[tannergooding]

The compiler would only be able to detect the constant case and there are other "issues" with floating-point that are likely more prominent.

For example, converting int->float is potentially lossy for anything over 2^24 and the same for long->double for anything over 2^52.

Likewise, conversion of float->IntegerType or double->IntegerType is undefined for anything greater than IntegerType.MaxValue or less than IntegerType.MinValue (which is the more general version of the original post).

Users also frequently hit issues with doing things like float f = -0, which is not the same as float f = -0.0f and assuming that float f = 3.14f will be exactly 3.14f (its actually 3.1400001049041748046875).

There isn't really an action the user can take either, outside maybe putting it in unchecked(), which functionally does nothing since checked/unchecked doesn't impact floating-point operations (C#/.NET don't expose the concept of floating-point exceptions so you don't get flagged for division by zero, inexact result, overflow/underflow, etc). So it would just be a "hey, be aware this behavior isn't well-defined" type scenario and only for constant inputs.

[CyrusNajmabadi]

hey, be aware this behavior isn't well-defined" type scenario and only for constant inputs.

That already seems helpful :-)