Roslyn: Should nullability attributes affect method bodies and OHI?

[jcouv]

Status:

• [x] unconditional attributes (PR https://github.com/dotnet/roslyn/pull/40789, 16.5p3)
• [x] Not/MaybeNullWhen, DoesNotReturn/If (PR https://github.com/dotnet/roslyn/pull/41098, 16.5p3)
• [x] warn on overrides/implementation (PR https://github.com/dotnet/roslyn/pull/41336, 16.6p1)

---

Also, should they affect OHI (override/hiding/implementation)?

Relates to https://github.com/dotnet/roslyn/issues/35816
Relates to https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md#discussion

```C#
// should this warn?
static void F1(T t, [NotNull]out T t2) { t2 = t; }

```C#
    // should this warn?
    static void F1<T>([AllowNull]T t, out T t2) where T : class { t2 = t; }

```C#
virtual void F1(T t) { } // on base
override void F1([DisallowNull]T t) { } // on derived. Should this warn?

Stephen pointed out a scenario they hit:
```C#
internal class Foo<T> where T : class
{
    public bool TryGetValue2([MaybeNullWhen(false)] out T value) => 
        TryGetValue1(out value); // currently warns, but shouldn't

    public bool TryGetValue1([MaybeNullWhen(false)] out T value)
    {
        value = default!;
        return false;
    }
}

[jcouv]

Relates to https://github.com/dotnet/roslyn/issues/30953 ([MaybeNull]: Cannot implement a generic method with a "default" return value with nullable references)

[cartermp]

Tracking my answer for posterity: Yes, they should warn.

[jnm2]

@RikkiGibson suggested that I add my concern about the lack of equivalence of [NotNullWhen(true)]/[MaybeNullWhen(false)] on reference types in delegate conversions.

Let's say I write a nongeneric public API with [NotNullWhen(true)] out string?. Should I preemptively make it [MaybeNullWhen(false)] out string instead, just in case someone wants to pass it to some generic API from another library with an out T parameter?

What's driving the signature is no longer anything intrinsic to the method. Now it's constrained by how it might be used.

Real-world examples and flaws with each alternative are shown in depth at https://github.com/dotnet/roslyn/issues/38191.

[sharwell]

Tracking my answer for posterity: Yes, they should warn.

Agreed, although it wouldn't be terrible if the warning was produced by an analyzer.

[RikkiGibson]

Late response, but I think this needs to happen in the compiler's nullability analysis, as the analysis of the attributes may result in removing warnings as well as adding them.

[jcouv]

All three components of this issue are done (two were in 16.5 and one is in 16.6p1).
This was validated against the runtime repo to confirm the impact is expected and useful (PR https://github.com/dotnet/runtime/pull/32090)
I'll go ahead and close this issue now.