Sonar-dotnet: Fix S2583: FP when using `??` with an empty string

[ramondeklein]

Description

Sonarqube raises a false positive when using an ?? operation in combination with string.IsNullOrEmpty.

Repro steps

The following code raises a code-smell with the string.IsNullOrEmpty check, because it assumes the expression always true. When parameter is in fact an empty string (not null, but string.Empty), then the expression will be false.

public static string ShowCSharpSquidS2583(string parameter)
{
    var useParameter = parameter ?? "non-empty";

    // The following line shows an error that the condition is always true,
    // but if 'parameter' has value 'string.Empty' then it will be false.
    if (!string.IsNullOrEmpty(useParameter))
        return "non-empty";

    return "empty";
}

The same code-smell is given for string.IsNullOrWhiteSpace (which is a bit harder to prevent).

Expected behavior

No code-smell should be given.

Actual behavior

Code smell csharpsquid:S2583 is raised.

Known workarounds

In this case we could check if the string is not empty !useParameter.Equals(string.Empty).

Related information

• SonarC# Version 6.7.1 (build 35068)
• Visual Studio Version: Visual Studio 2017 (15.6.6)

[valhristov]

S2583, and in fact all of our dataflow analysis rules, do not "understand" values outside of: null, non-null, true, false and a few other special types, but not string. That's why the few hardcoded functions, such as IsNullOrEmpty and IsNullOrWhitespace behave as IsNull...

My personal opinion is that since the null check is done twice (once in the coalesce operator and once in IsNullOrEmpty), perhaps it is a bit better to either use !useParameter.Equals(string.Empty) as you suggest for workaround, or just remove the coalesce operator.

[ramondeklein]

It does seem that it has some intimate knowledge of the internals of string.IsNullOrEmpty and string.IsNullOrWhitespace, because it knows how these functions evaluate the incoming parameter. For the IsNullOrEmpty the workaround can be made (although it's a bit less readable, IMHO).

For string.IsNullOrWhitespace the workaround is even worse. You could write useParameter.TrimStart().Equals(string.Empty), but this creates a new string on the heap that needs to be collected. It is also pretty non-descriptive.

I agree that the null-check is done twice, so it would have been better to have use useParameter.IsEmpty or useParameter.IsWhitespace properties, but these are not available. That's why I fall-back to the static methods and take the 'penalty' of the additional null-check.

But my actual point is that the statement that the expression is always true, is not correct.

[valhristov]

I agree with you, the rule generates a false positive indeed.

[flcdrg]

Wouldn't it be more better to use userParameter.Length == 0 rather than comparing to an empty string?

[danielwagn3r]

I think it's nothing special about the ??operator, as the following examples shows:

public class Worker
{
   public string DoWork(string message)
   {
      if (message == null)
      {
         throw  new ArgumentNullException($"{nameof(message)} shouldn't be null!");
      }

      if (string.IsNullOrEmpty(message))
      {
         throw new ArgumentNullException($"{nameof(message)} shouldn't be empty!");
      }

   return String.Empty;
   }
}

Here SonarQube (7.1) flags a major bug at the string.IsNullOrEmpty.

[valhristov]

Thanks for the additional example @kwaxi. The problem is exactly the same and indeed it has nothing to do with the ?? operator. Since our analyzer assumes that string.IsNullOrEmpty works as string == null, the second if is redundant and it raises an issue.

[danielwagn3r]

@valhristov I already assumed this, I experimented a little bit and found out that other combinations of both checks with different order or nested conditionals also raise the same issue.

[MatthewLymer]

I have the same false positive with the following code

class MyClass
{
    private readonly IReadOnlyCollection<string> _collection;

    public MyClass(IReadOnlyCollection<string> collection)
    {
        _collection = collection;
    }

    public void MyMethod()
    {
        if (_collection?.Any() ?? false) // S2583
        {
            Console.WriteLine("Hello World");
        }
    }
}

[martinfletcher]

Any update on this?

[andrei-epure-sonarsource]

@martinfletcher until the end of the year, we plan to fix our CFG + Symbolic Execution debt (see tickets with the "Symbolic Execution" label)

[markusbrueckner]

Will the merged change also fix an issue of the form

var values = someObject as new Dictionary<string, object>;
if (values?.ContainsKey("someKey") ?? false)
...

This triggers S2583 in my code, but I'd rather not remove the null-coalesce here, as it does exactly, what I want and the alternative

if (values != null && values.Contains("someKey")) 

is exactly what the operator should replace

[andrei-epure-sonarsource]

yes @markusbrueckner this fix has been released in SonarCSharp 7.16

[markusbrueckner]

That's good to hear. Thanks for the extremly quick reply! 👍