Roslyn: Incorrect warning CS8762 generated when using `NotNullWhen(true)`.

[safern]

Version Used: Package Microsoft.Net.Compilers.Toolset Version - 3.6.0-2.20166.2.

Steps to Reproduce:

#nullable enable
using System;
using System.Diagnostics.CodeAnalysis;
public class C {
    public bool TryFoo(bool fail, [NotNullWhen(true)] out String? value) =>
        Foo(fail, out value) == Result.Success;

    public Result Foo(bool fail, out String? value)
    {
        Result result = Result.Fail;
        value = null;

        if (!fail)
        {
            value = "not null";
            result= Result.Success;
        }

        return result;
    }
}

public enum Result
{
    Fail,
    Success
}

Expected Behavior:

No warnings produced as [NotNullWhen(true)] out String? value is saying, value will not be null when I return true.

Actual Behavior: I'm getting a warning:

Program.cs(6,9): warning CS8762: Parameter 'value' may not have a null value when exiting with 'true'.

This is weird, as the warning is saying, value may not have have a null value when exiting with true and the attribute annotation is exactly saying that, "This will not be null when returning true".

cc: @cston @agocke @jcouv
FYI: @dotnet/nullablefc @eiriktsarpalis

[safern]

Another repro:

        internal static bool IsStringType(Type? type, [NotNullWhen(true)] out object? typeInformation)
        {
            bool bIsPrimitive = true;

            if (type == typeof(string))
            {
                 typeInformation = "String";
            }
            else
            {
                typeInformation = null;
                bIsPrimitive = false;
            }

            return bIsPrimitive;
        }

Another case

        public bool VerifySignatureForData(
            ReadOnlySpan<byte> data,
            [NotNullWhen(true)] out X509Certificate2? signerCertificate,
            X509Certificate2Collection? extraCandidates = null)
        {
            signerCertificate = null;

            X509Certificate2? cert = GetSignerCertificate(extraCandidates);

            if (cert == null)
            {
                return false;
            }

            bool ret = VerifyData(data);

            if (ret)
            {
                signerCertificate = cert;
            }

            return ret;
        }

If I change the last example to instead of caching VerifyData(data) in ret then it doesn't give the warning, to:

if (VerifyData(data))
{
      signerCertificate = cert;
      return true;
}

  return false;

[tannergooding]

I'd guess this has to do with Foo not having an equivalent annotation (which I dont think it can currently). And that impacting the nullability analysis

[stephentoub]

Yeah, I don't think that's a bug. The warning is saying that the out must not be null when returning true but that from the compiler's perspective it might be, therefore it's a warning. From the signature of Foo, the compiler has no way of knowing that the out will be non-null when Success is returned.

For the second and third ones, the compiler doesn't currently track the relationship between multiple variables. I assume that's the by-design reason.

[safern]

From the signature of Foo, the compiler has no way of knowing that the out will be non-null when Success is returned.

Would it make sense to add a new annotation that allows the compiler know that?

For the second and third ones, the compiler doesn't currently track the relationship between multiple variables. I assume that's the by-design reason.

Yeah, I thought about that, but maybe, it could?

[safern]

I'll close this per discussion above.

[tannergooding]

Do we want an issue opened for the other two issues?

[safern]

Do we want an issue opened for the other two issues?

It seems like those are by design. https://github.com/dotnet/runtime/pull/33646#commitcomment-37880344

[jcouv]

FWIW, here's the issue for tracking things when temp variables are introduced: https://github.com/dotnet/roslyn/issues/34800