Check for invalid cycles after type solving recursive defs

Disallow cycles that pass through a non-function value. Since we
evaluate eagerly, having one such cycle means there is at least one path
in the program that (likely) has unbounded recursion. Of course we can't
be certain (halting problem), but it's very likely, and avoids stuff
like #1926. Also, mono (as it's done today) won't work if things in a
cycle aren't functions.

Closes #1926

compiler/can/src/constraint.rs

@@ -5,7 +5,7 @@ use roc_collections::soa::{EitherIndex, Index, Slice};
 use roc_module::ident::TagName;
 use roc_module::symbol::{ModuleId, Symbol};
 use roc_region::all::{Loc, Region};
-use roc_types::subs::{ExhaustiveMark, Variable};
+use roc_types::subs::{ExhaustiveMark, IllegalCycleMark, Variable};
 use roc_types::types::{Category, PatternCategory, Type};
 
 #[derive(Debug)]
@@ -13,7 +13,8 @@ pub struct Constraints {
     pub constraints: Vec<Constraint>,
     pub types: Vec<Type>,
     pub variables: Vec<Variable>,
-    pub loc_symbols: Vec<(Symbol, Region)>,
+    pub symbols: Vec<Symbol>,
+    pub regions: Vec<Region>,
     pub let_constraints: Vec<LetConstraint>,
     pub categories: Vec<Category>,
     pub pattern_categories: Vec<PatternCategory>,
@@ -24,6 +25,7 @@ pub struct Constraints {
     pub sketched_rows: Vec<SketchedRows>,
     pub eq: Vec<Eq>,
     pub pattern_eq: Vec<PatternEq>,
+    pub cycles: Vec<Cycle>,
 }
 
 impl Default for Constraints {
@@ -37,7 +39,8 @@ impl Constraints {
         let constraints = Vec::new();
         let mut types = Vec::new();
         let variables = Vec::new();
-        let loc_symbols = Vec::new();
+        let symbols = Vec::new();
+        let regions = Vec::new();
         let let_constraints = Vec::new();
         let mut categories = Vec::with_capacity(16);
         let mut pattern_categories = Vec::with_capacity(16);
@@ -48,6 +51,7 @@ impl Constraints {
         let sketched_rows = Vec::new();
         let eq = Vec::new();
         let pattern_eq = Vec::new();
+        let cycles = Vec::new();
 
         types.extend([
             Type::EmptyRec,
@@ -90,7 +94,8 @@ impl Constraints {
             constraints,
             types,
             variables,
-            loc_symbols,
+            symbols,
+            regions,
             let_constraints,
             categories,
             pattern_categories,
@@ -101,6 +106,7 @@ impl Constraints {
             sketched_rows,
             eq,
             pattern_eq,
+            cycles,
         }
     }
 
@@ -363,24 +369,28 @@ impl Constraints {
         let it = it.into_iter();
 
         let types_start = self.types.len();
-        let loc_symbols_start = self.loc_symbols.len();
+        let symbols_start = self.symbols.len();
+        let regions_start = self.regions.len();
 
         // because we have an ExactSizeIterator, we can reserve space here
         let length = it.len();
 
         self.types.reserve(length);
-        self.loc_symbols.reserve(length);
+        self.symbols.reserve(length);
+        self.regions.reserve(length);
 
         for (symbol, loc_type) in it {
             let Loc { region, value } = loc_type;
 
             self.types.push(value);
-            self.loc_symbols.push((symbol, region));
+            self.symbols.push(symbol);
+            self.regions.push(region);
         }
 
         DefTypes {
             types: Slice::new(types_start as _, length as _),
-            loc_symbols: Slice::new(loc_symbols_start as _, length as _),
+            symbols: Slice::new(symbols_start as _, length as _),
+            regions: Slice::new(regions_start as _, length as _),
         }
     }
 
@@ -582,7 +592,8 @@ impl Constraints {
             | Constraint::IncludesTag(_)
             | Constraint::PatternPresence(_, _, _, _)
             | Constraint::Exhaustive { .. }
-            | Constraint::Resolve(..) => false,
+            | Constraint::Resolve(..)
+            | Constraint::CheckCycle(..) => false,
         }
     }
 
@@ -645,6 +656,31 @@ impl Constraints {
 
         Constraint::Exhaustive(equality, sketched_rows, context, exhaustive)
     }
+
+    pub fn check_cycle<I, I1, I2>(
+        &mut self,
+        symbols: I,
+        symbol_regions: I1,
+        expr_regions: I2,
+        cycle_mark: IllegalCycleMark,
+    ) -> Constraint
+    where
+        I: IntoIterator<Item = Symbol>,
+        I1: IntoIterator<Item = Region>,
+        I2: IntoIterator<Item = Region>,
+    {
+        let symbols = Slice::extend_new(&mut self.symbols, symbols);
+        let symbol_regions = Slice::extend_new(&mut self.regions, symbol_regions);
+        let expr_regions = Slice::extend_new(&mut self.regions, expr_regions);
+        let cycle = Cycle {
+            symbols,
+            symbol_regions,
+            expr_regions,
+        };
+        let cycle_index = Index::push_new(&mut self.cycles, cycle);
+
+        Constraint::CheckCycle(cycle_index, cycle_mark)
+    }
 }
 
 roc_error_macros::assert_sizeof_default!(Constraint, 3 * 8);
@@ -734,12 +770,14 @@ pub enum Constraint {
     ),
     /// Attempt to resolve a specialization.
     Resolve(OpportunisticResolve),
+    CheckCycle(Index<Cycle>, IllegalCycleMark),
 }
 
 #[derive(Debug, Clone, Copy, Default)]
 pub struct DefTypes {
     pub types: Slice<Type>,
-    pub loc_symbols: Slice<(Symbol, Region)>,
+    pub symbols: Slice<Symbol>,
+    pub regions: Slice<Region>,
 }
 
 #[derive(Debug, Clone)]
@@ -759,6 +797,13 @@ pub struct IncludesTag {
     pub region: Region,
 }
 
+#[derive(Debug, Clone, Copy)]
+pub struct Cycle {
+    pub symbols: Slice<Symbol>,
+    pub symbol_regions: Slice<Region>,
+    pub expr_regions: Slice<Region>,
+}
+
 /// Custom impl to limit vertical space used by the debug output
 impl std::fmt::Debug for Constraint {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
@@ -798,6 +843,9 @@ impl std::fmt::Debug for Constraint {
             Self::Resolve(arg0) => {
                 write!(f, "Resolve({:?})", arg0)
             }
+            Self::CheckCycle(arg0, arg1) => {
+                write!(f, "CheckCycle({:?}, {:?})", arg0, arg1)
+            }
         }
     }
 }

compiler/can/src/def.rs

@@ -28,6 +28,7 @@ use roc_parse::pattern::PatternType;
 use roc_problem::can::ShadowKind;
 use roc_problem::can::{CycleEntry, Problem, RuntimeError};
 use roc_region::all::{Loc, Region};
+use roc_types::subs::IllegalCycleMark;
 use roc_types::subs::{VarStore, Variable};
 use roc_types::types::AliasKind;
 use roc_types::types::AliasVar;
@@ -158,8 +159,10 @@ impl PendingTypeDef<'_> {
 #[allow(clippy::large_enum_variant)]
 pub enum Declaration {
     Declare(Def),
-    DeclareRec(Vec<Def>),
+    DeclareRec(Vec<Def>, IllegalCycleMark),
     Builtin(Def),
+    /// If we know a cycle is illegal during canonicalization.
+    /// Otherwise we will try to detect this during solving; see [`IllegalCycleMark`].
     InvalidCycle(Vec<CycleEntry>),
 }
 
@@ -168,7 +171,7 @@ impl Declaration {
         use Declaration::*;
         match self {
             Declare(_) => 1,
-            DeclareRec(defs) => defs.len(),
+            DeclareRec(defs, _) => defs.len(),
             InvalidCycle { .. } => 0,
             Builtin(_) => 0,
         }
@@ -748,6 +751,7 @@ impl DefOrdering {
 #[inline(always)]
 pub(crate) fn sort_can_defs(
     env: &mut Env<'_>,
+    var_store: &mut VarStore,
     defs: CanDefs,
     mut output: Output,
 ) -> (Vec<Declaration>, Output) {
@@ -810,7 +814,10 @@ pub(crate) fn sort_can_defs(
                 debug_assert!(!is_specialization, "Self-recursive specializations can only be determined during solving - but it was determined for {:?} now, that's a bug!", def);
 
                 // this function calls itself, and must be typechecked as a recursive def
-                Declaration::DeclareRec(vec![mark_def_recursive(def)])
+                Declaration::DeclareRec(
+                    vec![mark_def_recursive(def)],
+                    IllegalCycleMark::new(var_store),
+                )
             } else {
                 Declaration::Declare(def)
             };
@@ -827,7 +834,9 @@ pub(crate) fn sort_can_defs(
             //
             // boom = \{} -> boom {}
             //
-            // In general we cannot spot faulty recursion (halting problem) so this is our best attempt
+            // In general we cannot spot faulty recursion (halting problem), so this is our
+            // purely-syntactic heuristic. We'll have a second attempt once we know the types in
+            // the cycle.
             let direct_sccs = def_ordering
                 .direct_references
                 .strongly_connected_components_subset(group);
@@ -857,7 +866,7 @@ pub(crate) fn sort_can_defs(
                     .map(|index| mark_def_recursive(take_def!(index)))
                     .collect();
 
-                Declaration::DeclareRec(rec_defs)
+                Declaration::DeclareRec(rec_defs, IllegalCycleMark::new(var_store))
             };
 
             declarations.push(declaration);
@@ -1288,7 +1297,7 @@ pub fn can_defs_with_return<'a>(
         }
     }
 
-    let (declarations, output) = sort_can_defs(env, unsorted, output);
+    let (declarations, output) = sort_can_defs(env, var_store, unsorted, output);
 
     let mut loc_expr: Loc<Expr> = ret_expr;
 
@@ -1305,7 +1314,9 @@ pub fn can_defs_with_return<'a>(
 fn decl_to_let(decl: Declaration, loc_ret: Loc<Expr>) -> Expr {
     match decl {
         Declaration::Declare(def) => Expr::LetNonRec(Box::new(def), Box::new(loc_ret)),
-        Declaration::DeclareRec(defs) => Expr::LetRec(defs, Box::new(loc_ret)),
+        Declaration::DeclareRec(defs, cycle_mark) => {
+            Expr::LetRec(defs, Box::new(loc_ret), cycle_mark)
+        }
         Declaration::InvalidCycle(entries) => {
             Expr::RuntimeError(RuntimeError::CircularDef(entries))
         }

compiler/can/src/effect_module.rs

@@ -8,7 +8,7 @@ use roc_module::called_via::CalledVia;
 use roc_module::ident::TagName;
 use roc_module::symbol::Symbol;
 use roc_region::all::{Loc, Region};
-use roc_types::subs::{ExhaustiveMark, RedundantMark, VarStore, Variable};
+use roc_types::subs::{ExhaustiveMark, IllegalCycleMark, RedundantMark, VarStore, Variable};
 use roc_types::types::{AliasKind, LambdaSet, OptAbleType, OptAbleVar, Type, TypeExtension};
 
 #[derive(Debug, Default, Clone, Copy)]
@@ -72,13 +72,19 @@ pub(crate) fn build_effect_builtins(
     // Effect.forever : Effect a -> Effect b
     if generated_functions.forever {
         let def = helper!(build_effect_forever);
-        declarations.push(Declaration::DeclareRec(vec![def]));
+        declarations.push(Declaration::DeclareRec(
+            vec![def],
+            IllegalCycleMark::new(var_store),
+        ));
     }
 
     // Effect.loop : a, (a -> Effect [ Step a, Done b ]) -> Effect b
     if generated_functions.loop_ {
         let def = helper!(build_effect_loop);
-        declarations.push(Declaration::DeclareRec(vec![def]));
+        declarations.push(Declaration::DeclareRec(
+            vec![def],
+            IllegalCycleMark::new(var_store),
+        ));
     }
 
     // Useful when working on functions in this module. By default symbols that we named do now

compiler/can/src/expr.rs

@@ -19,7 +19,7 @@ use roc_parse::ast::{self, EscapedChar, StrLiteral};
 use roc_parse::pattern::PatternType::*;
 use roc_problem::can::{PrecedenceProblem, Problem, RuntimeError};
 use roc_region::all::{Loc, Region};
-use roc_types::subs::{ExhaustiveMark, RedundantMark, VarStore, Variable};
+use roc_types::subs::{ExhaustiveMark, IllegalCycleMark, RedundantMark, VarStore, Variable};
 use roc_types::types::{Alias, Category, LambdaSet, OptAbleVar, Type};
 use std::fmt::{Debug, Display};
 use std::{char, u32};
@@ -115,7 +115,7 @@ pub enum Expr {
     },
 
     // Let
-    LetRec(Vec<Def>, Box<Loc<Expr>>),
+    LetRec(Vec<Def>, Box<Loc<Expr>>, IllegalCycleMark),
     LetNonRec(Box<Def>, Box<Loc<Expr>>),
 
     /// This is *only* for calling functions, not for tag application.
@@ -224,7 +224,7 @@ impl Expr {
             &Self::AbilityMember(sym, _, _) => Category::Lookup(sym),
             Self::When { .. } => Category::When,
             Self::If { .. } => Category::If,
-            Self::LetRec(_, expr) => expr.value.category(),
+            Self::LetRec(_, expr, _) => expr.value.category(),
             Self::LetNonRec(_, expr) => expr.value.category(),
             &Self::Call(_, _, called_via) => Category::CallResult(None, called_via),
             &Self::RunLowLevel { op, .. } => Category::LowLevelOpResult(op),
@@ -1516,7 +1516,7 @@ pub fn inline_calls(var_store: &mut VarStore, scope: &mut Scope, expr: Expr) ->
             Expect(Box::new(loc_condition), Box::new(loc_expr))
         }
 
-        LetRec(defs, loc_expr) => {
+        LetRec(defs, loc_expr, mark) => {
             let mut new_defs = Vec::with_capacity(defs.len());
 
             for def in defs {
@@ -1537,7 +1537,7 @@ pub fn inline_calls(var_store: &mut VarStore, scope: &mut Scope, expr: Expr) ->
                 value: inline_calls(var_store, scope, loc_expr.value),
             };
 
-            LetRec(new_defs, Box::new(loc_expr))
+            LetRec(new_defs, Box::new(loc_expr), mark)
         }
 
         LetNonRec(def, loc_expr) => {

compiler/can/src/module.rs

@@ -351,7 +351,7 @@ pub fn canonicalize_module_defs<'a>(
         ..Default::default()
     };
 
-    let (mut declarations, mut output) = sort_can_defs(&mut env, defs, new_output);
+    let (mut declarations, mut output) = sort_can_defs(&mut env, var_store, defs, new_output);
 
     let symbols_from_requires = symbols_from_requires
         .iter()
@@ -463,7 +463,7 @@ pub fn canonicalize_module_defs<'a>(
                     }
                 }
             }
-            DeclareRec(defs) => {
+            DeclareRec(defs, _) => {
                 for def in defs {
                     for (symbol, _) in def.pattern_vars.iter() {
                         if exposed_but_not_defined.contains(symbol) {
@@ -559,7 +559,9 @@ pub fn canonicalize_module_defs<'a>(
     for declaration in declarations.iter_mut() {
         match declaration {
             Declare(def) => fix_values_captured_in_closure_def(def, &mut VecSet::default()),
-            DeclareRec(defs) => fix_values_captured_in_closure_defs(defs, &mut VecSet::default()),
+            DeclareRec(defs, _) => {
+                fix_values_captured_in_closure_defs(defs, &mut VecSet::default())
+            }
             InvalidCycle(_) | Builtin(_) => {}
         }
     }
@@ -667,7 +669,7 @@ fn fix_values_captured_in_closure_expr(
             fix_values_captured_in_closure_def(def, no_capture_symbols);
             fix_values_captured_in_closure_expr(&mut loc_expr.value, no_capture_symbols);
         }
-        LetRec(defs, loc_expr) => {
+        LetRec(defs, loc_expr, _) => {
             // LetRec(Vec<Def>, Box<Located<Expr>>, Variable, Aliases),
             fix_values_captured_in_closure_defs(defs, no_capture_symbols);
             fix_values_captured_in_closure_expr(&mut loc_expr.value, no_capture_symbols);

compiler/can/src/traverse.rs

@@ -28,7 +28,7 @@ pub fn walk_decl<V: Visitor>(visitor: &mut V, decl: &Declaration) {
         Declaration::Declare(def) => {
             visitor.visit_def(def);
         }
-        Declaration::DeclareRec(defs) => {
+        Declaration::DeclareRec(defs, _cycle_mark) => {
             visit_list!(visitor, visit_def, defs)
         }
         Declaration::Builtin(def) => visitor.visit_def(def),
@@ -92,7 +92,7 @@ pub fn walk_expr<V: Visitor>(visitor: &mut V, expr: &Expr, var: Variable) {
             branch_var,
             final_else,
         } => walk_if(visitor, *cond_var, branches, *branch_var, final_else),
-        Expr::LetRec(defs, body) => {
+        Expr::LetRec(defs, body, _cycle_mark) => {
             defs.iter().for_each(|def| visitor.visit_def(def));
             visitor.visit_expr(&body.value, body.region, var);
         }