SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
796 stars 228 forks source link

Improve nullable reference type handling with NUnit [SetUp] #9522

Closed sbergen closed 4 months ago

sbergen commented 4 months ago

Issue description

With nullable reference types enabled, a CS8618 Non-nullable field must contain a non-null value when exiting constructor warning is (expectedly) emitted with the following code (using NUnit):

class MyTest
{
    private object field;

    [SetUp]
    public void SetUp()
    {
        this.field = new();
    }
}

This warning is suppressed by some other tools (namely Rider), as it's not really useful. It would be nice if Sonar would also suppress it.

Context

While this is not really a false positive, e.g. Rider contains logic to suppress the warning, when the field is assigned in a method with the SetUp or OneTimeSetUp attribute, and having a disparity between IDE and Sonar behaviour is somewhat problematic.

Also, I am aware that using FixtureLifeCycleAttribute with LifeCycle.InstancePerTestCase works much nicer with NRT enabled, but since we are working in Unity, which is stuck with an ancient version of NUnit, we can not use this.

martin-strecker-sonarsource commented 4 months ago

Hello @sbergen,

CS8618 is a warning from the csharp compiler, and we don't have a mechanism to suppress these warnings. Microsoft describes a couple of possible solutions to get rid of the warning in this article. The canonical way in a case like yours is to use a field initializer with a null-forgiving operator:

class MyTest
{
    private object field = default!;

    [SetUp]
    public void SetUp()
    {
        this.field = new();
    }
}

This pattern is also used for entity classes which face the same problem.

sbergen commented 4 months ago

Thanks for detailed answer! However, how to get rid of the warning was not really my issue:

having a disparity between IDE and Sonar behaviour is somewhat problematic

The issue I was trying to outline is delayed feedback in the environment I work in: Rider suppresses this warning in this specific context, so I'm only guaranteed to see the warning once my changes hit CI and get inspected by Sonar. (The warning might pop up in Unity also, but it's easy to miss, as it may get cleared on a following compilation.)

If this is still a "won't do", I won't push any further - I know I'm working in a very specific environment. Just wanted to make sure we are on the same page here.

martin-strecker-sonarsource commented 4 months ago

The Roslyn analyzer APIs do not support suppressing issues raised by other analyzers (in your case, Roslyn itself). Tools like Rider or our IDE plugin SonarLint can filter out such warnings on an IDE-to-IDE basis. Technically, there is nothing we can do here because the issue flow is like this:

The analyzer developed in this Repo is run along with the Roslyn compilation and can raise additional issues. It can not suppress any issues reported by another DiagnosticAnalyzer, like the one that raises CS8618. Therefore, it is reported to the server, and we do not have the logic to filter out any issues in the report.

I see how this is cumbersome for you, but I do not see any option for fixing it.