Roslyn: Covariant return types with respect to nullable reference types

[sharwell]

Version Used: Preview 11152017

Steps to Reproduce:

abstract class A {
  public abstract object? GetData();
}

class B : A {
  public override object GetData() => new object();
}

Expected Behavior:

No warning is reported.

Actual Behavior:

CS8609 is reported for B.GetData().

---

Update (jcouv, 3/29/2019): as part of this, if I do b.GetData().ToString(), there should be no warning. Confirmed with Mads.

[yaakov-h]

Did you mean abstract class A and class B : A?

[sharwell]

@yaakov-h Yes 😊

[jcouv]

Per last LDM discussion, overridden members must match in nullability, except when oblivious is concerned.
https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-09-10.md#inheritance-and-explicit-interface-implementation

I'll go ahead and close this issue.
Tagging @AlekseyTs FYI

[sharwell]

@jcouv The linked discussion appear to cover a different topic than this bug report. For the case of generic arguments, this issue would be loosely related to generic parameters with the out modifier, such as interface I1<out T>, but not related at all to cases where the out modifier isn't used.

[jcouv]

The relevant line:

If the member and the implementing type share the same NonNullTypes context, the annotations must match [...]

[jaredpar]

Closing as this behavior is by design as noted by @jcouv

[sharwell]

@jaredpar This situation is increasingly problematic (so far proportional to the amount of code that uses #nullable enable). I have some object graphs where a smal number of implementations of an interface member return null, but the majority do not by contract of the implementation. I need to be able to mark these implementations as non-null, even though the base type or interface allows a different implementation to return null.

It's like dotnet/csharplang#49, except it doesn't require special handling in the CLR and only limited changes in the compiler.

[sharwell]

A suitable workaround would involve splitting CS8608 and CS8609 into two warnings each: one for covariant types and a second for other cases. This would allow the warning for the safe usage case to be disabled.

[AlekseyTs]

@sharwell

I will add this issue to the list of design issues to discuss at LDM as soon as possible.

[sharwell]

@AlekseyTs Thanks! Note that so far I hit this issue (return values / property types) much more frequently than #30958, so while both are sound in the type system, if I only got one I would prefer this one.

[jcouv]

Discussed in LDM today. Decided to support this scenario. See upcoming notes.

```C#
abstract class A {
public abstract object? GetData();
}

class B : A {
public override object GetData() => new object(); // ok
}

class C : B
{
public override object? GetData() => null; // should warn because could result in an NRE for a consumer of B.GetData
}
```

[buyaa-n]

I am having similar issue with properties, when property getter decalred as nullable in parent class, subclass cannot overwrite its nullablity:

abstract class A {
  public abstract string? Name { get; }
}

class B : A {
  public override string Name     // warning here, even it should be OK
  {
      get
      {
          if (m_name == null)
              m_name = string.Empty;
          return m_name;
      }
   }
}

[stephentoub]

Discussed in LDM today. Decided to support this scenario. See upcoming notes.

@jcouv, I assume this will also include interfaces? e.g. today this currently warns:
```C#
public interface IFoo
{
object? Value { get; }
}

public class Foo : IFoo
{
public object Value { get; } // WARNS CS8612 Nullability of reference types in type doesn't match implicitly implemented member
}
```
but we should remove that warning.

[jcouv]

@stephentoub Yes, that scenario should also work. It should be covered by @agocke's PR. We'll make sure that it's validated.

[stephentoub]

Terrific. Thanks.