Roslyn-analyzers: CA1062 false positive with C# 8.0 nullable reference types

[SixFive7]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.5-beta1.final (Latest)

Diagnostic ID

CA1062

Repro steps - Code

using System;

namespace CA1062
{
    public static class Program
    {
        [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1062:Validate arguments of public methods", Justification = "ToDo:https://github.com/dotnet/roslyn-analyzers/issues/2578#issuecomment-502883836")]
        public static Guid FromString(string myString)
        {            
            return new Guid(myString.PadLeft(32, '0'));
        }
    }
}

Repro steps - Project.csproj

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

  <PropertyGroup>
    <TargetFramework>netstandard2.1</TargetFramework>
    <RootNamespace>CA1062</RootNamespace>
  </PropertyGroup>

  <PropertyGroup>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.9.5-beta1.final">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

</Project>

Expected behavior

No warning as the C# 8.0 "nullable reference types" support has already guaranteed the value to not be null.

Actual behavior

CA1062 warning, requiring redundant checks.

[mavasani]

You need to explicitly turn off CA1062 when you enable nullable references. It currently does not support analysis in presence of that new language feature. We would likely just bail out in such case as there is no reason for duplicate nullable analysis.

[333fred]

I will also point out that it's usually a good idea to validate the inputs of public functions, as they could be coming from clients that haven't enabled nullable and aren't aware they shouldn't be passing null. I think CA1062 is still useful with NRT, and is part of good defensive coding techniques.

[mavasani]

Ah, yes @333fred is correct. I am going to mark this issue as by design and open a separate work item for the flow analysis in the repo to understand the new nullable attributes and annotations to reduce false positives.

[mavasani]

Filed https://github.com/dotnet/roslyn-analyzers/issues/2877