rework how values from a pattern are assigned

compiler/gen/src/llvm/build.rs

@@ -395,7 +395,6 @@ pub fn build_exp_expr<'a, 'ctx, 'env>(
                 }
             }
 
-            dbg!(&field_vals);
             // If the struct has only one field that isn't zero-sized,
             // unwrap it. This is what the layout expects us to do.
             if field_vals.len() == 1 {

compiler/gen/tests/gen_num.rs

@@ -480,7 +480,7 @@ mod gen_num {
     }
 
     #[test]
-    fn if_guard_bind_variable() {
+    fn if_guard_bind_variable_false() {
         assert_evals_to!(
             indoc!(
                 r#"
@@ -492,7 +492,10 @@ mod gen_num {
             42,
             i64
         );
+    }
 
+    #[test]
+    fn if_guard_bind_variable_true() {
         assert_evals_to!(
             indoc!(
                 r#"

compiler/mono/src/decision_tree.rs

@@ -1,6 +1,6 @@
 use crate::exhaustive::{Ctor, RenderAs, TagId, Union};
-use crate::ir::{DestructType, Env, Expr, JoinPointId, Literal, Pattern, Stmt};
+use crate::ir::{DestructType, Env, Expr, JoinPointId, Literal, Pattern, Procs, Stmt};
-use crate::layout::{Builtin, Layout};
+use crate::layout::{Builtin, Layout, LayoutCache};
 use roc_collections::all::{MutMap, MutSet};
 use roc_module::ident::TagName;
 use roc_module::low_level::LowLevel;
@@ -882,6 +882,8 @@ type StoresVec<'a> = bumpalo::collections::Vec<'a, (Symbol, Layout<'a>, Expr<'a>
 
 pub fn optimize_when<'a>(
     env: &mut Env<'a, '_>,
+    procs: &mut Procs<'a>,
+    layout_cache: &mut LayoutCache<'a>,
     cond_symbol: Symbol,
     cond_layout: Layout<'a>,
     ret_layout: Layout<'a>,
@@ -918,6 +920,8 @@ pub fn optimize_when<'a>(
 
     let expr = decide_to_branching(
         env,
+        procs,
+        layout_cache,
         cond_symbol,
         cond_layout,
         ret_layout,
@@ -1125,8 +1129,14 @@ fn test_to_equality<'a>(
     }
 }
 
+// TODO procs and layout are currently unused, but potentially required
+// for defining optional fields?
+// if not, do remove
+#[allow(clippy::too_many_arguments)]
 fn decide_to_branching<'a>(
     env: &mut Env<'a, '_>,
+    procs: &mut Procs<'a>,
+    layout_cache: &mut LayoutCache<'a>,
     cond_symbol: Symbol,
     cond_layout: Layout<'a>,
     ret_layout: Layout<'a>,
@@ -1155,6 +1165,8 @@ fn decide_to_branching<'a>(
 
             let pass_expr = decide_to_branching(
                 env,
+                procs,
+                layout_cache,
                 cond_symbol,
                 cond_layout.clone(),
                 ret_layout.clone(),
@@ -1164,6 +1176,8 @@ fn decide_to_branching<'a>(
 
             let fail_expr = decide_to_branching(
                 env,
+                procs,
+                layout_cache,
                 cond_symbol,
                 cond_layout.clone(),
                 ret_layout.clone(),
@@ -1264,6 +1278,8 @@ fn decide_to_branching<'a>(
                     continuation: env.arena.alloc(cond),
                 };
 
+                // load all the variables (the guard might need them);
+
                 current_symbol = accum;
             }
 
@@ -1323,6 +1339,8 @@ fn decide_to_branching<'a>(
 
             let default_branch = decide_to_branching(
                 env,
+                procs,
+                layout_cache,
                 cond_symbol,
                 cond_layout.clone(),
                 ret_layout.clone(),
@@ -1335,6 +1353,8 @@ fn decide_to_branching<'a>(
             for (test, decider) in tests {
                 let branch = decide_to_branching(
                     env,
+                    procs,
+                    layout_cache,
                     cond_symbol,
                     cond_layout.clone(),
                     ret_layout.clone(),

compiler/mono/src/ir.rs

@@ -1007,16 +1007,15 @@ pub fn with_hole<'a>(
             // WRONG! this is introduces new control flow, and should call `from_can` again
             if let roc_can::pattern::Pattern::Identifier(symbol) = def.loc_pattern.value {
                 let stmt = with_hole(env, cont.value, procs, layout_cache, assigned, hole);
-                let expr = with_hole(
+
+                with_hole(
                     env,
                     def.loc_expr.value,
                     procs,
                     layout_cache,
                     symbol,
                     env.arena.alloc(stmt),
-                );
+                )
-
-                expr
             } else {
                 todo!()
             }
@@ -1782,17 +1781,20 @@ pub fn from_can<'a>(
                     .from_var(env.arena, def.expr_var, env.subs, env.pointer_size)
                     .expect("invalid layout");
 
-                let mut stores = Vec::new_in(env.arena);
-                let outer_symbol = env.unique_symbol();
-                store_pattern(env, &mono_pattern, outer_symbol, layout, &mut stores).unwrap();
-
                 // convert the continuation
                 let mut stmt = from_can(env, cont.value, procs, layout_cache);
 
-                // unpack the body of the def based on the pattern
+                let outer_symbol = env.unique_symbol();
-                for (symbol, layout, expr) in stores.iter().rev() {
+                stmt = store_pattern(
-                    stmt = Stmt::Let(*symbol, expr.clone(), layout.clone(), env.arena.alloc(stmt));
+                    env,
-                }
+                    procs,
+                    layout_cache,
+                    &mono_pattern,
+                    outer_symbol,
+                    layout,
+                    stmt,
+                )
+                .unwrap();
 
                 // convert the def body, store in outer_symbol
                 with_hole(
@@ -1897,6 +1899,7 @@ fn to_opt_branches<'a>(
     opt_branches
 }
 
+#[allow(clippy::too_many_arguments)]
 fn from_can_when<'a>(
     env: &mut Env<'a, '_>,
     cond_var: Variable,
@@ -1927,10 +1930,7 @@ fn from_can_when<'a>(
     let it = opt_branches
         .into_iter()
         .map(|(pattern, opt_guard, can_expr)| {
-            let mut stores = Vec::with_capacity_in(1, env.arena);
+            let branch_stmt = match join_point {
-            let res_stores =
-                store_pattern(env, &pattern, cond_symbol, cond_layout.clone(), &mut stores);
-            let mut stmt = match join_point {
                 None => from_can(env, can_expr, procs, layout_cache),
                 Some(id) => {
                     let symbol = env.unique_symbol();
@@ -1942,44 +1942,62 @@ fn from_can_when<'a>(
             };
 
             use crate::decision_tree::Guard;
-            match res_stores {
+            if let Some(loc_expr) = opt_guard {
-                Ok(_) => {
-                    for (symbol, layout, expr) in stores.iter().rev() {
-                        stmt =
-                            Stmt::Let(*symbol, expr.clone(), layout.clone(), env.arena.alloc(stmt));
-                    }
-
-                    let guard = if let Some(loc_expr) = opt_guard {
                 let id = JoinPointId(env.unique_symbol());
                 let symbol = env.unique_symbol();
                 let jump = env.arena.alloc(Stmt::Jump(id, env.arena.alloc([symbol])));
 
-                        let mut stmt =
+                let guard_stmt = with_hole(env, loc_expr.value, procs, layout_cache, symbol, jump);
-                            with_hole(env, loc_expr.value, procs, layout_cache, symbol, jump);
 
-                        // guard must have access to bound values
+                match store_pattern(
-                        for (symbol, layout, expr) in stores.into_iter().rev() {
+                    env,
-                            stmt = Stmt::Let(symbol, expr, layout, env.arena.alloc(stmt));
+                    procs,
-                        }
+                    layout_cache,
-
+                    &pattern,
-                        Guard::Guard { id, symbol, stmt }
+                    cond_symbol,
-                    } else {
+                    cond_layout.clone(),
-                        Guard::NoGuard
+                    guard_stmt,
-                    };
+                ) {
-
+                    Ok(new_guard_stmt) => (
-                    (pattern, guard, stmt)
+                        pattern,
-                }
+                        Guard::Guard {
+                            id,
+                            symbol,
+                            stmt: new_guard_stmt,
+                        },
+                        branch_stmt,
+                    ),
                     Err(msg) => (
                         Pattern::Underscore,
                         Guard::NoGuard,
                         Stmt::RuntimeError(env.arena.alloc(msg)),
                     ),
                 }
+            } else {
+                match store_pattern(
+                    env,
+                    procs,
+                    layout_cache,
+                    &pattern,
+                    cond_symbol,
+                    cond_layout.clone(),
+                    branch_stmt,
+                ) {
+                    Ok(new_branch_stmt) => (pattern, Guard::NoGuard, new_branch_stmt),
+                    Err(msg) => (
+                        Pattern::Underscore,
+                        Guard::NoGuard,
+                        Stmt::RuntimeError(env.arena.alloc(msg)),
+                    ),
+                }
+            }
         });
     let mono_branches = Vec::from_iter_in(it, arena);
 
     crate::decision_tree::optimize_when(
         env,
+        procs,
+        layout_cache,
         cond_symbol,
         cond_layout.clone(),
         ret_layout,
@@ -1987,25 +2005,31 @@ fn from_can_when<'a>(
     )
 }
 
+#[allow(clippy::too_many_arguments)]
 fn store_pattern<'a>(
     env: &mut Env<'a, '_>,
+    procs: &mut Procs<'a>,
+    layout_cache: &mut LayoutCache<'a>,
     can_pat: &Pattern<'a>,
     outer_symbol: Symbol,
     layout: Layout<'a>,
-    stored: &mut Vec<'a, (Symbol, Layout<'a>, Expr<'a>)>,
+    mut stmt: Stmt<'a>,
-) -> Result<(), String> {
+) -> Result<Stmt<'a>, &'a str> {
     use Pattern::*;
 
     match can_pat {
         Identifier(symbol) => {
-            // TODO surely something should happen here?
+            let expr = Expr::Alias(outer_symbol);
-            stored.push((*symbol, layout, Expr::Alias(outer_symbol)));
+            stmt = Stmt::Let(*symbol, expr, layout, env.arena.alloc(stmt));
         }
         Underscore => {
-            // Since _ is never read, it's safe to reassign it.
+            // do nothing
-            // stored.push((Symbol::UNDERSCORE, layout, Expr::Load(outer_symbol)))
         }
-        IntLiteral(_) | FloatLiteral(_) | EnumLiteral { .. } | BitLiteral { .. } => {}
+        IntLiteral(_)
+        | FloatLiteral(_)
+        | EnumLiteral { .. }
+        | BitLiteral { .. }
+        | StrLiteral(_) => {}
         AppliedTag {
             union, arguments, ..
         } => {
@@ -2022,7 +2046,7 @@ fn store_pattern<'a>(
                 arg_layouts.push(layout.clone());
             }
 
-            for (index, (argument, arg_layout)) in arguments.iter().enumerate() {
+            for (index, (argument, arg_layout)) in arguments.iter().enumerate().rev() {
                 let load = Expr::AccessAtIndex {
                     is_unwrapped,
                     index: (!is_unwrapped as usize + index) as u64,
@@ -2032,57 +2056,79 @@ fn store_pattern<'a>(
                 match argument {
                     Identifier(symbol) => {
                         // store immediately in the given symbol
-                        stored.push((*symbol, arg_layout.clone(), load));
+                        stmt = Stmt::Let(*symbol, load, arg_layout.clone(), env.arena.alloc(stmt));
                     }
                     Underscore => {
                         // ignore
                     }
-                    IntLiteral(_) | FloatLiteral(_) | EnumLiteral { .. } | BitLiteral { .. } => {}
+                    IntLiteral(_)
+                    | FloatLiteral(_)
+                    | EnumLiteral { .. }
+                    | BitLiteral { .. }
+                    | StrLiteral(_) => {}
                     _ => {
                         // store the field in a symbol, and continue matching on it
                         let symbol = env.unique_symbol();
-                        stored.push((symbol, arg_layout.clone(), load));
 
-                        store_pattern(env, argument, symbol, arg_layout.clone(), stored)?;
+                        // first recurse, continuing to unpack symbol
+                        stmt = store_pattern(
+                            env,
+                            procs,
+                            layout_cache,
+                            argument,
+                            symbol,
+                            arg_layout.clone(),
+                            stmt,
+                        )?;
+
+                        // then store the symbol
+                        stmt = Stmt::Let(symbol, load, arg_layout.clone(), env.arena.alloc(stmt));
                     }
                 }
             }
         }
         RecordDestructure(destructs, Layout::Struct(sorted_fields)) => {
-            for (index, destruct) in destructs.iter().enumerate() {
+            for (index, destruct) in destructs.iter().enumerate().rev() {
-                store_record_destruct(
+                stmt = store_record_destruct(
                     env,
+                    procs,
+                    layout_cache,
                     destruct,
                     index as u64,
                     outer_symbol,
                     sorted_fields,
-                    stored,
+                    stmt,
                 )?;
             }
         }
 
-        Shadowed(region, ident) => {
+        RecordDestructure(_, _) => {
-            return Err(format!(
+            unreachable!("a record destructure must always occur on a struct layout");
-                "The pattern at {:?} shadows variable {:?}",
-                region, ident
-            ));
         }
-        _ => {
+
-            panic!("TODO store_pattern for {:?}", can_pat);
+        Shadowed(_region, _ident) => {
+            return Err(&"TODO");
+        }
+
+        UnsupportedPattern(_region) => {
+            return Err(&"TODO");
         }
     }
 
-    Ok(())
+    Ok(stmt)
 }
 
+#[allow(clippy::too_many_arguments)]
 fn store_record_destruct<'a>(
     env: &mut Env<'a, '_>,
+    procs: &mut Procs<'a>,
+    layout_cache: &mut LayoutCache<'a>,
     destruct: &RecordDestruct<'a>,
     index: u64,
     outer_symbol: Symbol,
     sorted_fields: &'a [Layout<'a>],
-    stored: &mut Vec<'a, (Symbol, Layout<'a>, Expr<'a>)>,
+    mut stmt: Stmt<'a>,
-) -> Result<(), String> {
+) -> Result<Stmt<'a>, &'a str> {
     use Pattern::*;
 
     let load = Expr::AccessAtIndex {
@@ -2094,14 +2140,24 @@ fn store_record_destruct<'a>(
 
     match &destruct.typ {
         DestructType::Required => {
-            stored.push((destruct.symbol, destruct.layout.clone(), load));
+            stmt = Stmt::Let(
+                destruct.symbol,
+                load,
+                destruct.layout.clone(),
+                env.arena.alloc(stmt),
+            );
         }
         DestructType::Optional(_expr) => {
             todo!("TODO monomorphize optional field destructure's default expr");
         }
         DestructType::Guard(guard_pattern) => match &guard_pattern {
             Identifier(symbol) => {
-                stored.push((*symbol, destruct.layout.clone(), load));
+                stmt = Stmt::Let(
+                    *symbol,
+                    load,
+                    destruct.layout.clone(),
+                    env.arena.alloc(stmt),
+                );
             }
             Underscore => {
                 // important that this is special-cased to do nothing: mono record patterns will extract all the
@@ -2116,19 +2172,34 @@ fn store_record_destruct<'a>(
                 //
                 // internally. But `y` is never used, so we must make sure it't not stored/loaded.
             }
-            IntLiteral(_) | FloatLiteral(_) | EnumLiteral { .. } | BitLiteral { .. } => {}
+            IntLiteral(_)
+            | FloatLiteral(_)
+            | EnumLiteral { .. }
+            | BitLiteral { .. }
+            | StrLiteral(_) => {}
+
             _ => {
                 let symbol = env.unique_symbol();
-                stored.push((symbol, destruct.layout.clone(), load));
 
-                store_pattern(env, guard_pattern, symbol, destruct.layout.clone(), stored)?;
+                stmt = store_pattern(
+                    env,
+                    procs,
+                    layout_cache,
+                    guard_pattern,
+                    symbol,
+                    destruct.layout.clone(),
+                    stmt,
+                )?;
+
+                stmt = Stmt::Let(symbol, load, destruct.layout.clone(), env.arena.alloc(stmt));
             }
         },
     }
 
-    Ok(())
+    Ok(stmt)
 }
 
+#[allow(clippy::too_many_arguments)]
 fn call_by_name<'a>(
     env: &mut Env<'a, '_>,
     procs: &mut Procs<'a>,

compiler/mono/tests/test_mono.rs

@@ -534,8 +534,8 @@ mod test_mono {
             indoc!(
                 r#"
                 procedure Num.14 (#Attr.2, #Attr.3):
-                    let Test.8 = lowlevel NumAdd #Attr.2 #Attr.3;
+                    let Test.7 = lowlevel NumAdd #Attr.2 #Attr.3;
-                    ret Test.8;
+                    ret Test.7;
 
                 let Test.19 = 0i64;
                 let Test.21 = 0i64;
@@ -552,11 +552,11 @@ mod test_mono {
                 let Test.17 = lowlevel Eq Test.14 Test.15;
                 let Test.10 = lowlevel And Test.17 Test.16;
                 if Test.10 then
-                    let Test.5 = Index 1 Test.1;
+                    let Test.8 = Index 1 Test.1;
-                    let Test.2 = Index 1 Test.5;
+                    let Test.2 = Index 1 Test.8;
-                    let Test.7 = 1i64;
+                    let Test.6 = 1i64;
-                    let Test.6 = CallByName Num.14 Test.2 Test.7;
+                    let Test.5 = CallByName Num.14 Test.2 Test.6;
-                    jump Test.4 Test.6;
+                    jump Test.4 Test.5;
                 else
                     let Test.9 = 1i64;
                     jump Test.4 Test.9;
@@ -760,11 +760,48 @@ mod test_mono {
                 r#"
                 let Test.4 = 2i64;
                 let Test.5 = 3.14f64;
-                let Test.2 = Struct {Test.4, Test.5};
+                let Test.3 = Struct {Test.4, Test.5};
-                let Test.0 = Index 0 Test.2;
+                let Test.0 = Index 0 Test.3;
                 ret Test.0;
                 "#
             ),
         )
     }
+
+    #[test]
+    fn if_guard_bind_variable_false() {
+        compiles_to_ir(
+            indoc!(
+                r#"
+                when 10 is
+                    x if x == 5 -> 0
+                    _ -> 42
+                "#
+            ),
+            indoc!(
+                r#"
+                procedure Bool.5 (#Attr.2, #Attr.3):
+                    let Test.8 = lowlevel Eq #Attr.2 #Attr.3;
+                    ret Test.8;
+
+                let Test.2 = 10i64;
+                let Test.11 = true;
+                let Test.0 = alias Test.2;
+                let Test.7 = 5i64;
+                let Test.6 = CallByName Bool.5 Test.0 Test.7;
+                jump Test.5 Test.6;
+                joinpoint Test.5 Test.12:
+                    let Test.10 = lowlevel And Test.12 Test.11;
+                    if Test.10 then
+                        let Test.4 = 0i64;
+                        jump Test.3 Test.4;
+                    else
+                        let Test.9 = 42i64;
+                        jump Test.3 Test.9;
+                joinpoint Test.3 Test.1:
+                    ret Test.1;
+                "#
+            ),
+        )
+    }
 }