dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.97k stars 4.03k forks source link

Analyzers - Nullable incorrectly flags null even if checked for non-null #56064

Open apsthisdev opened 3 years ago

apsthisdev commented 3 years ago

I create a simple project, and enabled the nullable

    <PropertyGroup>
        <TargetFrameworks>netcoreapp3.1;net5.0;net6.0</TargetFrameworks>
        <Nullable>enable</Nullable>
        <LangVersion>latest</LangVersion>
    </PropertyGroup>

Sample

Result.cs

public sealed class Result<T>
    {
        /// <summary>
        /// Indicates that there is an error.
        /// </summary>
        public bool IsError
        {
            get
            {
                // EXPLICIT NULL CHECK DONE HERE
                return Error != null;
            }
        }

        /// <summary>
        /// The error in the result.
        /// </summary>
        public ImlxError? Error { get; set; }
    }

Usage code...

Result<BlahClass> result = blahService.GetResult();
if(result.IsError)
{
     // THE WARNING is shown here  'Error may be null here'
     _logger.LogError(result.Error.Code, result.Error.Description);
}

This is incorrect, the Error is never null here because the IsError accessor already checks for null and the code would not do inside the if condition.

This is the simplistic use case and the VS analyzer flags these kinds of incorrect null warning all over the place in the code. This is very irritating and non productive to even try and investigate the cause.

C# Class Libraries, Blazor Projects, ASP.NET Core Projects VS 2019, 2022

BrennanConroy commented 3 years ago

You need to add nullable attributes to your code in order for the analyzer to know that Error can't be null after the IsError check See this example https://github.com/dotnet/aspnetcore/blob/8b30d862de6c9146f466061d51aa3f1414ee2337/src/Mvc/Mvc.Razor/src/RazorPageFactoryResult.cs#L43

You can read about the attributes at https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis

apsthisdev commented 3 years ago

@BrennanConroy, you closed the issue already but how does MemberNotNullWhenAttribute solve the problem for class libraries targeting .NET Standard 2.0 and 2.1 ? Looks like MemberNotNullWhenAttribute is only available .NET5 + ?

Am I missing something here ?

jcouv commented 3 years ago

This is by-design. When looking at the call-site if(result.IsError) { ... } the compiler only sees the signature of IsError, not the body of the implementation. For this call to affect nullability, IsError needs to declare that it informs nullability. The MemberNotNullWhenAttribute is designed for this. In this case [MemberNotNullWhen(true, "Error")] will do it.

apsthisdev commented 3 years ago

@jcouv, thanks I understand the MemberNotNullWhen and that part is OK. But it is not available for .NET Standard 2.0 and 2.1. At least the docs says so. So how can .NET Standard class libraries use them to inform nullability to callers ?

jcouv commented 3 years ago

The nullability feature isn't supported on .NET Standard 2.0. I'd have to dig into the announcement posts to check which version of .NET it was initially supported for. That said, some people define their own MemberNotNullWhenAttribute type for such scenarios.