Runtime: Span/ReadOnlySpan constructors throw with wrong parameter name

[fredrikhr]

Problem

Both Span<T> and ReadOnlySpan<T> have constructors that take either a pointer or array together with a length argument. If a negative value for length is passed, the constructor throws an ArgumentOutOfRangeException, but the ParamName property of the thrown exception is start instead of length.

namespace System
{
    partial struct Span<T>
    {
        public unsafe Span(void* pointer, int length) { throw null; }
        public Span(T[] array, int start, int length)  { throw null; }
    }
    partial struct ReadOnlySpan<T>
    {
        public unsafe ReadOnlySpan(void* pointer, int length) { throw null; }
        public ReadOnlySpan(T[] array, int start, int length)  { throw null; }
    }
}

Receiving an exception that tells me that the value for start is confusing and misleading, as start is not the offending parameter in the array-case, while the pointer constructor does not even have a start parameter.

Code locations

By examining the source code for the four constructors, it easy to the see that issue comes from calling ThrowHelper.ThrowArgumentOutOfRangeException() in all cases, regardless of which parameter is invalid.

https://github.com/dotnet/corefx/blob/4acf7e78385e673798c4aad33cdf7fe9b2971cfe/src/Common/src/CoreLib/System/Span.Fast.cs#L82-L89
https://github.com/dotnet/corefx/blob/4acf7e78385e673798c4aad33cdf7fe9b2971cfe/src/Common/src/CoreLib/System/Span.Fast.cs#L115-L116
https://github.com/dotnet/corefx/blob/4acf7e78385e673798c4aad33cdf7fe9b2971cfe/src/Common/src/CoreLib/System/ReadOnlySpan.Fast.cs#L76-L83
https://github.com/dotnet/corefx/blob/4acf7e78385e673798c4aad33cdf7fe9b2971cfe/src/Common/src/CoreLib/System/ReadOnlySpan.Fast.cs#L109-L110

Expected Behaviour

The Span/ReadOnlySpan constructor throws ArgumentOutOfRangeException with a ParamName equal to length when called with a negative value for length (and otherwise legal argument values).

Actual Behaviour

The Span/ReadOnlySpan constructor throws ArgumentOutOfRangeException with a ParamName equal to start when called with a negative value for length (and otherwise legal argument values).

Test Code

using Xunit;

namespace System.Test
{
    public static class SpanConstructorTest
    {
        [Fact]
        public static void Span_ctor_with_array_and_negative_length_throws()
        {
            var bytes = new byte[10];

            var argExcept = Assert.ThrowsAny<ArgumentOutOfRangeException>(() =>
            {
                _ = new Span<byte>(bytes, start: 0, length: -1);
            });
            Assert.Equal("length", argExcept.ParamName);
        }

        [Fact]
        public static unsafe void Span_ctor_with_pointer_and_negative_length_throws()
        {
            var bytes = stackalloc byte[10];

            var argExcept = Assert.ThrowsAny<ArgumentOutOfRangeException>(() =>
            {
                _ = new Span<byte>(bytes, length: -1);
            });
            Assert.Equal("length", argExcept.ParamName);
        }
    }
    public static class ReadOnlySpanConstructorTest
    {
        [Fact]
        public static void Span_ctor_with_array_and_negative_length_throws()
        {
            var bytes = new byte[10];

            var argExcept = Assert.ThrowsAny<ArgumentOutOfRangeException>(() =>
            {
                _ = new ReadOnlySpan<byte>(bytes, start: 0, length: -1);
            });
            Assert.Equal("length", argExcept.ParamName);
        }

        [Fact]
        public static unsafe void Span_ctor_with_pointer_and_negative_length_throws()
        {
            var bytes = stackalloc byte[10];

            var argExcept = Assert.ThrowsAny<ArgumentOutOfRangeException>(() =>
            {
                _ = new ReadOnlySpan<byte>(bytes, length: -1);
            });
            Assert.Equal("length", argExcept.ParamName);
        }
    }
}

[ahsonkhan]

By examining the source code for the four constructors, it easy to the see that issue comes from calling ThrowHelper.ThrowArgumentOutOfRangeException() in all cases, regardless of which parameter is invalid.

To clarify, this issue does not exist on the in-box "Fast" Span (on .NET Core 2.1 or 3.0). In fact, we don't specify the parameter name at all regardless if the start or length were out of bounds.

This is the in-box behavior:
```C#
var bytes = new byte[10];

var argExcept = Assert.ThrowsAny(() =>
{
_ = new Span(bytes, start: 0, length: -1);
});
Assert.Equal(null, argExcept.ParamName);
```

This issue only exists on the portable span implementation (accessible from the standalone NuGet package), which only shipped during .NET Core 2.1.
See the portable implementation passing the argument name itself:
https://github.com/dotnet/corefx/blob/39fcb4b73c1a7dfca721d5f727603ffd35a428d8/src/System.Memory/src/System/Span.Portable.cs#L91

I believe this was an oversight when reducing the number of checks during argument validation, and we could have passed both start/length to the ThrowHelper to discern what argument is actually invalid. However, the fix here would be to remove the argument name all together (to be consistent) and I don't think this meets the servicing bar, especially given the current (and in-box) behavior. Therefore, I don't think this issue is actionable. Please let me know if you disagree (or I am mistaken).

Alternatively, we could consider fixing it going forward (something like this):
https://github.com/dotnet/corefx/blob/39fcb4b73c1a7dfca721d5f727603ffd35a428d8/src/System.Memory/src/System/ThrowHelper.cs#L158-L168

cc @jkotas, @GrabYourPitchforks, @danmosemsft

[fredrikhr]

@ahsonkhan I encountered this issue during development of a class library targeting netstandard2.0 (and also netstandard1.3 with the explicit inclusion of the System.Memory version 4.5.2 NuGet package).

While it is nice to know that the issue does not exist in .NET Core 2.1 and 3.0, this does not really help me as a library author. The whole point of .NET Standard is that I can target .NET Standard in my libraries and don't have to provide runtime-specific implementations. And as far as I can see, this issue exists when I target netstandard2.0 (and also netstandard1.3).

The fix for this really does not require any change to publically exposed APIs, only how an exception is thrown (internal implemntation detail). It could arguablly be fixed in in a patch-version (like for example System.Memory version 4.5.3).

[jkotas]

The whole point of .NET Standard is that I can target .NET Standard in my libraries and don't have to provide runtime-specific implementations

This bug in exception argument name should not be preventing that.

It could arguably be fixed in in a patch-version

It would be doable if the System.Memory .NET Standard 2.0 package was actively maintained. We do not actively maintain this package anymore. We are fixing security and other critical bugs in it only.

You will get best experience, best performance, or non-critical bug fixes (like this one) with System.Memory APIs on latest .NET Core.

[ahsonkhan]

Closing as non-actionable. Thanks.