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

Ensure arguments for [MaybeNullWhen] parameters don't contribute top-level nullability to type argument inference #60726

Open RikkiGibson opened 2 years ago

RikkiGibson commented 2 years ago

Discussed in https://github.com/dotnet/roslyn/discussions/60519

Originally posted by **lorcanmooney** March 31, 2022 I wouldn't have expected the `out`-ness of a type parameter to affect nullable analysis, yet it seems to introduce a spurious warning in the code below. Invariant and `in` parameters don't trigger the warning. Am I missing something? ```csharp #nullable enable using System; using System.Diagnostics.CodeAnalysis; class Foo : IFoo { } interface IFoo { } // Remove 'out' and the CS8603 warning below goes away static class FooExt { public static bool TryGetFoo(this IFoo a, [MaybeNullWhen(false)] out T b) { throw new NotImplementedException(); } } class FooTest { string Test() { IFoo foo = new Foo(); if (foo.TryGetFoo(out string? f1)) { return f1; // CS8603: Possible null reference return. } throw new NotImplementedException(); } } ```

My thinking here is that the nullability of an argument for a 'MaybeNullWhen' parameter shouldn't contribute its nullability to type inference. If we ensure that, then we should get the desired behavior that the user has described here.

jcouv commented 2 years ago

My thinking here is that the nullability of an argument for a 'MaybeNullWhen' parameter shouldn't contribute its nullability to type inference.

LDM had decided that nullability attributes should not affect method type inference.

In the example provided above, would the following annotation be more suitable instead? public static bool TryGetFoo<T>(this IFoo<T> a, [NotNullWhen(true)] out T b)

RikkiGibson commented 2 years ago

would the following annotation be more suitable instead? public static bool TryGetFoo<T>(this IFoo<T> a, [NotNullWhen(true)] out T b)

I do not think so. This feels similar to the dictionary scenario, where a lookup failure means the output may be null even if T disallows null. A successful lookup is non-null depending on whether T allows null. If the standard Dictionary, IReadOnlyDictionary, etc. interfaces happened to be variant, we would have usability problems like what you see in this issue.

If you could share any relevant LDM notes, as well as any scenarios which were problematic when factoring attributes into type inference, that would help here.

It feels like the value which a method will assign into an out parameter never depends on whether the out parameter happens to be annotated as nullable. I wonder if there's some change to the rules we could make which reflects that and solves this scenario. Obviously we still have to do something reasonable for the void M<T>(out T value); scenario, though.