Warn when comparing spans to null

[terrajobst]

The following code

var array = new char[0];
var span = array.AsSpan();
var emptyArray = Array.Empty<char>();
var emptySpan = Span<char>.Empty;

if (array == null)
    Console.WriteLine("array is null");
else
    Console.WriteLine("array is not null");

if (span == null)
    Console.WriteLine("span is null");
else
    Console.WriteLine("span is not null");

if (emptyArray == null)
    Console.WriteLine("emptyArray is null");
else
    Console.WriteLine("emptyArray is not null");

if (emptySpan == null)
    Console.WriteLine("emptySpan is null");
else
    Console.WriteLine("emptySpan is not null");

outputs the following:

array is not null
span is not null
emptyArray is not null
emptySpan is null

The only surprising one is Span<char>.Empty == null resulting in true.

Spans can be compared to null because there is an implicit conversion from arrays to spans and the null literal can be converted to an array, so the compiler ends up emitting this:

if (Span<char>.op_Equals(emptySpan, (Span<char>)((char[])null)))
    Console.WriteLine("emptySpan is null");
else
    Console.WriteLine("emptySpan is not null");

The empty span is defined as the null literal converted to a span, so this resulting in true is, albeit surprising, sensible and by design.

This effectively hides the built-in diagnostic CS8073 against comparing value types to null. We should consider adding a diagnostic for comparisons between spans and null.

As pointed out, this should probably also result in a warning:

Span<Cell> line = null;

More context on Twitter.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following code

var array = new char[0];
var span = array.AsSpan();
var emptyArray = Array.Empty<char>();
var emptySpan = Span<char>.Empty;

if (array == null)
    Console.WriteLine("array is null");
else
    Console.WriteLine("array is not null");

if (span == null)
    Console.WriteLine("span is null");
else
    Console.WriteLine("span is not null");

if (emptyArray == null)
    Console.WriteLine("emptyArray is null");
else
    Console.WriteLine("emptyArray is not null");

if (emptySpan == null)
    Console.WriteLine("emptySpan is null");
else
    Console.WriteLine("emptySpan is not null");

outputs the following:

array is not null
span is not null
emptyArray is not null
emptySpan is null

The only surprising one is Span<char>.Empty == null resulting in true.

Spans can be compared to null because there is an implicit conversion from arrays to spans and the null literal can be converted to an array, so the compiler ends up emitting this:

if (Span<char>.op_Equals(emptySpan, (Span<char>)((char[])null)))
    Console.WriteLine("emptySpan is null");
else
    Console.WriteLine("emptySpan is not null");

The empty span is defined as the null literal converted to a span, so this resulting in true is, albeit surprising, sensible and by design.

This effectively hides the built-in diagnostic CS8073 against comparing value types to null. We should consider adding a diagnostic for comparisons between spans and null.

More context on Twitter.

Author:	terrajobst
Assignees:	-
Labels:	api-suggestion, area-System.Runtime, code-analyzer
Milestone:	-

[EgorBo]

#76735, I thought the plan was to add MemoryExtensions.ReferenceEqual and mark Span.Equals as obsolete/hide from intelisense?

[MichaelRumpler]

Maybe also a warning if you set a Span<T> to null? Because this is also possible:

Span<Cell> line = null;

This is one more reason that users could think Span is an object and therefore also the comparison is ok.

[terrajobst]

@EgorBo

#76735, I thought the plan was to add MemoryExtensions.ReferenceEqual and mark Span.Equals as obsolete/hide from intelisense?

Looks like that got closed in favor of #54794. I'm not sure whether this issue is now a dupe or related.

@GrabYourPitchforks?

@MichaelRumpler

Maybe also a warning if you set a Span<T> to null?

Good point!

[terrajobst]

Video

We should warn for any comparison against spans and memories (==, !=, is, is not):

• Comparisons against the default span should be written as span.IsDefault. Which doesn't exist but we could add one.
• Comparisons against content should be done as span.SequenceEqual(...)
• Comparisons against null don't make sense

It seems we don't have consensus on this view point. We should discuss this with once we have quorum again.

[terrajobst]

Video

We should warn for any comparison against spans and memories involving the null literal (== null, != null, is null, is not null). If someone writes == default then they likely meant the default span, which is meaningful and should be allowed.

• Comparisons against the default span should be written as == default.
• Asking whether the span is empty should be written as span.IsEmpty

Category: Usage
Severity: Warning