Argument state after call for AllowNull parameters

[RikkiGibson]

Related to dotnet/roslyn#48605

In order to reduce nullable warnings down to the most "actionable" subset, we modified analysis such that arguments to parameters with non-nullable types would update the argument's flow state to not-null after the call. This also achieved a goal of increasing uniformity between e.g. string s and [NotNull] string s.

string? s = null;
M(s); // warning
s.ToString(); // no warning

void M(string s) { }

For by-value parameters this ended up interacting with [AllowNull] in a potentially undesired way.

string? s = null;
M(s); // no warning
s.ToString(); // no warning..?

void M([AllowNull] string s) { }

Arguably this is an intended consequence and enables users to write AssertNotNull-type methods which only return if the argument is not-null. However, C# 8 users ended up needing [AllowNull] in generics to express that the input is always allowed to be null:

namespace Newtonsoft.Json
{
    public abstract class JsonConverter<T>
    {
        // note: caller might pass a maybe-null argument here and have its state quietly updated to not-null
        public abstract void WriteJson(JsonWriter writer, [AllowNull]T value, JsonSerializer serializer);
    }
}

We also decided that a non-nullable reference type ref parameter should be considered to assign a value of its type to the argument. Since we assume that 'ref' parameters write to their argument, we have some motivation to preserve this behavior even if we decide to change the behavior for non-ref parameters.

string? s = null;
M(ref s); // no warning
s.ToString(); // no warning

void M([AllowNull] ref string s) { }

We may wish to adjust the heuristic for whether to update the argument state to not-null. One way we could do this is by saying that:

• the declared type (sans attributes) of a by-value or in parameter does not update the argument state after the call. We likely want to preserve the [NotNull] postcondition behavior to enable writing AssertNotNull([NotNull] object? obj).
• if a by-value or in parameter does not accept a null argument, then the argument state must be updated to not-null after the call (similar to how a call receiver is updated to not-null after the call).
• For ref/out parameters, the argument state after the call is the effective output type of the parameter, e.g. not-null for ref string s as well as [NotNull] ref string s.

[RikkiGibson]

We recently met and discussed how to alleviate issues we've found with the [AllowNull] string s parameter scenario. Our proposal is to modify the rules for nullable argument analysis as follows:

• An input parameter doesn't update the argument state after the call based on its declared type.
• When we give a safety warning on an argument, we update the argument state to not-null.

Discussion

AllowNull/DisallowNull and MaybeNull/NotNull were initially designed with an emphasis on the respective concepts of "input state" and "output state" of members and parameters. However, we found that in the absence of T?, developers used [AllowNull] a stand-in for T? on input parameters. For example, the JsonConverter<T>.WriteJson([AllowNull]T value) scenario. It feels like the concept of a separate "input state" and "output state" on input parameters is not desirable to include by default.

Instead, we think the better mental model is to say that an input parameter usually only has an input state. By default there is no "output state" being applied to the argument. Instead, we say that if a safety warning is given for the argument (due to failing to convert to the parameter type), the argument's state should be updated to not-null. In this model, [NotNull] or [MaybeNull] can be used on input parameters to introduce the appropriate output state to the parameter. We expect this to be infrequently used, mostly for AssertNotNull()-style methods.

This proposal addresses the main pain point that was raised, by not updating argument state to not-null in more scenarios:

string? s = null;
M(s);
s.ToString(); // new warning

void M([AllowNull] string s) { }

However, behavior around ref/out parameters will not change in practice.

string? s = null;
M(ref s);
s.ToString(); // no warning

void M([AllowNull] ref string s) { }

This scenario is also unchanged.

string? s = null;
AssertNotNull(s);
s.ToString(); // no warning

void AssertNotNull([NotNull] string? s) { }

[RikkiGibson]

It looks like we will adopt this proposal and try to get the implementation change done in 16.9.

[RikkiGibson]

I am going to modify the following language:

When we give a safety warning on an argument, we update the argument state to not-null.

to:

When we give a safety warning on an argument, we update argument state to the declared type of the parameter met with the argument state.

Net effect of this is that passing an argument for an unconstrained type parameter can update the state from maybe-default to maybe-null, but will not update state from not-null to maybe-null, for example.

Tagging @jcouv @jaredpar in case you have concerns.