There is a difference between a Span created from a null array and an empty array: SharpLab

int[]? array1 = null;
int[]  array2 = [];

Span<int> span1 = array1;
Span<int> span2 = array2;

Console.WriteLine(span1 == null); // Prints 'True'
Console.WriteLine(span2 == null); // Prints 'False'

So the null check is not redundant and was added on purpose to catch cases where the user passes a null array.

Also, changing parameter types breaks compatibility.

So the null check is not redundant and was added on purpose to catch cases where the user passes a null array.

I didn't know this.

Nevermind @raulsntos I closed this too early. I think you have gotten confused here.

using System;

Span<int> span = [];

Console.WriteLine(span == null); // Prints 'True'

SharpLab

In C#, converting a null array to a span always creates an empty but usable span. See the code:

Hi. You're absolutely right that spans created from null arrays are functionally identical to spans created from empty arrays.

I imagine what raulsntos was going for is that they can still be differentiated, and thus, we might want to purposefully design the API to not accept spans created from null arrays as a valid argument here.

For me, it's a matter of looking at how the BCL handles this kind of cases, and stick to it as long as it makes sense. I personally can't think of a method anywhere that'd treat both cases differently. Even this atrocity is valid C# code:

byte[] array = ((Span<byte>)null).ToArray();

And Span<T>.ToArray() is probably the closest thing we'll find to the methods we're discussing.

So, long story short, I think we can merge this PR.

I'm not entirely sure how breaking this is in terms of compatibility, since any previous call to Array(Span<T> array) should still be valid for Array(ReadOnlySpan<T> span).

I'm not entirely sure how breaking this is in terms of compatibility, since any previous call to Array(Span<T> array) should still be valid for Array(ReadOnlySpan<T> span).

Changing the parameter type from Span<T> to ReadOnlySpan<T> maintains source compatibility since Span<T> is implicitly convertible to ReadOnlySpan<T>. But it breaks binary compatibility.

Changing the name of the parameter from array to span breaks source compatibility because the callsite could be using named parameters (e.g.: new Array<int>(array: [1, 2, 3])) but we normally don't care too much about this.

We have to keep the Array(Span<T> array) constructors to avoid breaking binary compatibility. Breaking source compatibility is less disruptive.

We have to keep the Array(Span<T> array) constructors to avoid breaking binary compatibility. Breaking source compatibility is less disruptive.

@raulsntos I added back the old constructors as deprecated and hidden, to be removed in the next major version of Godot. Let me know if you want something else.

I don't think there's a reason to deprecate them, it will produce warnings when constructing from T[] or Span<T> which just adds noise.

I don't think there's a reason to deprecate them, it will produce warnings when constructing from T[] or Span<T> which just adds noise.

Ideally this would be a good case for OverloadResolutionPriorityAttribute but GodotSharp is targeting .NET 8.0. Removing the deprecations.

Thanks!