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

[jnm2]

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?

[RikkiGibson]

Tagging @jcouv

[jcouv]

How about using [MaybeNullWhen(false) out string combined in TryCombineStrings? That matches the delegate definition when T is substituted with string.
Illustration

More generally, [MaybeNullWhen(false)] and [NotNullWhen(true)] are not equivalent for unconstrained T.

[RikkiGibson]

Is that the same as Attempt 2 in the issue description? Is it intended that the implementation will still give a safety warning when assigning 'null' to such an out parameter?

[jcouv]

Is it intended that the implementation will still give a safety warning when assigning 'null' to such an out parameter?

Yes, nullable attributes currently only affect callers, not methods bodies. There is an issue tracking that (#36039)

[RikkiGibson]

OK, so it sounds like the answer for @jnm2 is: for now, go with attempt 2, and use null! for now when you need to assign null to the out parameter.

[jnm2]

@jcouv @RikkiGibson Thanks. Would you say that [MaybeNullWhen(false)] out string combined is not a good way to handle out parameters in general? What still bothers me about that answer is that the nullability annotations have to switch back and forth depending on whether someone wants to pass the method to a generic method. How can I think of this in a way that makes sense?

[RikkiGibson]

It is used in the effective signature of Dictionary<int, string>.TryGetValue, so all I can say is if we think of a better way we'll let you know 😄

[jnm2]

@RikkiGibson Right, but let's say I write a nongeneric public API with [NotNullWhen(true)] out string?. Should I preemptively make it [MaybeNullWhen(false)] out string instead in case someone wants to use it with some generic API from another library?

What's driving the signature is no longer anything intrinsic to the method. Now it's constrained by how it might be used.

[RikkiGibson]

Right now the flow analysis attributes are just being completely ignored when we examine the method group -> delegate conversion. In a future release we may start examining the attributes when determining nullable convertibility for delegates, for OHI, etc. It will be difficult to give a definitive answer until then.

It would probably be helpful to have folks upvote or comment on the linked issue #36039 in order to gauge the demand for this.

[jnm2]

'OHI' is a bit of jargon I haven't picked up on yet. I'll go ahead and leave a comment about the delegate conversion example in this thread.

[RikkiGibson]

Thanks for pointing that out, I went ahead and expanded the acronym in that issue's description 😄

[jnm2]

Awesome, thanks!

Is it okay to leave this open until there is a definitive answer?

[Denis535]

Author's example is a bit long, so I decided to invest my 5 cents and write simple examples.

// I use aliases because default attributes names are very confusing
using AllowNullInputAttribute = System.Diagnostics.CodeAnalysis.AllowNullAttribute;
using DisallowNullInputAttribute = System.Diagnostics.CodeAnalysis.DisallowNullAttribute;
using AllowNullOutputAttribute = System.Diagnostics.CodeAnalysis.MaybeNullAttribute;
using DisallowNullOutputAttribute = System.Diagnostics.CodeAnalysis.NotNullAttribute;

public static void Func_NullableInput_ExistableOutput([AllowNullInput] string nullableInput, [AllowNullInput] out string existableOutput) {
    string? nullable = nullableInput;
    string existable = nullableInput; // missing warning
    existableOutput = null; // warning
    existableOutput = string.Empty;
}
public static void Func_ExistableInput_NullableOutput([DisallowNullInput] string? existableInput, [DisallowNullInput] out string? nullableOutput) {
    string? nullable = existableInput;
    string existable = existableInput; // redundant warning
    nullableOutput = null;
    nullableOutput = string.Empty;
}

public static void Func_ExistableInput_NullableOutput([AllowNullOutput] string existableInput, [AllowNullOutput] out string nullableOutput) {
    string? nullable = existableInput;
    string existable = existableInput;
    nullableOutput = null; // redundant warning
    nullableOutput = string.Empty;
}
public static void Func_NullableInput_ExistableOutput([DisallowNullOutput] string? nullableInput, [DisallowNullOutput] out string? existableOutput) {
    string? nullable = nullableInput;
    string existable = nullableInput; // warning
    existableOutput = null; // missing warning
    existableOutput = string.Empty;
}

The problem is that analyzer does not pay attention on attributes. Only nullable value or not.

So, for analyzer: [NotNullWhen(true)] out string? value and [MaybeNullWhen(false)] out string value is just: out string? value and out string value.

I hope this helps someone.

[jcouv]

@jnm2 Looking up this thread, I believe the questions have been addressed. I'll go ahead and close as I'm cleaning up old nullability issues.
If not, would you mind summarizing the remaining question and re-open?

FYI, I believe this issue is affected by a recent change (16.5p3) which enforces nullability attributes within method bodies. So that a method with a [MaybeNullWhen(true)] out string s parameter could be implemented by calling a method with a [MaybeNullWhen(true)] out string s2 or [NotNullWhen(false)] out string? s2 parameter.

[onchuner]

i think we should have clear guideline documented somewhere for below combinations
(MaybeNullWhen|NotNullWhen)(generic|constrainted generic|non-generic)(with ?|without ?)
when use what basically and cases like [MaybeNullWhen(false), NotNullWhen(true)] too

[RikkiGibson]

I think these conventions are determined and implemented in the standard libraries over in https://github.com/dotnet/runtime, perhaps it would make sense to search for or raise an issue over there.

[RikkiGibson]

@onchuner this document may be of interest: https://github.com/dotnet/runtime/blob/abfdb542e8dfd72ab2715222edf527952e9fda10/docs/coding-guidelines/api-guidelines/nullability.md#nullable-attributes

[onchuner]

@RikkiGibson thank you for help