False positive "Change this condition so that it does not always evaluate to 'false'; some subsequent code is never executed" from SonarC#

[domi...@gmail.com]

Hi,

I'm evaluating SonarQube for the company I work for. It looks very good but I found two families of false positive occurring frequently across the code I tested. I've been able to reproduce the first one on SonarCloud, see details below. I wonder if you can give me an idea of how long it might be before this is fixed or otherwise point out what I'm doing wrong which means we're getting this?

Setup

C#, Sonar Scanner for MSBuild 4.0.2.892, Running on Windows 10, using the SonarC# analyser on SonarCloud and also SonarQube 6.7.1 LTS + SonarC# 6.7 running on a Windows server.

In both cases the MSBuild was MSBuild 15 from Visual Studio 2017.

I ran exactly the commands SonarCloud suggested:

SonarQube.Scanner.MSBuild.exe begin /k:"SonarQubeTest" /o:"djn24-github" /d:sonar.host.url="https://sonarcloud.io" /d:sonar.login="<<my token>>"

"c:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\MSBuild.exe" /t:Rebuild SonarQubeTest\SonarQubeTest.sln

SonarQube.Scanner.MSBuild.exe end /d:sonar.login="<<my token>>"

Problem

This link should take you straight to it on SonarCloud: https://sonarcloud.io/project/issues?id=SonarQubeTest&open=AWF69CQUjU3w4cAur4Uq&resolved=false&types=BUG

Here's the code:

        public bool AreFullyQualified(string[] files)

        {

            bool anyPathRooted = false;

            bool allPathsRooted = true;

            foreach (var file in files)

            {

                if (Path.IsPathRooted(file))

                {

                    anyPathRooted = true;

                }

                else

                {

                    allPathsRooted = false;

                }

            }

            if (anyPathRooted && !allPathsRooted)

            {

                throw new InvalidOperationException("Paths must be all rooted or all unrooted");

            }

            return allPathsRooted;

        }

SonarC# reports "Change this condition so that it does not always evaluate to 'false'; some subsequent code is never executed" on "if (anyPathRooted && !allPathRooted)".

It is possible for any but not all paths to be rooted, if the IsPathRooted check passes for some but not all paths.

We found similar patterns in several places in the code which I think are all the same problem, this was just the easiest to extract from the code.

Thanks for your time,

Dominic

[valeri....@sonarsource.com]

Hi Dominic,

Thank you for the excellent bug report. You are right, the issue is a false positive, but unfortunately there is no workaround or easy fix. In two words, our symbolic execution engine (or dataflow analysis) is limited and cannot always provide 100% correct suggestions when loops are involved. I will try to elaborate a bit.

The dataflow analysis engine builds a control flow graph of the analyzed method and evaluates the possible values of the variables. If a variable is being known to have certain value (true, false, null, not null, etc.) the engine can evaluate the conditions in the code. This is how it knows whether a condition is always true or false.

When there are loops involved the things are a little more complex, because it cannot know how many times the loop should be "executed" (especially if it is foreach). That's why the engine tries to simplify the situation and "executes" the loop only once. In cases like yours, the single execution misses the fact that the two variables could be both changed and reports an issue.

What we could do is to execute the loop two times. This will work for simple cases, but if you have three variables, set in three different executions, there will be a false positive again and so on. Some language analyzers in SonarSource execute loops twice and have slightly better results than SonarC#, but so far we did not have the opportunity to improve in this area. Hopefully we will in the near future, but I cannot promise anything right now.

Unfortunately the only workarounds are to either check all issues and mark the false positives as such or disable the rule. However I think this rule is too valuable to be disabled...

I thought we already have a ticket for this limitation, but apparently it was only in our heads, so I created one:
https://github.com/SonarSource/sonar-csharp/issues/1160

Kind regards,
Valeri