False positive in rule: is null on at least one execution path

false-positive
csharp

(Agustin Eloy Barrios) #1

Sonar Qube Version 7.2.1
this sentence is a valid and suggested, when we use recent versions of C#

  if ((keys?.Count ?? 0) > 0) {
    int min = Math.Min(chunkSize, keys.Count  // <----------
...
  } 

we check for Nulls on the if using the operator ?. and the ?? together .


(Valeri Hristov) #2

Hi Agustin,

You are right, when keys is null the result of the coalesce operator will be 0 and the if body will not be executed. The problem is that the SonarC# Data Flow Analysis (e.g. Symbolic Execution) does not “understand” numbers and cannot “calculate” the result of the coalesce operator when keys is null. What it sees is a null check and then regardless of the result the keys collection is dereferenced, hence it raises the issue.

I used to use this syntax a lot before, but now I find it more difficult to understand than the plain null check:

if (keys != null && keys.Count > 0)

It is not much more characters, but in my opinion is more straightforward to read from left to right.

If you like this syntax, the only option so far is to mark the issues as false positive or won’t fix in your SonarQube instance. If you use SonarLint for Visual Studio they will stop appearing there as well.


(zatko) #3

Hello,

I got similar warning for “if (codes?.ContainsKey(enterCode) ?? false)”. This operator (or both rather) is (are) really used (and useful) and when flow analysis cannot calculate value, I think it should not report this as “Change this condition so that it does not always evaluate to ‘false’; some subsequent code is never executed”.

Best regards
Josef


(Wouter) #4

I have a slightly different case:
using boolean like
if (Keys?.Exists ?? false)
{

}
This gives the the false positive: “Change this condition so that it does not always evaluate to ‘false’; some subsequent code is never executed.”
This is a much easier case to support than the above post, because it doesn’t require evaluating an expression and checking “value > 0”.

I understand that this could be rewritten using old-school null checks, but I would rather use the newer languages features and not have to write my code just to make the Sonar rules happy.

is there any chance this will be supported in the future?


(Valeri Hristov) #5

Hi @Wouter, we definitely want to support this syntax, but I cannot say when we are going to work on the dataflow analysis engine (e.g. symbolic execution). This is not a trivial change and would require significant amount of work and testing.


(Pavel Biryukov) #6

Hi! Same thing, false reporting:

if (context?.Request?.Headers?.TryGetValue(headerName, out values) ?? false)
{
///
}