SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
756 stars 223 forks source link

Fix S1905 FP: Floating point casts are not redundant #9498

Open doterik opened 1 week ago

doterik commented 1 week ago

Description

I stumbled upon this somewhat dated blog post (by @jaredpar) https://blog.paranoidcoding.org/2014/12/22/redundant-cast.html.

This made me question whether S1905 correctly handles casting to float/double.

Repro steps

float x = GetValue();
float y = z / (float)(x * 2);

Expected behavior

It might be a FP, according to the article "Floating point casts are rarely redundant". Although there may have been changes since it was written, it could still be worth considering.

Actual behavior

S1905 Remove this unnecessary cast to 'float'.

Related information

jaredpar commented 1 week ago

The post is a bit old but the behavior it describes still exists. It's very challenging to identity truly redundant floating point casts. A safe, but very conservative approach, is only raising the warning when the RHS is a local, array element or field.

doterik commented 1 week ago

@jaredpar Thank you for your comment.

My main concern was about heeding the warning, removing the cast, and then not grasping the potential consequences, especially in unfamiliar code.

It seems this might only be relevant in rare and specific instances, so the warning could possibly stay, or, in the case of floating-point casts, be modified to a suggestion.

gregory-paidis-sonarsource commented 5 days ago

Hey @doterik , thanks for raising this. After reading Jared's post, I can confirm this is an FP. I added a reproducer and we will tackle this at some hardening effort in the near future.

Thanks a lot to both of you!

doterik commented 3 days ago

@gregory-paidis-sonarsource 👍🏼

SharpLab.io seems to agree...

https://gist.github.com/doterik/b529113a92e37e306805d1c2359bc7ee

image

Tim-Pohlmann commented 3 days ago

@jaredpar Fascinating post! Out of curiosity, do you have an example with actual numbers where removing the cast changes the result?

jaredpar commented 3 days ago

Consider this code

class Program
{
    static void Main()
    {
        Console.WriteLine(M1(float.Epsilon));
        Console.WriteLine(M2(float.Epsilon));
    }

    static bool M1(float f) => (f / 2.0f) == 0;
    static bool M2(float f) => (float)(f / 2.0f) == 0;
}

On .NET Framework x86 this will print

False
True

Note: this bug is harder to reproduce with float on .NET Core because the JIT uses native SSE/SSE2 instruction sets.