roslyn: [NotNullWhen(true)] doesn't match [MaybeNullWhen(false)]—bug? Which one is right?

Version Used: VS 16.3 Preview 2

I don’t know which is right, [NotNullWhen(true)] or [MaybeNullWhen(false)]. Either way I have to use the ! operator to silence a warning.

And isn’t it a bug that they aren’t considered equivalent?

See standalone repro below.

Setup

Given this NRT adaptation of real-world code:

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;

public sealed class CombiningAggregator<T>
{
    public delegate bool TryCombineCalculator(
        T first,
        T second,
        [MaybeNullWhen(false)] out T combined);

    private readonly TryCombineCalculator calculator;

    // I have separate questions about this, but they can be ignored for the purpose of this
    // issue.
    [AllowNull, MaybeNull] // Still requires the initializer which then shows as redundant
    private T lastValue = default;

    private bool hasLastValue;

    public CombiningAggregator(TryCombineCalculator calculator)
    {
        this.calculator = calculator ?? throw new ArgumentNullException(nameof(calculator));
    }

    public IReadOnlyList<T> Process(IReadOnlyList<T> values)
    {
        if (values == null) throw new ArgumentNullException(nameof(values));

        var r = new List<T>();

        using (var en = values.GetEnumerator())
        {
            if (!hasLastValue)
            {
                if (!en.MoveNext()) return r;
                lastValue = en.Current;
                hasLastValue = true;
            }

            while (en.MoveNext())
            {
                var next = en.Current;
                if (calculator.Invoke(lastValue, next, out var combined))
                {
                    lastValue = combined;
                }
                else
                {
                    r.Add(lastValue);
                    lastValue = next;
                }
            }
        }

        return r;
    }

    public IReadOnlyList<T> Flush()
    {
        if (!hasLastValue) return Array.Empty<T>();
        var r = new[] { lastValue };
        lastValue = default!; // And this
        hasLastValue = false;
        return r;
    }
}

Attempt 1: [NotNullWhen(true)]

How should TryCombineStrings be annotated? It seemed [NotNullWhen(true)] out string? combined was the right thing to do because that’s what I would do if it was a standalone method.

But Roslyn says that it doesn’t match [MaybeNullWhen(false)] out string combined even though it seems like they should be exactly equivalent:

using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;

// Demonstration code, using string instead of domain class
public static class Program
{
    public static void Main()
    {
        // ⚠ CS8622 Nullability of reference types in type of parameter 'combined' of 'bool
        // Program.TryCombineStrings(string first, string second, out string? combined)'
        // doesn't match the target delegate 'CombiningAggregator<string>.TryCombineCalculator'
        //                                               ↓
        var aggregator = new CombiningAggregator<string>(TryCombineStrings);

        // Prints: AABB
        Console.WriteLine(string.Join(", ", aggregator.Process(new[] { "AA", "BB", "cc" })));

        // Prints: cc, DD
        Console.WriteLine(string.Join(", ", aggregator.Process(new[] { "DD", "ee", "ff" })));

        // Prints: eeff
        Console.WriteLine(string.Join(", ", aggregator.Flush()));
    }

    private static bool TryCombineStrings(
        string first,
        string second,
        [NotNullWhen(true)] out string? combined)
    {
        if (first.All(char.IsUpper) == second.All(char.IsUpper))
        {
            combined = first + second;
            return true;
        }

        combined = null;
        return false;
    }
}

Attempt 2: [MaybeNullWhen(false)]

using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;

// Demonstration code, using string instead of domain class
public static class Program
{
    public static void Main()
    {
        var aggregator = new CombiningAggregator<string>(TryCombineStrings);

        // Prints: AABB
        Console.WriteLine(string.Join(", ", aggregator.Process(new[] { "AA", "BB", "cc" })));

        // Prints: cc, DD
        Console.WriteLine(string.Join(", ", aggregator.Process(new[] { "DD", "ee", "ff" })));

        // Prints: eeff
        Console.WriteLine(string.Join(", ", aggregator.Flush()));
    }

    private static bool TryCombineStrings(
        string first,
        string second,
        [MaybeNullWhen(false)] out string combined)
    {
        if (first.All(char.IsUpper) == second.All(char.IsUpper))
        {
            combined = first + second;
            return true;
        }

        // ⚠ CS8625 Cannot convert null literal to non-nullable reference type.
        //         ↓
        combined = null;
        return false;
    }
}

Attempt 3: Generic instantiation with string? instead of string

This is all over the wrong thing to do. Null values should never be sent into TryCombineStrings and they should never come out of CombiningAggregator.

In this demonstration, null strings are happening to not cause any warnings because the .All extension method and string.Join accept nulls. In the real-world project, there are a bunch of warnings because nothing was supposed to be nullable.

using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;

// Demonstration code, using string instead of domain class
public static class Program
{
    public static void Main()
    {
        var aggregator = new CombiningAggregator<string?>(TryCombineStrings);

        // ⚠ Process and Flush return IReadOnlyList<string?> – SHOULD NOT BE NULLABLE

        // Prints: AABB
        Console.WriteLine(string.Join(", ", aggregator.Process(new[] { "AA", "BB", "cc" })));

        // Prints: cc, DD
        Console.WriteLine(string.Join(", ", aggregator.Process(new[] { "DD", "ee", "ff" })));

        // Prints: eeff
        Console.WriteLine(string.Join(", ", aggregator.Flush()));
    }

    private static bool TryCombineStrings(
        string? first, // ⚠ SHOULD NOT BE NULLABLE
        string? second, // ⚠ SHOULD NOT BE NULLABLE
        [NotNullWhen(true)] out string? combined)
    {
        if (first.All(char.IsUpper) == second.All(char.IsUpper))
        {
            combined = first + second;
            return true;
        }

        combined = null;
        return false;
    }
}

Shouldn’t attempt 1 just work? If not, why not? You’d never write attempt 2 if you were only calling the method directly, right?

Is the original, pre-C# 8 code flawed to begin with, and that’s why I’m running into this problem?