From 7f5998b9e65ed1b96e846357daddf95dfcdc1e18 Mon Sep 17 00:00:00 2001 From: Folkert Date: Thu, 16 Jan 2020 21:48:19 +0100 Subject: [PATCH] Use DeclareRec for all recursive functions Even ones that are only self-recursive. e.g. LinkedList.map. --- src/can/def.rs | 65 ++++++++++++++++++++++++++++++++++---- tests/test_canonicalize.rs | 53 ++++++++++++++++++------------- 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/src/can/def.rs b/src/can/def.rs index bd659f6ac4..b3a01689ee 100644 --- a/src/can/def.rs +++ b/src/can/def.rs @@ -229,7 +229,7 @@ pub fn sort_can_defs( // recursive definitions. // All successors that occur in the body of a symbol. - let mut all_successors = |symbol: &Symbol| -> ImSet { + let all_successors_without_self = |symbol: &Symbol| -> ImSet { // This may not be in refs_by_symbol. For example, the `f` in `f x` here: // // f = \z -> z @@ -272,6 +272,49 @@ pub fn sort_can_defs( } }; + // All successors that occur in the body of a symbol, including the symbol itself + // This is required to determine whether a symbol is recursive. Recursive symbols + // (that are not faulty) always need a DeclareRec, even if there is just one symbol in the + // group + let mut all_successors_with_self = |symbol: &Symbol| -> ImSet { + // This may not be in refs_by_symbol. For example, the `f` in `f x` here: + // + // f = \z -> z + // + // (\x -> + // a = f x + // x + // ) + // + // It's not part of the current defs (the one with `a = f x`); rather, + // it's in the enclosing scope. It's still referenced though, so successors + // will receive it as an argument! + match refs_by_symbol.get(symbol) { + Some((_, references)) => { + // We can only sort the symbols at the current level. That is safe because + // symbols defined at higher levels cannot refer to symbols at lower levels. + // Therefore they can never form a cycle! + // + // In the above example, `f` cannot reference `a`, and in the closure + // a call to `f` cannot cycle back to `a`. + let mut loc_succ = local_successors(&references, &env.closures); + + // if the current symbol is a closure, peek into its body + if let Some(References { locals, .. }) = env.closures.get(symbol) { + for s in locals.clone() { + loc_succ.insert(s); + } + } + + // remove anything that is not defined in the current block + loc_succ.retain(|key| defined_symbols_set.contains(key)); + + loc_succ + } + None => ImSet::default(), + } + }; + // If a symbol is a direct successor of itself, there is an invalid cycle. // The difference with the function above is that this one does not look behind lambdas, // but does consider direct self-recursion. @@ -295,15 +338,16 @@ pub fn sort_can_defs( // TODO also do the same `addDirects` check elm/compiler does, so we can // report an error if a recursive definition can't possibly terminate! - match topological_sort_into_groups(defined_symbols.as_slice(), all_successors) { + match topological_sort_into_groups(defined_symbols.as_slice(), all_successors_without_self) { Ok(groups) => { let mut declarations = Vec::new(); + // groups are in reversed order for group in groups.into_iter().rev() { group_to_declaration( group, &env.closures, - &mut all_successors, + &mut all_successors_with_self, &can_defs_by_symbol, &mut declarations, ); @@ -320,7 +364,7 @@ pub fn sort_can_defs( group_to_declaration( group, &env.closures, - &mut all_successors, + &mut all_successors_with_self, &can_defs_by_symbol, &mut declarations, ); @@ -337,7 +381,8 @@ pub fn sort_can_defs( // // foo = if b then foo else bar - for cycle in strongly_connected_components(&nodes_in_cycle, all_successors) { + for cycle in strongly_connected_components(&nodes_in_cycle, all_successors_without_self) + { // check whether the cycle is faulty, which is when has a direct successor in the // current cycle. This catches things like: // @@ -386,7 +431,7 @@ pub fn sort_can_defs( group_to_declaration( cycle, &env.closures, - &mut all_successors, + &mut all_successors_with_self, &can_defs_by_symbol, &mut declarations, ); @@ -442,8 +487,14 @@ fn group_to_declaration( new_def.loc_expr.value = Closure(fn_var, name, recursion, args, body); } + let is_recursive = successors(&symbol).contains(&symbol); + if !seen_pattern_regions.contains(&new_def.loc_pattern.region) { - declarations.push(Declare(new_def.clone())); + if is_recursive { + declarations.push(DeclareRec(vec![new_def.clone()])); + } else { + declarations.push(Declare(new_def.clone())); + } } seen_pattern_regions.insert(new_def.loc_pattern.region); } diff --git a/tests/test_canonicalize.rs b/tests/test_canonicalize.rs index d09fe0e47e..6bfcd58251 100644 --- a/tests/test_canonicalize.rs +++ b/tests/test_canonicalize.rs @@ -262,13 +262,21 @@ mod test_canonicalize { fn get_closure(expr: &Expr, i: usize) -> roc::can::expr::Recursive { match expr { - LetRec(assignments, _, _) => match &assignments.get(i).map(|def| &def.loc_expr.value) { - Some(Closure(_, _, recursion, _, _)) => recursion.clone(), - Some(other @ _) => { - panic!("assignment at {} is not a closure, but a {:?}", i, other) + LetRec(assignments, body, _) => { + match &assignments.get(i).map(|def| &def.loc_expr.value) { + Some(Closure(_, _, recursion, _, _)) => recursion.clone(), + Some(other @ _) => { + panic!("assignment at {} is not a closure, but a {:?}", i, other) + } + None => { + if i > 0 { + get_closure(&body.value, i - 1) + } else { + panic!("Looking for assignment at {} but the list is too short", i) + } + } } - None => panic!("Looking for assignment at {} but the list is too short", i), - }, + } LetNonRec(def, body, _) => { if i > 0 { // recurse in the body (not the def!) @@ -298,32 +306,33 @@ mod test_canonicalize { _ -> g (x - 1) h = \x -> - when x is - 0 -> 0 - _ -> g (x - 1) - + when x is + 0 -> 0 + _ -> g (x - 1) + p = \x -> - when x is - 0 -> 0 - 1 -> g (x - 1) - _ -> p (x - 1) + when x is + 0 -> 0 + 1 -> g (x - 1) + _ -> p (x - 1) + # variables must be (indirectly) referenced in the body for analysis to work - { x: p, y: h } + # { x: p, y: h } + g "# ); let arena = Bump::new(); let (actual, _output, _problems, _var_store, _vars, _constraint) = can_expr_with(&arena, "Blah", src, &ImMap::default()); - let detected = get_closure(&actual.value, 0); - assert_eq!(detected, Recursive::TailRecursive); + let detected0 = get_closure(&actual.value, 0); + let detected1 = get_closure(&actual.value, 1); + let detected2 = get_closure(&actual.value, 2); - let detected = get_closure(&actual.value, 1); - assert_eq!(detected, Recursive::NotRecursive); - - let detected = get_closure(&actual.value, 2); - assert_eq!(detected, Recursive::TailRecursive); + assert_eq!(detected0, Recursive::TailRecursive); + assert_eq!(detected1, Recursive::NotRecursive); + assert_eq!(detected2, Recursive::TailRecursive); }); }