Merge pull request #4896 from roc-lang/expect-crash

reproduce crash in `expect` with Lists of different length

crates/compiler/gen_llvm/src/llvm/expect.rs

@@ -1042,9 +1042,13 @@ fn build_clone_builtin<'a, 'ctx, 'env>(
 
             let elements_width = bd.build_int_mul(element_width, len, "elements_width");
 
+            // We clone the elements into the extra_offset address.
+            let _ = offset;
+            let elements_start_offset = cursors.extra_offset;
+
             if layout_interner.safe_to_memcpy(elem) {
                 // NOTE we are not actually sure the dest is properly aligned
-                let dest = pointer_at_offset(bd, env.context.i8_type(), ptr, offset);
+                let dest = pointer_at_offset(bd, env.context.i8_type(), ptr, elements_start_offset);
                 let src = bd.build_pointer_cast(
                     elements,
                     env.context.i8_type().ptr_type(AddressSpace::Generic),
@@ -1052,11 +1056,8 @@ fn build_clone_builtin<'a, 'ctx, 'env>(
                 );
                 bd.build_memcpy(dest, 1, src, 1, elements_width).unwrap();
 
-                bd.build_int_add(offset, elements_width, "new_offset")
+                bd.build_int_add(elements_start_offset, elements_width, "new_offset")
             } else {
-                // We cloned the elements into the extra_offset address.
-                let elements_start_offset = cursors.extra_offset;
-
                 let element_type = basic_type_from_layout(env, layout_interner, elem);
                 let elements = bd.build_pointer_cast(
                     elements,

crates/repl_eval/src/eval.rs

@@ -1,4 +1,4 @@
-use bumpalo::collections::Vec;
+use bumpalo::collections::{CollectIn, Vec};
 use bumpalo::Bump;
 use roc_types::types::AliasKind;
 use std::cmp::{max_by_key, min_by_key};
@@ -10,8 +10,8 @@ use roc_module::ident::TagName;
 use roc_module::symbol::{Interns, ModuleId, Symbol};
 use roc_mono::ir::ProcLayout;
 use roc_mono::layout::{
-    self, union_sorted_tags_pub, Builtin, InLayout, Layout, LayoutCache, LayoutInterner,
+    self, cmp_fields, union_sorted_tags_pub, Builtin, InLayout, Layout, LayoutCache,
-    TLLayoutInterner, UnionLayout, UnionVariant, WrappedVariant,
+    LayoutInterner, TLLayoutInterner, UnionLayout, UnionVariant, WrappedVariant,
 };
 use roc_parse::ast::{AssignedField, Collection, Expr, Pattern, StrLiteral};
 use roc_region::all::{Loc, Region};
@@ -341,7 +341,7 @@ fn jit_to_ast_help<'a, A: ReplApp<'a>>(
     layout: InLayout<'a>,
     var: Variable,
 ) -> Expr<'a> {
-    let (newtype_containers, alias_content, raw_var) = unroll_newtypes_and_aliases(env, var);
+    let (newtype_containers, _alias_content, raw_var) = unroll_newtypes_and_aliases(env, var);
 
     macro_rules! num_helper {
         ($ty:ty) => {
@@ -365,7 +365,6 @@ fn jit_to_ast_help<'a, A: ReplApp<'a>>(
                 (Alias(Symbol::NUM_UNSIGNED8 | Symbol::NUM_U8, ..), U8) => num_helper!(u8),
                 (_, U8) => {
                     // This is not a number, it's a tag union or something else
-                    dbg!(&alias_content);
                     app.call_function(main_fn_name, |_mem: &A::Memory, num: u8| {
                         byte_to_ast(env, num, env.subs.get_content_without_compacting(raw_var))
                     })
@@ -559,9 +558,11 @@ fn addr_to_ast<'a, M: ReplAppMemory>(
         (_, Layout::Builtin(Builtin::Bool)) => {
             // TODO: bits are not as expected here.
             // num is always false at the moment.
-            let num: bool = mem.deref_bool(addr);
+            let num: u8 = mem.deref_u8(addr);
 
-            bool_to_ast(env, num, raw_content)
+            debug_assert!(num == 0 || num == 1);
+
+            bool_to_ast(env, num != 0, raw_content)
         }
         (_, Layout::Builtin(Builtin::Int(int_width))) => {
             use IntWidth::*;
@@ -1053,16 +1054,34 @@ fn struct_to_ast<'a, 'env, M: ReplAppMemory>(
         // We'll advance this as we iterate through the fields
         let mut field_addr = addr;
 
-        // We recalculate the layouts here because we will have compiled the record so that its fields
+        // the type checker stores record fields in alphabetical order
-        // are sorted by descending alignment, and then alphabetic, but the type of the record is
+        let alphabetical_fields: Vec<_> = record_fields
-        // always only sorted alphabetically. We want to arrange the rendered record in the order of
+            .sorted_iterator(subs, Variable::EMPTY_RECORD)
-        // the type.
+            .map(|(l, field)| {
-        for (label, field) in record_fields.sorted_iterator(subs, Variable::EMPTY_RECORD) {
+                let layout = env
+                    .layout_cache
+                    .from_var(arena, field.into_inner(), env.subs)
+                    .unwrap();
+
+                (l, field, layout)
+            })
+            .collect_in(arena);
+
+        // but the memory representation sorts first by size (and uses field name as a tie breaker)
+        let mut in_memory_fields = alphabetical_fields;
+        in_memory_fields.sort_by(|(label1, _, layout1), (label2, _, layout2)| {
+            cmp_fields(
+                &env.layout_cache.interner,
+                label1,
+                *layout1,
+                label2,
+                *layout2,
+                env.target_info,
+            )
+        });
+
+        for (label, field, field_layout) in in_memory_fields {
             let field_var = field.into_inner();
-            let field_layout = env
-                .layout_cache
-                .from_var(arena, field.into_inner(), env.subs)
-                .unwrap();
 
             let loc_expr = &*arena.alloc(Loc {
                 value: addr_to_ast(
@@ -1091,6 +1110,15 @@ fn struct_to_ast<'a, 'env, M: ReplAppMemory>(
             field_addr += env.layout_cache.interner.stack_size(field_layout) as usize;
         }
 
+        // to the user we want to present the fields in alphabetical order again, so re-sort
+        fn sort_key<'a, T>(loc_field: &'a Loc<AssignedField<'a, T>>) -> &'a str {
+            match &loc_field.value {
+                AssignedField::RequiredValue(field_name, _, _) => field_name.value,
+                _ => unreachable!("was not added to output"),
+            }
+        }
+
+        output.sort_by(|a, b| sort_key(a).cmp(sort_key(b)));
         let output = output.into_bump_slice();
 
         Expr::Record(Collection::with_items(output))

crates/repl_expect/src/app.rs

@@ -17,7 +17,15 @@ macro_rules! deref_number {
 }
 
 impl ReplAppMemory for ExpectMemory {
-    deref_number!(deref_bool, bool);
+    fn deref_bool(&self, addr: usize) -> bool {
+        let ptr = unsafe { self.start.add(addr) } as *const u8;
+        let value = unsafe { std::ptr::read_unaligned(ptr) };
+
+        // bool values should only ever be 0 or 1
+        debug_assert!(value == 0 || value == 1);
+
+        value != 0
+    }
 
     deref_number!(deref_u8, u8);
     deref_number!(deref_u16, u16);

crates/repl_expect/src/lib.rs

@@ -975,4 +975,128 @@ mod test {
             ),
         );
     }
+
+    #[test]
+    fn adjacent_lists() {
+        run_expect_test(
+            indoc!(
+                r#"
+                interface Test exposes [] imports []
+
+                expect
+                    actual : { headers: List U8, body: List U8, x: List U8 }
+                    actual = {
+                        body: [],
+                        headers: [],
+                        x: [],
+                    }
+
+                    expected : { headers: List U8, body: List U8, x: List U8 }
+                    expected = {
+                        body: [ 42, 43, 44 ],
+                        headers: [15, 16, 17],
+                        x: [115, 116, 117],
+                    }
+                    actual == expected
+                "#
+            ),
+            indoc!(
+                r#"
+                This expectation failed:
+
+                 3│>  expect
+                 4│>      actual : { headers: List U8, body: List U8, x: List U8 }
+                 5│>      actual = {
+                 6│>          body: [],
+                 7│>          headers: [],
+                 8│>          x: [],
+                 9│>      }
+                10│>
+                11│>      expected : { headers: List U8, body: List U8, x: List U8 }
+                12│>      expected = {
+                13│>          body: [ 42, 43, 44 ],
+                14│>          headers: [15, 16, 17],
+                15│>          x: [115, 116, 117],
+                16│>      }
+                17│>      actual == expected
+
+                When it failed, these variables had these values:
+
+                actual : {
+                    body : List (Int Unsigned8),
+                    headers : List (Int Unsigned8),
+                    x : List (Int Unsigned8),
+                }
+                actual = { body: [], headers: [], x: [] }
+
+                expected : {
+                    body : List (Int Unsigned8),
+                    headers : List (Int Unsigned8),
+                    x : List (Int Unsigned8),
+                }
+                expected = { body: [42, 43, 44], headers: [15, 16, 17], x: [115, 116, 117] }
+                "#
+            ),
+        );
+    }
+
+    #[test]
+    fn record_field_ordering() {
+        run_expect_test(
+            indoc!(
+                r#"
+                interface Test exposes [] imports []
+
+                Request : {
+                    fieldA : [Get, Post],
+                    fieldB : Str,
+                }
+
+                expect
+
+                    actual : Request
+                    actual = {
+                        fieldA: Get,
+                        fieldB: "/things?id=2",
+                    }
+
+                    expected : Request
+                    expected = {
+                        fieldA: Get,
+                        fieldB: "/things?id=1",
+                    }
+                    actual == expected
+                "#
+            ),
+            indoc!(
+                r#"
+                This expectation failed:
+
+                 8│>  expect
+                 9│>
+                10│>      actual : Request
+                11│>      actual = {
+                12│>          fieldA: Get,
+                13│>          fieldB: "/things?id=2",
+                14│>      }
+                15│>
+                16│>      expected : Request
+                17│>      expected = {
+                18│>          fieldA: Get,
+                19│>          fieldB: "/things?id=1",
+                20│>      }
+                21│>      actual == expected
+
+                When it failed, these variables had these values:
+
+                actual : Request
+                actual = { fieldA: Get, fieldB: "/things?id=2" }
+
+                expected : Request
+                expected = { fieldA: Get, fieldB: "/things?id=1" }
+
+                "#
+            ),
+        );
+    }
 }

examples/parser/Parser/Http.roc

@@ -134,6 +134,8 @@ expect
         """
     actual =
         parseStr request requestText
+
+    expected : Result Request [ParsingFailure Str, ParsingIncomplete Str]
     expected = Ok {
         method: Get,
         uri: "/things?id=1",