diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index 0fe7cf0a9a..59f92c66a3 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -510,9 +510,9 @@ pub fn types() -> MutMap { ), ); - // push : List elem, elem -> List elem + // append : List elem, elem -> List elem add_type( - Symbol::LIST_PUSH, + Symbol::LIST_APPEND, SolvedType::Func( vec![list_type(flex(TVAR1)), flex(TVAR1)], Box::new(list_type(flex(TVAR1))), @@ -552,15 +552,6 @@ pub fn types() -> MutMap { ), ); - // append : List elem, List elem -> List elem - add_type( - Symbol::LIST_APPEND, - SolvedType::Func( - vec![list_type(flex(TVAR1)), list_type(flex(TVAR1))], - Box::new(list_type(flex(TVAR1))), - ), - ); - // len : List * -> Int add_type( Symbol::LIST_LEN, diff --git a/compiler/builtins/src/unique.rs b/compiler/builtins/src/unique.rs index e2e0b514e0..37b678ed3b 100644 --- a/compiler/builtins/src/unique.rs +++ b/compiler/builtins/src/unique.rs @@ -638,8 +638,8 @@ pub fn types() -> MutMap { ) }); - // append : Attr * (List (Attr * a)), Attr * (List (Attr * a)) -> Attr * (List (Attr * a)) - add_type(Symbol::LIST_APPEND, { + // concat : Attr * (List (Attr * a)), Attr * (List (Attr * a)) -> Attr * (List (Attr * a)) + add_type(Symbol::LIST_CONCAT, { let_tvars! { a, star1, star2, star3 }; unique_function( @@ -669,13 +669,13 @@ pub fn types() -> MutMap { ) }); - // push : Attr * (List a) + // append : Attr * (List a) // , a // -> Attr * (List a) // // NOTE: we demand the new item to have the same uniqueness as the other list items. // It could be allowed to add unique items to shared lists, but that requires special code gen - add_type(Symbol::LIST_PUSH, { + add_type(Symbol::LIST_APPEND, { let_tvars! { a, star1, star2 }; unique_function( diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index 4646dd70bb..2c111c8411 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -53,13 +53,13 @@ pub fn builtin_defs(var_store: &mut VarStore) -> MutMap { Symbol::LIST_LEN => list_len, Symbol::LIST_GET => list_get, Symbol::LIST_SET => list_set, - Symbol::LIST_PUSH => list_push, + Symbol::LIST_APPEND => list_append, Symbol::LIST_FIRST => list_first, Symbol::LIST_IS_EMPTY => list_is_empty, Symbol::LIST_SINGLE => list_single, Symbol::LIST_REPEAT => list_repeat, Symbol::LIST_REVERSE => list_reverse, - Symbol::LIST_APPEND => list_append, + Symbol::LIST_CONCAT => list_concat, Symbol::LIST_PREPEND => list_prepend, Symbol::NUM_ADD => num_add, Symbol::NUM_SUB => num_sub, @@ -618,12 +618,12 @@ fn list_reverse(symbol: Symbol, var_store: &mut VarStore) -> Def { ) } -/// List.append : List elem, List elem -> List elem -fn list_append(symbol: Symbol, var_store: &mut VarStore) -> Def { +/// List.concat : List elem, List elem -> List elem +fn list_concat(symbol: Symbol, var_store: &mut VarStore) -> Def { let list_var = var_store.fresh(); let body = RunLowLevel { - op: LowLevel::ListAppend, + op: LowLevel::ListConcat, args: vec![ (list_var, Var(Symbol::ARG_1)), (list_var, Var(Symbol::ARG_2)), @@ -857,13 +857,13 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { ) } -/// List.push : List elem, elem -> List elem -fn list_push(symbol: Symbol, var_store: &mut VarStore) -> Def { +/// List.append : List elem, elem -> List elem +fn list_append(symbol: Symbol, var_store: &mut VarStore) -> Def { let list_var = var_store.fresh(); let elem_var = var_store.fresh(); let body = RunLowLevel { - op: LowLevel::ListPush, + op: LowLevel::ListAppend, args: vec![ (list_var, Var(Symbol::ARG_1)), (elem_var, Var(Symbol::ARG_2)), diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs index 92ee9e007b..342abbb396 100644 --- a/compiler/gen/src/llvm/build.rs +++ b/compiler/gen/src/llvm/build.rs @@ -1905,8 +1905,8 @@ fn run_low_level<'a, 'ctx, 'env>( } } } - ListAppend => list_append(env, layout_ids, scope, parent, args), - ListPush => { + ListConcat => list_concat(env, layout_ids, scope, parent, args), + ListAppend => { // List.push List elem, elem -> List elem debug_assert_eq!(args.len(), 2); @@ -2163,14 +2163,14 @@ fn build_int_binop<'a, 'ctx, 'env>( } } -fn list_append<'a, 'ctx, 'env>( +fn list_concat<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, layout_ids: &mut LayoutIds<'a>, scope: &Scope<'a, 'ctx>, parent: FunctionValue<'ctx>, args: &[(Expr<'a>, Layout<'a>)], ) -> BasicValueEnum<'ctx> { - // List.append : List elem, List elem -> List elem + // List.concat : List elem, List elem -> List elem debug_assert_eq!(args.len(), 2); // This implementation is quite long, let me explain what is complicating it. Here are our @@ -2259,7 +2259,7 @@ fn list_append<'a, 'ctx, 'env>( } _ => { unreachable!( - "Invalid List layout for second input list of List.append: {:?}", + "Invalid List layout for second input list of List.concat: {:?}", second_list_layout ); } @@ -2336,7 +2336,7 @@ fn list_append<'a, 'ctx, 'env>( // FIRST LOOP { let first_loop_bb = - ctx.append_basic_block(parent, "first_list_append_loop"); + ctx.append_basic_block(parent, "first_list_concat_loop"); builder.build_unconditional_branch(first_loop_bb); builder.position_at_end(first_loop_bb); @@ -2407,7 +2407,7 @@ fn list_append<'a, 'ctx, 'env>( // SECOND LOOP { let second_loop_bb = - ctx.append_basic_block(parent, "second_list_append_loop"); + ctx.append_basic_block(parent, "second_list_concat_loop"); builder.build_unconditional_branch(second_loop_bb); builder.position_at_end(second_loop_bb); @@ -2533,7 +2533,7 @@ fn list_append<'a, 'ctx, 'env>( } _ => { unreachable!( - "Invalid List layout for second input list of List.append: {:?}", + "Invalid List layout for second input list of List.concat: {:?}", second_list_layout ); } @@ -2578,7 +2578,7 @@ fn list_append<'a, 'ctx, 'env>( } _ => { unreachable!( - "Invalid List layout for second input list of List.append: {:?}", + "Invalid List layout for second input list of List.concat: {:?}", second_list_layout ); } @@ -2596,7 +2596,7 @@ fn list_append<'a, 'ctx, 'env>( } _ => { unreachable!( - "Invalid List layout for first list in List.append : {:?}", + "Invalid List layout for first list in List.concat : {:?}", first_list_layout ); } diff --git a/compiler/gen/tests/gen_list.rs b/compiler/gen/tests/gen_list.rs index 4b3d0e28fd..bcd18eef8c 100644 --- a/compiler/gen/tests/gen_list.rs +++ b/compiler/gen/tests/gen_list.rs @@ -38,10 +38,10 @@ mod gen_list { } #[test] - fn list_push() { - assert_evals_to!("List.push [1] 2", &[1, 2], &'static [i64]); - assert_evals_to!("List.push [1, 1] 2", &[1, 1, 2], &'static [i64]); - assert_evals_to!("List.push [] 3", &[3], &'static [i64]); + fn list_append() { + assert_evals_to!("List.append [1] 2", &[1, 2], &'static [i64]); + assert_evals_to!("List.append [1, 1] 2", &[1, 1, 2], &'static [i64]); + assert_evals_to!("List.append [] 3", &[3], &'static [i64]); assert_evals_to!( indoc!( r#" @@ -49,19 +49,19 @@ mod gen_list { initThrees = [] - List.push (List.push initThrees 3) 3 + List.append (List.append initThrees 3) 3 "# ), &[3, 3], &'static [i64] ); assert_evals_to!( - "List.push [ True, False ] True", + "List.append [ True, False ] True", &[true, false, true], &'static [bool] ); assert_evals_to!( - "List.push [ 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22 ] 23", + "List.append [ 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22 ] 23", &[11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23], &'static [i64] ); @@ -157,8 +157,8 @@ mod gen_list { } #[test] - fn list_append() { - assert_evals_to!("List.append [] []", &[], &'static [i64]); + fn list_concat() { + assert_evals_to!("List.concat [] []", &[], &'static [i64]); assert_evals_to!( indoc!( @@ -171,30 +171,30 @@ mod gen_list { secondList = [] - List.append firstList secondList + List.concat firstList secondList "# ), &[], &'static [i64] ); - assert_evals_to!("List.append [ 12, 13 ] []", &[12, 13], &'static [i64]); + assert_evals_to!("List.concat [ 12, 13 ] []", &[12, 13], &'static [i64]); assert_evals_to!( - "List.append [ 34, 43 ] [ 64, 55, 66 ]", + "List.concat [ 34, 43 ] [ 64, 55, 66 ]", &[34, 43, 64, 55, 66], &'static [i64] ); - assert_evals_to!("List.append [] [ 23, 24 ]", &[23, 24], &'static [i64]); + assert_evals_to!("List.concat [] [ 23, 24 ]", &[23, 24], &'static [i64]); assert_evals_to!( - "List.append [ 1, 2 ] [ 3, 4 ]", + "List.concat [ 1, 2 ] [ 3, 4 ]", &[1, 2, 3, 4], &'static [i64] ); } - fn assert_append_worked(num_elems1: i64, num_elems2: i64) { + fn assert_concat_worked(num_elems1: i64, num_elems2: i64) { let vec1: Vec = (0..num_elems1) .map(|i| 12345 % (i + num_elems1 + num_elems2 + 1)) .collect(); @@ -210,51 +210,51 @@ mod gen_list { let expected_slice: &[i64] = expected.as_ref(); assert_evals_to!( - &format!("List.append {} {}", slice_str1, slice_str2), + &format!("List.concat {} {}", slice_str1, slice_str2), expected_slice, &'static [i64] ); } #[test] - fn list_append_empty_list() { - assert_append_worked(0, 0); - assert_append_worked(1, 0); - assert_append_worked(2, 0); - assert_append_worked(3, 0); - assert_append_worked(4, 0); - assert_append_worked(7, 0); - assert_append_worked(8, 0); - assert_append_worked(9, 0); - assert_append_worked(25, 0); - assert_append_worked(150, 0); - assert_append_worked(0, 1); - assert_append_worked(0, 2); - assert_append_worked(0, 3); - assert_append_worked(0, 4); - assert_append_worked(0, 7); - assert_append_worked(0, 8); - assert_append_worked(0, 9); - assert_append_worked(0, 25); - assert_append_worked(0, 150); + fn list_concat_empty_list() { + assert_concat_worked(0, 0); + assert_concat_worked(1, 0); + assert_concat_worked(2, 0); + assert_concat_worked(3, 0); + assert_concat_worked(4, 0); + assert_concat_worked(7, 0); + assert_concat_worked(8, 0); + assert_concat_worked(9, 0); + assert_concat_worked(25, 0); + assert_concat_worked(150, 0); + assert_concat_worked(0, 1); + assert_concat_worked(0, 2); + assert_concat_worked(0, 3); + assert_concat_worked(0, 4); + assert_concat_worked(0, 7); + assert_concat_worked(0, 8); + assert_concat_worked(0, 9); + assert_concat_worked(0, 25); + assert_concat_worked(0, 150); } #[test] - fn list_append_nonempty_lists() { - assert_append_worked(1, 1); - assert_append_worked(1, 2); - assert_append_worked(1, 3); - assert_append_worked(2, 3); - assert_append_worked(2, 1); - assert_append_worked(2, 2); - assert_append_worked(3, 1); - assert_append_worked(3, 2); - assert_append_worked(2, 3); - assert_append_worked(3, 3); - assert_append_worked(4, 4); - assert_append_worked(150, 150); - assert_append_worked(129, 350); - assert_append_worked(350, 129); + fn list_concat_nonempty_lists() { + assert_concat_worked(1, 1); + assert_concat_worked(1, 2); + assert_concat_worked(1, 3); + assert_concat_worked(2, 3); + assert_concat_worked(2, 1); + assert_concat_worked(2, 2); + assert_concat_worked(3, 1); + assert_concat_worked(3, 2); + assert_concat_worked(2, 3); + assert_concat_worked(3, 3); + assert_concat_worked(4, 4); + assert_concat_worked(150, 150); + assert_concat_worked(129, 350); + assert_concat_worked(350, 129); } #[test] diff --git a/compiler/load/tests/fixtures/build/app_with_deps/AStar.roc b/compiler/load/tests/fixtures/build/app_with_deps/AStar.roc index 0901f80fe9..30c45eacf3 100644 --- a/compiler/load/tests/fixtures/build/app_with_deps/AStar.roc +++ b/compiler/load/tests/fixtures/build/app_with_deps/AStar.roc @@ -54,7 +54,7 @@ reconstructPath = \cameFrom, goal -> [] Ok next -> - List.push (reconstructPath cameFrom next) goal + List.append (reconstructPath cameFrom next) goal updateCost : position, position, Model position -> Model position updateCost = \current, neighbour, model -> diff --git a/compiler/load/tests/fixtures/build/interface_with_deps/AStar.roc b/compiler/load/tests/fixtures/build/interface_with_deps/AStar.roc index 0901f80fe9..30c45eacf3 100644 --- a/compiler/load/tests/fixtures/build/interface_with_deps/AStar.roc +++ b/compiler/load/tests/fixtures/build/interface_with_deps/AStar.roc @@ -54,7 +54,7 @@ reconstructPath = \cameFrom, goal -> [] Ok next -> - List.push (reconstructPath cameFrom next) goal + List.append (reconstructPath cameFrom next) goal updateCost : position, position, Model position -> Model position updateCost = \current, neighbour, model -> diff --git a/compiler/module/src/low_level.rs b/compiler/module/src/low_level.rs index 261a871e10..ceffef2430 100644 --- a/compiler/module/src/low_level.rs +++ b/compiler/module/src/low_level.rs @@ -10,9 +10,9 @@ pub enum LowLevel { ListSingle, ListRepeat, ListReverse, + ListConcat, ListAppend, ListPrepend, - ListPush, NumAdd, NumSub, NumMul, diff --git a/compiler/module/src/symbol.rs b/compiler/module/src/symbol.rs index 90c68cf241..d74c522c01 100644 --- a/compiler/module/src/symbol.rs +++ b/compiler/module/src/symbol.rs @@ -648,7 +648,7 @@ define_builtins! { 2 LIST_IS_EMPTY: "isEmpty" 3 LIST_GET: "get" 4 LIST_SET: "set" - 5 LIST_PUSH: "push" + 5 LIST_APPEND: "append" 6 LIST_MAP: "map" 7 LIST_LEN: "len" 8 LIST_FOLDL: "foldl" @@ -658,8 +658,7 @@ define_builtins! { 12 LIST_SINGLE: "single" 13 LIST_REPEAT: "repeat" 14 LIST_REVERSE: "reverse" - 15 LIST_APPEND: "append" - 16 LIST_PREPEND: "prepend" + 15 LIST_PREPEND: "prepend" } 5 RESULT: "Result" => { 0 RESULT_RESULT: "Result" imported // the Result.Result type alias diff --git a/compiler/mono/src/decision_tree.rs b/compiler/mono/src/decision_tree.rs index ad3e3fef73..c423118104 100644 --- a/compiler/mono/src/decision_tree.rs +++ b/compiler/mono/src/decision_tree.rs @@ -147,9 +147,8 @@ fn to_decision_tree(raw_branches: Vec) -> DecisionTree { match check_for_match(&branches) { Some(goal) => DecisionTree::Match(goal), None => { - // TODO remove clone - let path = pick_path(branches.clone()); - + // must clone here to release the borrow on `branches` + let path = pick_path(&branches).clone(); let (edges, fallback) = gather_edges(branches, &path); let mut decision_edges: Vec<_> = edges @@ -211,43 +210,47 @@ fn flatten<'a>( path_pattern: (Path, Guard<'a>, Pattern<'a>), path_patterns: &mut Vec<(Path, Guard<'a>, Pattern<'a>)>, ) { - match &path_pattern.2 { + match path_pattern.2 { Pattern::AppliedTag { union, arguments, tag_id, - .. - } => { - // TODO do we need to check that guard.is_none() here? - if union.alternatives.len() == 1 { - let path = path_pattern.0; - // Theory: unbox doesn't have any value for us, because one-element tag unions - // don't store the tag anyway. - if arguments.len() == 1 { - path_patterns.push(( - Path::Unbox(Box::new(path)), - path_pattern.1.clone(), - path_pattern.2.clone(), - )); - } else { - for (index, (arg_pattern, _)) in arguments.iter().enumerate() { - flatten( - ( - Path::Index { - index: index as u64, - tag_id: *tag_id, - path: Box::new(path.clone()), - }, - // same guard here? - path_pattern.1.clone(), - arg_pattern.clone(), - ), - path_patterns, - ); - } - } + tag_name, + layout, + } if union.alternatives.len() == 1 => { + // TODO ^ do we need to check that guard.is_none() here? + + let path = path_pattern.0; + // Theory: unbox doesn't have any value for us, because one-element tag unions + // don't store the tag anyway. + if arguments.len() == 1 { + path_patterns.push(( + Path::Unbox(Box::new(path)), + path_pattern.1.clone(), + Pattern::AppliedTag { + union, + arguments, + tag_id, + tag_name, + layout, + }, + )); } else { - path_patterns.push(path_pattern); + for (index, (arg_pattern, _)) in arguments.iter().enumerate() { + flatten( + ( + Path::Index { + index: index as u64, + tag_id, + path: Box::new(path.clone()), + }, + // same guard here? + path_pattern.1.clone(), + arg_pattern.clone(), + ), + path_patterns, + ); + } } } @@ -282,8 +285,7 @@ fn gather_edges<'a>( branches: Vec>, path: &Path, ) -> (Vec<(Test<'a>, Vec>)>, Vec>) { - // TODO remove clone - let relevant_tests = tests_at_path(path, branches.clone()); + let relevant_tests = tests_at_path(path, &branches); let check = is_complete(&relevant_tests); @@ -307,12 +309,12 @@ fn gather_edges<'a>( /// FIND RELEVANT TESTS -fn tests_at_path<'a>(selected_path: &Path, branches: Vec>) -> Vec> { +fn tests_at_path<'a>(selected_path: &Path, branches: &[Branch<'a>]) -> Vec> { // NOTE the ordering of the result is important! let mut all_tests = Vec::new(); - for branch in branches.into_iter() { + for branch in branches { test_at_path(selected_path, branch, &mut all_tests); } @@ -341,7 +343,7 @@ fn tests_at_path<'a>(selected_path: &Path, branches: Vec>) -> Vec(selected_path: &Path, branch: Branch<'a>, all_tests: &mut Vec>) { +fn test_at_path<'a>(selected_path: &Path, branch: &Branch<'a>, all_tests: &mut Vec>) { use Pattern::*; use Test::*; @@ -457,7 +459,7 @@ fn edges_for<'a>( ) -> (Test<'a>, Vec>) { let mut new_branches = Vec::new(); - for branch in branches.into_iter() { + for branch in branches.iter() { to_relevant_branch(&test, path, branch, &mut new_branches); } @@ -467,13 +469,13 @@ fn edges_for<'a>( fn to_relevant_branch<'a>( test: &Test<'a>, path: &Path, - branch: Branch<'a>, + branch: &Branch<'a>, new_branches: &mut Vec>, ) { // TODO remove clone match extract(path, branch.patterns.clone()) { Extract::NotFound => { - new_branches.push(branch); + new_branches.push(branch.clone()); } Extract::Found { start, @@ -509,7 +511,7 @@ fn to_relevant_branch_help<'a>( path: &Path, mut start: Vec<(Path, Guard<'a>, Pattern<'a>)>, end: Vec<(Path, Guard<'a>, Pattern<'a>)>, - branch: Branch<'a>, + branch: &Branch<'a>, guard: Guard<'a>, pattern: Pattern<'a>, ) -> Option> { @@ -517,7 +519,7 @@ fn to_relevant_branch_help<'a>( use Test::*; match pattern { - Identifier(_) | Underscore | Shadowed(_, _) | UnsupportedPattern(_) => Some(branch), + Identifier(_) | Underscore | Shadowed(_, _) | UnsupportedPattern(_) => Some(branch.clone()), RecordDestructure(destructs, _) => match test { IsCtor { @@ -678,19 +680,14 @@ fn extract<'a>( ) -> Extract<'a> { let mut start = Vec::new(); - // TODO remove this clone - let mut copy = path_patterns.clone(); - // TODO potential ordering problem - for (index, current) in path_patterns.into_iter().enumerate() { + let mut it = path_patterns.into_iter(); + while let Some(current) = it.next() { if ¤t.0 == selected_path { return Extract::Found { start, found_pattern: (current.1, current.2), - end: { - copy.drain(0..=index); - copy - }, + end: it.collect::>(), }; } else { start.push(current); @@ -731,22 +728,27 @@ fn needs_tests<'a>(pattern: &Pattern<'a>) -> bool { /// PICK A PATH -fn pick_path(branches: Vec) -> Path { - // TODO remove this clone - let all_paths = branches - .clone() - .into_iter() - .map(|v| v.patterns) - .flatten() - .filter_map(is_choice_path); +fn pick_path<'a>(branches: &'a [Branch]) -> &'a Path { + let mut all_paths = Vec::with_capacity(branches.len()); - let mut by_small_defaults = bests_by_small_defaults(&branches, all_paths); + // is choice path + for branch in branches { + for (path, guard, pattern) in &branch.patterns { + if !guard.is_none() || needs_tests(&pattern) { + all_paths.push(path); + } else { + // do nothing + } + } + } + + let mut by_small_defaults = bests_by_small_defaults(branches, all_paths.into_iter()); if by_small_defaults.len() == 1 { by_small_defaults.remove(0) } else { debug_assert!(!by_small_defaults.is_empty()); - let mut result = bests_by_small_branching_factor(&branches, by_small_defaults.into_iter()); + let mut result = bests_by_small_branching_factor(branches, by_small_defaults.into_iter()); match result.pop() { None => unreachable!("bests_by will always return at least one value in the vec"), @@ -755,33 +757,23 @@ fn pick_path(branches: Vec) -> Path { } } -fn is_choice_path<'a>(path_and_pattern: (Path, Guard<'a>, Pattern<'a>)) -> Option { - let (path, guard, pattern) = path_and_pattern; - - if !guard.is_none() || needs_tests(&pattern) { - Some(path) - } else { - None - } -} - -fn bests_by_small_branching_factor(branches: &Vec, mut all_paths: I) -> Vec +fn bests_by_small_branching_factor<'a, I>(branches: &[Branch], mut all_paths: I) -> Vec<&'a Path> where - I: Iterator, + I: Iterator, { match all_paths.next() { None => panic!("Cannot choose the best of zero paths. This should never happen."), Some(first_path) => { - let mut min_weight = small_branching_factor(branches, &first_path); + let mut min_weight = small_branching_factor(branches, first_path); let mut min_paths = vec![first_path]; for path in all_paths { - let weight = small_branching_factor(branches, &path); + let weight = small_branching_factor(branches, path); use std::cmp::Ordering; match weight.cmp(&min_weight) { Ordering::Equal => { - min_paths.push(path.clone()); + min_paths.push(path); } Ordering::Less => { min_weight = weight; @@ -797,14 +789,14 @@ where } } -fn bests_by_small_defaults(branches: &Vec, mut all_paths: I) -> Vec +fn bests_by_small_defaults<'a, I>(branches: &[Branch], mut all_paths: I) -> Vec<&'a Path> where - I: Iterator, + I: Iterator, { match all_paths.next() { None => panic!("Cannot choose the best of zero paths. This should never happen."), Some(first_path) => { - let mut min_weight = small_defaults(branches, &first_path); + let mut min_weight = small_defaults(branches, first_path); let mut min_paths = vec![first_path]; for path in all_paths { @@ -813,7 +805,7 @@ where use std::cmp::Ordering; match weight.cmp(&min_weight) { Ordering::Equal => { - min_paths.push(path.clone()); + min_paths.push(path); } Ordering::Less => { min_weight = weight; @@ -831,7 +823,7 @@ where /// PATH PICKING HEURISTICS -fn small_defaults(branches: &Vec, path: &Path) -> usize { +fn small_defaults(branches: &[Branch], path: &Path) -> usize { branches .iter() .filter(|b| is_irrelevant_to(path, b)) @@ -839,7 +831,7 @@ fn small_defaults(branches: &Vec, path: &Path) -> usize { .sum() } -fn small_branching_factor(branches: &Vec, path: &Path) -> usize { +fn small_branching_factor(branches: &[Branch], path: &Path) -> usize { // TODO remove clone let (edges, fallback) = gather_edges(branches.to_vec(), path); diff --git a/compiler/mono/src/pattern.rs b/compiler/mono/src/pattern.rs index 4a78de5260..bb1cbddc81 100644 --- a/compiler/mono/src/pattern.rs +++ b/compiler/mono/src/pattern.rs @@ -53,7 +53,6 @@ fn simplify<'a>(pattern: &crate::expr::Pattern<'a>) -> Pattern { StrLiteral(v) => Literal(Literal::Str(v.clone())), // To make sure these are exhaustive, we have to "fake" a union here - // TODO: use the hash or some other integer to discriminate between constructors BitLiteral { value, union, .. } => Ctor(union.clone(), TagId(*value as u8), vec![]), EnumLiteral { tag_id, union, .. } => Ctor(union.clone(), TagId(*tag_id), vec![]), @@ -217,7 +216,7 @@ fn is_exhaustive(matrix: &PatternMatrix, n: usize) -> PatternMatrix { let last: _ = alt_list .iter() - .filter_map(|r| is_missing(alts.clone(), ctors.clone(), r)); + .filter_map(|r| is_missing(alts.clone(), &ctors, r)); let mut result = Vec::new(); @@ -257,7 +256,7 @@ fn is_exhaustive(matrix: &PatternMatrix, n: usize) -> PatternMatrix { } } -fn is_missing(union: Union, ctors: MutMap, ctor: &Ctor) -> Option { +fn is_missing(union: Union, ctors: &MutMap, ctor: &Ctor) -> Option { let Ctor { arity, tag_id, .. } = ctor; if ctors.contains_key(tag_id) { @@ -336,7 +335,7 @@ fn to_nonredundant_rows<'a>( vec![simplify(&loc_pat.value)] }; - if is_useful(&checked_rows, &next_row) { + if is_useful(checked_rows.clone(), next_row.clone()) { checked_rows.push(next_row); } else { return Err(Error::Redundant { @@ -351,83 +350,142 @@ fn to_nonredundant_rows<'a>( } /// Check if a new row "vector" is useful given previous rows "matrix" -fn is_useful(matrix: &PatternMatrix, vector: &Row) -> bool { - if matrix.is_empty() { - // No rows are the same as the new vector! The vector is useful! - true - } else if vector.is_empty() { - // There is nothing left in the new vector, but we still have - // rows that match the same things. This is not a useful vector! - false - } else { - // NOTE: if there are bugs in this code, look at the ordering of the row/matrix - let mut vector = vector.clone(); - let first_pattern = vector.remove(0); - let patterns = vector; +fn is_useful(mut old_matrix: PatternMatrix, mut vector: Row) -> bool { + let mut matrix = Vec::with_capacity(old_matrix.len()); - match first_pattern { - // keep checking rows that start with this Ctor or Anything - Ctor(_, id, args) => { - let new_matrix: Vec<_> = matrix - .iter() - .filter_map(|r| specialize_row_by_ctor(id, args.len(), r)) - .collect(); - - let mut new_row = Vec::new(); - new_row.extend(patterns); - new_row.extend(args); - - is_useful(&new_matrix, &new_row) + // this loop ping-pongs the rows between old_matrix and matrix + 'outer: loop { + match vector.pop() { + _ if old_matrix.is_empty() => { + // No rows are the same as the new vector! The vector is useful! + break true; } + None => { + // There is nothing left in the new vector, but we still have + // rows that match the same things. This is not a useful vector! + break false; + } + Some(first_pattern) => { + // NOTE: if there are bugs in this code, look at the ordering of the row/matrix - Anything => { - // check if all alts appear in matrix - match is_complete(matrix) { - Complete::No => { - // This Anything is useful because some Ctors are missing. - // But what if a previous row has an Anything? - // If so, this one is not useful. - let new_matrix: Vec<_> = matrix - .iter() - .filter_map(|r| specialize_row_by_anything(r)) - .collect(); + match first_pattern { + // keep checking rows that start with this Ctor or Anything + Ctor(_, id, args) => { + specialize_row_by_ctor2(id, args.len(), &mut old_matrix, &mut matrix); - is_useful(&new_matrix, &patterns) + std::mem::swap(&mut old_matrix, &mut matrix); + + vector.extend(args); } - Complete::Yes(alts) => { - // All Ctors are covered, so this Anything is not needed for any - // of those. But what if some of those Ctors have subpatterns - // that make them less general? If so, this actually is useful! - let is_useful_alt = |Ctor { arity, tag_id, .. }| { - let new_matrix = matrix - .iter() - .filter_map(|r| specialize_row_by_ctor(tag_id, arity, r)) - .collect(); - let mut new_row: Vec = - std::iter::repeat(Anything).take(arity).collect::>(); - new_row.extend(patterns.clone()); + Anything => { + // check if all alternatives appear in matrix + match is_complete(&old_matrix) { + Complete::No => { + // This Anything is useful because some Ctors are missing. + // But what if a previous row has an Anything? + // If so, this one is not useful. + for mut row in old_matrix.drain(..) { + if let Some(Anything) = row.pop() { + matrix.push(row); + } + } - is_useful(&new_matrix, &new_row) - }; + std::mem::swap(&mut old_matrix, &mut matrix); + } + Complete::Yes(alternatives) => { + // All Ctors are covered, so this Anything is not needed for any + // of those. But what if some of those Ctors have subpatterns + // that make them less general? If so, this actually is useful! + for alternative in alternatives { + let Ctor { arity, tag_id, .. } = alternative; - alts.iter().cloned().any(is_useful_alt) + let mut old_matrix = old_matrix.clone(); + let mut matrix = vec![]; + specialize_row_by_ctor2( + tag_id, + arity, + &mut old_matrix, + &mut matrix, + ); + + let mut vector = vector.clone(); + vector.extend(std::iter::repeat(Anything).take(arity)); + + if is_useful(matrix, vector) { + break 'outer true; + } + } + + break false; + } + } + } + + Literal(literal) => { + // keep checking rows that start with this Literal or Anything + + for mut row in old_matrix.drain(..) { + let head = row.pop(); + let patterns = row; + + match head { + Some(Literal(lit)) => { + if lit == literal { + matrix.push(patterns); + } else { + // do nothing + } + } + Some(Anything) => matrix.push(patterns), + + Some(Ctor(_, _, _)) => panic!( + r#"Compiler bug! After type checking, constructors and literals should never align in pattern match exhaustiveness checks."# + ), + + None => panic!( + "Compiler error! Empty matrices should not get specialized." + ), + } + } + std::mem::swap(&mut old_matrix, &mut matrix); } } } - - Literal(literal) => { - // keep checking rows that start with this Literal or Anything - let new_matrix = matrix - .iter() - .filter_map(|r| specialize_row_by_literal(&literal, r)) - .collect(); - is_useful(&new_matrix, &patterns) - } } } } +/// INVARIANT: (length row == N) ==> (length result == arity + N - 1) +fn specialize_row_by_ctor2( + tag_id: TagId, + arity: usize, + old_matrix: &mut PatternMatrix, + matrix: &mut PatternMatrix, +) { + for mut row in old_matrix.drain(..) { + let head = row.pop(); + let mut patterns = row; + + match head { + Some(Ctor(_, id, args)) => + if id == tag_id { + patterns.extend(args); + matrix.push(patterns); + } else { + // do nothing + } + Some(Anything) => { + // TODO order! + patterns.extend(std::iter::repeat(Anything).take(arity)); + matrix.push(patterns); + } + Some(Literal(_)) => panic!( "Compiler bug! After type checking, constructors and literal should never align in pattern match exhaustiveness checks."), + None => panic!("Compiler error! Empty matrices should not get specialized."), + } + } +} + /// INVARIANT: (length row == N) ==> (length result == arity + N - 1) fn specialize_row_by_ctor(tag_id: TagId, arity: usize, row: &Row) -> Option { let mut row = row.clone(); @@ -436,7 +494,7 @@ fn specialize_row_by_ctor(tag_id: TagId, arity: usize, row: &Row) -> Option let patterns = row; match head { - Some(Ctor(_, id, args)) => + Some(Ctor(_, id, args)) => { if id == tag_id { // TODO order! let mut new_patterns = Vec::new(); @@ -446,38 +504,18 @@ fn specialize_row_by_ctor(tag_id: TagId, arity: usize, row: &Row) -> Option } else { None } + } Some(Anything) => { // TODO order! - let new_patterns = - std::iter::repeat(Anything).take(arity).chain(patterns).collect(); + let new_patterns = std::iter::repeat(Anything) + .take(arity) + .chain(patterns) + .collect(); Some(new_patterns) - } - Some(Literal(_)) => panic!( "Compiler bug! After type checking, constructors and literal should never align in pattern match exhaustiveness checks."), - None => panic!("Compiler error! Empty matrices should not get specialized."), - } -} - -/// INVARIANT: (length row == N) ==> (length result == N-1) -fn specialize_row_by_literal(literal: &Literal, row: &Row) -> Option { - let mut row = row.clone(); - - let head = row.pop(); - let patterns = row; - - match head { - Some(Literal(lit)) => { - if &lit == literal { - Some(patterns) - } else { - None - } } - Some(Anything) => Some(patterns), - - Some(Ctor(_, _, _)) => panic!( - r#"Compiler bug! After type checking, constructors and literals should never align in pattern match exhaustiveness checks."# + Some(Literal(_)) => unreachable!( + r#"Compiler bug! After type checking, a constructor can never align with a literal: that should be a type error!"# ), - None => panic!("Compiler error! Empty matrices should not get specialized."), } } @@ -501,14 +539,14 @@ pub enum Complete { fn is_complete(matrix: &PatternMatrix) -> Complete { let ctors = collect_ctors(matrix); - - let mut it = ctors.values(); + let length = ctors.len(); + let mut it = ctors.into_iter(); match it.next() { None => Complete::No, - Some(Union { alternatives, .. }) => { - if ctors.len() == alternatives.len() { - Complete::Yes(alternatives.to_vec()) + Some((_, Union { alternatives, .. })) => { + if length == alternatives.len() { + Complete::Yes(alternatives) } else { Complete::No } diff --git a/compiler/reporting/tests/test_reporting.rs b/compiler/reporting/tests/test_reporting.rs index 5a9ab9ee97..a0b0ffdf21 100644 --- a/compiler/reporting/tests/test_reporting.rs +++ b/compiler/reporting/tests/test_reporting.rs @@ -3771,4 +3771,48 @@ mod test_reporting { ), ) } + + #[test] + fn first_wildcard_is_required() { + report_problem_as( + indoc!( + r#" + when Foo 1 2 3 is + Foo _ 1 _ -> 1 + _ -> 2 + "# + ), + "", + ) + } + + #[test] + fn second_wildcard_is_redundant() { + report_problem_as( + indoc!( + r#" + when Foo 1 2 3 is + Foo _ 1 _ -> 1 + _ -> 2 + _ -> 3 + "# + ), + indoc!( + r#" + -- REDUNDANT PATTERN ----------------------------------------------------------- + + The 3rd pattern is redundant: + + 1 ┆ when Foo 1 2 3 is + 2 ┆ Foo _ 1 _ -> 1 + 3 ┆ _ -> 2 + 4 ┆ _ -> 3 + ┆ ^ + + Any value of this shape will be handled by a previous pattern, so this + one should be removed. + "# + ), + ) + } } diff --git a/compiler/solve/tests/solve_expr.rs b/compiler/solve/tests/solve_expr.rs index bd8d09f3ce..c2864e99ec 100644 --- a/compiler/solve/tests/solve_expr.rs +++ b/compiler/solve/tests/solve_expr.rs @@ -2362,7 +2362,7 @@ mod solve_expr { [] Ok next -> - List.push (reconstructPath cameFrom next) goal + List.append (reconstructPath cameFrom next) goal reconstructPath "# @@ -2534,7 +2534,7 @@ mod solve_expr { x = [] when List.get input 0 is - Ok val -> List.push x val + Ok val -> List.append x val Err _ -> f input f "# diff --git a/compiler/solve/tests/solve_uniq_expr.rs b/compiler/solve/tests/solve_uniq_expr.rs index e5776d70b3..b5bbf75904 100644 --- a/compiler/solve/tests/solve_uniq_expr.rs +++ b/compiler/solve/tests/solve_uniq_expr.rs @@ -2275,9 +2275,9 @@ mod solve_uniq_expr { } #[test] - fn list_push() { + fn list_append() { infer_eq( - "List.push", + "List.append", "Attr * (Attr * (List a), a -> Attr * (List a))", ); } @@ -2303,7 +2303,7 @@ mod solve_uniq_expr { infer_eq( indoc!( r#" - singleton = \x -> List.push [] x + singleton = \x -> List.append [] x singleton "# @@ -2317,7 +2317,7 @@ mod solve_uniq_expr { infer_eq( indoc!( r#" - reverse = \list -> List.foldr list (\e, l -> List.push l e) [] + reverse = \list -> List.foldr list (\e, l -> List.append l e) [] reverse "# @@ -2742,7 +2742,7 @@ mod solve_uniq_expr { [] Ok next -> - List.push (reconstructPath cameFrom next) goal + List.append (reconstructPath cameFrom next) goal reconstructPath "# @@ -2812,7 +2812,7 @@ mod solve_uniq_expr { [] Ok next -> - List.push (reconstructPath cameFrom next) goal + List.append (reconstructPath cameFrom next) goal updateCost : position, position, Model position -> Model position updateCost = \current, neighbour, model -> @@ -2897,7 +2897,7 @@ mod solve_uniq_expr { [] Ok next -> - List.push (reconstructPath cameFrom next) goal + List.append (reconstructPath cameFrom next) goal updateCost : position, position, Model position -> Model position