nunit/nunit.analyzers

Diagnostic suppressor for null references works only for open files

lennartb- opened this issue · 4 comments

Hi,

let me preface that I'm not sure whether this is an issue with Roslyn or the NUnit.Analyzers, I'd happily report this to the Roslyn repo if this isn't something that can be fixed here. I only discovered that our issue is even remotely related to the NUnit.Analyzers after finding dotnet/roslyn#68885 and that external analyzers could suppress warnings emitted by Roslyn, after tinkering for hours with various code style and nullability settings.

We recently changed a legacy test project that previously had NRTs disabled to enable them. We saw hundreds of "CS8602 - Dereference of a possibly null reference" warnings for objects that can be in fact null, but were checked with some variation of "Assert this is not null". Curiously, the warnings weren't reported at build time and disappeared after the file with the warning was opened.

Here's a repro, mostly based Roslyn issue above:
Project file:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
    <IsPackable>false</IsPackable>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <PropertyGroup>
    <AccelerateBuildsInVisualStudio>true</AccelerateBuildsInVisualStudio>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.11.1" />
    <PackageReference Include="NUnit" Version="4.2.2">
      <NoWarn>NU1608</NoWarn>
    </PackageReference>
    <PackageReference Include="NUnit.Analyzers" Version="4.3.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>
</Project>

Test:

using NUnit.Framework;

namespace NUnitNullabilityTest
{
    [TestFixture]
    public class Tests
    {
        private class Model
        {
            public string? Name { get; set; }

            public string? SomeMethod() => Name;
        }

        [Test]
        public void Model_SomeMethod()
        {
            var target = new Model { Name = "name" };

            var result = target.SomeMethod();

            Assert.That(result, Is.Not.Null);
            Assert.That(result.Length, Is.EqualTo(4)); // This is of course a nonsense assertion, but repros the issue
        }
    }
}

result can technically be null, so result.Length could theoretically throw an NRE. Since Is.Not.Null is asserted, we can be sure it's not null.

This produces the warning if the file is not opened:
image

An vanishes if the file is opened:
image

Has this something to do with Roslyn, or the NUnit analyzers?

I have seen it as well. It is a Visual Studio issue.
On one hand it tries to improve performance by not running analyzers on all builds.
The problem is that such a thing doesn't work with DiagnosticSuppressors. Those need running all the time to prevent errors.
But I haven't seem VS making such a distinction.

There are some setting you can change, see below:

One that might help is to untick: Skip analyzers for implicitly triggered builds

The Run background code analysis for can be set to Entire solution instead of just Open documents. This however for large solutions make the initial load slow as VS analyzes everything. Not useful if editing 1 of XXX projects.

The last setting Show compiler error and warnings for can limit this to only open documents.
The downside is that you don't see errors in files that you don't have open.

image

Unticking the Skip analyzers for implicitly triggered builds seems to also be MS's answer

Thanks, then I'll report it to Roslyn as well, maybe this is something they can check on.

Background analysis settings don't seem to affect this, I've iterated through all settings and it's displayed in all cases. Show compiler errors and warnings for only open documents (or the Intellisense setting to show only build errors) would probably work, but I honestly like to see as many (correct) warnings as possible, since we have a pretty decently tweaked analyzer configuration running in our projects.

I'll try the Skip analyzers for implicitly triggered builds option though, thanks.

Closing this then, since it's not actionable here.

Apparently, in VS 17.12 DiagnoticSuppressor are broken completely, and Skip analyzers for implicitly triggered builds option does not help for me.

I've logged an issue:
https://developercommunity.visualstudio.com/t/DiagnosticSuppressor-do-not-work-in-171/10788940