remove label from Struct layout

compiler/gen/src/crane/build.rs

@@ -238,7 +238,7 @@ pub fn build_expr<'a, B: Backend>(
                 .ins()
                 .stack_addr(cfg.pointer_type(), slot, Offset32::new(0))
         }
-        Tag { tag_layout, arguments , tag_id, union_size, .. } => {
+        Tag { tag_layout, arguments, .. } => {
             let cfg = env.cfg;
             let ptr_bytes = cfg.pointer_bytes() as u32;
 
@@ -256,14 +256,6 @@ pub fn build_expr<'a, B: Backend>(
             // Create instructions for storing each field's expression
             let mut offset = 0;
 
-            // still need to insert the tag discriminator for non-single unions
-            // when there are no arguments, e.g. `Nothing : Maybe a`
-            if *union_size > 1 {
-                let val = builder.ins().iconst(types::I64, *tag_id as i64);
-                builder.ins().stack_store(val, slot, Offset32::new(0));
-                offset += ptr_bytes;
-            }
-
             for (field_expr, field_layout) in arguments.iter() {
                 let val = build_expr(env, &scope, module, builder, field_expr, procs);
 
@@ -276,7 +268,6 @@ pub fn build_expr<'a, B: Backend>(
                 offset += field_size;
             }
 
-
             builder
                 .ins()
                 .stack_addr(cfg.pointer_type(), slot, Offset32::new(0))

compiler/gen/src/llvm/build.rs

@@ -370,8 +370,6 @@ pub fn build_expr<'a, 'ctx, 'env>(
         Tag {
             arguments,
             tag_layout,
-            union_size,
-            tag_id,
             ..
         } => {
             let ptr_size = env.ptr_bytes;
@@ -387,23 +385,6 @@ pub fn build_expr<'a, 'ctx, 'env>(
             let mut field_types = Vec::with_capacity_in(num_fields, env.arena);
             let mut field_vals = Vec::with_capacity_in(num_fields, env.arena);
 
-            // insert the discriminant value
-            if *union_size > 1 {
-                let val = env
-                    .context
-                    .i64_type()
-                    .const_int(*tag_id as u64, true)
-                    .into();
-
-                let field_type = env.context.i64_type().into();
-
-                field_types.push(field_type);
-                field_vals.push(val);
-
-                let field_size = ptr_size;
-                filler -= field_size;
-            }
-
             for (field_expr, field_layout) in arguments.iter() {
                 let val = build_expr(env, &scope, parent, field_expr, procs);
                 let field_type =

compiler/gen/src/llvm/convert.rs

@@ -66,7 +66,7 @@ pub fn basic_type_from_layout<'ctx>(
             // Determine types
             let mut field_types = Vec::with_capacity_in(sorted_fields.len(), arena);
 
-            for (_, field_layout) in sorted_fields.iter() {
+            for field_layout in sorted_fields.iter() {
                 field_types.push(basic_type_from_layout(
                     arena,
                     context,

compiler/mono/src/decision_tree.rs

@@ -942,10 +942,7 @@ fn path_to_expr_help<'a>(
 
             let (is_unwrapped, field_layouts) = match outer_layout {
                 Layout::Union(layouts) => (layouts.is_empty(), layouts[*tag_id as usize].to_vec()),
-                Layout::Struct(layouts) => (
+                Layout::Struct(layouts) => (true, layouts.to_vec()),
-                    true,
-                    layouts.iter().map(|v| v.1.clone()).collect::<Vec<_>>(),
-                ),
                 other => (true, vec![other]),
             };
 

compiler/mono/src/expr.rs

@@ -1,4 +1,4 @@
-use crate::layout::{Builtin, Layout};
+use crate::layout::{Builtin, Layout, MAX_ENUM_SIZE};
 use crate::pattern::{Ctor, Guard};
 use bumpalo::collections::Vec;
 use bumpalo::Bump;
@@ -654,26 +654,22 @@ fn from_can<'a>(
             ..
         } => {
             let arena = env.arena;
-            let mut field_tuples = Vec::with_capacity_in(fields.len(), arena);
 
-            match Layout::from_var(arena, record_var, env.subs, env.pointer_size) {
+            let btree = crate::layout::record_fields_btree(
-                Ok(Layout::Struct(field_layouts)) => {
+                env.arena,
-                    for (label, field_layout) in field_layouts.iter() {
+                record_var,
-                        let loc_expr = fields.remove(label).unwrap().loc_expr;
+                env.subs,
-                        let expr = from_can(env, loc_expr.value, procs, None);
+                env.pointer_size,
+            );
 
-                        // TODO try to remove this clone
+            let mut field_tuples = Vec::with_capacity_in(btree.len(), arena);
-                        field_tuples.push((expr, field_layout.clone()));
+
+            for (label, layout) in btree {
+                let field = fields.remove(&label).unwrap();
+                let expr = from_can(env, field.loc_expr.value, procs, None);
+
+                field_tuples.push((expr, layout));
             }
-                }
-                Ok(_) => {
-                    unreachable!("Somehow a Record did not end up with a Struct layout");
-                }
-                Err(()) => {
-                    // Invalid field!
-                    panic!("TODO gracefully handle Record with invalid struct_layout");
-                }
-            };
 
             Expr::Struct(field_tuples.into_bump_slice())
         }
@@ -688,69 +684,76 @@ fn from_can<'a>(
         } => {
             let arena = env.arena;
 
-            let mut fields = std::vec::Vec::new();
+            let (is_enum_candidate, sorted_tag_layouts) = crate::layout::union_sorted_tags(
+                env.arena,
+                variant_var,
+                env.subs,
+                env.pointer_size,
+            );
 
-            match roc_types::pretty_print::chase_ext_tag_union(env.subs, variant_var, &mut fields) {
+            let union_size = sorted_tag_layouts.len() as u8;
-                Ok(()) | Err((_, Content::FlexVar(_))) => {}
-                Err(content) => panic!("invalid content in ext_var: {:?}", content),
-            }
 
-            fields.sort();
+            let (tag_id, (_, argument_layouts)) = sorted_tag_layouts
-            let tag_id = fields
                 .iter()
-                .position(|(key, _)| key == &tag_name)
+                .enumerate()
+                .find(|(_, (key, _))| key == &tag_name)
                 .expect("tag must be in its own type");
 
-            match Layout::from_var(arena, variant_var, &env.subs, env.pointer_size) {
+            match sorted_tag_layouts.len() {
-                Ok(Layout::Builtin(Builtin::Bool)) => Expr::Bool(tag_id != 0),
+                2 if is_enum_candidate => Expr::Bool(tag_id != 0),
-                Ok(Layout::Builtin(Builtin::Byte)) => Expr::Byte(tag_id as u8),
+                3..=MAX_ENUM_SIZE if is_enum_candidate => Expr::Byte(tag_id as u8),
-                Ok(layout) => {
+                len => {
                     let mut arguments = Vec::with_capacity_in(args.len(), arena);
 
-                    for (arg_var, arg) in args {
+                    let is_unwrapped = len == 1;
-                        let arg_layout =
-                            Layout::from_var(env.arena, arg_var, env.subs, env.pointer_size)
-                                .expect("invalid ret_layout");
 
-                        arguments.push((from_can(env, arg.value, procs, None), arg_layout));
+                    // when the tag is not unwrapped, the layout will contain a slot for the tag
+                    // discriminant, but it's not yet part of the argument expressions. So we need
+                    // to conditionally insert it. We don't want to collect() into a data
+                    // structure, and thus must conditionally pick one of two iterators. that's a
+                    // bit tricky, as you can see below. Based on
+                    // https://stackoverflow.com/questions/29760668/conditionally-iterate-over-one-of-several-possible-iterators
+                    let argument_exprs = {
+                        let mut iter_1 = None;
+                        let mut iter_2 = None;
+
+                        let shared = args
+                            .into_iter()
+                            .map(|(_, arg)| from_can(env, arg.value, procs, None));
+
+                        if is_unwrapped {
+                            iter_1 = Some(shared);
+                        } else {
+                            iter_2 = Some(std::iter::once(Expr::Int(tag_id as i64)).chain(shared));
                         }
 
-                    let mut tags = std::vec::Vec::new();
+                        iter_1
-                    match roc_types::pretty_print::chase_ext_tag_union(
+                            .into_iter()
-                        env.subs,
+                            .flatten()
-                        variant_var,
+                            .chain(iter_2.into_iter().flatten())
-                        &mut tags,
+                    };
-                    ) {
+
-                        Ok(()) | Err((_, Content::FlexVar(_))) => {
+                    for (arg_layout, arg_expr) in argument_layouts.iter().zip(argument_exprs) {
-                            tags.sort();
+                        arguments.push((arg_expr, arg_layout.clone()));
-                        }
-                        other => panic!("invalid value in ext_var {:?}", other),
                     }
 
-                    let mut opt_tag_id = None;
+                    let mut layouts: Vec<&'a [Layout<'a>]> =
-                    for (index, (name, _)) in tags.iter().enumerate() {
+                        Vec::with_capacity_in(sorted_tag_layouts.len(), env.arena);
-                        if name == &tag_name {
+
-                            opt_tag_id = Some(index as u8);
+                    for (_, arg_layouts) in sorted_tag_layouts.into_iter() {
-                            break;
+                        layouts.push(arg_layouts);
-                        }
                     }
 
-                    let union_size = tags.len() as u8;
+                    let layout = Layout::Union(layouts.into_bump_slice());
-
-                    let tag_id = opt_tag_id.expect("Tag must be in its own type");
 
                     Expr::Tag {
                         tag_layout: layout,
                         tag_name,
-                        tag_id,
+                        tag_id: tag_id as u8,
                         union_size,
                         arguments: arguments.into_bump_slice(),
                     }
                 }
-                Err(()) => {
-                    // Invalid field!
-                    panic!("TODO gracefully handle Access with invalid struct_layout");
-                }
             }
         }
 
@@ -762,18 +765,32 @@ fn from_can<'a>(
         } => {
             let arena = env.arena;
 
-            let record_layout =
+            let btree = crate::layout::record_fields_btree(
-                match Layout::from_var(arena, record_var, env.subs, env.pointer_size) {
+                env.arena,
-                    Ok(layout) => layout,
+                record_var,
-                    Err(()) => {
+                env.subs,
-                        // Invalid field!
+                env.pointer_size,
-                        panic!("TODO gracefully handle Access with invalid struct_layout");
+            );
+
+            let mut index = None;
+            let mut field_layouts = Vec::with_capacity_in(btree.len(), env.arena);
+
+            for (current, (label, field_layout)) in btree.into_iter().enumerate() {
+                field_layouts.push(field_layout);
+
+                if label == field {
+                    index = Some(current);
+                }
             }
-                };
 
             let record = arena.alloc(from_can(env, loc_expr.value, procs, None));
 
-            record_access_by_field(env, record_layout, record, field)
+            Expr::AccessAtIndex {
+                index: index.expect("field not in its own type") as u64,
+                field_layouts: field_layouts.into_bump_slice(),
+                expr: record,
+                is_unwrapped: true,
+            }
         }
 
         List {
@@ -810,45 +827,6 @@ fn from_can<'a>(
     }
 }
 
-fn record_access_by_field<'a>(
-    env: &mut Env<'a, '_>,
-    record_layout: Layout<'a>,
-    record_expr: &'a Expr<'a>,
-    field: Lowercase,
-) -> Expr<'a> {
-    let (field_layouts, index) = match record_layout {
-        Layout::Struct(sorted_fields) => {
-            let index = sorted_fields
-                .iter()
-                .position(|(local_label, _)| local_label == &field)
-                .unwrap() as u64;
-
-            let mut only_fields = Vec::with_capacity_in(sorted_fields.len(), env.arena);
-
-            for (_, field) in sorted_fields.iter() {
-                only_fields.push(field.clone());
-            }
-
-            (only_fields.into_bump_slice(), index)
-        }
-
-        other => {
-            // Invalid field!
-            panic!(
-                "TODO gracefully handle Access with invalid struct_layout {:?}",
-                other
-            );
-        }
-    };
-
-    Expr::AccessAtIndex {
-        index,
-        field_layouts,
-        expr: record_expr,
-        is_unwrapped: true,
-    }
-}
-
 fn store_pattern<'a>(
     env: &mut Env<'a, '_>,
     can_pat: &Pattern<'a>,
@@ -910,9 +888,16 @@ fn store_pattern<'a>(
                 }
             }
         }
-        RecordDestructure(destructs, layout) => {
+        RecordDestructure(destructs, Layout::Struct(sorted_fields)) => {
-            for destruct in destructs {
+            for (index, destruct) in destructs.iter().enumerate() {
-                store_record_destruct(env, destruct, outer_symbol, layout.clone(), stored)?;
+                store_record_destruct(
+                    env,
+                    destruct,
+                    index as u64,
+                    outer_symbol,
+                    sorted_fields,
+                    stored,
+                )?;
             }
         }
 
@@ -933,13 +918,21 @@ fn store_pattern<'a>(
 fn store_record_destruct<'a>(
     env: &mut Env<'a, '_>,
     destruct: &RecordDestruct<'a>,
+    index: u64,
     outer_symbol: Symbol,
-    struct_layout: Layout<'a>,
+    sorted_fields: &'a [Layout<'a>],
     stored: &mut Vec<'a, (Symbol, Layout<'a>, Expr<'a>)>,
 ) -> Result<(), String> {
     use Pattern::*;
+
     let record = env.arena.alloc(Expr::Load(outer_symbol));
-    let load = record_access_by_field(env, struct_layout, record, destruct.label.clone());
+    let load = Expr::AccessAtIndex {
+        index,
+        field_layouts: sorted_fields,
+        expr: record,
+        is_unwrapped: true,
+    };
+
     match &destruct.guard {
         None => {
             stored.push((destruct.symbol, destruct.layout.clone(), load));
@@ -1411,8 +1404,7 @@ fn from_can_pattern<'a>(
             whole_var,
             destructs,
             ..
-        } => match Layout::from_var(env.arena, *whole_var, env.subs, env.pointer_size) {
+        } => {
-            Ok(Layout::Struct(field_layouts)) => {
             let mut mono_destructs = Vec::with_capacity_in(destructs.len(), env.arena);
             let mut destructs = destructs.clone();
             destructs.sort_by(|a, b| a.value.label.cmp(&b.value.label));
@@ -1420,11 +1412,18 @@ fn from_can_pattern<'a>(
             let mut it = destructs.iter();
             let mut opt_destruct = it.next();
 
-                // insert underscore patterns for unused fields. We need the record to be fully
+            let btree = crate::layout::record_fields_btree(
-                // matched for pattern exhaustiveness checking
+                env.arena,
-                for (label, field_layout) in field_layouts.iter() {
+                *whole_var,
+                env.subs,
+                env.pointer_size,
+            );
+
+            let mut field_layouts = Vec::with_capacity_in(btree.len(), env.arena);
+
+            for (label, field_layout) in btree.into_iter() {
                 if let Some(destruct) = opt_destruct {
-                        if &destruct.value.label == label {
+                    if destruct.value.label == label {
                         opt_destruct = it.next();
 
                         mono_destructs.push(from_can_record_destruct(
@@ -1450,12 +1449,14 @@ fn from_can_pattern<'a>(
                         guard: Some(Pattern::Underscore),
                     });
                 }
+                field_layouts.push(field_layout);
             }
 
-                Pattern::RecordDestructure(mono_destructs, Layout::Struct(field_layouts))
+            Pattern::RecordDestructure(
+                mono_destructs,
+                Layout::Struct(field_layouts.into_bump_slice()),
+            )
         }
-            Ok(_) | Err(()) => panic!("Invalid layout"),
-        },
     }
 }
 

compiler/mono/src/layout.rs

@@ -4,12 +4,15 @@ use roc_collections::all::MutMap;
 use roc_module::ident::{Lowercase, TagName};
 use roc_module::symbol::Symbol;
 use roc_types::subs::{Content, FlatType, Subs, Variable};
+use std::collections::BTreeMap;
+
+pub const MAX_ENUM_SIZE: usize = (std::mem::size_of::<u8>() * 8) as usize;
 
 /// Types for code gen must be monomorphic. No type variables allowed!
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub enum Layout<'a> {
     Builtin(Builtin<'a>),
-    Struct(&'a [(Lowercase, Layout<'a>)]),
+    Struct(&'a [Layout<'a>]),
     Union(&'a [&'a [Layout<'a>]]),
     /// A function. The types of its arguments, then the type of its return value.
     FunctionPointer(&'a [Layout<'a>], &'a Layout<'a>),
@@ -86,7 +89,7 @@ impl<'a> Layout<'a> {
             Builtin(builtin) => builtin.safe_to_memcpy(),
             Struct(fields) => fields
                 .iter()
-                .all(|(_, field_layout)| field_layout.safe_to_memcpy()),
+                .all(|field_layout| field_layout.safe_to_memcpy()),
             Union(tags) => tags
                 .iter()
                 .all(|tag_layout| tag_layout.iter().all(|field| field.safe_to_memcpy())),
@@ -109,7 +112,7 @@ impl<'a> Layout<'a> {
             Struct(fields) => {
                 let mut sum = 0;
 
-                for (_, field_layout) in *fields {
+                for field_layout in *fields {
                     sum += field_layout.stack_size(pointer_size);
                 }
 
@@ -258,34 +261,14 @@ fn layout_from_flat_type<'a>(
         }
         Record(fields, ext_var) => {
             debug_assert!(ext_var_is_empty_record(subs, ext_var));
-            let ext_content = subs.get_without_compacting(ext_var).content;
-            let ext_layout = match Layout::from_content(arena, ext_content, subs, pointer_size) {
-                Ok(layout) => layout,
-                Err(()) => {
-                    // Invalid record!
-                    panic!("TODO gracefully handle record with invalid ext_var");
-                }
-            };
 
-            let mut field_layouts: Vec<'a, (Lowercase, Layout<'a>)>;
+            let btree = fields
+                .into_iter()
+                .collect::<BTreeMap<Lowercase, Variable>>();
 
-            match ext_layout {
+            let mut layouts = Vec::with_capacity_in(btree.len(), arena);
-                Layout::Struct(more_fields) => {
-                    field_layouts = Vec::with_capacity_in(fields.len() + more_fields.len(), arena);
 
-                    for (label, field) in more_fields {
+            for (_, field_var) in btree {
-                        field_layouts.push((label.clone(), field.clone()));
-                    }
-                }
-                _ => {
-                    panic!(
-                        "TODO handle Layout for invalid record extension, specifically {:?}",
-                        ext_layout
-                    );
-                }
-            }
-
-            for (label, field_var) in fields {
                 let field_content = subs.get_without_compacting(field_var).content;
                 let field_layout =
                     match Layout::from_content(arena, field_content, subs, pointer_size) {
@@ -296,13 +279,10 @@ fn layout_from_flat_type<'a>(
                         }
                     };
 
-                field_layouts.push((label.clone(), field_layout));
+                layouts.push(field_layout);
             }
 
-            // Sort fields by label
+            Ok(Layout::Struct(layouts.into_bump_slice()))
-            field_layouts.sort_by(|(a, _), (b, _)| a.cmp(b));
-
-            Ok(Layout::Struct(field_layouts.into_bump_slice()))
         }
         TagUnion(tags, ext_var) => {
             debug_assert!(ext_var_is_empty_tag_union(subs, ext_var));
@@ -323,6 +303,73 @@ fn layout_from_flat_type<'a>(
     }
 }
 
+pub fn record_fields_btree<'a>(
+    arena: &'a Bump,
+    var: Variable,
+    subs: &Subs,
+    pointer_size: u32,
+) -> BTreeMap<Lowercase, Layout<'a>> {
+    let mut fields_map = MutMap::default();
+    match roc_types::pretty_print::chase_ext_record(subs, var, &mut fields_map) {
+        Ok(()) | Err((_, Content::FlexVar(_))) => {
+            // collect into btreemap to sort
+            fields_map
+                .into_iter()
+                .map(|(label, var)| {
+                    (
+                        label,
+                        Layout::from_var(arena, var, subs, pointer_size)
+                            .expect("invalid layout from var"),
+                    )
+                })
+                .collect::<BTreeMap<Lowercase, Layout<'a>>>()
+        }
+        Err(other) => panic!("invalid content in record variable: {:?}", other),
+    }
+}
+
+pub fn union_sorted_tags<'a>(
+    arena: &'a Bump,
+    var: Variable,
+    subs: &Subs,
+    pointer_size: u32,
+) -> (bool, Vec<'a, (TagName, &'a [Layout<'a>])>) {
+    let mut tags_vec = std::vec::Vec::new();
+    match roc_types::pretty_print::chase_ext_tag_union(subs, var, &mut tags_vec) {
+        Ok(()) | Err((_, Content::FlexVar(_))) => {
+            // for this union be be an enum, none of the tags may have any arguments
+            let is_enum_candidate =
+                tags_vec.len() <= MAX_ENUM_SIZE && tags_vec.iter().all(|(_, args)| args.is_empty());
+
+            let is_unwrapped = tags_vec.len() == 1;
+            // collect into btreemap to sort
+            tags_vec.sort();
+
+            let mut result = Vec::with_capacity_in(tags_vec.len(), arena);
+            for (tag_name, arguments) in tags_vec {
+                let mut arg_layouts =
+                    Vec::with_capacity_in(arguments.len() + !is_unwrapped as usize, arena);
+
+                // if not unwrapped, add a slot for the tag discriminant
+                if !is_unwrapped && !is_enum_candidate {
+                    arg_layouts.push(Layout::Builtin(Builtin::Int64))
+                }
+
+                for var in arguments {
+                    let layout = Layout::from_var(arena, var, subs, pointer_size)
+                        .expect("invalid layout from var");
+                    arg_layouts.push(layout);
+                }
+
+                result.push((tag_name, arg_layouts.into_bump_slice()));
+            }
+
+            (is_enum_candidate, result)
+        }
+        Err(other) => panic!("invalid content in record variable: {:?}", other),
+    }
+}
+
 pub fn layout_from_tag_union<'a>(
     arena: &'a Bump,
     tags: &MutMap<TagName, std::vec::Vec<Variable>>,

compiler/types/src/pretty_print.rs

@@ -547,6 +547,11 @@ pub fn chase_ext_record(
 
         Structure(EmptyRecord) => Ok(()),
 
+        Content::Structure(Apply(Symbol::ATTR_ATTR, arguments)) => {
+            debug_assert!(arguments.len() == 2);
+            chase_ext_record(subs, arguments[1], fields)
+        }
+
         Alias(_, _, var) => chase_ext_record(subs, var, fields),
 
         content => Err((var, content)),