csharpsquid:S2583: over-aggressive check and false-positives

sonarlint
csharp
(Melissa Benua) #1
  • Reported using C# plugin version 7.5 (build 6605) in SonarQube 7.3

I believe the rule csharpsquid:S2583 is overaggressively flagging a specific scenario as false-positive, around null-or-empty checking for strings. The standard to verify a string ‘has content’ in C# is the built-in function String.IsNullOrWhitespace(). However, there is often a valid reason for having previously nullchecked a string - for example, determining whether to throw an ArgumentNullException or an ArgumentException at the beginning of the function.

However, the rule flags this as having ‘unused’ code, even though realistically speaking there is no other pattern to follow. There is no built-in function to check if a string is just whitespace or just empty without doing the bundled nullcheck.

public void MethodFoo(String someInput)
{
  if (someInput == null)
       throw new ArgumentNullException(nameof(someInput));

   if (String.IsNullOrWhitespace(someInput))
        someInput = "default";

   .....
}
(Valeri Hristov) #2

Hi @Melissa_Benua, I completely agree with your analysis and we really want to improve this behavior. The problem is that the dataflow engine (e.g. symbolic execution) we are using to detect these issues has to do some simplifications of the analysis, and one of them is that it does not understand strings. In other words, for this and several other rules based on the same engine a string could be null and not null. The empty string is just “not null” and this leads to the false positive you are seeing.

We already have an open issue that is related to this problem, but unfortunately I am still unable to tell when we are going to address it: