Guard.IsNotNull doesn't prevent warning CA1062 on .NET Standard 2.0

[rdeago]

Describe the bug

TL;DR:

• Guard.IsNotNull<T> and Guard.IsNotNullOrEmpty need a ValidatedNotNull attribute, in addition to NotNull, on their first parameter.
• This will prevent Roslyn analyzers from issuing warning CA1062 on every call site, when building for .NET Standard 2.0.
• I'm willing to submit a PR to that purpose.

---

Background information

The first parameter of Guard.IsNotNull<T> and Guard.IsNotNullOrEmpty is decorated with NotNullAttribute, which tells Roslyn's nullability analysis that said parameter will be non-null when (if) the method returns.

For reference, the signature of Guard.IsNotNull<T>, as seen here, is as follows:

// Here it is ----------------------+
//                                  |
//                                  v
public static void IsNotNull<T>([NotNull] T? value, [CallerArgumentExpression(nameof(value))] string name = "")

.NET Standard 2.0 is the only platform targeted by CommunityToolkit.Diagnostics that predates nullable reference types, and therefore has neither NotNull, nor any other attribute used by nullability analysis (MaybeNullWhen etc.) When CommunityToolkit.Diagnostics is built for .NET Standard 2.0, the NotNull attribute is provided by @Sergio0694's PolySharp.

Nullability analysis recognizes attributes by full name (in this case System.Diagnostics.CodeAnalysis.NotNullAttribute), regardless of their visibility or the assembly containing them; for this reason, polyfills of nullability-related attributes are perfectly valid replacements for "the real thing" i.e. attributes provided by the .NET runtime.

...Or are they?

The problem

Even before nullable reference types were a thing, validating that a parameter is non-null was of course considered good practice. So much so that even FxCop, whose latest stable release dates back to 2010, had a rule that enforced null checking of reference-type parameters.

The authors of FxCop couldn't rely on System.Diagnostics.CodeAnalysis.NotNullAttribute (which would make its appearance years later) to tell whether a method checked a parameter for null, so they had an idea: you could write your own attribute, put it in any assembly, make it public or internal, give it any namespace, and as long as its name was ValidatedNotNullAttribute FxCop would take it as a guarantee that the parameter to which you applied it was non-null when (if) the decorated method returned.

With the advent of Roslyn, with its architecture supporting "pluggable" code analyzers, FxCop's most important rules were rewritten as Roslyn analyzers. By that time, a good number of projects had rolled each their own ValidatedNotNullAttribute, so there was no choice but to support it, with the exact same semantics, i.e. recognizing it by name.

After nullable reference types were introduced, people started using NotNullAttribute in their own validation methods, just the same way Community Toolkit does. This worked fine with Roslyn itself, but Roslyn Analyzers knew nothing about it and kept issuing warning CA1062 on the first use of a validated parameter, just as if no null-checking whatsoever had taken place.

This was reported in dotnet/roslyn-analyzers#4248 and supposedly corrected by dotnet/roslyn-analyzers#4249 in 2020. Unfortunately, though, the PR's author didn't follow the same semantics as Roslyn, namely recognizing the attribute by its full name; instead, he used a mechanism already well established in the roslyn-analyzers repo, which would look for a type and cache a reference to it to speed up further retrieval.

Said mechanism works well with nullability attributes provided by the .NET runtime, but fails to recognize a polyfilled NotNullAttribute (unless it is made public, which would start a whole universe of pain for different reasons). The bottom line is that the first use of an argument, which has previously been validated by calling Guard.IsNotNull<T> and/or Guard.IsNotNullOrEmpty, triggers warning CA1062 in situations where NotNullAttribute is a polyfill, namely when using CommunityToolkit.Diagnostics built for .NET Standard 2.0.

So, to recap:

• the .NET runtime team sort of messed up in the first place, by failing to provide a compatibility package (à la System.Memory) containing all nullability-related attributes, thus forcing everyone to either roll their own, or pick their choice of polyfill library.
  I'm not just talking about legacy projects here: analyzers and code generators, for instance, must be compiled for .NET Standard 2.0 to be able to work under both .NET (for actual builds) and Visual Studio (for real-time analysis, which uses .NET Framework).
• Roslyn analyzers messed up further, by failing to recognize polyfilled attributes;
• even if none of this is the Community Toolkit's fault, it's definitely too late to ask the runtime team for a compatibility package; as for Roslyn analyzers, even if the fix there would entail modifying a mere line of code and deleting two, a look at their issue history reveals that they're not exactly fans of touching the CA1062 analyzer.
• furthermore, I believe that not getting warning CA1062 after using the appropriate Guard methods is a reasonable expectation, even if not explicitly stated in the documentation.

The solution

Fortunately, there is a rather quick and painless solution: just add an internal ValidatedNotNullAttribute class and add ValidatedNotNull to parameters currently decorated with NotNull. The two attributes can happily coexist: Roslyn's nullability analysis will "see" NotNull, while the CA1062 analyzer will "see" ValidatedNotNull for the same purpose.

A minimal repro project

Create the following two files in an empty directory, on a machine where the .NET SDK (I used version 8.0.200) is installed and reachable via the dotnet command:

Repro.csproj

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

  <PropertyGroup>
    <OutputType>library</OutputType>
    <TargetFrameworks>netstandard2.0;netstandard2.1;net6.0</TargetFrameworks>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
    <AnalysisMode>AllEnabledByDefault</AnalysisMode>
    <AnalysisLevel>latest</AnalysisLevel>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="CommunityToolkit.Diagnostics" Version="8.2.2" />
  </ItemGroup>

</Project>

Foo.cs

using System;
using CommunityToolkit.Diagnostics;

namespace Repro;

public static class Foo
{
    public static void Bar(object obj)
    {
        Guard.IsNotNull(obj);
        Console.WriteLine(obj.GetType().Name);
    }
}

Notice how the target frameworks for this library are the same as those of CommunityToolkit.Diagnostics. Also note that all .NET analyzers are enabled (CA1062, as useful as it is, is disabled by default).

Now open a command prompt in the directory and type dotnet build. Verify that you get a CA1062 warning only for .NET Standard 2.0:

d:\Repro>dotnet build
MSBuild version 17.9.4+90725d08d for .NET
  Determining projects to restore...
  Restored d:\Repro\Repro.csproj (in 1,76 sec).
d:\Repro\Foo.cs(11,27): warning CA1062: In externally visible method 'void Foo.Bar(object obj)', validate parameter 'obj' is non-null before using it. If appropriate, throw an 'ArgumentNullException' when the argument is 'null'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1062) [d:\Repro\Repro.csproj::TargetFramework=netstandard2.0]
  Repro -> d:\Repro\bin\Debug\netstandard2.0\Repro.dll
  Repro -> d:\Repro\bin\Debug\netstandard2.1\Repro.dll
  Repro -> d:\Repro\bin\Debug\net6.0\Repro.dll

Build succeeded.

d:\Repro\Foo.cs(11,27): warning CA1062: In externally visible method 'void Foo.Bar(object obj)', validate parameter 'obj' is non-null before using it. If appropriate, throw an 'ArgumentNullException' when the argument is 'null'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1062) [d:\Repro\Repro.csproj::TargetFramework=netstandard2.0]
    1 Warning(s)
    0 Error(s)

Time Elapsed 00:00:03.77

d:\Repro>_

"Build succeeded", yes, but what if warnings were treated as errors, as is hardly unknown of in CI for example?

OK, now lets' demonstrate the solution. First, add the following file to the directory:

ValidatedNotNullAttribute.cs

using System;

namespace Repro;

[AttributeUsage(AttributeTargets.Parameter)]
internal sealed class ValidatedNotNullAttribute : Attribute
{
}

Now add a minimal Guard that uses the above attribute (without NotNull and CallerArgumentExpression, or we'd have to polyfill them for .NET Standard 2.0, and this is a minimal repro):

NewGuard.cs

using System;

namespace Repro;

internal static class NewGuard
{
    public static void IsNotNull<T>([ValidatedNotNull] T? value, string? name)
    {
        if (value is not null)
        {
            return;
        }

        throw new ArgumentNullException(name);
    }
}

Finally, modify the Foo.Bar method to use NewGuard. In Foo.cs, replace the following line:

Guard.IsNotNull(obj);

with this:

NewGuard.IsNotNull(obj, nameof(obj));

Run dotnet build again and voila:

d:\Repro>dotnet build
MSBuild version 17.9.4+90725d08d for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
  Repro -> d:\Repro\bin\Debug\netstandard2.0\Repro.dll
  Repro -> d:\Repro\bin\Debug\netstandard2.1\Repro.dll
  Repro -> d:\Repro\bin\Debug\net6.0\Repro.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.80

d:\Repro>_

The warning is gone.
Since nothing prevents us to use both the NotNull and ValidatedNotNull attributes on a parameter, I believe this demonstrates how Guard.IsNotNull<T> and Guard.IsNotNullOrEmpty can be made resilient to the idiosyncrasies of Roslyn analyzers, without breaking any existing code: namely, by adding ValidatedNotNull to parameters already decorated with NotNull.

Conclusion

A quirk in Roslyn analyzers leads to annoying warnings when using methods Guard.IsNotNull<T> and/or Guard.IsNotNullOrEmpty in both .NET Framework projects and .NET Standard 2.0 libraries.

Adding ValidatedNotNullAttribute to the Community Toolkit (as an internal class, of course – no external-facing changes necessary) can both remove the annoyance and potentially protect against similar issues in the future.

Regression

No response

Steps to reproduce

See above.

Expected behavior

See above.

Screenshots

No response

IDE and version

Other

IDE version

Happens with any IDE; even with dotnet CLI.

Nuget packages

• CommunityToolkit.Common
• CommunityToolkit.Diagnostics
• CommunityToolkit.HighPerformance
• CommunityToolkit.Mvvm (aka MVVM Toolkit)

Nuget package version(s)

8.2.2, but any version starting from at least 8.0.0-preview.1 will exhibit the same behavior.

Additional context

No response

Help us help you

Yes, I'd like to be assigned to work on this item