[Bug] Some (Array) CopyTo methods seems to be broken

[Hiroko103]

System.Array1.CopyTo(Span destination, int srcOffset)

public void CopyTo(Span<T> destination, int srcOffset)
{
  Debug.Assert((uint)srcOffset + (uint)destination.[Friend]mLength <= (uint)mLength);
  Internal.MemMove(destination.Ptr, &GetRef(srcOffset), strideof(T) * (destination.[Friend]mLength - srcOffset), alignof(T));
}

My expectation about the above method's name and parameters is this:

The method would copy everything from the SOURCE array from srcOffset until the end of SOURCE to the start of the DESTINATION array.

So If I have a source array (A) with element count of 10, and I run A.CopyTo(B, 5); I would expect the last 5 items of array A to be copied to array B.

For start, just one simple example where this method fails:

• If the source and destination arrays are the same size, and srcOffset is greater than 0

In this case, the assertion wants the SOURCE array to be bigger than the DESTINATION array, but why? If srcOffset is greater than zero, that means we want to copy less elements than the size of SOURCE itself, so it should still fit into DESTINATION.

I think there are two valid assertions that can be made here:

• Checking that srcOffset is not greater than the size of SOURCE array
• Checking that 'SOURCE - srcOffset' (the element count we want to copy) is not larger than the DESTINATION (so that it can fit in)

The MemMove part also contains wrong calculations I think. The third parameter (length) takes into account the length of the DESTINATION array, while I think it should subtract srcOffset from the SOURCE array's length.

namespace beefbug;

using System;
using System.Diagnostics;

class Program
{
    public static void Main(){
        char8[] a = scope char8[10]('0', '1', '2', '3', '4', '5', '6', '7', '8', '9');
        char8[] b = scope char8[5]();

        Print("A", a);
        Print("B", b);

        a.CopyTo(b, 4);

        Print("A", a);
        Print("B", b);
    }

    public static void Print(StringView name, char8[] arr){
        Debug.Write("{}: [", name);
        for(let i < arr.Count){
            Debug.Write("{}", arr[i] == 0 ? '-' : arr[i]);
        }
        Debug.WriteLine("]");
    }
}

This prints the following:

A: [0123456789]
B: [-----]
A: [0123456789]
B: [4----]

First, It should't even run, because CopyTo(b, 4) would mean that we want to copy 6 elements to an array whose size is 5. But because MemMove calculates wrong length, it doesn't crash.

This one is a great example because it highlights problems in both lines in CopyTo.

Let's continue:

Changing the size of array B to 6 and leaving CopyTo(b, 4) to the same, I would expect the last 6 elements of array A to be copied to array B (filling it fully) like this: B: [456789]

But this is what I get:

B: [45----]

public void CopyTo<T2>(Span<T2> destination, int srcOffset) where T2 : operator explicit T This also may be affected by wrong calculations.

I don't know if these CopyTo were meant to:

• fill the destination fully (according to srcOffset). By looking at the assertion, I would guess this one. OR
• copy everything from srcOffset until the end TO the destination (in which case the destination may not be filled fully)

Either way, I think it has bugs.

Version: 0.43.5 (Nightly 08/23/2024)

[bfiete]

Yeah, looks like a bad checkin back here https://github.com/beefytech/Beef/commit/dd33da0d7cbe56f9e32b981e4f31d28e78d18261

[bfiete]

Fixed at 9f7793a87021b7441032f208fcc8623bc0d971c1

I think there was some confusion on the part of the original contributor as to whether the length should be inferred from the destination span length or from the source array length.

I actually put a little documentation on the methods to clarify, but it's the source array length that determines the number of copied elements.

[Hiroko103]

Thank you for the quick fix, the extra methods and for the clarification. I also thought this should be the correct behaviour.

Those methods that convert the elements from T to T2, they still contain wrong loop condition:

for (int i = 0; i < destination.[Friend]mLength; i++) should be for (int i = 0; i < length; i++)

Everything else is looking good.

[bfiete]

Ah yes good catch. Fixed at 5a31cc35ba754baefb49af73683a9d7b1e19764e