Fixed array helper improvements

[JeremyKuhne]

Is your feature request related to a problem? Please describe.

There are a few issues with the fixed array helpers structs (such as internal partial struct __char_32):

1. They do not play nicely with C# 11 (generate new warnings).
2. They also are bulkier than necessary.
3. They could be faster by leveraging built-in helpers (such as .IndexOf).
4. Equals logic is unexpected.

Describe the solution you'd like

• Utilization of [UnscopedRef] to eliminate warnings in C# 11. (Available in RC1)
• Provide an AsReadOnlySpan that can be marked as readonly to avoid implicit struct copies.
• Introduce a span extensions class to help with interop span usage.
• Fix and clarify Equals logic (it currently matches incoming spans that are longer than the content).
• Move the structs to a shared location to avoid duplicate copies for same sized structs.
• Consider removing methods that don't provide much value (CopyTo, ToArray, ToString(int length)).

Existing

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
internal partial struct __char_32
{
    internal char _0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, _25, _26, _27, _28, _29, _30, _31;
    internal readonly int Length => 32;
    internal ref char this[int index] => ref AsSpan()[index];
    internal Span<char> AsSpan() => MemoryMarshal.CreateSpan(ref _0, 32);
    internal unsafe readonly void CopyTo(Span<char> target, int length = 32);
    internal readonly char[] ToArray(int length = 32);
    internal unsafe readonly bool Equals(ReadOnlySpan<char> value);
    internal readonly bool Equals(string value) => Equals(value.AsSpan());
    internal unsafe readonly string ToString(int length);
    public override readonly unsafe string ToString();
    public static implicit operator __char_32(string value) => value.AsSpan();
    public static implicit operator __char_32(ReadOnlySpan<char> value);
}

Proposed

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
internal partial struct __char_32
{
    private unsafe fixed char _value[32];

    internal readonly int Length => 32;

    // How useful is this?
    [UnscopedRef]
    internal ref char this[int index] => ref AsWritableSpan()[index];

    [UnscopedRef]
    internal unsafe Span<char> AsWritableSpan() => MemoryMarshal.CreateSpan(ref _value[0], 32);

    // Suggesting making the `ReadOnlySpan` method `AsSpan()` to align with `string.AsSpan()`.
    // Need a `readonly` method to allow other `readonly` methods to utilize without creating implicit struct copies.
    [UnscopedRef]
    internal unsafe readonly ReadOnlySpan<char> AsSpan()
        => MemoryMarshal.CreateReadOnlySpan(ref Unsafe.AsRef(in _value[0]), 32);

    // This should probably be removed. `AsSpan().CopyTo(target)` is pretty straight-forward.
    internal unsafe readonly void CopyTo(Span<char> target, int length = 32)
    {
        // If we don't remove the methods I've marked, the checks could be removed and we could allow bounds checks to fail.
        if (length > 32) throw new ArgumentOutOfRangeException(nameof(length));
        AsSpan()[..length].CopyTo(target);
    }

    // This should probably be removed. `AsSpan().ToArray()` is pretty straight-forward.
    internal readonly char[] ToArray(int length = 32)
    {
        if (length > 32) throw new ArgumentOutOfRangeException(nameof(length));
        return AsSpan()[..length].ToArray();
    }

    internal unsafe readonly bool Equals(ReadOnlySpan<char> value)
        => AsSpan().SliceAtNull().SequenceEqual(value);

    // This also is unnecessary. `string` is implicitly convertable to `ReadonlySpan<char>`.
    internal readonly bool Equals(string value) => Equals(value.AsSpan());

    // This is also something that should be removed. 
    internal unsafe readonly string ToString(int length)
    {
        if (length < 0 || length > Length) throw new ArgumentOutOfRangeException(nameof(length), length, "Length must be between 0 and the fixed array length.");
        return AsSpan()[..length].ToString();
    }

    public override readonly unsafe string ToString() => AsSpan().SliceAtNull().ToString();

    public static implicit operator __char_32(string value) => value.AsSpan();
    public static implicit operator __char_32(ReadOnlySpan<char> value)
    {
        __char_32 result = default;
        value.CopyTo(result.AsWritableSpan());
        return result;
    }
}

// NEW: Extension class to make working with spans easier.
internal static class SpanExtensions
{
    public static ReadOnlySpan<char> SliceAtNull(this ReadOnlySpan<char> value)
    {
        int length = value.IndexOf('\0');
        return new string(length < 0 ? value : value[..length]);
    }
}

Proposed without unnecessary methods

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
internal partial struct __char_32
{
    private unsafe fixed char _value[32];

    internal readonly int Length => 32;

    [UnscopedRef]
    internal ref char this[int index] => ref AsWritableSpan()[index];

    [UnscopedRef]
    internal unsafe Span<char> AsWritableSpan() => MemoryMarshal.CreateSpan(ref _value[0], 32);

    [UnscopedRef]
    internal unsafe readonly ReadOnlySpan<char> AsSpan()
        => MemoryMarshal.CreateReadOnlySpan(ref Unsafe.AsRef(in _value[0]), 32);

    internal unsafe readonly bool Equals(ReadOnlySpan<char> value)
        => AsSpan().SliceAtNull().SequenceEqual(value);

    public override readonly unsafe string ToString() => AsSpan().SliceAtNull().ToString();

    public static implicit operator __char_32(string value) => value.AsSpan();
    public static implicit operator __char_32(ReadOnlySpan<char> value)
    {
        __char_32 result = default;
        value.CopyTo(result.AsWritableSpan());
        return result;
    }
}

cc: @lonitra

[AArnott]

They do not play nicely with C# 11 (generate new warnings).

Fixed in #660

[JeremyKuhne]

Making the AsSpan() be a ReadOnlySpan not only follows along with string.AsSpan(), it encourages not taking a writable view when not needed.

[AArnott]

[0] fixed length arrays aren't handled properly either, probably

[AArnott]

@JeremyKuhne Why does your SliceAtNull method create a copy of the slice as a string, only to then expose as a span? The signature makes it look alloc-free, but there's a hidden and apparently unnecessary allocation there. If the caller wants a copy, couldn't they make that themselves?

[AArnott]

Your bool Equals(ReadOnlySpan<char> value) method looks reasonable, until I consider it returns false when passed its own value and that value contains a null character. The goal of abc\0\0 matching abc seems reasonable but maybe not at the expense of matching abc\0\0. Maybe Equals can have it both ways though, by implementing it thusly:

    internal unsafe readonly bool Equals(ReadOnlySpan<char> value)
        => value.Length == this.Length ? AsReadOnlySpan().SequenceEqual(value) : AsReadOnlySpan().SliceAtNull().SequenceEqual(value);

[AArnott]

Making the AsSpan() be a ReadOnlySpan not only follows along with string.AsSpan(), it encourages not taking a writable view when not needed.

But string is fundamentally read-only, so string.AsSpan() couldn't possibly do anything different.
For a mutable struct, when there are Span and ReadOnlySpan types, I feel AsSpan should return what it says it will return (a Span) and AsReadOnlySpan can be there too, also usefully identifying its return type, for the rare case where a readonly span is actually necessary.

The only time I can think of where it's poor for the user to get a Span<T> where a ReadOnlySpan<T> would suffice is when this struct is stored in a readonly field and therefore retrieving a Span<T> results in a defensive copy of the content. But I marginalize this concern with the following:

1. These fixed length structs seem extremely unlikely to be stored in a readonly field. They certainly won't be in their natural habitat of their parent win32 struct. So unless some app decides to reuse these structs in their own types, this will never come up.
2. When in a readonly field, it's unlikely that most uses will meaningfully suffer from the perf impact of the copy.
3. There is an easy fix when the perf does have an impact: use AsReadOnlySpan instead.

Am I missing anything?

[AArnott]

Here is a hybrid of your proposal, plus code that still works on .NET Framework (I believe).
It includes the 'unnecessary' members because on .NET Framework where the AsSpan methods aren't available, there isn't an obvious workaround to their being missing. But I at least reimplemented them so they use span internally and thus can use the runtime methods for the bulk of their work. And we could decide to not define them at all on .NET Core 3.1+, so essentially users get either the AsSpan methods or the other helper methods. That just makes multitargeting hard for the user though, since the APIs aren't common at all, so I'm split on that.

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
internal partial struct __char_1
{
    internal unsafe fixed char Value[1];

    /// <summary>Always <c>1</c>.</summary>
    internal readonly int Length => 1;

    [UnscopedRef]
    internal ref char this[int index]
    {
        get
        {
#if NETCOREAPP3_1_OR_GREATER
            return ref AsSpan()[index];
#else
            unsafe
            {
                return ref Value[index];
            }
#endif
        }
    }

#if NETCOREAPP3_1_OR_GREATER
    [UnscopedRef]
    internal unsafe Span<char> AsSpan() => MemoryMarshal.CreateSpan(ref Value[0], 1);

    [UnscopedRef]
    internal unsafe readonly ReadOnlySpan<char> AsReadOnlySpan() => MemoryMarshal.CreateReadOnlySpan(ref Unsafe.AsRef(Value[0]), 1);
#endif

    internal readonly void CopyTo(Span<char> target, int length = 1)
    {
#if NETCOREAPP3_1_OR_GREATER
        AsReadOnlySpan().Slice(0, length).CopyTo(target);
#else
        unsafe
        {
            fixed (char* p0 = Value)
            {
                new ReadOnlySpan<char>(p0, Length).Slice(0, length).CopyTo(target);
            }
        }
#endif
    }

    internal readonly char[] ToArray(int length = 1)
    {
#if NETCOREAPP3_1_OR_GREATER
        return AsReadOnlySpan().Slice(length).ToArray();
#else
        unsafe
        {
            fixed (char* p0 = Value)
            {
                return new ReadOnlySpan<char>(p0, Length).Slice(length).ToArray();
            }
        }
#endif
    }

    internal unsafe readonly bool Equals(ReadOnlySpan<char> value)
    {
#if NETCOREAPP3_1_OR_GREATER
        return value.Length == this.Length ? AsReadOnlySpan().SequenceEqual(value) : AsReadOnlySpan().SliceAtNull().SequenceEqual(value);
#else
        unsafe
        {
            fixed (char* p0 = Value)
            {
                ReadOnlySpan<char> span = new(p0, Length);
                return value.Length == this.Length ? span.SequenceEqual(value) : span.SliceAtNull().SequenceEqual(value);
            }
        }
#endif
    }

    internal readonly bool Equals(string value) => Equals(value.AsSpan());

    internal unsafe readonly string ToString(int length)
    {
#if NETCOREAPP3_1_OR_GREATER
        return AsReadOnlySpan().Slice(0, length).ToString();
#else
        unsafe
        {
            fixed (char* p0 = Value)
            {
                ReadOnlySpan<char> span = new(p0, Length);
                return span.Slice(0, length).ToString();
            }
        }
#endif
    }

    public override readonly unsafe string ToString()
    {
#if NETCOREAPP3_1_OR_GREATER
        return AsReadOnlySpan().SliceAtNull().ToString();
#else
        unsafe
        {
            fixed (char* p0 = Value)
            {
                ReadOnlySpan<char> span = new(p0, Length);
                return span.SliceAtNull().ToString();
            }
        }
#endif
    }

    public static implicit operator __char_1(string value) => value.AsSpan();
    public static implicit operator __char_1(ReadOnlySpan<char> value)
    {
        Unsafe.SkipInit(out __char_1 result);
#if NETCOREAPP3_1_OR_GREATER
        value.CopyTo(result.AsSpan());
        int initLength = value.Length;
        result.AsSpan().Slice(initLength, result.Length - initLength).Clear();
        return result;
#else
        unsafe
        {
            Span<char> target = new(result.Value, result.Length);
            value.CopyTo(target);
            int initLength = value.Length;
            target.Slice(initLength, result.Length - initLength).Clear();
            return result;
        }
#endif
    }
}

#if !NETCOREAPP3_1_OR_GREATER
internal static partial class InlineArrayIndexerExtensions
{
    internal static unsafe ref readonly char ReadOnlyItemRef(this in __char_1 @this, int index)
    {
        return ref @this.Value[index];
    }
}
#endif

internal static class SpanExtensions
{
    public static ReadOnlySpan<char> SliceAtNull(this ReadOnlySpan<char> value)
    {
        int length = value.IndexOf('\0');
        return length < 0 ? value : value.Slice(0, length);
    }
}

#if NETFRAMEWORK

namespace System.Diagnostics.CodeAnalysis
{
    [AttributeUsageAttribute(AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    internal sealed class UnscopedRefAttribute
        : Attribute
    {
    }
}

#endif

[AArnott]

Maybe we can have a NativeMethods.json setting where folks can switch between minimal APIs vs. multi-targeting friendliness. Minimal might be default and what WinForms goes for.

[AArnott]

Well, I remember now why we can't (always) use fixed T Value[x] fields: C# only lets us use them for a small set of predefined types.

[AArnott]

@tannergooding @AaronRobinsonMSFT I'm trying to find any way besides T _0, _1, _2 to declare an inline an array of non-primitive types. Can you advise?

What packing concerns apply when we're talking about inline arrays? Is a C# declaration of T[3] inline array equivalent to T _0, _1, _2 in terms of memory locations?
Since C# doesn't allow fixed CustomStruct Value[3], what if I did fixed byte Value[3*sizeof(CustomStruct)]? Would that screw up the memory layout by assuming one CustomStruct comes immediately after the prior one without concerns of memory alignment?

[AaronRobinsonMSFT]

what if I did fixed byte Value[3*sizeof(CustomStruct)]?

That is illegal because sizeof(CustomStruct) isn't known at compile time. My recommendation is to have N fields for the desired size. This isn't what everyone agrees with but is what I would recommend. It is also possible to define an arbitrary size similar to what Roslyn generates for the fixed keyword. Notice the below is of size 12, so this is generated for a field such as fixed int f[3].

    [StructLayout(LayoutKind.Sequential, Size = 12)]
    [CompilerGenerated]
    [UnsafeValueType]
    public struct FixedBuffer
    {
        public int FixedElementField;
    }

There is discussion of supporting something like this at dotnet/runtime#61135.

Since C# doesn't allow fixed CustomStruct Value[3]

I'm going to assume that CustomStruct is unmanaged and should ideally be blittable.

[AArnott]

I'm going to assume that CustomStruct is unmanaged and should ideally be blittable.

That depends. Some structs we generate are just unmanaged and blittable naturally. Others may be managed, and in fact are by default unless the allowMarshaling option is set to false.

It is also possible to define an arbitrary size similar to what Roslyn generates for the fixed keyword.

But that requires knowing the size at compile-time, which as you point out, we don't, generally. I see now why C# has such a limited set of types it allows fixed to be used for.

I guess I'll use fixed where it is allowed, and the _0, _1, _2 approach in other cases.
Thanks.