Add a test case for List.dropAt at index in the middle of list

[ageron]

No description provided.

[lukewilliamboswell]

@ageron will you investigate these test failures?

I am guess you added this test as you suspected there was a bug in this implementation. Just wanting to clarify what you are thinking here.

[bhansconnect]

This should be the fix I think:

diff --git a/crates/compiler/builtins/bitcode/src/list.zig b/crates/compiler/builtins/bitcode/src/list.zig
index e89b39c561..c95e6e8063 100644
--- a/crates/compiler/builtins/bitcode/src/list.zig
+++ b/crates/compiler/builtins/bitcode/src/list.zig
@@ -618,6 +618,16 @@ pub fn listDropAt(
 ) callconv(.C) RocList {
     const size = list.len();
     const size_u64 = @as(u64, @intCast(size));
+
+    // NOTE
+    // we need to return an empty list explicitly,
+    // because we rely on the pointer field being null if the list is empty
+    // which also requires duplicating the utils.decref call to spend the RC token
+    if (size <= 1) {
+        list.decref(alignment, element_width, elements_refcounted, dec);
+        return RocList.empty();
+    }
+
     // If droping the first or last element, return a seamless slice.
     // For simplicity, do this by calling listSublist.
     // In the future, we can test if it is faster to manually inline the important parts here.
@@ -638,25 +648,16 @@ pub fn listDropAt(
         // were >= than `size`, and we know `size` fits in usize.
         const drop_index: usize = @intCast(drop_index_u64);
 
-        if (elements_refcounted) {
-            const element = source_ptr + drop_index * element_width;
-            dec(element);
-        }
-
-        // NOTE
-        // we need to return an empty list explicitly,
-        // because we rely on the pointer field being null if the list is empty
-        // which also requires duplicating the utils.decref call to spend the RC token
-        if (size < 2) {
-            list.decref(alignment, element_width, elements_refcounted, dec);
-            return RocList.empty();
-        }
-
         if (list.isUnique()) {
-            var i = drop_index;
-            const copy_target = source_ptr;
+            if (elements_refcounted) {
+                const element = source_ptr + drop_index * element_width;
+                dec(element);
+            }
+
+            const copy_target = source_ptr + (drop_index * element_width);
             const copy_source = copy_target + element_width;
-            std.mem.copyForwards(u8, copy_target[i..size], copy_source[i..size]);
+            const copy_size = (size - drop_index - 1) * element_width;
+            std.mem.copyForwards(u8, copy_target[0..copy_size], copy_source[0..copy_size]);
 
             var new_list = list;
 

[JRI98]

#7065

[ageron]

@ageron will you investigate these test failures?

I am guess you added this test as you suspected there was a bug in this implementation. Just wanting to clarify what you are thinking here.

I initially wanted to fix the issue, but I started digging into the Zig code and realized I was in over my head, so I preferred to explain what I found and add a test case, and hope that someone smarter than me can fix the actual issue.

[ageron]

Hi @bhansconnect , thanks for the fix, I just tried it and it works. 👍 I misunderstood your comment above, I thought you had pushed the fix in another PR, but I think you meant for me to add it here, so I just did. Sorry for the confusion!

[bhansconnect]

Yeah, I probably should have just pushed a PR, but I wasn't sure if someone was already working on a fix or not. I just thought I knew the what the fix would be so I wrote something up and posted, but didn't want to stomp someone else's work if this was already being fixed.

[ageron]

@bhansconnect , please feel to approve this PR or to submit another one, as you prefer.

[lukewilliamboswell]

Looks good 😄