Validation of not nullable reference type demanded (S3900)

Template for a good false-positive report, formatted with Markdown:

  • SonarQube 8.9 (build 43852)

  • SonarAnalyzer.CSharp8.32.0.39516

  • Visual Studio 2019.11.5

  • C# 8.0 and Nullable enable

  • minimal code sample to reproduce (with analysis parameter, and potential instructions to compile).

      public void Test(TestClass xx)
      {
          var to = xx.TestProperty;
      }
    

Visual Studio light bulb reports:

  • ‘xx’ is not null here
  • S3900: Refactor this method to add validation of parameter ‘xx’ before using it.

Obviously SonarScanner doesn’t recognize that the non-nullable reference type ‘xx’ cannot be null. xx?.TestProperty would be accepted.

The code smell is also reported in SonarQube, not only in Visual Studio.

Obviously SonarScanner doesn’t recognize that the non-nullable reference type ‘xx’ cannot be null. xx?.TestProperty would be accepted.

Where does it say that TestClass cannot be null? Reference types are nullable and there is no ! to indicate that it wont be.

var to = xx?.TestProperty would be null coalescing and is the same as doing the code below but with shorter syntax.

if (xx == null) 
{
    to = null;
}
else 
{
    to = xx.TestProperty;
}

Sonar isnt going to complain about the null coalescing because its not going to throw a NullReferenceException, which is what S3900 is supposed to be preventing.

This does I think (C# project setting):

I am seeing the same problem with C# 9 and nullable enabled.

@milbrandt - did you find a way to resolve this?

1 Like

@matt Unfortunately no solution. Even worse I get compareable messages even on methods where parameters are annotaed in .NET Standard 2.1:

public void Test([NotNull] TestClass xx)

Thank you @milbrandt , I’ve opened #5217.

On a different thread for S3900, I’ve explained in detail our plans with our Symbolic Execution (data flow analysis) rules - please read that to have more context on our plans to improve the situation.

Having this said, I’ll look into how we can reduce the noise of the current implementation until we finish the migration. When re-writing the rule on top of the Roslyn CFG, we’ll consider this scenario for sure.

Enjoy the Christmas holidays.

Best regards,
Andrei

Hello @milbrandt

We looked into this issue while migrating the S3900 to the new symbolic execution engine. We decided to keep the behavior of the rule as is for the moment. We will invest the time to create a second rule complementing S3900 to address your concerns. You can read more about the reasoning at the GitHub issue created by @Andrei_Epure: S3900 FP: with nullable context enabled · Issue #5217 · SonarSource/sonar-dotnet · GitHub
Please answer there or here if you have concerns about our proposed solution. Please let us know if our plan suits your needs.

Best, Martin