False positive: CS8600 - Converting null literal or possible null value to non-nullable type

[peter-perot]

Version Used: VS 17.8.7 (Compiler Version 4.8.0-7.23572.1, Language Version 12.0)

Steps to Reproduce:

Try to compile the code below (see also sharplab).

#nullable enable

using System.Diagnostics.CodeAnalysis;

public interface IValueHolder<out T>
{
  public T? Value { get; }

  public bool HasValue { get; set; }
}

public class ValueHolder<T> : IValueHolder<T>
{
  public ValueHolder(T value)
  {
    this.Value = value;
    this.HasValue = true;
  }

  public ValueHolder()
  {
  }

  public T? Value { get; }

  public bool HasValue { get; set; }
}

public static class ValueHolderExtensions
{
  public static bool TryGetValue<T>(this IValueHolder<T> holder, [MaybeNullWhen(false)] out T value)
  {
    value = holder.HasValue ? holder.Value : default;
    return holder.HasValue;
  }
}

public static class Test
{
  public static void Run()
  {
    var holder = new ValueHolder<string>("foo");

    if (holder.TryGetValue(out string? value))
    {
      string copy = value; // !!!! WARNING !!!!
    }
  }
}

Diagnostic Id: CS8600

Expected Behavior:

Should compile without warning.

Actual Behavior:

Issues a warning.

Observation:

If you remove the covariance from the interface definition like

public interface IValueHolder<T>

..., the code compiles without warning.

[CyrusNajmabadi]

Why MaybeNullWhen(false) instead of NotNullWhen(true)

[peter-perot]

Why MaybeNullWhen(false) instead of NotNullWhen(true)

Since this is the way generic arguments are annotated, see here and search for "annotate generic out arguments".

You can only use NotNullWhen in case you have concrete types.

[chrisoverzero]

To me, this looks correct. The type parameter to IValueHolder<out T> is out in variance, so the extension method call to TryGetValue<T> is called as:

bool IValueHolder<string?>.TryGetValue<string?>(out string? value)

At this point, the note from a little later in the same bullet point of the linked nullability guidelines comes into play:

If the consumer defined the generic type as being nullable, then the attribute simply ends up being a nop, because a value of that generic type could always be null[.]

As evidence of this, the warning goes away (8.0.203!) if the call is made in one of these ways:

// here, `var` is inferred as `string?` due to the annotation
// but holder is still typed as `IValueHolder<string>`
if (holder.TryGetValue(out var value))
{
  string copy = value; // no warning
}

if (holder.TryGetValue<string>(out string? value))
{
  string copy = value; // no warning
}

…though IDE0001 kicks in for me for the type argument for the latter. Another option would be to implement TryGetValue as an instance method:

public bool TryGetValue([MaybeNullWhen(false)] out T value)
{
  value = HasValue ? Value : default;
  return HasValue;
}

This also produces no warning, but I don't know what business value IValueHolder<out T> is providing you, so I couldn't concretely say that this is a better solution. It couldn't be added to that interface because of the variance.

[peter-perot]

@chrisoverzero Yeah, thanks to your explanation regarding type inference, I think I found an explanation for the covariance thing and the different behaviors (covariance vs. no variance).

With covariance, the following assignment is valid:

IValueHolder<string?> left;
IValueHolder<string> right = new ValueHolder<string>("foo");
left = right;

With no variance, this assignment results into a compile time warning.

Therefore I think this is the reason why with covariance the following code

IValueHolder<string> holder = new ValueHolder<string>("foo");
holder.TryGetValue(out string? value); // (1)

is inferred as

IValueHolder<string> holder = new ValueHolder<string>("foo");
holder.TryGetValue<string?>(out string? value);

..., because an IValueHolder<string> value can be assigned to an IValueHolder<string?> value, and since we only provide the type of the out-var (string? at (1)), but not the type of the generic type parameter, the type inference algorithm goes for holder.TryGetValue<string?>, or in other words, the type of holder is temporarily cast to IValueHolder<string?>.

In contrast, when we do not use covariance the following code

IValueHolder<string> holder = new ValueHolder<string>("foo");
holder.TryGetValue(out string? value); // (1)

is inferred as

IValueHolder<string> holder = new ValueHolder<string>("foo");
holder.TryGetValue<string>(out string? value);

..., because an IValueHolder<string> value cannot be assigned to an IValueHolder<string?> value, and since we only provide the type of the out-var (string?), but not the type of the generic type parameter, the type inference algorithm realizes that it cannot go for holder.TryGetValue<string?>(as we have no variance here), so it chooses to infer holder.TryGetValue<string>.

If some one of the language designers can confirm my interpretation of the behavior, it's obviously not a bug, and in turn we could close this issue.

HINT

Of course I could use an instance method instead of an extension method, or I could use var instead of specifying the type explicitly, but my intention was to point out some peculiarity of the compiler we need to be aware of.

[peter-perot]

There is one more observation I made. The following code (as proposed by @chrisoverzero) seems to work correctly:

public static class ValueHolderExtensions
{
  public static bool TryGetValue<T>(this IValueHolder<T> holder, [NotNullWhen(true)] out T? value)
  {
    value = holder.HasValue ? holder.Value : default;
    return holder.HasValue;
  }
}

But this is against the guidelines describes here, so I wonder if the specification has changed in the meantime. IIRC writing T? was not possible in earlier versions, so the MaybeNullWhen annotation was introduced, but does this limitation still exist nowadays?

[CyrusNajmabadi]

Why MaybeNullWhen(false) instead of NotNullWhen(true)

Since this is the way generic arguments are annotated, see here and search for "annotate generic out arguments".

You can only use NotNullWhen in case you have concrete types.

You can use NotNullWhen with generics just fine :-)

Are you sure that doesn't fix things here?

[peter-perot]

Why MaybeNullWhen(false) instead of NotNullWhen(true)

Since this is the way generic arguments are annotated, see here and search for "annotate generic out arguments".
You can only use NotNullWhen in case you have concrete types.

You can use NotNullWhen with generics just fine :-)

Are you sure that doesn't fix things here?

Yeah, NotNullWhen seems to work with generics nowadays, and it even fixes the problem here. But I remember it didn't in the past. And Microsoft still uses MaybeNullWhen with generics, e.g. in class Dictionary<,>.

Can somebody from the language designers confirm that NotNullWhen is okay with generics now?

[peter-perot]

Interesting:

https://github.com/dotnet/runtime/blob/7ac5ef959579da0202e6ad618924dfc8fe129e89/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs#L569

https://github.com/dotnet/runtime/blob/7ac5ef959579da0202e6ad618924dfc8fe129e89/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IDictionary.cs#L46

Both attributes appear to be allowed with generics.

[CyrusNajmabadi]

Can somebody from the language designers

I am one of hte lang designers :D

[sharwell]

NotNullWhen is allowed but not valid here. IValueHolder<string?> is allowed to return null as a value, and that value fails to adhere to a NotNullWhen constraint. If you further have where T : notnull, it would become valid, but the original code does not constrain T in that manner.

Note that if you do have a notnull constraint, NotNullWhen(true) would be the preferred attribute here. MaybeNullWhen(false) is only required for correctness in cases like this involving unconstrained generics.

[peter-perot]

@sharwell I see, and now I understand why they used both attributes in ConditionalWeakTable. The fact that the behaviour appears to be a little "awkward" as soon as covariance comes into play, seems to be by design and can be overcome by using var instead of the concrete type.

[RikkiGibson]

I feel there is some bug here because:

1. you can remove the nullable warning by explicitly adding <string> type argument to TryGetValue
2. an info diagnostic for an unnecessary type argument is reported when you do this
3. we think of var as implicitly nullable-annotated, so it doesn't make sense to me that var is not contributing its nullability to type inference while string? is contributing its nullability.

Curious what you think @jcouv.

[jaredpar]

@jcouv?