S1940 (`no-inverted-boolean-check`) suggests breaking changes for floating point < <= > >=

Clean Code Report False-positive / False-negative...
1
  • What language is this for? JavaScript
  • Which rule? S1940
  • Why do you believe it’s a false-positive/false-negative? NaN < 0 and NaN >= 0 are both false, so suggesting to change !(x < 0) to x >= 0 may change the behaviour of the code if x is floating point (which is almost always the case in JavaScript). What’s more, many people do not know this detail of floating point in the IEEE 754 standard.
  • Are you using
    • SonarCloud? yes
  • How can we reproduce the problem?
function pauseIfAppropriate(video) {
    if (!(video.duration < Infinity)) {
        return false;
    }
    if (!video.paused) {
        video.pause();
    }
    return true;
}

notes

It looks like The inverted boolean-check introduces subtle mistakes · Issue #211 · SonarSource/eslint-plugin-sonarjs · GitHub deactivated this rule by default, but I still see S1940 in the Sonar Way in SonarCloud.

There is no issue with the ==, ===, != and !== operators. NaN is not equal to itself but the != and !== operators properly reflect that.

The issue might also occur in other languages, but not as commonly as in JavaScript and TypeScript which implicitly create NaN from converting a non-numeric string to a number and have floating point as the most common type of number.

There is also no issue if neither side can be a number or boolean, but in most cases it is not possible to be sure (!('' + x < 'A') would be such a rare case where it is possible).

The negation could be removed by adding a call to isNaN (or Number.isNaN with an explicit + conversion to number) to keep the behaviour the same, but that may not be an improvement.

3

Hello Jilles,

Thank you for the feedback. We have indeed a warning about not doing it in case we expect a NaN, but it’s only in the quickfix.
We should consider adding it to the rule description as well.

We could also suggest this rule only in case we have type information.

I will come back to you once we decide what the best course of action is.

5

We could also suggest this rule only in case we have type information.

This type information would have to be derived from a sound analysis different from what TypeScript does, since values at run-time need not comply to the TypeScript types. For example, indexing an array of strings with the default configuration of the TypeScript compiler results in the type string, while the run-time result will be undefined if the index is out of bounds. The < <= > >= operators perform a numeric comparison if one or both operands is undefined, converting the undefined to NaN, so the result will always be false.

Likewise, indexing an array of bigints out of bounds will result in undefined, and comparing a bigint to undefined for less/greater always returns false.

I just looked this up in the ECMAScript spec; it’s worse than I thought when I wrote the original post. Even comparisons between string and undefined behave weirdly.

An example of such a sound analysis is that '' + x is a string regardless of what x is.

6

I opened a ticket to improve the rule description: Jira