Proposal: Improve analysis of [MaybeNull]T values

cston commented 5 years ago

Improve analysis of [MaybeNull]T values by adding a third possible flow state.

Additional flow state

A third flow state is added that represents maybe null even when substituted with a non-nullable reference type. The additional state applies only to values of type parameters that are not constrained not nullable. Flow analysis will use two bits for each tracked value.

internal enum NullableFlowState : byte
{
    /// <summary>
    /// Not null.
    /// </summary>
    NotNull = 0b00,

    /// <summary>
    /// Maybe null (type is nullable).
    /// </summary>
    MaybeNull = 0b01,

    /// <summary>
    /// Maybe null (type may be not nullable).
    /// </summary>
    MaybeDefault = 0b11,
}

The merging rules are unchanged: Join() uses bitwise | and Meet() uses bitwise &.

Member signatures affect method analysis

Attributes on members, including attributes on the containing method signature, are considered when analyzing the method body.

static T F1<T>(IEnumerable<T> e)
{
    return e.FirstOrDefault(); // warning: returning [MaybeNull]T
}

[return: MaybeNull] static T F2<T>(IEnumerable<T> e)
{
    return e.FirstOrDefault(); // ok
}

No W warnings

Locals cannot use [MaybeNull] and neither can explicit casts. Because of that limitation, W warnings are not reported for unconstrained types.

static T Default<T>()
{
    T t = default(T); // ok: T t
    return t;         // warning: returning [MaybeNull]T
}

static U Cast<T, U>([AllowNull]T t) where U : T
{
    var u = (U)t; // ok: U u
    return u;     // warning: returning [MaybeNull]U
}

Warnings are produced for compound types though:

T[] array = new[] { default(T) }; // warning: default(T) nullability does not match T

List<T> list = MakeList(default(T)); // warning: default(T) nullability does not match T arg

No warning for null expressions

Expressions (of a type parameter type not constrained to not nullable) that may produce null values are treated as MaybeDefault. Warnings are not reported for the expressions directly.

static T Default<T>()
{
    T t = default(T); // ok
    return t;         // warning: returning [MaybeNull]T
}

static U As<T, U>(T t) where U : class?
{
    U u = t as U; // ok
    return u;     // warning: returning [MaybeNull]U
}

static T ConditionalAccess<T>(IEnumerable<T>? e) where T : class?
{
    T t = e?.First(); // ok
    return t;         // warning: returning [MaybeNull]T
}

Type inference

[MaybeNull] is ignored in method type inference.

T t = Identity(default(T)); // warning: default(T) nullability does not match T arg

Best type algorithm relies on the merging of states to choose [MaybeNull]T over T.

static T Choose<T, U>(bool b, T t, [MaybeNull]U u) where U : T
{
    return b ? t : u; // warning: returning [MaybeNull]T
}

LDM Notes

https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-09-11.md
https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-11-11.md
https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-11-13.md (adding new state to track [MaybeNull] T)

huoyaoyuan commented 5 years ago

There should be a clearified state of "if reference type then nullable", in both source and analysis, aka <[MaybeNull]T>.

orthoxerox commented 5 years ago

New notes:

https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-11-25.md

qrli commented 4 years ago

The above design note contains important info on nullability of unconstrained generic T, which is IMO important for the community to read, as many have opinions on this matter. However, the issue title is a bit subtle, which reads like an internal issue for the roslyn team.

Maybe it is better to create a separate issue for the design note and link back?

dotnet / csharplang