From 48954eb52b5f7a8d5ac194e6d3bdea3a05d9bcb0 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 21:10:43 -0400
Subject: [PATCH 01/24] Update to Inkwell version with memcpy

---
 Cargo.lock              | 28 ++++++++++++++--------------
 compiler/gen/Cargo.toml |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 0fc3211e9b..6bace12f3e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2,9 +2,9 @@
 # It is not intended for manual editing.
 [[package]]
 name = "aho-corasick"
-version = "0.7.9"
+version = "0.7.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d5e63fd144e18ba274ae7095c0197a870a7b9468abc801dd62f190d80817d2ec"
+checksum = "8716408b8bc624ed7f65d223ddb9ac2d044c0547b6fa4b0d554f3a9540496ada"
 dependencies = [
  "memchr",
 ]
@@ -337,7 +337,7 @@ dependencies = [
 [[package]]
 name = "inkwell"
 version = "0.1.0"
-source = "git+https://github.com/rtfeldman/inkwell?tag=llvm8-0.release1#647a7cbfc2a6be3b41b8de3b1c63ec1fe4e79267"
+source = "git+https://github.com/rtfeldman/inkwell?tag=llvm8-0.release2#afdfd85c1183869e5d17b930c798c3087ff1c737"
 dependencies = [
  "either",
  "inkwell_internals",
@@ -351,7 +351,7 @@ dependencies = [
 [[package]]
 name = "inkwell_internals"
 version = "0.1.0"
-source = "git+https://github.com/rtfeldman/inkwell?tag=llvm8-0.release1#647a7cbfc2a6be3b41b8de3b1c63ec1fe4e79267"
+source = "git+https://github.com/rtfeldman/inkwell?tag=llvm8-0.release2#afdfd85c1183869e5d17b930c798c3087ff1c737"
 dependencies = [
  "proc-macro2 0.4.30",
  "quote 0.6.13",
@@ -372,9 +372,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646"
 
 [[package]]
 name = "libc"
-version = "0.2.67"
+version = "0.2.68"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "eb147597cdf94ed43ab7a9038716637d2d1bf2bc571da995d0028dec06bd3018"
+checksum = "dea0c0405123bba743ee3f91f49b1c7cfb684eef0da0a50110f758ccf24cdff0"
 
 [[package]]
 name = "llvm-sys"
@@ -486,9 +486,9 @@ dependencies = [
 
 [[package]]
 name = "proc-macro-hack"
-version = "0.5.11"
+version = "0.5.12"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ecd45702f76d6d3c75a80564378ae228a85f0b59d2f3ed43c91b4a69eb2ebfc5"
+checksum = "f918f2b601f93baa836c1c2945faef682ba5b6d4828ecb45eeb7cc3c71b811b4"
 dependencies = [
  "proc-macro2 1.0.9",
  "quote 1.0.3",
@@ -703,9 +703,9 @@ checksum = "2439c63f3f6139d1b57529d16bc3b8bb855230c8efcc5d3a896c8bea7c3b1e84"
 
 [[package]]
 name = "regex"
-version = "1.3.4"
+version = "1.3.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "322cf97724bea3ee221b78fe25ac9c46114ebb51747ad5babd51a2fc6a8235a8"
+checksum = "8900ebc1363efa7ea1c399ccc32daed870b4002651e0bed86e72d501ebbe0048"
 dependencies = [
  "aho-corasick",
  "memchr",
@@ -715,9 +715,9 @@ dependencies = [
 
 [[package]]
 name = "regex-syntax"
-version = "0.6.16"
+version = "0.6.17"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1132f845907680735a84409c3bebc64d1364a5683ffbce899550cd09d5eaefc1"
+checksum = "7fe5bd57d1d7414c6b5ed48563a2c855d995ff777729dcd91c369ec7fea395ae"
 
 [[package]]
 name = "region"
@@ -1085,9 +1085,9 @@ checksum = "388a1df253eca08550bef6c72392cfe7c30914bf41df5269b68cbd6ff8f570a3"
 
 [[package]]
 name = "sized-chunks"
-version = "0.5.1"
+version = "0.5.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6f59f81ec9833a580d2448e958d16bd872637798f3ab300b693c48f136fb76ff"
+checksum = "d59044ea371ad781ff976f7b06480b9f0180e834eda94114f2afb4afc12b7718"
 dependencies = [
  "bitmaps",
  "typenum",
diff --git a/compiler/gen/Cargo.toml b/compiler/gen/Cargo.toml
index 20838969b7..72cb231524 100644
--- a/compiler/gen/Cargo.toml
+++ b/compiler/gen/Cargo.toml
@@ -37,7 +37,7 @@ inlinable_string = "0.1.0"
 # commit of TheDan64/inkwell, push a new tag which points to the latest commit,
 # change the tag value in this Cargo.toml to point to that tag, and `cargo update`.
 # This way, GitHub Actions works and nobody's builds get broken.
-inkwell = { git = "https://github.com/rtfeldman/inkwell", tag = "llvm8-0.release1" }
+inkwell = { git = "https://github.com/rtfeldman/inkwell", tag = "llvm8-0.release2" }
 target-lexicon = "0.10" # NOTE: we must use the same version of target-lexicon as cranelift!
 cranelift           = "0.59" # All cranelift crates should have the same version!
 cranelift-simplejit = "0.59" # All cranelift crates should have the same version!

From fc036453d9b30bc97377ffb4d4ab6c6d19653897 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 21:11:37 -0400
Subject: [PATCH 02/24] Have List.set in LLVM clone before mutating

---
 compiler/gen/src/llvm/build.rs | 161 ++++++++++++++++++++++++++++-----
 compiler/gen/tests/test_gen.rs |   5 +-
 2 files changed, 142 insertions(+), 24 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 1d1e015355..948d5ee463 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -287,7 +287,7 @@ pub fn build_expr<'a, 'ctx, 'env>(
                 let len = BasicValueEnum::IntValue(env.ptr_int().const_int(len_u64, false));
                 let mut struct_val;
 
-                // Field 0: pointer
+                // Store the pointer
                 struct_val = builder
                     .build_insert_value(
                         struct_type.const_zero(),
@@ -297,7 +297,7 @@ pub fn build_expr<'a, 'ctx, 'env>(
                     )
                     .unwrap();
 
-                // Field 1: length
+                // Store the length
                 struct_val = builder
                     .build_insert_value(struct_val, len, Builtin::WRAPPER_LEN, "insert_len")
                     .unwrap();
@@ -997,18 +997,14 @@ fn call_with_args<'a, 'ctx, 'env>(
         Symbol::LIST_LEN => {
             debug_assert!(args.len() == 1);
 
-            let wrapper_struct = args[0].0.into_struct_value();
-            let builder = env.builder;
-
-            // Get the usize int length
-            builder.build_extract_value(wrapper_struct, Builtin::WRAPPER_LEN, "unwrapped_list_len").unwrap().into_int_value().into()
+            BasicValueEnum::IntValue(load_list_len(env.builder, args[0].0.into_struct_value()))
         }
         Symbol::LIST_IS_EMPTY => {
             debug_assert!(args.len() == 1);
 
             let list_struct = args[0].0.into_struct_value();
             let builder = env.builder;
-            let list_len = builder.build_extract_value(list_struct, 1, "unwrapped_list_len").unwrap().into_int_value();
+            let list_len = load_list_len(builder, list_struct);
             let zero = env.ptr_int().const_zero();
             let answer = builder.build_int_compare(IntPredicate::EQ, list_len, zero, "is_zero");
 
@@ -1073,16 +1069,15 @@ fn call_with_args<'a, 'ctx, 'env>(
             let wrapper_struct = args[0].0.into_struct_value();
             let elem_index = args[1].0.into_int_value();
 
-            // Get the length from the wrapper struct
-            let _list_len = builder.build_extract_value(wrapper_struct, Builtin::WRAPPER_LEN, "unwrapped_list_len").unwrap().into_int_value();
+            // Get the usize length from the wrapper struct
+            let _list_len = load_list_len(builder, wrapper_struct);
 
             // TODO here, check to see if the requested index exceeds the length of the array.
 
             match list_layout {
                 Layout::Builtin(Builtin::List(elem_layout)) => {
-                    // Get the pointer to the array data
-                    let array_data_ptr = builder.build_extract_value(wrapper_struct, Builtin::WRAPPER_PTR, "unwrapped_list_ptr").unwrap().into_pointer_value();
-
+                    // Load the pointer to the array data
+                    let array_data_ptr = load_list_ptr(builder, wrapper_struct);
                     let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
                     let elem_size = env.context.i64_type().const_int(elem_bytes, false);
 
@@ -1090,7 +1085,9 @@ fn call_with_args<'a, 'ctx, 'env>(
                     let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
                     // We already checked the bounds earlier.
-                    let elem_ptr = unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+                    let elem_ptr = unsafe {
+                        builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem")
+                    };
 
                     builder.build_load(elem_ptr, "List.get")
                 }
@@ -1099,7 +1096,7 @@ fn call_with_args<'a, 'ctx, 'env>(
                 }
             }
         }
-        Symbol::LIST_SET /* TODO clone first for LIST_SET! */ | Symbol::LIST_SET_IN_PLACE => {
+        Symbol::LIST_SET => {
             let builder = env.builder;
 
             debug_assert!(args.len() == 3);
@@ -1108,14 +1105,17 @@ fn call_with_args<'a, 'ctx, 'env>(
             let elem_index = args[1].0.into_int_value();
             let (elem, elem_layout) = args[2];
 
-            // Slot 1 in the wrapper struct is the length
-            let _list_len = builder.build_extract_value(wrapper_struct, Builtin::WRAPPER_LEN, "unwrapped_list_len").unwrap().into_int_value();
+            // Load the usize length
+            let _list_len = load_list_len(builder, wrapper_struct);
 
             // TODO here, check to see if the requested index exceeds the length of the array.
             // If so, bail out and return the list unaltered.
 
-            // Slot 0 in the wrapper struct is the pointer to the array data
-            let array_data_ptr = builder.build_extract_value(wrapper_struct, Builtin::WRAPPER_PTR, "unwrapped_list_ptr").unwrap().into_pointer_value();
+            // TODO pass the length and data ptr to clone_list, so it doesn't load them twice
+            let wrapper_struct = clone_list(env, wrapper_struct, elem_layout);
+
+            // Load the pointer to the elements
+            let array_data_ptr = load_list_ptr(builder, wrapper_struct);
 
             let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
             let elem_size = env.context.i64_type().const_int(elem_bytes, false);
@@ -1124,7 +1124,42 @@ fn call_with_args<'a, 'ctx, 'env>(
             let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
             // We already checked the bounds earlier.
-            let elem_ptr = unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+            let elem_ptr =
+                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+
+            // Mutate the array in-place.
+            builder.build_store(elem_ptr, elem);
+
+            // Return the wrapper unchanged, since pointer, length and capacity are all unchanged
+            wrapper_struct.into()
+        }
+        Symbol::LIST_SET_IN_PLACE => {
+            let builder = env.builder;
+
+            debug_assert!(args.len() == 3);
+
+            let wrapper_struct = args[0].0.into_struct_value();
+            let elem_index = args[1].0.into_int_value();
+            let (elem, elem_layout) = args[2];
+
+            // Load the usize length
+            let _list_len = load_list_len(builder, wrapper_struct);
+
+            // TODO here, check to see if the requested index exceeds the length of the array.
+            // If so, bail out and return the list unaltered.
+
+            // Load the pointer to the elements
+            let array_data_ptr = load_list_ptr(builder, wrapper_struct);
+
+            let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
+            let elem_size = env.context.i64_type().const_int(elem_bytes, false);
+
+            // Calculate the offset at runtime by multiplying the index by the size of an element.
+            let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
+
+            // We already checked the bounds earlier.
+            let elem_ptr =
+                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
 
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
@@ -1144,7 +1179,9 @@ fn call_with_args<'a, 'ctx, 'env>(
                 arg_vals.push(*arg);
             }
 
-            let call = env.builder.build_call(fn_val, arg_vals.into_bump_slice(), "call");
+            let call = env
+                .builder
+                .build_call(fn_val, arg_vals.into_bump_slice(), "call");
 
             call.try_as_basic_value()
                 .left()
@@ -1152,3 +1189,85 @@ fn call_with_args<'a, 'ctx, 'env>(
         }
     }
 }
+
+fn load_list_len<'a, 'ctx, 'env>(
+    builder: &Builder<'ctx>,
+    wrapper_struct: StructValue<'ctx>,
+) -> IntValue<'ctx> {
+    builder
+        .build_extract_value(wrapper_struct, Builtin::WRAPPER_LEN, "list_len")
+        .unwrap()
+        .into_int_value()
+}
+
+fn load_list_ptr<'ctx>(
+    builder: &Builder<'ctx>,
+    wrapper_struct: StructValue<'ctx>,
+) -> PointerValue<'ctx> {
+    builder
+        .build_extract_value(wrapper_struct, Builtin::WRAPPER_PTR, "list_ptr")
+        .unwrap()
+        .into_pointer_value()
+}
+
+fn clone_list<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    wrapper_struct: StructValue<'ctx>,
+    elem_layout: &Layout<'_>,
+) -> StructValue<'ctx> {
+    let builder = env.builder;
+    let ctx = env.context;
+    let elems_ptr = load_list_ptr(builder, wrapper_struct);
+    let list_len = load_list_len(builder, wrapper_struct);
+    let ptr_bytes = env.ptr_bytes;
+
+    // Calculate the number of bytes we'll need to allocate.
+    let elem_bytes = env
+        .ptr_int()
+        .const_int(elem_layout.stack_size(env.ptr_bytes) as u64, false);
+    let size = env
+        .builder
+        .build_int_mul(elem_bytes, list_len, "mul_len_by_elem_bytes");
+
+    // Allocate space for the new array that we'll copy into.
+    let elem_type = basic_type_from_layout(env.arena, ctx, elem_layout, env.ptr_bytes);
+    let ptr_val = builder
+        .build_array_malloc(elem_type, list_len, "list_ptr")
+        .unwrap();
+
+    // Either memcpy or deep clone the array elements
+    if elem_layout.safe_to_memcpy() {
+        // Copy the bytes from the original array into the new
+        // one we just malloc'd.
+        //
+        // TODO how do we decide when to do the small memcpy vs the normal one?
+        builder
+            .build_memcpy(ptr_val, ptr_bytes, elems_ptr, ptr_bytes, size)
+            .unwrap_or_else(|err| {
+                panic!("Error while attempting LLVM memcpy: {:?}", err);
+            });
+    } else {
+        panic!("TODO Cranelift currently only knows how to clone list elements that are Copy.");
+    }
+
+    //// Create a fresh wrapper struct for the newly populated array
+    let struct_type = collection_wrapper(ctx, ptr_val.get_type(), env.ptr_bytes);
+    let mut struct_val;
+
+    // Store the pointer
+    struct_val = builder
+        .build_insert_value(
+            struct_type.const_zero(),
+            ptr_val,
+            Builtin::WRAPPER_PTR,
+            "insert_ptr",
+        )
+        .unwrap();
+
+    // Store the length
+    struct_val = builder
+        .build_insert_value(struct_val, list_len, Builtin::WRAPPER_LEN, "insert_len")
+        .unwrap();
+
+    struct_val.into_struct_value()
+}
diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 42a5716fdd..aef9216078 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -553,7 +553,7 @@ mod test_gen {
 
     #[test]
     fn set_shared_int_list() {
-        assert_crane_evals_to!(
+        assert_evals_to!(
             indoc!(
                 r#"
                     shared = [ 2, 4 ]
@@ -565,8 +565,7 @@ mod test_gen {
                 "#
             ),
             4,
-            i64,
-            |x| x
+            i64
         );
     }
 

From f2419bb9f8296315ffeaee1b09acca497fb21a32 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 22:13:41 -0400
Subject: [PATCH 03/24] Use struct_type.get_undef()

---
 compiler/gen/src/llvm/build.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 948d5ee463..b3161d749c 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -250,7 +250,7 @@ pub fn build_expr<'a, 'ctx, 'env>(
                 // The first field in the struct should be the pointer.
                 let struct_val = builder
                     .build_insert_value(
-                        struct_type.const_zero(),
+                        struct_type.get_undef(),
                         BasicValueEnum::PointerValue(ptr_type.const_null()),
                         Builtin::WRAPPER_PTR,
                         "insert_ptr",
@@ -290,7 +290,7 @@ pub fn build_expr<'a, 'ctx, 'env>(
                 // Store the pointer
                 struct_val = builder
                     .build_insert_value(
-                        struct_type.const_zero(),
+                        struct_type.get_undef(),
                         ptr_val,
                         Builtin::WRAPPER_PTR,
                         "insert_ptr",
@@ -1250,14 +1250,14 @@ fn clone_list<'a, 'ctx, 'env>(
         panic!("TODO Cranelift currently only knows how to clone list elements that are Copy.");
     }
 
-    //// Create a fresh wrapper struct for the newly populated array
+    // Create a fresh wrapper struct for the newly populated array
     let struct_type = collection_wrapper(ctx, ptr_val.get_type(), env.ptr_bytes);
     let mut struct_val;
 
     // Store the pointer
     struct_val = builder
         .build_insert_value(
-            struct_type.const_zero(),
+            struct_type.get_undef(),
             ptr_val,
             Builtin::WRAPPER_PTR,
             "insert_ptr",

From 5b902b5a49110c274d88e1d7a2a541fb1b8c6453 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 22:20:13 -0400
Subject: [PATCH 04/24] Use ptr_int over hardcoded i64

---
 compiler/gen/src/llvm/build.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index b3161d749c..c8dcf5b08c 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1079,7 +1079,7 @@ fn call_with_args<'a, 'ctx, 'env>(
                     // Load the pointer to the array data
                     let array_data_ptr = load_list_ptr(builder, wrapper_struct);
                     let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-                    let elem_size = env.context.i64_type().const_int(elem_bytes, false);
+                    let elem_size = env.ptr_int().const_int(elem_bytes, false);
 
                     // Calculate the offset at runtime by multiplying the index by the size of an element.
                     let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
@@ -1118,7 +1118,7 @@ fn call_with_args<'a, 'ctx, 'env>(
             let array_data_ptr = load_list_ptr(builder, wrapper_struct);
 
             let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-            let elem_size = env.context.i64_type().const_int(elem_bytes, false);
+            let elem_size = env.ptr_int().const_int(elem_bytes, false);
 
             // Calculate the offset at runtime by multiplying the index by the size of an element.
             let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");

From f86c3f1e7e8e38aba7f3b1317b020e345c4367d4 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 22:20:28 -0400
Subject: [PATCH 05/24] Avoid some redundant stores/loads

---
 compiler/gen/src/llvm/build.rs | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index c8dcf5b08c..f20914ac20 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1101,21 +1101,26 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             debug_assert!(args.len() == 3);
 
-            let wrapper_struct = args[0].0.into_struct_value();
             let elem_index = args[1].0.into_int_value();
             let (elem, elem_layout) = args[2];
+            let (wrapper_struct, array_data_ptr) = {
+                // This original wrapper_struct should only stay in scope long enough to clone it.
+                // From then on, we should only ever reference the clone!
+                let wrapper_struct = args[0].0.into_struct_value();
 
-            // Load the usize length
-            let _list_len = load_list_len(builder, wrapper_struct);
+                // Load the usize length
+                let list_len = load_list_len(builder, wrapper_struct);
 
-            // TODO here, check to see if the requested index exceeds the length of the array.
-            // If so, bail out and return the list unaltered.
+                // TODO here, check to see if the requested index exceeds the length of the array.
+                // If so, bail out and return the list unaltered.
 
-            // TODO pass the length and data ptr to clone_list, so it doesn't load them twice
-            let wrapper_struct = clone_list(env, wrapper_struct, elem_layout);
-
-            // Load the pointer to the elements
-            let array_data_ptr = load_list_ptr(builder, wrapper_struct);
+                clone_list(
+                    env,
+                    list_len,
+                    load_list_ptr(builder, wrapper_struct),
+                    elem_layout,
+                )
+            };
 
             let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
             let elem_size = env.ptr_int().const_int(elem_bytes, false);
@@ -1212,13 +1217,12 @@ fn load_list_ptr<'ctx>(
 
 fn clone_list<'a, 'ctx, 'env>(
     env: &Env<'a, 'ctx, 'env>,
-    wrapper_struct: StructValue<'ctx>,
+    list_len: IntValue<'ctx>,
+    elems_ptr: PointerValue<'ctx>,
     elem_layout: &Layout<'_>,
-) -> StructValue<'ctx> {
+) -> (StructValue<'ctx>, PointerValue<'ctx>) {
     let builder = env.builder;
     let ctx = env.context;
-    let elems_ptr = load_list_ptr(builder, wrapper_struct);
-    let list_len = load_list_len(builder, wrapper_struct);
     let ptr_bytes = env.ptr_bytes;
 
     // Calculate the number of bytes we'll need to allocate.
@@ -1269,5 +1273,5 @@ fn clone_list<'a, 'ctx, 'env>(
         .build_insert_value(struct_val, list_len, Builtin::WRAPPER_LEN, "insert_len")
         .unwrap();
 
-    struct_val.into_struct_value()
+    (struct_val.into_struct_value(), ptr_val)
 }

From a286dec4cc0c2190ad09a73b08a2f934ed1d2a43 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 22:38:28 -0400
Subject: [PATCH 06/24] Reorganize some things

---
 compiler/gen/src/llvm/build.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index f20914ac20..89b65d06c7 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1101,7 +1101,6 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             debug_assert!(args.len() == 3);
 
-            let elem_index = args[1].0.into_int_value();
             let (elem, elem_layout) = args[2];
             let (wrapper_struct, array_data_ptr) = {
                 // This original wrapper_struct should only stay in scope long enough to clone it.
@@ -1126,6 +1125,7 @@ fn call_with_args<'a, 'ctx, 'env>(
             let elem_size = env.ptr_int().const_int(elem_bytes, false);
 
             // Calculate the offset at runtime by multiplying the index by the size of an element.
+            let elem_index = args[1].0.into_int_value();
             let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
             // We already checked the bounds earlier.
@@ -1135,8 +1135,8 @@ fn call_with_args<'a, 'ctx, 'env>(
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
 
-            // Return the wrapper unchanged, since pointer, length and capacity are all unchanged
-            wrapper_struct.into()
+            // Return the cloned wrapper
+            BasicValueEnum::StructValue(wrapper_struct)
         }
         Symbol::LIST_SET_IN_PLACE => {
             let builder = env.builder;
@@ -1169,8 +1169,8 @@ fn call_with_args<'a, 'ctx, 'env>(
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
 
-            // Return the wrapper unchanged, since pointer, length and capacity are all unchanged
-            wrapper_struct.into()
+            // Return the wrapper unchanged, since pointer and length are unchanged
+            BasicValueEnum::StructValue(wrapper_struct)
         }
         _ => {
             let fn_val = env

From 3a6018d22315bf76b7347b4a9da2d7e3e3931701 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 23:17:38 -0400
Subject: [PATCH 07/24] Reproduce List.set uniqueness bug

---
 compiler/mono/tests/test_opt.rs | 193 ++++++++++++++++++++++++++++++++
 1 file changed, 193 insertions(+)

diff --git a/compiler/mono/tests/test_opt.rs b/compiler/mono/tests/test_opt.rs
index 7acdd776cb..773648ddea 100644
--- a/compiler/mono/tests/test_opt.rs
+++ b/compiler/mono/tests/test_opt.rs
@@ -20,6 +20,183 @@ mod test_opt {
 
     // HELPERS
 
+    #[derive(Debug, Default, PartialEq, Eq)]
+    struct CallProblems {
+        missing: Vec<Symbol>,
+        unexpected: Vec<Symbol>,
+    }
+
+    fn contains_named_calls(src: &str, mut calls: Vec<Symbol>) {
+        let arena = Bump::new();
+        let (loc_expr, _, _problems, subs, var, constraint, home, mut interns) = uniq_expr(src);
+
+        let mut unify_problems = Vec::new();
+        let (_content, mut subs) = infer_expr(subs, &mut unify_problems, &constraint, var);
+
+        // Compile and add all the Procs before adding main
+        let mut procs = Procs::default();
+        let mut ident_ids = interns.all_ident_ids.remove(&home).unwrap();
+
+        // assume 64-bit pointers
+        let pointer_size = std::mem::size_of::<u64>() as u32;
+
+        // Populate Procs and Subs, and get the low-level Expr from the canonical Expr
+        let mono_expr = Expr::new(
+            &arena,
+            &mut subs,
+            loc_expr.value,
+            &mut procs,
+            home,
+            &mut ident_ids,
+            pointer_size,
+        );
+
+        let unexpected_calls = extract_named_calls(&mono_expr, &mut calls);
+        let expected = CallProblems::default();
+        let actual = CallProblems {
+            missing: calls,
+            unexpected: unexpected_calls,
+        };
+
+        assert_eq!(expected, actual);
+    }
+
+    fn extract_named_calls(expr: &Expr<'_>, calls: &mut Vec<Symbol>) -> Vec<Symbol> {
+        let mut unexpected_calls = Vec::new();
+
+        // The calls must be sorted so we can binary_search them for matches.
+        calls.sort();
+
+        extract_named_calls_help(expr, calls, &mut unexpected_calls);
+
+        unexpected_calls
+    }
+    fn extract_named_calls_help(
+        expr: &Expr<'_>,
+        calls: &mut Vec<Symbol>,
+        unexpected_calls: &mut Vec<Symbol>,
+    ) {
+        match expr {
+            Int(_) | Float(_) | Str(_) | Bool(_) | Byte(_) | Load(_) | FunctionPointer(_)
+            | Jump(_) | RuntimeError(_) => (),
+
+            Store(paths, sub_expr) => {
+                for (_, _, path_expr) in paths.iter() {
+                    extract_named_calls_help(path_expr, calls, unexpected_calls);
+                }
+
+                extract_named_calls_help(sub_expr, calls, unexpected_calls);
+            }
+
+            CallByPointer(sub_expr, args, _) => {
+                extract_named_calls_help(sub_expr, calls, unexpected_calls);
+
+                for arg in args.iter() {
+                    extract_named_calls_help(arg, calls, unexpected_calls);
+                }
+            }
+
+            CallByName(symbol, args) => {
+                // Search for the symbol. If we found it, check it off the list.
+                // If we didn't find it, add it to the list of unexpected calls.
+                match calls.binary_search(symbol) {
+                    Ok(index) => {
+                        calls.remove(index);
+                    }
+                    Err(_) => {
+                        unexpected_calls.push(*symbol);
+                    }
+                }
+
+                for (arg, _) in args.iter() {
+                    extract_named_calls_help(arg, calls, unexpected_calls);
+                }
+            }
+
+            Cond {
+                cond,
+                cond_layout: _,
+                pass,
+                fail,
+                ret_layout: _,
+            } => {
+                extract_named_calls_help(cond, calls, unexpected_calls);
+                extract_named_calls_help(pass, calls, unexpected_calls);
+                extract_named_calls_help(fail, calls, unexpected_calls);
+            }
+            Branches {
+                cond,
+                branches,
+                default,
+                ret_layout: _,
+            } => {
+                extract_named_calls_help(cond, calls, unexpected_calls);
+                extract_named_calls_help(default, calls, unexpected_calls);
+
+                for (a, b, c) in branches.iter() {
+                    extract_named_calls_help(a, calls, unexpected_calls);
+                    extract_named_calls_help(b, calls, unexpected_calls);
+                    extract_named_calls_help(c, calls, unexpected_calls);
+                }
+            }
+            Switch {
+                cond,
+                cond_layout: _,
+                branches,
+                default_branch,
+                ret_layout: _,
+            } => {
+                extract_named_calls_help(cond, calls, unexpected_calls);
+                extract_named_calls_help(default_branch, calls, unexpected_calls);
+
+                for (_, branch_expr) in branches.iter() {
+                    extract_named_calls_help(branch_expr, calls, unexpected_calls);
+                }
+            }
+
+            Tag {
+                tag_layout: _,
+                tag_name: _,
+                tag_id: _,
+                union_size: _,
+                arguments,
+            } => {
+                for (tag_expr, _) in arguments.iter() {
+                    extract_named_calls_help(tag_expr, calls, unexpected_calls);
+                }
+            }
+            Struct(fields) => {
+                for (field, _) in fields.iter() {
+                    extract_named_calls_help(field, calls, unexpected_calls);
+                }
+            }
+            Access {
+                label: _,
+                field_layout: _,
+                struct_layout: _,
+                record: sub_expr,
+            }
+            | AccessAtIndex {
+                index: _,
+                field_layouts: _,
+                expr: sub_expr,
+                is_unwrapped: _,
+            }
+            | Label(_, sub_expr) => {
+                extract_named_calls_help(sub_expr, calls, unexpected_calls);
+            }
+
+            Array {
+                elem_layout: _,
+                elems,
+            } => {
+                for elem in elems.iter() {
+                    extract_named_calls_help(elem, calls, unexpected_calls);
+                }
+            }
+        }
+    }
+
     fn compiles_to(src: &str, expected: Expr<'_>) {
         let arena = Bump::new();
         let (loc_expr, _, _problems, subs, var, constraint, home, mut interns) = uniq_expr(src);
@@ -90,4 +267,20 @@ mod test_opt {
             ),
         );
     }
+
+    #[test]
+    fn set_shared_int_list() {
+        // This should *NOT* optimize List.set to List.set_in_place
+        contains_named_calls(
+            r#"
+                shared = [ 2, 4 ]
+
+                # This should not mutate the original
+                x = List.set shared 1 77
+
+                List.getUnsafe shared 1
+            "#,
+            vec![Symbol::LIST_SET, Symbol::LIST_GET_UNSAFE],
+        );
+    }
 }

From d2e7e373e664855fb7bc3eb20712d1f8ca886c5b Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Fri, 20 Mar 2020 23:20:23 -0400
Subject: [PATCH 08/24] Remove unused lifetimes

---
 compiler/gen/src/llvm/build.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 89b65d06c7..d2d2da712c 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1195,7 +1195,7 @@ fn call_with_args<'a, 'ctx, 'env>(
     }
 }
 
-fn load_list_len<'a, 'ctx, 'env>(
+fn load_list_len<'ctx>(
     builder: &Builder<'ctx>,
     wrapper_struct: StructValue<'ctx>,
 ) -> IntValue<'ctx> {

From e6704d1fcb07253689925833ab4b71048a456e2f Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 12:52:41 -0400
Subject: [PATCH 09/24] Streamline empty list LLVM code gen

---
 compiler/gen/src/llvm/build.rs   | 22 ++++++----------------
 compiler/gen/src/llvm/convert.rs | 26 +++++++++++++++++---------
 compiler/gen/tests/test_gen.rs   |  5 +++++
 3 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index d2d2da712c..7ed0e04cbd 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -6,10 +6,10 @@ use inkwell::module::{Linkage, Module};
 use inkwell::types::{BasicTypeEnum, IntType, StructType};
 use inkwell::values::BasicValueEnum::{self, *};
 use inkwell::values::{FunctionValue, IntValue, PointerValue, StructValue};
-use inkwell::{AddressSpace, FloatPredicate, IntPredicate};
+use inkwell::{FloatPredicate, IntPredicate};
 
 use crate::llvm::convert::{
-    basic_type_from_layout, collection_wrapper, get_array_type, get_fn_type, ptr_int,
+    basic_type_from_layout, collection_wrapper, empty_collection, get_fn_type, ptr_int,
 };
 use roc_collections::all::ImMap;
 use roc_module::symbol::{Interns, Symbol};
@@ -243,21 +243,11 @@ pub fn build_expr<'a, 'ctx, 'env>(
             let builder = env.builder;
 
             if elems.is_empty() {
-                let array_type = get_array_type(&elem_type, 0);
-                let ptr_type = array_type.ptr_type(AddressSpace::Generic);
-                let struct_type = collection_wrapper(ctx, ptr_type, env.ptr_bytes);
+                let struct_type = empty_collection(ctx, env.ptr_bytes);
 
-                // The first field in the struct should be the pointer.
-                let struct_val = builder
-                    .build_insert_value(
-                        struct_type.get_undef(),
-                        BasicValueEnum::PointerValue(ptr_type.const_null()),
-                        Builtin::WRAPPER_PTR,
-                        "insert_ptr",
-                    )
-                    .unwrap();
-
-                BasicValueEnum::StructValue(struct_val.into_struct_value())
+                // THe pointer should be null (aka zero) and the length should be zero,
+                // so the whole struct should be a const_zero
+                BasicValueEnum::StructValue(struct_type.const_zero())
             } else {
                 let len_u64 = elems.len() as u64;
                 let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
diff --git a/compiler/gen/src/llvm/convert.rs b/compiler/gen/src/llvm/convert.rs
index 515910d4c8..57916f8a77 100644
--- a/compiler/gen/src/llvm/convert.rs
+++ b/compiler/gen/src/llvm/convert.rs
@@ -5,7 +5,7 @@ use inkwell::types::BasicTypeEnum::{self, *};
 use inkwell::types::{ArrayType, BasicType, FunctionType, IntType, PointerType, StructType};
 use inkwell::AddressSpace;
 
-use roc_mono::layout::Layout;
+use roc_mono::layout::{Builtin, Layout};
 
 /// TODO could this be added to Inkwell itself as a method on BasicValueEnum?
 pub fn get_fn_type<'ctx>(
@@ -123,18 +123,26 @@ pub fn basic_type_from_layout<'ctx>(
 
                 collection_wrapper(context, ptr_type, ptr_bytes).into()
             }
-            EmptyList => {
-                let array_type =
-                    get_array_type(&context.opaque_struct_type("empty_list_elem").into(), 0);
-                let ptr_type = array_type.ptr_type(AddressSpace::Generic);
-
-                collection_wrapper(context, ptr_type, ptr_bytes).into()
-            }
+            EmptyList => BasicTypeEnum::StructType(empty_collection(context, ptr_bytes)),
         },
     }
 }
 
-/// (pointer: usize, length: u32, capacity: u32)
+/// A length usize and a pointer to some elements.
+/// Could be a wrapper for a List or a Str.
+///
+/// The order of these doesn't matter, since they should be initialized
+/// to zero anyway for an empty collection; as such, we return a
+/// (usize, usize) struct layout no matter what.
+pub fn empty_collection<'ctx>(ctx: &'ctx Context, ptr_bytes: u32) -> StructType<'ctx> {
+    let usize_type = BasicTypeEnum::IntType(ptr_int(ctx, ptr_bytes));
+
+    ctx.struct_type(&[usize_type, usize_type], false)
+}
+
+/// A length usize and a pointer to some elements.
+///
+/// Could be a wrapper for a List or a Str.
 pub fn collection_wrapper<'ctx>(
     ctx: &'ctx Context,
     ptr_type: PointerType<'ctx>,
diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index aef9216078..46f5f48a65 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -535,6 +535,11 @@ mod test_gen {
     //     fn int_list_is_empty() {
     //         assert_evals_to!("List.isEmpty [ 12, 9, 6, 3 ]", 0, u8, |x| x);
     //     }
+    //
+    #[test]
+    fn empty_list_literal() {
+        assert_llvm_evals_to!("[]", &[], &'static [i64], |x| x);
+    }
 
     #[test]
     fn head_int_list() {

From 4182e5187758469546d11467643912680a59739a Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 20:50:03 -0400
Subject: [PATCH 10/24] Fix set_shared_int_list test

---
 compiler/gen/tests/test_gen.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 46f5f48a65..617c94ce8c 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -564,13 +564,13 @@ mod test_gen {
                     shared = [ 2, 4 ]
 
                     # This should not mutate the original
-                    x = List.set shared 1 77
+                    x = List.getUnsafe (List.set shared 1 77) 1
 
-                    List.getUnsafe shared 1
+                    { x, y: List.getUnsafe shared 1 }
                 "#
             ),
-            4,
-            i64
+            (4, 77),
+            (i64, i64)
         );
     }
 

From cc8683d2416ed0f929e5e211487adeb68a1c9f34 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 13:37:37 -0400
Subject: [PATCH 11/24] Fix use of GEP (was using byte offset, not index)

---
 Cargo.lock                       |  1 +
 compiler/gen/Cargo.toml          |  1 +
 compiler/gen/src/crane/build.rs  |  5 +++
 compiler/gen/src/llvm/build.rs   | 53 ++++++++++++++------------------
 compiler/gen/src/llvm/convert.rs |  9 +++++-
 compiler/gen/tests/test_gen.rs   |  6 ++++
 compiler/mono/src/layout.rs      |  6 ++--
 7 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 6bace12f3e..a537330d71 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -833,6 +833,7 @@ dependencies = [
  "indoc",
  "inkwell",
  "inlinable_string",
+ "libc",
  "maplit",
  "pretty_assertions",
  "quickcheck",
diff --git a/compiler/gen/Cargo.toml b/compiler/gen/Cargo.toml
index 72cb231524..c6fc276305 100644
--- a/compiler/gen/Cargo.toml
+++ b/compiler/gen/Cargo.toml
@@ -54,3 +54,4 @@ quickcheck = "0.8"
 quickcheck_macros = "0.8"
 tokio = { version = "0.2", features = ["blocking", "fs", "sync", "rt-threaded"] }
 bumpalo = { version = "3.2", features = ["collections"] }
+libc = "0.2"
diff --git a/compiler/gen/src/crane/build.rs b/compiler/gen/src/crane/build.rs
index 4727f9e65e..91baa2ad4f 100644
--- a/compiler/gen/src/crane/build.rs
+++ b/compiler/gen/src/crane/build.rs
@@ -351,6 +351,7 @@ pub fn build_expr<'a, B: Backend>(
                 let bytes_len = str_literal.len() + 1/* TODO drop the +1 when we have structs and this is no longer a NUL-terminated CString.*/;
                 let size = builder.ins().iconst(types::I64, bytes_len as i64);
                 let ptr = call_malloc(env, module, builder, size);
+                // TODO check if malloc returned null; if so, runtime error for OOM!
                 let mem_flags = MemFlags::new();
 
                 // Store the bytes from the string literal in the array
@@ -389,6 +390,8 @@ pub fn build_expr<'a, B: Backend>(
                 let bytes_len = elem_bytes as usize * elems.len();
                 let size = builder.ins().iconst(types::I64, bytes_len as i64);
                 let elems_ptr = call_malloc(env, module, builder, size);
+
+                // TODO check if malloc returned null; if so, runtime error for OOM!
                 let mem_flags = MemFlags::new();
 
                 // Copy the elements from the literal into the array
@@ -1044,6 +1047,8 @@ fn clone_list<B: Backend>(
     // Allocate space for the new array that we'll copy into.
     let new_elems_ptr = call_malloc(env, module, builder, size);
 
+    // TODO check if malloc returned null; if so, runtime error for OOM!
+
     // Either memcpy or deep clone the array elements
     if elem_layout.safe_to_memcpy() {
         // Copy the bytes from the original array into the new
diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 7ed0e04cbd..a10c2df0f3 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -208,20 +208,23 @@ pub fn build_expr<'a, 'ctx, 'env>(
             } else {
                 let ctx = env.context;
                 let builder = env.builder;
-                let bytes_len = str_literal.len() + 1/* TODO drop the +1 when we have structs and this is no longer a NUL-terminated CString.*/;
+                let str_len = str_literal.len() + 1/* TODO drop the +1 when we have structs and this is no longer a NUL-terminated CString.*/;
 
                 let byte_type = ctx.i8_type();
                 let nul_terminator = byte_type.const_zero();
-                let len = ctx.i32_type().const_int(bytes_len as u64, false);
+                let len_val = ctx.i32_type().const_int(str_len as u64, false);
                 let ptr = env
                     .builder
-                    .build_array_malloc(ctx.i8_type(), len, "str_ptr")
+                    .build_array_malloc(ctx.i8_type(), len_val, "str_ptr")
                     .unwrap();
 
+                // TODO check if malloc returned null; if so, runtime error for OOM!
+
                 // Copy the bytes from the string literal into the array
                 for (index, byte) in str_literal.bytes().enumerate() {
-                    let index = ctx.i32_type().const_int(index as u64, false);
-                    let elem_ptr = unsafe { builder.build_gep(ptr, &[index], "byte") };
+                    let index_val = ctx.i32_type().const_int(index as u64, false);
+                    let elem_ptr =
+                        unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "byte") };
 
                     builder.build_store(elem_ptr, byte_type.const_int(byte as u64, false));
                 }
@@ -229,8 +232,9 @@ pub fn build_expr<'a, 'ctx, 'env>(
                 // Add a NUL terminator at the end.
                 // TODO: Instead of NUL-terminating, return a struct
                 // with the pointer and also the length and capacity.
-                let index = ctx.i32_type().const_int(bytes_len as u64 - 1, false);
-                let elem_ptr = unsafe { builder.build_gep(ptr, &[index], "nul_terminator") };
+                let index_val = ctx.i32_type().const_int(str_len as u64 - 1, false);
+                let elem_ptr =
+                    unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "nul_terminator") };
 
                 builder.build_store(elem_ptr, nul_terminator);
 
@@ -260,13 +264,15 @@ pub fn build_expr<'a, 'ctx, 'env>(
                     env.builder
                         .build_array_malloc(elem_type, len, "create_list_ptr")
                         .unwrap()
+
+                    // TODO check if malloc returned null; if so, runtime error for OOM!
                 };
 
                 // Copy the elements from the list literal into the array
                 for (index, elem) in elems.iter().enumerate() {
-                    let offset = ctx.i32_type().const_int(elem_bytes * index as u64, false);
-                    let elem_ptr = unsafe { builder.build_gep(ptr, &[offset], "elem") };
-
+                    let index_val = ctx.i32_type().const_int(index as u64, false);
+                    let elem_ptr =
+                        unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "index") };
                     let val = build_expr(env, &scope, parent, &elem, procs);
 
                     builder.build_store(elem_ptr, val);
@@ -1065,18 +1071,13 @@ fn call_with_args<'a, 'ctx, 'env>(
             // TODO here, check to see if the requested index exceeds the length of the array.
 
             match list_layout {
-                Layout::Builtin(Builtin::List(elem_layout)) => {
+                Layout::Builtin(Builtin::List(_)) => {
                     // Load the pointer to the array data
                     let array_data_ptr = load_list_ptr(builder, wrapper_struct);
-                    let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-                    let elem_size = env.ptr_int().const_int(elem_bytes, false);
-
-                    // Calculate the offset at runtime by multiplying the index by the size of an element.
-                    let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
                     // We already checked the bounds earlier.
                     let elem_ptr = unsafe {
-                        builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem")
+                        builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem")
                     };
 
                     builder.build_load(elem_ptr, "List.get")
@@ -1111,16 +1112,12 @@ fn call_with_args<'a, 'ctx, 'env>(
                 )
             };
 
-            let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-            let elem_size = env.ptr_int().const_int(elem_bytes, false);
-
             // Calculate the offset at runtime by multiplying the index by the size of an element.
             let elem_index = args[1].0.into_int_value();
-            let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
             // We already checked the bounds earlier.
             let elem_ptr =
-                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
 
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
@@ -1135,7 +1132,7 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             let wrapper_struct = args[0].0.into_struct_value();
             let elem_index = args[1].0.into_int_value();
-            let (elem, elem_layout) = args[2];
+            let (elem, _elem_layout) = args[2];
 
             // Load the usize length
             let _list_len = load_list_len(builder, wrapper_struct);
@@ -1146,15 +1143,9 @@ fn call_with_args<'a, 'ctx, 'env>(
             // Load the pointer to the elements
             let array_data_ptr = load_list_ptr(builder, wrapper_struct);
 
-            let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-            let elem_size = env.context.i64_type().const_int(elem_bytes, false);
-
-            // Calculate the offset at runtime by multiplying the index by the size of an element.
-            let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
-
             // We already checked the bounds earlier.
             let elem_ptr =
-                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
 
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
@@ -1229,6 +1220,8 @@ fn clone_list<'a, 'ctx, 'env>(
         .build_array_malloc(elem_type, list_len, "list_ptr")
         .unwrap();
 
+    // TODO check if malloc returned null; if so, runtime error for OOM!
+
     // Either memcpy or deep clone the array elements
     if elem_layout.safe_to_memcpy() {
         // Copy the bytes from the original array into the new
diff --git a/compiler/gen/src/llvm/convert.rs b/compiler/gen/src/llvm/convert.rs
index 57916f8a77..bbbc117c53 100644
--- a/compiler/gen/src/llvm/convert.rs
+++ b/compiler/gen/src/llvm/convert.rs
@@ -151,7 +151,14 @@ pub fn collection_wrapper<'ctx>(
     let ptr_type_enum = BasicTypeEnum::PointerType(ptr_type);
     let len_type = BasicTypeEnum::IntType(ptr_int(ctx, ptr_bytes));
 
-    ctx.struct_type(&[ptr_type_enum, len_type], false)
+    // This conditional is based on a constant, so the branch should be optimized away.
+    // The reason for keeping the conditional here is so we can flip the order
+    // of the fields (by changing the constants) without breaking this code.
+    if Builtin::WRAPPER_PTR == 0 {
+        ctx.struct_type(&[ptr_type_enum, len_type], false)
+    } else {
+        ctx.struct_type(&[len_type, ptr_type_enum], false)
+    }
 }
 
 pub fn ptr_int(ctx: &Context, ptr_bytes: u32) -> IntType<'_> {
diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 617c94ce8c..ea7ede9851 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -5,6 +5,7 @@ extern crate indoc;
 
 extern crate bumpalo;
 extern crate inkwell;
+extern crate libc;
 extern crate roc_gen;
 
 mod helpers;
@@ -541,6 +542,11 @@ mod test_gen {
         assert_llvm_evals_to!("[]", &[], &'static [i64], |x| x);
     }
 
+    #[test]
+    fn int_list_literal() {
+        assert_llvm_evals_to!("[ 12, 9, 6, 3 ]", &[12, 9, 6, 3], &'static [i64], |x| x);
+    }
+
     #[test]
     fn head_int_list() {
         assert_evals_to!("List.getUnsafe [ 12, 9, 6, 3 ] 0", 12, i64);
diff --git a/compiler/mono/src/layout.rs b/compiler/mono/src/layout.rs
index 939062c832..e6e312cb76 100644
--- a/compiler/mono/src/layout.rs
+++ b/compiler/mono/src/layout.rs
@@ -143,10 +143,12 @@ impl<'a> Builtin<'a> {
     pub const SET_WORDS: u32 = Builtin::MAP_WORDS; // Set is an alias for Map with {} for value
     pub const LIST_WORDS: u32 = 2;
 
-    /// Layout of collection wrapper - a struct of (pointer, length, capacity)
+    /// Layout of collection wrapper for List and Str - a struct of (pointre, length).
+    ///
+    /// We choose this layout (with pointer first) because it's how
+    /// Rust slices are laid out, meaning we can cast to/from them for free.
     pub const WRAPPER_PTR: u32 = 0;
     pub const WRAPPER_LEN: u32 = 1;
-    pub const WRAPPER_CAPACITY: u32 = 2;
 
     pub fn stack_size(&self, pointer_size: u32) -> u32 {
         use Builtin::*;

From facdc1e2f40d4c8e05c74dd7d8c8926241a5ac4b Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 14:52:05 -0400
Subject: [PATCH 12/24] Revise some tests a bit

---
 compiler/gen/tests/test_gen.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index ea7ede9851..98a52be041 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -554,11 +554,11 @@ mod test_gen {
 
     #[test]
     fn get_int_list() {
-        assert_evals_to!("List.getUnsafe [ 12, 9, 6, 3 ] 1", 9, i64);
+        assert_evals_to!("List.getUnsafe [ 12, 9, 6 ] 1", 9, i64);
     }
 
     #[test]
-    fn set_unique_int_list() {
+    fn get_set_unique_int_list() {
         assert_evals_to!("List.getUnsafe (List.set [ 12, 9, 7, 3 ] 1 42) 1", 42, i64);
     }
 

From 8a26cac3f25cb398fd4dd99d12afff01e5650634 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 14:52:20 -0400
Subject: [PATCH 13/24] Add a test of just List.set by itself

---
 compiler/gen/tests/test_gen.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 98a52be041..4bc6b294bd 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -562,6 +562,16 @@ mod test_gen {
         assert_evals_to!("List.getUnsafe (List.set [ 12, 9, 7, 3 ] 1 42) 1", 42, i64);
     }
 
+    #[test]
+    fn set_unique_int_list() {
+        assert_llvm_evals_to!(
+            "List.set [ 12, 9, 7, 1, 5 ] 2 33",
+            &[12, 9, 33, 1, 5],
+            &'static [i64],
+            |x| x
+        );
+    }
+
     #[test]
     fn set_shared_int_list() {
         assert_evals_to!(

From d0a1adfef949296f5737454f64e7fa21a3837dd2 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 20:55:23 -0400
Subject: [PATCH 14/24] Fix set_shared_int_list for LLVM

---
 compiler/gen/src/llvm/build.rs | 1 +
 compiler/gen/tests/test_gen.rs | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index a10c2df0f3..1eccf1e5da 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1088,6 +1088,7 @@ fn call_with_args<'a, 'ctx, 'env>(
             }
         }
         Symbol::LIST_SET => {
+            // List.set : List elem, Int, elem -> List elem
             let builder = env.builder;
 
             debug_assert!(args.len() == 3);
diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 4bc6b294bd..135cdefcb1 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -574,7 +574,7 @@ mod test_gen {
 
     #[test]
     fn set_shared_int_list() {
-        assert_evals_to!(
+        assert_llvm_evals_to!(
             indoc!(
                 r#"
                     shared = [ 2, 4 ]
@@ -585,8 +585,9 @@ mod test_gen {
                     { x, y: List.getUnsafe shared 1 }
                 "#
             ),
-            (4, 77),
-            (i64, i64)
+            (77, 4),
+            (i64, i64),
+            |x| x
         );
     }
 

From d9fe38efce6ba23683df1bb0757ccf5106fbe690 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 20:56:29 -0400
Subject: [PATCH 15/24] Remove unnecessary lifetime annotations

---
 compiler/gen/src/llvm/convert.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compiler/gen/src/llvm/convert.rs b/compiler/gen/src/llvm/convert.rs
index bbbc117c53..74956ce45c 100644
--- a/compiler/gen/src/llvm/convert.rs
+++ b/compiler/gen/src/llvm/convert.rs
@@ -134,7 +134,7 @@ pub fn basic_type_from_layout<'ctx>(
 /// The order of these doesn't matter, since they should be initialized
 /// to zero anyway for an empty collection; as such, we return a
 /// (usize, usize) struct layout no matter what.
-pub fn empty_collection<'ctx>(ctx: &'ctx Context, ptr_bytes: u32) -> StructType<'ctx> {
+pub fn empty_collection(ctx: &Context, ptr_bytes: u32) -> StructType<'_> {
     let usize_type = BasicTypeEnum::IntType(ptr_int(ctx, ptr_bytes));
 
     ctx.struct_type(&[usize_type, usize_type], false)

From 63a8daa4c7e3ba7f09b9e1acbde38392ba8cbd0a Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 20:46:42 -0400
Subject: [PATCH 16/24] Reproduce no bounds checking on List.set in LLVM

---
 compiler/gen/tests/test_gen.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 135cdefcb1..4d09a97c65 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -572,6 +572,16 @@ mod test_gen {
         );
     }
 
+    #[test]
+    fn set_unique_list_oob() {
+        assert_llvm_evals_to!(
+            "List.set [ 3, 17, 4 ] 1337 42",
+            &[3, 17, 4],
+            &'static [i64],
+            |x| x
+        );
+    }
+
     #[test]
     fn set_shared_int_list() {
         assert_llvm_evals_to!(

From a0dd31ee51a46e5600d3f1e3bdb67924bc61dd7b Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 20:47:07 -0400
Subject: [PATCH 17/24] Add bounds checking for List.set in LLVM

---
 compiler/gen/src/llvm/build.rs | 71 ++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 1eccf1e5da..bf8b9a7261 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -160,7 +160,7 @@ pub fn build_expr<'a, 'ctx, 'env>(
                     arg_tuples.push((build_expr(env, scope, parent, arg, procs), layout));
                 }
 
-                call_with_args(*symbol, arg_tuples.into_bump_slice(), env)
+                call_with_args(*symbol, parent, arg_tuples.into_bump_slice(), env)
             }
         },
         FunctionPointer(symbol) => {
@@ -922,6 +922,7 @@ pub fn verify_fn(fn_val: FunctionValue<'_>) {
 #[allow(clippy::cognitive_complexity)]
 fn call_with_args<'a, 'ctx, 'env>(
     symbol: Symbol,
+    parent: FunctionValue<'ctx>,
     args: &[(BasicValueEnum<'ctx>, &'a Layout<'a>)],
     env: &Env<'a, 'ctx, 'env>,
 ) -> BasicValueEnum<'ctx> {
@@ -1093,38 +1094,50 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             debug_assert!(args.len() == 3);
 
-            let (elem, elem_layout) = args[2];
-            let (wrapper_struct, array_data_ptr) = {
-                // This original wrapper_struct should only stay in scope long enough to clone it.
-                // From then on, we should only ever reference the clone!
-                let wrapper_struct = args[0].0.into_struct_value();
-
-                // Load the usize length
-                let list_len = load_list_len(builder, wrapper_struct);
-
-                // TODO here, check to see if the requested index exceeds the length of the array.
-                // If so, bail out and return the list unaltered.
-
-                clone_list(
-                    env,
-                    list_len,
-                    load_list_ptr(builder, wrapper_struct),
-                    elem_layout,
-                )
-            };
-
-            // Calculate the offset at runtime by multiplying the index by the size of an element.
+            let original_wrapper = args[0].0.into_struct_value();
             let elem_index = args[1].0.into_int_value();
 
-            // We already checked the bounds earlier.
-            let elem_ptr =
-                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
+            // Load the usize length from the wrapper. We need it for bounds checking.
+            let list_len = load_list_len(builder, original_wrapper);
 
-            // Mutate the array in-place.
-            builder.build_store(elem_ptr, elem);
+            // Bounds check: only proceed if index < length.
+            // Otherwise, return the list unaltered.
+            //
+            // Note: Check for index < length as the "true" condition,
+            // to avoid misprediction. (In practice this should usually pass,
+            // and CPUs generally default to predicting that a forward jump
+            // shouldn't be taken; that is, they predict "else" won't be taken.)
+            let comparison =
+                builder.build_int_compare(IntPredicate::ULT, elem_index, list_len, "bounds_check");
 
-            // Return the cloned wrapper
-            BasicValueEnum::StructValue(wrapper_struct)
+            // If the index is in bounds, clone and mutate in place.
+            let then_val: BasicValueEnum = {
+                let (elem, elem_layout) = args[2];
+                let (cloned_wrapper, array_data_ptr) = {
+                    clone_list(
+                        env,
+                        list_len,
+                        load_list_ptr(builder, original_wrapper),
+                        elem_layout,
+                    )
+                };
+
+                // If we got here, we passed the bounds check, so this is an in-bounds GEP
+                let elem_ptr = unsafe {
+                    builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "load_index")
+                };
+
+                // Mutate the new array in-place to change the element.
+                builder.build_store(elem_ptr, elem);
+
+                BasicValueEnum::StructValue(cloned_wrapper)
+            };
+
+            // If the index was out of bounds, return the original list unaltered.
+            let else_val = BasicValueEnum::StructValue(original_wrapper);
+            let ret_type = original_wrapper.get_type();
+
+            build_basic_phi2(env, parent, comparison, then_val, else_val, ret_type.into())
         }
         Symbol::LIST_SET_IN_PLACE => {
             let builder = env.builder;

From ef38095003928ff9e0875f8d06f6f6a697f72c78 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 21:01:14 -0400
Subject: [PATCH 18/24] Fix optimization test

The reason this was failing was that `x` was getting
dropped by dead code elimination, and therefore wasn't
considered a reason to share any values it used.
---
 compiler/mono/tests/test_opt.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compiler/mono/tests/test_opt.rs b/compiler/mono/tests/test_opt.rs
index 773648ddea..88fe0728c3 100644
--- a/compiler/mono/tests/test_opt.rs
+++ b/compiler/mono/tests/test_opt.rs
@@ -278,7 +278,7 @@ mod test_opt {
                 # This should not mutate the original
                 x = List.set shared 1 77
 
-                List.getUnsafe shared 1
+                { x, y: List.getUnsafe shared 1 }
             "#,
             vec![Symbol::LIST_SET, Symbol::LIST_GET_UNSAFE],
         );

From 3e71b5a38d1bf9aa91feca911a8b3180f4f4b266 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 22:11:06 -0400
Subject: [PATCH 19/24] Extract bounds_check_comparison

---
 compiler/gen/src/llvm/build.rs | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index bf8b9a7261..3b99235197 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1102,13 +1102,7 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             // Bounds check: only proceed if index < length.
             // Otherwise, return the list unaltered.
-            //
-            // Note: Check for index < length as the "true" condition,
-            // to avoid misprediction. (In practice this should usually pass,
-            // and CPUs generally default to predicting that a forward jump
-            // shouldn't be taken; that is, they predict "else" won't be taken.)
-            let comparison =
-                builder.build_int_compare(IntPredicate::ULT, elem_index, list_len, "bounds_check");
+            let comparison = bounds_check_comparison(builder, elem_index, list_len);
 
             // If the index is in bounds, clone and mutate in place.
             let then_val: BasicValueEnum = {
@@ -1272,3 +1266,16 @@ fn clone_list<'a, 'ctx, 'env>(
 
     (struct_val.into_struct_value(), ptr_val)
 }
+
+fn bounds_check_comparison<'ctx>(
+    builder: &Builder<'ctx>,
+    elem_index: IntValue<'ctx>,
+    len: IntValue<'ctx>,
+) -> IntValue<'ctx> {
+    //
+    // Note: Check for index < length as the "true" condition,
+    // to avoid misprediction. (In practice this should usually pass,
+    // and CPUs generally default to predicting that a forward jump
+    // shouldn't be taken; that is, they predict "else" won't be taken.)
+    builder.build_int_compare(IntPredicate::ULT, elem_index, len, "bounds_check")
+}

From 52bfe3ce9e3a9f56a77f2c14f20c6517cdbe0c25 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 22:12:51 -0400
Subject: [PATCH 20/24] Add bounds checking to List.setInPlace

---
 compiler/gen/src/llvm/build.rs | 38 +++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 3b99235197..2e060e9195 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1105,7 +1105,7 @@ fn call_with_args<'a, 'ctx, 'env>(
             let comparison = bounds_check_comparison(builder, elem_index, list_len);
 
             // If the index is in bounds, clone and mutate in place.
-            let then_val: BasicValueEnum = {
+            let then_val = {
                 let (elem, elem_layout) = args[2];
                 let (cloned_wrapper, array_data_ptr) = {
                     clone_list(
@@ -1138,28 +1138,38 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             debug_assert!(args.len() == 3);
 
-            let wrapper_struct = args[0].0.into_struct_value();
+            let original_wrapper = args[0].0.into_struct_value();
             let elem_index = args[1].0.into_int_value();
             let (elem, _elem_layout) = args[2];
 
             // Load the usize length
-            let _list_len = load_list_len(builder, wrapper_struct);
+            let list_len = load_list_len(builder, original_wrapper);
 
-            // TODO here, check to see if the requested index exceeds the length of the array.
-            // If so, bail out and return the list unaltered.
+            // Bounds check: only proceed if index < length.
+            // Otherwise, return the list unaltered.
+            let comparison = bounds_check_comparison(builder, elem_index, list_len);
 
-            // Load the pointer to the elements
-            let array_data_ptr = load_list_ptr(builder, wrapper_struct);
+            // If the index is in bounds, clone and mutate in place.
+            let then_val = {
+                // Load the pointer to the elements
+                let array_data_ptr = load_list_ptr(builder, original_wrapper);
 
-            // We already checked the bounds earlier.
-            let elem_ptr =
-                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
+                // We already checked the bounds earlier.
+                let elem_ptr =
+                    unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
 
-            // Mutate the array in-place.
-            builder.build_store(elem_ptr, elem);
+                // Mutate the array in-place.
+                builder.build_store(elem_ptr, elem);
 
-            // Return the wrapper unchanged, since pointer and length are unchanged
-            BasicValueEnum::StructValue(wrapper_struct)
+                // Return the wrapper unchanged, since pointer and length are unchanged
+                BasicValueEnum::StructValue(original_wrapper)
+            };
+
+            // If the index was out of bounds, return the original list unaltered.
+            let else_val = BasicValueEnum::StructValue(original_wrapper);
+            let ret_type = original_wrapper.get_type();
+
+            build_basic_phi2(env, parent, comparison, then_val, else_val, ret_type.into())
         }
         _ => {
             let fn_val = env

From 39249f3905c9aabfb2bf922fed6bc37b557955a5 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 22:14:02 -0400
Subject: [PATCH 21/24] Add test for List.set bounds checking

---
 compiler/gen/tests/test_gen.rs | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 4d09a97c65..61805f7098 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -601,6 +601,25 @@ mod test_gen {
         );
     }
 
+    #[test]
+    fn set_shared_list_oob() {
+        assert_llvm_evals_to!(
+            indoc!(
+                r#"
+                    shared = [ 2, 4 ]
+
+                    # This should not mutate the original
+                    x = List.getUnsafe (List.set shared 9000 77) 1
+
+                    { x, y: List.getUnsafe shared 1 }
+                "#
+            ),
+            (4, 4),
+            (i64, i64),
+            |x| x
+        );
+    }
+
     #[test]
     fn get_unique_int_list() {
         assert_evals_to!(

From db502fe2e73b77bf662b5d7331f540d99f0794bb Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sat, 21 Mar 2020 22:51:11 -0400
Subject: [PATCH 22/24] Revise some tests

---
 compiler/gen/src/llvm/build.rs  | 10 +++++-----
 compiler/gen/tests/test_gen.rs  | 24 ++++++++++++------------
 compiler/mono/tests/test_opt.rs |  2 +-
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs
index 2e060e9195..7ace324eef 100644
--- a/compiler/gen/src/llvm/build.rs
+++ b/compiler/gen/src/llvm/build.rs
@@ -1234,7 +1234,7 @@ fn clone_list<'a, 'ctx, 'env>(
 
     // Allocate space for the new array that we'll copy into.
     let elem_type = basic_type_from_layout(env.arena, ctx, elem_layout, env.ptr_bytes);
-    let ptr_val = builder
+    let clone_ptr = builder
         .build_array_malloc(elem_type, list_len, "list_ptr")
         .unwrap();
 
@@ -1247,7 +1247,7 @@ fn clone_list<'a, 'ctx, 'env>(
         //
         // TODO how do we decide when to do the small memcpy vs the normal one?
         builder
-            .build_memcpy(ptr_val, ptr_bytes, elems_ptr, ptr_bytes, size)
+            .build_memcpy(clone_ptr, ptr_bytes, elems_ptr, ptr_bytes, size)
             .unwrap_or_else(|err| {
                 panic!("Error while attempting LLVM memcpy: {:?}", err);
             });
@@ -1256,14 +1256,14 @@ fn clone_list<'a, 'ctx, 'env>(
     }
 
     // Create a fresh wrapper struct for the newly populated array
-    let struct_type = collection_wrapper(ctx, ptr_val.get_type(), env.ptr_bytes);
+    let struct_type = collection_wrapper(ctx, clone_ptr.get_type(), env.ptr_bytes);
     let mut struct_val;
 
     // Store the pointer
     struct_val = builder
         .build_insert_value(
             struct_type.get_undef(),
-            ptr_val,
+            clone_ptr,
             Builtin::WRAPPER_PTR,
             "insert_ptr",
         )
@@ -1274,7 +1274,7 @@ fn clone_list<'a, 'ctx, 'env>(
         .build_insert_value(struct_val, list_len, Builtin::WRAPPER_LEN, "insert_len")
         .unwrap();
 
-    (struct_val.into_struct_value(), ptr_val)
+    (struct_val.into_struct_value(), clone_ptr)
 }
 
 fn bounds_check_comparison<'ctx>(
diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 61805f7098..630cd9ced8 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -564,7 +564,7 @@ mod test_gen {
 
     #[test]
     fn set_unique_int_list() {
-        assert_llvm_evals_to!(
+        assert_opt_evals_to!(
             "List.set [ 12, 9, 7, 1, 5 ] 2 33",
             &[12, 9, 33, 1, 5],
             &'static [i64],
@@ -574,42 +574,42 @@ mod test_gen {
 
     #[test]
     fn set_unique_list_oob() {
-        assert_llvm_evals_to!(
-            "List.set [ 3, 17, 4 ] 1337 42",
-            &[3, 17, 4],
-            &'static [i64],
+        assert_opt_evals_to!(
+            "List.set [ 3, 17, 4.1 ] 1337 9.25",
+            &[3.0, 17.0, 4.1],
+            &'static [f64],
             |x| x
         );
     }
 
     #[test]
     fn set_shared_int_list() {
-        assert_llvm_evals_to!(
+        assert_opt_evals_to!(
             indoc!(
                 r#"
-                    shared = [ 2, 4 ]
+                    shared = [ 2.1, 4.3 ]
 
                     # This should not mutate the original
-                    x = List.getUnsafe (List.set shared 1 77) 1
+                    x = List.getUnsafe (List.set shared 1 7.7) 1
 
                     { x, y: List.getUnsafe shared 1 }
                 "#
             ),
-            (77, 4),
-            (i64, i64),
+            (7.7, 4.3),
+            (f64, f64),
             |x| x
         );
     }
 
     #[test]
     fn set_shared_list_oob() {
-        assert_llvm_evals_to!(
+        assert_opt_evals_to!(
             indoc!(
                 r#"
                     shared = [ 2, 4 ]
 
                     # This should not mutate the original
-                    x = List.getUnsafe (List.set shared 9000 77) 1
+                    x = List.set shared 1 0
 
                     { x, y: List.getUnsafe shared 1 }
                 "#
diff --git a/compiler/mono/tests/test_opt.rs b/compiler/mono/tests/test_opt.rs
index 88fe0728c3..324df1f3a9 100644
--- a/compiler/mono/tests/test_opt.rs
+++ b/compiler/mono/tests/test_opt.rs
@@ -276,7 +276,7 @@ mod test_opt {
                 shared = [ 2, 4 ]
 
                 # This should not mutate the original
-                x = List.set shared 1 77
+                x = List.set shared 1 0
 
                 { x, y: List.getUnsafe shared 1 }
             "#,

From c6fa301d8b7df92f3046e380315a3742deb22ae8 Mon Sep 17 00:00:00 2001
From: Folkert <folkert@folkertdev.nl>
Date: Sun, 22 Mar 2020 13:22:52 +0100
Subject: [PATCH 23/24] fix bug in is_unique

---
 compiler/types/src/boolean_algebra.rs | 52 ++++++++++++++++-----------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/compiler/types/src/boolean_algebra.rs b/compiler/types/src/boolean_algebra.rs
index bcaa337cb8..1e2b24914e 100644
--- a/compiler/types/src/boolean_algebra.rs
+++ b/compiler/types/src/boolean_algebra.rs
@@ -94,34 +94,44 @@ impl Bool {
             Atom::Zero => Err(Atom::Zero),
             Atom::One => Err(Atom::One),
             Atom::Variable(var) => {
-                let mut result = Vec::new();
-                result.push(var);
+                // The var may still point to Zero or One!
+                match subs.get_without_compacting(var).content {
+                    Content::Structure(FlatType::Boolean(nested)) => nested.simplify(subs),
+                    _ => {
+                        let mut result = Vec::new();
+                        result.push(var);
 
-                for atom in &self.1 {
-                    match atom {
-                        Atom::Zero => {}
-                        Atom::One => return Err(Atom::One),
-                        Atom::Variable(v) => match subs.get_without_compacting(*v).content {
-                            Content::Structure(FlatType::Boolean(nested)) => {
-                                match nested.simplify(subs) {
-                                    Ok(variables) => {
-                                        for var in variables {
-                                            result.push(var);
+                        for atom in &self.1 {
+                            match atom {
+                                Atom::Zero => {}
+                                Atom::One => return Err(Atom::One),
+                                Atom::Variable(v) => {
+                                    match subs.get_without_compacting(*v).content {
+                                        Content::Structure(FlatType::Boolean(nested)) => {
+                                            match nested.simplify(subs) {
+                                                Ok(variables) => {
+                                                    for var in variables {
+                                                        result.push(var);
+                                                    }
+                                                }
+                                                Err(Atom::Zero) => {}
+                                                Err(Atom::One) => return Err(Atom::One),
+                                                Err(Atom::Variable(_)) => {
+                                                    panic!("TODO nested variable")
+                                                }
+                                            }
+                                        }
+                                        _ => {
+                                            result.push(*v);
                                         }
                                     }
-                                    Err(Atom::Zero) => {}
-                                    Err(Atom::One) => return Err(Atom::One),
-                                    Err(Atom::Variable(_)) => panic!("TODO nested variable"),
                                 }
                             }
-                            _ => {
-                                result.push(*v);
-                            }
-                        },
+                        }
+
+                        Ok(result)
                     }
                 }
-
-                Ok(result)
             }
         }
     }

From ee12e56d1c8aa98d07c1495ff4c7edb0251f87b5 Mon Sep 17 00:00:00 2001
From: Richard Feldman <oss@rtfeldman.com>
Date: Sun, 22 Mar 2020 11:34:36 -0400
Subject: [PATCH 24/24] Fix set_shared_list_oob test

---
 compiler/gen/tests/test_gen.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compiler/gen/tests/test_gen.rs b/compiler/gen/tests/test_gen.rs
index 630cd9ced8..c709489e17 100644
--- a/compiler/gen/tests/test_gen.rs
+++ b/compiler/gen/tests/test_gen.rs
@@ -608,8 +608,8 @@ mod test_gen {
                 r#"
                     shared = [ 2, 4 ]
 
-                    # This should not mutate the original
-                    x = List.set shared 1 0
+                    # This List.set is out of bounds, and should have no effect
+                    x = List.getUnsafe (List.set shared 422 0) 1
 
                     { x, y: List.getUnsafe shared 1 }
                 "#