csharp:S1244 for zero values

Rules and Languages Report False-positive / False-negative...
1

Hi,

I use SonarQube Community Build v25.5.0.107428 and I am getting lots of S1244 issues.

obrazek

I think comparisons with zero should be exempted from this rule. Zero can be represented exactly (as per IEEE 754) and it is the default value of a float or double value so this is perhaps a way to determine whether the variable has the default value.

I understand the current rule should stay as it is, but could a new rule be added which only marks comparisons with nonzero values as violations?

4

Hi @Pavel_Kotrc_EXT,

Thanks for the feedback. I agree that comparison against zero should be exempted from this rule.

It seems to me there are valid reasons to exclude == 0 and != 0 as well as default from the rule, e.g. to avoid a divide by zero.

I will add a repro to our codebase and a ticket to fix.

7

I’m not sure I agree with this change. Yes, 0 can be represented correctly, but there are other numbers too. And after some calculations that error could result in 0 not longer being zero, as explained nicely by 0.30000000000000004.com.

To me, the only valid exception should be comparing to default: stating: I want to know if the value has still the initial/default value. If I compare with 0.0, I except the outcome to be zero(-ish), and than == is not giving me what I might hope.

8

Hey @Corniel,

Thanks for the feedback. We are aware of this limitation and that it will result in False Negatives.

The problem is we can see from the data that developers compare against 0 to:

  • check to know if the field has been changed since initialisation.
    • these would be probably be better against default but are still correct against 0
  • prevent NaN/divide by zero.
    • these could be default but the intention is clearer with 0
  • after clamping the value
    • the value has been set to 0 these are correct as 0
  • default deserialisation
    • these would be better as default, but are correct as 0

We came to the decision that while we would miss a few FNs, but we would have significantly less FPs.

Does that sound reasonable?

9

The argumentation is solid. As you mentioned, there is no perfect solution here. I think I prefer my option, only ignoring the rule when comparing with default, but I can life with your choice too. My preference might be considered somehow opinionated, which is against your overall vision on the rules you provide.

Making it configurable to allow only default or zero as well might be an option, but you do not have many configurable rules, and that is - in my experience - also not always an ideal user experience.

10

One thing that I forgot the add: given how division by zero is handled by .NET, I would argue that you should check the following way:

var c = a / b;

if (!double.IsFinite(c))
{
   // handle division by null
}

The point is that dividing by zero will not throw, and dividing by something really close to zero could result in positive/negative infinity. Due to that, having it IsFinite() check actually is the best way to test for that, which is in line with the recommendation of the rule.

And to be clear: I did add this argument to try to convince you to change the advise/implementation of this rule, as your decision is good (too).