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
19.07k stars 4.04k forks source link

[NotNull] annotations are ignored for Nullable<T> fields/properties during nullable members' initialization checks #51366

Open TessenR opened 3 years ago

TessenR commented 3 years ago

Version Used:

Branch master (9 Feb 2021)
Latest commit 2d3ffe0 by msftbot[bot]:
Merge pull request #49990 from ryzngard/feature/namespace_analyzer

Add sync namespace analyzer

Steps to Reproduce:

Compile and run the following code

#nullable enable
using System.Diagnostics.CodeAnalysis;
public class C1
{
    [NotNull, DisallowNull] public int? Prop { get; set; }

    static void Main()
    {
        new C1().Prop.Value.ToString();
    }
}

Expected Behavior: CS8618: Non-nullable property 'Prop' must contain a non-null value when exiting constructor. Consider declaring the property as nullable.

Actual Behavior: No warnings at all. The program crashes with an InvalidOperationException: Nullable object must have a value.

Notes Note that Roslyn correctly reports warnings for fields and properties of reference and unconstrained generic types annotated with the [NotNull] attribute but fails to do so for Nullable<T>.

using System.Diagnostics.CodeAnalysis;
public class C1
{
    [NotNull] public string? x; // CS8618
    [NotNull] public int? y; // no warnings
}

public class C2
{
    [NotNull] public string? x;
    [NotNull] public int? y;

    public C2() { } // CS8618 reported for 'x' but not for 'y'
}

The same with properties instead of fields.

It looks like this check in NullableWalker shouldn't ignore Nullable<T>: https://github.com/dotnet/roslyn/blob/3ee6a8fea776de589329d6298df0dd80109bea3b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs#L554

RikkiGibson commented 3 years ago

The check needs to change to say "is it a non-nullable value type".