Roslyn fails to verify [NotNullWhen] contracts in methods returning Nullable<bool>

[TessenR]

Version Used:

Branch main (5 Nov 2021)
Latest commit 44f0576 by Joey Robichaud:
Revert to VS2019 for official build

Steps to Reproduce:

Compile and run the following code

#nullable enable
using System.Diagnostics.CodeAnalysis;

class C
{
  public static void Main()
  {
    if (M1(out var x) ?? false)
    {
      x.ToString();
    }
  }

  static bool? M1([NotNullWhen(true)] out string? x)
  {
    x = null;
    return true;
  }
}

Expected Behavior:
CS8762: Parameter 'x' must have a non-null value when exiting with 'true'. reported for return true; in M1

Actual Behavior:
No warnings at all in the program above. It crashes at runtime with a NullReferenceException

[RikkiGibson]

@jcouv did we inadvertently do the "call-site" side of this without doing the "declaration" side of it?

[jcouv]

Conditional nullable attributes are only expected to work on bool, not Nullable<bool>.
https://github.com/dotnet/csharplang/blob/main/meetings/2019/LDM-2019-05-15.md#conditional-postconditions

The bug here is actually on the call-site. We are not able to analyze the call-site because we only track one or two states (unconditional or conditional respectively), but Nullable<bool> involves more than two states...
The bug is likely in learnFromPostConditions which should check for bool type before splitting the state.

[TessenR]

@jcouv Does it mean that the following code should have a warning because the expression dictionary?.TryGetValue("", out var x) is of type Nullable<bool>?

#nullable enable
using System.Collections.Generic;

class C
{
  public void M(Dictionary<string, string>? dictionary)
  {
    if (dictionary?.TryGetValue("", out var x) ?? false)
    {
      x.ToString(); // currently no warning
    }
  }
}

[jcouv]

@TessenR No, that scenario will continue to work. TryGetValue returns a bool. We have a special treatment for this whole expression pattern (ie. x?.y ?? false) as part of improved definite assignment.

[sharwell]

It seems like one solution here is to report a diagnostic for use of NotNullWhen/MaybeNullWhen on non-bool-returning methods.

[RikkiGibson]

#36073 is an umbrella issue for reporting warnings on misuse of attributes based on the signature. We do already have some warnings on misuse in terms of what the implementation is doing, as @jaredpar has pointed out in email. For example, when the method has DoNotReturn, we warn if the implementation returns.