Roslyn: Missing nullability warnings for [MemberNotNullWhen] parameters

[TessenR]

Version Used:

Branch master (26 Sep 2020)
Latest commit 3e38d76 by CyrusNajmabadi:
Merge pull request #48002 from CyrusNajmabadi/implicitCreation

Use implicit object creation in the IDE layers.

Steps to Reproduce:

Compile and run the following code:

using System.Diagnostics.CodeAnalysis;

#nullable enable

class C
{
    public static void Main()
    {
        if (tryGetValue1(false, true, out var s1))
          s1.ToString(); // crash
    }

    static bool tryGetValue1(bool a, bool b, [MaybeNullWhen(false)] out string s1)
    {
        s1 = a ? "abc" : null;
        return b; // no warnings
    }
}

Expected Behavior:
Warning for return b; because s1 might be initialized to a nullable value

Actual Behavior:
No warnings. The program crashes at runtime with a NullReferenceException

Notes
Looks like Roslyn just ignores any returns of boolean variables but tries to verify e.g. returns of negated variables.
In the following code snippet all methods should have warnings but only the ones that return !a have them

using System.Diagnostics.CodeAnalysis;

#nullable enable

class C
{    
    static bool NullWhenFalseA(bool a, [MaybeNullWhen(false)] out string s1)
    {
        s1 = null;
        return a; // no warnings
    }

    static bool NullWhenFalseNotA(bool a, [MaybeNullWhen(false)] out string s1)
    {
        s1 = null;
        return !a; // warning
    }

    static bool NullWhenTrueA(bool a, [MaybeNullWhen(true)] out string s1)
    {
        s1 = null;
        return a; // no warnings
    }

    static bool NullWhenTrueNotA(bool a, [MaybeNullWhen(true)] out string s1)
    {
        s1 = null;
        return !a; // warning
    }
}

[jcouv]

Per LDM discussion on this around June, we want no warning in all those scenarios (returning a non-constant boolean). This was implemented in PR https://github.com/dotnet/roslyn/pull/46201.
Not sure why the negated boolean expressions are getting warnings. Will look into it. Thanks

[TessenR]

@jcouv Thanks for clarification.

Could you please also clarify what expressions should produce warnings?
I see a couple of scenarios where there's no warnings even though the expression is known to be constant e.g. there are no warnings in the following methods:

using System.Diagnostics.CodeAnalysis;

#nullable enable

class C
{    
    static bool M1([MaybeNullWhen(false)] out string s1)
    {
        const bool b = true;
        s1 = null;
        return b;
    }

    static bool M2([MaybeNullWhen(false)] out string s1)
    {
        s1 = null;
        return (bool) true;
    }
}

Is this intended? All these expressions can be used as constants so I expect the compiler to know their values and give warnings but it doesn't.

[TessenR]

Interesting that this M1 here doesn't have a warning while M2 does. Looks like a bug to me

using System.Diagnostics.CodeAnalysis;

#nullable enable

class C
{    
    static bool M1([MaybeNullWhen(false)] out string s1)
    {
        const bool b = true;
        s1 = null;
        return b; // no warning
    }

    static bool M2([MaybeNullWhen(false)] out string s1)
    {
        const bool b = true;
        s1 = null;
        return b && true; // warning
    }
}

[TessenR]

@jcouv Found another case that I'd consider a bug. Could you please clarify if the current behavior is expected?
Warning for return HasAnnotations(out _); but not for return NoAnnotations();

using System.Diagnostics.CodeAnalysis;

#nullable enable

namespace ConsoleApp
{
  internal static class Program
  {
    public static bool M1([MaybeNullWhen(true)] out string s)
    {
        s = null;
        return HasAnnotation(out _); // warning
    }

    public static bool M2([MaybeNullWhen(true)] out string s)
    {
        s = null;
        return NoAnnotations(); // no warnings
    }

    private static bool HasAnnotation([MaybeNullWhen(true)] out string s) { s = null; return true; }

    private static bool NoAnnotations() => true;
  }
}

[jcouv]

@TessenR Fix is in progress. I was relying on whether the analysis state was conditional, which is different from whether the returned value is constant.

[jcouv]

From discussion with LDM, we found a design that enables a few more useful warnings without introducing particularily annoying ones. We can report warnings on returns constants, but also on expressions where the analysis state is conditional and informs those particular parameters (or members).
For instance, return parameter == null; can be analyzed. But return M(); cannot.

[TessenR]

This looks like a brilliant solution, thanks a lot for fixing this!

[jcouv]

This looks like a brilliant solution, thanks a lot for fixing this!

I agree. Kudos to @RikkiGibson for coming up for that design :-)