Address warnings for possibly null array elements

[cston]

As of dotnet/roslyn#46405, the C# compiler considers constraint types for conversions involving type parameter types.

A warning is reported for the following for instance:

#nullable enable

interface I { }

class Program
{
    static void F0<T>(ref T x, ref T y)
    {
    }
    static void F<T>(ref T x, ref T y) where T : I
    {
        if (x == null) { }
        F0(ref x, ref y); // warning: x may be null
    }
}

This results in several new warnings building dotnet/runtime (see dotnet/roslyn#46577).

[stephentoub]

I'm not understanding why the compiler flags this. Why does the compiler think these methods could assign null to the ref?

[cston]

I believe the warning is for the value of ref argument when the method is called. On the second time through the loop, leftRef is assumed to be null based on && (leftRef = ...) == null.

[stephentoub]

I see. Is the right fix here then to actually annotate both arguments with [AllowNull]? (That would apply to all members of Unsafe that accept multiple ref Ts and only care about the address, not the actual value, e.g. AreSame, ByteOffset, IsAddressGreaterThan, IsAddressLessThan, and in both the public Unsafe.cs ref as well as the internal implementation in corelib.)

[cston]

Is the right fix here then to actually annotate both arguments with [AllowNull]?

The methods in Unsafe simply take an unconstrained T which seems like it should be sufficient for all callers. Should the leftRef, etc. locals be declared as ref T? here instead?

[cston]

Should the leftRef, etc. locals be declared as ref T? here instead?

Perhaps not. That would just move the warnings to the initializers of those locals.

[stephentoub]

Should the leftRef, etc. locals be declared as ref T? here instead?

Wouldn't that be a problem if a T t; were passed in, with the compiler then complaining that the implementation might assign null to it?

[stephentoub]

Should the leftRef, etc. locals be declared as ref T? here instead?

Perhaps not. That would just move the warnings to the initializers of those locals.

Right, hence my question about [AllowNull]. Does that make sense?

[sharwell]

This is similar to the Volatile.Read APIs, where ideally we could change ref T to in T, but that would be a breaking API change.

[stephentoub]

Thanks, @cston. Once this goes in, it should be ported to release/5.0 to unblock updating the compiler there as well. Thanks!
cc: @danmosemsft

[cston]

I assume you validated adding the AllowNull does indeed also fix the issue?

Yes, adding [AllowNull] resolved the warnings.

[safern]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/217132032