Merge pull request #2259 from rtfeldman/i/2227-record-layout-hang

Turn invalid record field types into runtime errors

ast/src/solve_type.rs

@@ -765,7 +765,8 @@ fn type_to_variable<'a>(
             let temp_ext_var = type_to_variable(arena, mempool, subs, rank, pools, cached, ext);
 
             let (it, new_ext_var) =
-                gather_fields_unsorted_iter(subs, RecordFields::empty(), temp_ext_var);
+                gather_fields_unsorted_iter(subs, RecordFields::empty(), temp_ext_var)
+                    .expect("Something ended up weird in this record type");
 
             let it = it
                 .into_iter()

compiler/mono/src/ir.rs

@@ -3325,8 +3325,15 @@ pub fn with_hole<'a>(
             mut fields,
             ..
         } => {
-            let sorted_fields =
+            let sorted_fields = match crate::layout::sort_record_fields(
-                crate::layout::sort_record_fields(env.arena, record_var, env.subs, env.ptr_bytes);
+                env.arena,
+                record_var,
+                env.subs,
+                env.ptr_bytes,
+            ) {
+                Ok(fields) => fields,
+                Err(_) => return Stmt::RuntimeError("Can't create record with improper layout"),
+            };
 
             let mut field_symbols = Vec::with_capacity_in(fields.len(), env.arena);
             let mut can_fields = Vec::with_capacity_in(fields.len(), env.arena);
@@ -3678,8 +3685,15 @@ pub fn with_hole<'a>(
             loc_expr,
             ..
         } => {
-            let sorted_fields =
+            let sorted_fields = match crate::layout::sort_record_fields(
-                crate::layout::sort_record_fields(env.arena, record_var, env.subs, env.ptr_bytes);
+                env.arena,
+                record_var,
+                env.subs,
+                env.ptr_bytes,
+            ) {
+                Ok(fields) => fields,
+                Err(_) => return Stmt::RuntimeError("Can't access record with improper layout"),
+            };
 
             let mut index = None;
             let mut field_layouts = Vec::with_capacity_in(sorted_fields.len(), env.arena);
@@ -3821,8 +3835,15 @@ pub fn with_hole<'a>(
             // This has the benefit that we don't need to do anything special for reference
             // counting
 
-            let sorted_fields =
+            let sorted_fields = match crate::layout::sort_record_fields(
-                crate::layout::sort_record_fields(env.arena, record_var, env.subs, env.ptr_bytes);
+                env.arena,
+                record_var,
+                env.subs,
+                env.ptr_bytes,
+            ) {
+                Ok(fields) => fields,
+                Err(_) => return Stmt::RuntimeError("Can't update record with improper layout"),
+            };
 
             let mut field_layouts = Vec::with_capacity_in(sorted_fields.len(), env.arena);
 
@@ -4391,11 +4412,7 @@ pub fn with_hole<'a>(
                 }
             }
         }
-        RuntimeError(e) => {
+        RuntimeError(e) => Stmt::RuntimeError(env.arena.alloc(format!("{:?}", e))),
-            eprintln!("emitted runtime error {:?}", &e);
-
-            Stmt::RuntimeError(env.arena.alloc(format!("{:?}", e)))
-        }
     }
 }
 
@@ -7383,7 +7400,8 @@ fn from_can_pattern_help<'a>(
             use crate::layout::UnionVariant::*;
 
             let res_variant =
-                crate::layout::union_sorted_tags(env.arena, *whole_var, env.subs, env.ptr_bytes);
+                crate::layout::union_sorted_tags(env.arena, *whole_var, env.subs, env.ptr_bytes)
+                    .map_err(Into::into);
 
             let variant = match res_variant {
                 Ok(cached) => cached,
@@ -7803,7 +7821,8 @@ fn from_can_pattern_help<'a>(
         } => {
             // sorted fields based on the type
             let sorted_fields =
-                crate::layout::sort_record_fields(env.arena, *whole_var, env.subs, env.ptr_bytes);
+                crate::layout::sort_record_fields(env.arena, *whole_var, env.subs, env.ptr_bytes)
+                    .map_err(RuntimeError::from)?;
 
             // sorted fields based on the destruct
             let mut mono_destructs = Vec::with_capacity_in(destructs.len(), env.arena);

compiler/mono/src/layout.rs

@@ -5,6 +5,7 @@ use roc_builtins::bitcode::{FloatWidth, IntWidth};
 use roc_collections::all::{default_hasher, MutMap};
 use roc_module::ident::{Lowercase, TagName};
 use roc_module::symbol::{Interns, Symbol};
+use roc_problem::can::RuntimeError;
 use roc_types::subs::{
     Content, FlatType, RecordFields, Subs, UnionTags, Variable, VariableSubsSlice,
 };
@@ -31,6 +32,15 @@ pub enum LayoutProblem {
     Erroneous,
 }
 
+impl From<LayoutProblem> for RuntimeError {
+    fn from(lp: LayoutProblem) -> Self {
+        match lp {
+            LayoutProblem::UnresolvedTypeVar(_) => RuntimeError::UnresolvedTypeVar,
+            LayoutProblem::Erroneous => RuntimeError::ErroneousType,
+        }
+    }
+}
+
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 pub enum RawFunctionLayout<'a> {
     Function(&'a [Layout<'a>], LambdaSet<'a>, &'a Layout<'a>),
@@ -1495,23 +1505,17 @@ fn layout_from_flat_type<'a>(
         Record(fields, ext_var) => {
             // extract any values from the ext_var
 
-            let pairs_it = fields
+            let mut pairs = Vec::with_capacity_in(fields.len(), arena);
-                .unsorted_iterator(subs, ext_var)
+            for (label, field) in fields.unsorted_iterator(subs, ext_var) {
-                .filter_map(|(label, field)| {
+                // drop optional fields
-                    // drop optional fields
+                let var = match field {
-                    let var = match field {
+                    RecordField::Optional(_) => continue,
-                        RecordField::Optional(_) => return None,
+                    RecordField::Required(var) => var,
-                        RecordField::Required(var) => var,
+                    RecordField::Demanded(var) => var,
-                        RecordField::Demanded(var) => var,
+                };
-                    };
 
-                    Some((
+                pairs.push((label, Layout::from_var(env, var)?));
-                        label,
+            }
-                        Layout::from_var(env, var).expect("invalid layout from var"),
-                    ))
-                });
-
-            let mut pairs = Vec::from_iter_in(pairs_it, arena);
 
             pairs.sort_by(|(label1, layout1), (label2, layout2)| {
                 let size1 = layout1.alignment_bytes(ptr_bytes);
@@ -1630,12 +1634,14 @@ fn layout_from_flat_type<'a>(
     }
 }
 
+pub type SortedField<'a> = (Lowercase, Variable, Result<Layout<'a>, Layout<'a>>);
+
 pub fn sort_record_fields<'a>(
     arena: &'a Bump,
     var: Variable,
     subs: &Subs,
     ptr_bytes: u32,
-) -> Vec<'a, (Lowercase, Variable, Result<Layout<'a>, Layout<'a>>)> {
+) -> Result<Vec<'a, SortedField<'a>>, LayoutProblem> {
     let mut env = Env {
         arena,
         subs,
@@ -1643,7 +1649,10 @@ pub fn sort_record_fields<'a>(
         ptr_bytes,
     };
 
-    let (it, _) = gather_fields_unsorted_iter(subs, RecordFields::empty(), var);
+    let (it, _) = match gather_fields_unsorted_iter(subs, RecordFields::empty(), var) {
+        Ok(it) => it,
+        Err(_) => return Err(LayoutProblem::Erroneous),
+    };
 
     let it = it
         .into_iter()
@@ -1655,26 +1664,23 @@ pub fn sort_record_fields<'a>(
 fn sort_record_fields_help<'a>(
     env: &mut Env<'a, '_>,
     fields_map: impl Iterator<Item = (Lowercase, RecordField<Variable>)>,
-) -> Vec<'a, (Lowercase, Variable, Result<Layout<'a>, Layout<'a>>)> {
+) -> Result<Vec<'a, SortedField<'a>>, LayoutProblem> {
     let ptr_bytes = env.ptr_bytes;
 
     // Sort the fields by label
     let mut sorted_fields = Vec::with_capacity_in(fields_map.size_hint().0, env.arena);
 
     for (label, field) in fields_map {
-        let var = match field {
+        match field {
-            RecordField::Demanded(v) => v,
+            RecordField::Demanded(v) | RecordField::Required(v) => {
-            RecordField::Required(v) => v,
+                let layout = Layout::from_var(env, v)?;
+                sorted_fields.push((label, v, Ok(layout)));
+            }
             RecordField::Optional(v) => {
-                let layout = Layout::from_var(env, v).expect("invalid layout from var");
+                let layout = Layout::from_var(env, v)?;
                 sorted_fields.push((label, v, Err(layout)));
-                continue;
             }
         };
-
-        let layout = Layout::from_var(env, var).expect("invalid layout from var");
-
-        sorted_fields.push((label, var, Ok(layout)));
     }
 
     sorted_fields.sort_by(
@@ -1690,7 +1696,7 @@ fn sort_record_fields_help<'a>(
         },
     );
 
-    sorted_fields
+    Ok(sorted_fields)
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
@@ -2428,7 +2434,10 @@ fn layout_from_tag_union<'a>(
 #[cfg(debug_assertions)]
 pub fn ext_var_is_empty_record(subs: &Subs, ext_var: Variable) -> bool {
     // the ext_var is empty
-    let fields = roc_types::types::gather_fields(subs, RecordFields::empty(), ext_var);
+    let fields = match roc_types::types::gather_fields(subs, RecordFields::empty(), ext_var) {
+        Ok(fields) => fields,
+        Err(_) => return false,
+    };
 
     fields.fields.is_empty()
 }

compiler/solve/src/solve.rs

@@ -764,7 +764,8 @@ fn type_to_variable<'a>(
             let temp_ext_var = type_to_variable(subs, rank, pools, arena, ext);
 
             let (it, new_ext_var) =
-                gather_fields_unsorted_iter(subs, RecordFields::empty(), temp_ext_var);
+                gather_fields_unsorted_iter(subs, RecordFields::empty(), temp_ext_var)
+                    .expect("Something ended up weird in this record type");
 
             let it = it
                 .into_iter()

compiler/test_gen/src/gen_list.rs

@@ -1,6 +1,9 @@
 #[cfg(feature = "gen-llvm")]
 use crate::helpers::llvm::assert_evals_to;
 
+#[cfg(feature = "gen-llvm")]
+use crate::helpers::llvm::expect_runtime_error_panic;
+
 // #[cfg(feature = "gen-dev")]
 // use crate::helpers::dev::assert_evals_to;
 
@@ -2528,7 +2531,10 @@ fn list_any_empty_with_unknown_element_type() {
     //     Application crashed with message
     //     UnresolvedTypeVar compiler/mono/src/ir.rs line 3775
     //     Shutting down
-    assert_evals_to!("List.any [] (\\_ -> True)", false, bool);
+    //
+    // TODO: eventually we should insert the empty type for unresolved type
+    // variables, since that means they're unbound.
+    expect_runtime_error_panic!("List.any [] (\\_ -> True)");
 }
 
 #[test]
@@ -2550,7 +2556,10 @@ fn list_all_empty_with_unknown_element_type() {
     //     Application crashed with message
     //     UnresolvedTypeVar compiler/mono/src/ir.rs line 3775
     //     Shutting down
-    assert_evals_to!("List.all [] (\\_ -> True)", false, bool);
+    //
+    // TODO: eventually we should insert the empty type for unresolved type
+    // variables, since that means they're unbound.
+    expect_runtime_error_panic!("List.all [] (\\_ -> True)");
 }
 
 #[test]

compiler/test_gen/src/gen_records.rs

@@ -1,6 +1,9 @@
 #[cfg(feature = "gen-llvm")]
 use crate::helpers::llvm::assert_evals_to;
 
+#[cfg(feature = "gen-llvm")]
+use crate::helpers::llvm::expect_runtime_error_panic;
+
 #[cfg(feature = "gen-dev")]
 use crate::helpers::dev::assert_evals_to;
 
@@ -1007,3 +1010,22 @@ fn both_have_unique_fields() {
         i64
     );
 }
+
+#[test]
+#[cfg(any(feature = "gen-llvm"))]
+#[should_panic(
+    // TODO: something upstream is escaping the '
+    expected = r#"Roc failed with message: "Can\'t create record with improper layout""#
+)]
+fn call_with_bad_record_runtime_error() {
+    expect_runtime_error_panic!(indoc!(
+        r#"
+            app "test" provides [ main ] to "./platform"
+
+            main =
+                get : {a: Bool} -> Bool
+                get = \{a} -> a
+                get {b: ""}
+            "#
+    ))
+}

compiler/test_gen/src/helpers/llvm.rs

@@ -28,8 +28,6 @@ pub fn test_builtin_defs(symbol: Symbol, var_store: &mut VarStore) -> Option<Def
     builtin_defs_map(symbol, var_store)
 }
 
-// this is not actually dead code, but only used by cfg_test modules
-// so "normally" it is dead, only at testing time is it used
 #[allow(clippy::too_many_arguments)]
 fn create_llvm_module<'a>(
     arena: &'a bumpalo::Bump,
@@ -589,16 +587,39 @@ macro_rules! assert_evals_to {
         assert_evals_to!($src, $expected, $ty, $crate::helpers::llvm::identity);
     }};
     ($src:expr, $expected:expr, $ty:ty, $transform:expr) => {
-        // Same as above, except with an additional transformation argument.
+        // same as above, except with an additional transformation argument.
         {
             #[cfg(feature = "wasm-cli-run")]
-            $crate::helpers::llvm::assert_wasm_evals_to!($src, $expected, $ty, $transform, false);
+            $crate::helpers::llvm::assert_wasm_evals_to!(
+                $src, $expected, $ty, $transform, false, false
+            );
 
             $crate::helpers::llvm::assert_llvm_evals_to!($src, $expected, $ty, $transform, false);
         }
     };
 }
 
+macro_rules! expect_runtime_error_panic {
+    ($src:expr) => {{
+        #[cfg(feature = "wasm-cli-run")]
+        $crate::helpers::llvm::assert_wasm_evals_to!(
+            $src,
+            false, // fake value/type for eval
+            bool,
+            $crate::helpers::llvm::identity,
+            true // ignore problems
+        );
+
+        $crate::helpers::llvm::assert_llvm_evals_to!(
+            $src,
+            false, // fake value/type for eval
+            bool,
+            $crate::helpers::llvm::identity,
+            true // ignore problems
+        );
+    }};
+}
+
 #[allow(dead_code)]
 pub fn identity<T>(value: T) -> T {
     value
@@ -617,7 +638,7 @@ macro_rules! assert_non_opt_evals_to {
     ($src:expr, $expected:expr, $ty:ty, $transform:expr) => {
         // Same as above, except with an additional transformation argument.
         {
-            $crate::helpers::llvm::assert_llvm_evals_to!($src, $expected, $ty, $transform, false);
+            $crate::helpers::llvm::assert_llvm_evals_to!($src, $expected, $ty, $transform);
         }
     };
     ($src:expr, $expected:expr, $ty:ty, $transform:expr) => {{
@@ -633,3 +654,5 @@ pub(crate) use assert_llvm_evals_to;
 pub(crate) use assert_non_opt_evals_to;
 #[allow(unused_imports)]
 pub(crate) use assert_wasm_evals_to;
+#[allow(unused_imports)]
+pub(crate) use expect_runtime_error_panic;

compiler/types/src/pretty_print.rs

@@ -519,7 +519,8 @@ fn write_flat_type(env: &Env, flat_type: &FlatType, subs: &Subs, buf: &mut Strin
             let RecordStructure {
                 fields: sorted_fields,
                 ext,
-            } = gather_fields(subs, *fields, *ext_var);
+            } = gather_fields(subs, *fields, *ext_var)
+                .expect("Something ended up weird in this record type");
             let ext_var = ext;
 
             if fields.is_empty() {

compiler/types/src/subs.rs

@@ -2010,7 +2010,8 @@ impl RecordFields {
         subs: &'a Subs,
         ext: Variable,
     ) -> impl Iterator<Item = (&Lowercase, RecordField<Variable>)> + 'a {
-        let (it, _) = crate::types::gather_fields_unsorted_iter(subs, *self, ext);
+        let (it, _) = crate::types::gather_fields_unsorted_iter(subs, *self, ext)
+            .expect("Something weird ended up in a record type");
 
         it
     }
@@ -2045,7 +2046,8 @@ impl RecordFields {
                 ext,
             )
         } else {
-            let record_structure = crate::types::gather_fields(subs, *self, ext);
+            let record_structure = crate::types::gather_fields(subs, *self, ext)
+                .expect("Something ended up weird in this record type");
 
             (
                 Box::new(record_structure.fields.into_iter()),

compiler/types/src/types.rs

@@ -1701,14 +1701,20 @@ pub fn name_type_var(letters_used: u32, taken: &mut MutSet<Lowercase>) -> (Lower
     }
 }
 
+#[derive(Debug, Copy, Clone)]
+pub struct RecordFieldsError;
+
 pub fn gather_fields_unsorted_iter(
     subs: &Subs,
     other_fields: RecordFields,
     mut var: Variable,
-) -> (
+) -> Result<
-    impl Iterator<Item = (&Lowercase, RecordField<Variable>)> + '_,
+    (
-    Variable,
+        impl Iterator<Item = (&Lowercase, RecordField<Variable>)> + '_,
-) {
+        Variable,
+    ),
+    RecordFieldsError,
+> {
     use crate::subs::Content::*;
     use crate::subs::FlatType::*;
 
@@ -1733,7 +1739,7 @@ pub fn gather_fields_unsorted_iter(
             // TODO investigate apparently this one pops up in the reporting tests!
             RigidVar(_) => break,
 
-            other => unreachable!("something weird ended up in a record type: {:?}", other),
+            _ => return Err(RecordFieldsError),
         }
     }
 
@@ -1749,11 +1755,15 @@ pub fn gather_fields_unsorted_iter(
             (field_name, record_field)
         });
 
-    (it, var)
+    Ok((it, var))
 }
 
-pub fn gather_fields(subs: &Subs, other_fields: RecordFields, var: Variable) -> RecordStructure {
+pub fn gather_fields(
-    let (it, ext) = gather_fields_unsorted_iter(subs, other_fields, var);
+    subs: &Subs,
+    other_fields: RecordFields,
+    var: Variable,
+) -> Result<RecordStructure, RecordFieldsError> {
+    let (it, ext) = gather_fields_unsorted_iter(subs, other_fields, var)?;
 
     let mut result: Vec<_> = it
         .map(|(ref_label, field)| (ref_label.clone(), field))
@@ -1761,10 +1771,10 @@ pub fn gather_fields(subs: &Subs, other_fields: RecordFields, var: Variable) ->
 
     result.sort_by(|(a, _), (b, _)| a.cmp(b));
 
-    RecordStructure {
+    Ok(RecordStructure {
         fields: result,
         ext,
-    }
+    })
 }
 
 pub fn gather_tags_unsorted_iter(