improvements

compiler/constrain/src/expr.rs

@@ -1,3 +1,5 @@
+use std::ops::Range;
+
 use crate::builtins::{
     empty_list_type, float_literal, int_literal, list_type, num_literal, num_u32, str_type,
 };
@@ -10,13 +12,13 @@ use roc_can::expected::Expected::{self, *};
 use roc_can::expected::PExpected;
 use roc_can::expr::Expr::{self, *};
 use roc_can::expr::{
-    AccessorData, AnnotatedMark, ClosureData, Declarations, DestructureDef, Field, FunctionDef,
+    AccessorData, AnnotatedMark, ClosureData, DeclarationTag, Declarations, DestructureDef, Field,
-    WhenBranch,
+    FunctionDef, WhenBranch,
 };
 use roc_can::pattern::Pattern;
 use roc_can::traverse::symbols_introduced_from_pattern;
 use roc_collections::all::{HumanIndex, MutMap, SendMap};
-use roc_collections::soa::{Index, Slice};
+use roc_collections::soa::Index;
 use roc_module::ident::{Lowercase, TagName};
 use roc_module::symbol::{ModuleId, Symbol};
 use roc_region::all::{Loc, Region};
@@ -1317,6 +1319,7 @@ fn constrain_function_def(
                 &loc_body_expr.value,
                 annotation_expected,
             );
+            let ret_constraint = attach_resolution_constraints(constraints, env, ret_constraint);
 
             vars.push(expr_var);
             let defs_constraint = constraints.and_constraint(argument_pattern_state.constraints);
@@ -1378,6 +1381,8 @@ fn constrain_function_def(
                 loc_symbol.value,
             );
 
+            let expr_con = attach_resolution_constraints(constraints, env, expr_con);
+
             constrain_value_def_make_constraint(
                 constraints,
                 vec![],
@@ -1551,6 +1556,7 @@ fn constrain_value_def(
                 &loc_expr.value,
                 annotation_expected,
             );
+            let ret_constraint = attach_resolution_constraints(constraints, env, ret_constraint);
 
             let cons = [
                 ret_constraint,
@@ -1581,6 +1587,8 @@ fn constrain_value_def(
                 NoExpectation(expr_type),
             );
 
+            let expr_con = attach_resolution_constraints(constraints, env, expr_con);
+
             constrain_value_def_make_constraint(
                 constraints,
                 vec![],
@@ -1723,12 +1731,14 @@ pub fn constrain_decls(
 
     debug_assert_eq!(declarations.declarations.len(), declarations.symbols.len());
 
-    for (index, tag) in declarations.iter_top_down() {
+    let mut index = 0;
+    while index < declarations.len() {
         // Clear the rigids from the previous iteration.
         // rigids are not shared between top-level definitions
         env.rigids.clear();
 
         use roc_can::expr::DeclarationTag::*;
+        let tag = declarations.declarations[index];
         match tag {
             Value => {
                 constraint =
@@ -1744,15 +1754,15 @@ pub fn constrain_decls(
                     constraint,
                 );
             }
-            Recursive(function_def_index) | TailRecursive(function_def_index) => {
+            Recursive(_) | TailRecursive(_) => {
                 // for the type it does not matter that a recursive call is a tail call
                 constraint = constrain_recursive_defs_simple(
                     constraints,
                     &mut env,
                     declarations,
-                    index,
+                    index..index,
-                    function_def_index.as_slice(),
                     constraint,
+                    IllegalCycleMark::empty(),
                 );
             }
             Destructure(destructure_def_index) => {
@@ -1765,10 +1775,26 @@ pub fn constrain_decls(
                     constraint,
                 );
             }
-            MutualRecursion { .. } => todo!(),
+            MutualRecursion { length, cycle_mark } => {
+                // the next `length` defs belong to this group
+                let length = length as usize;
+
+                constraint = constrain_recursive_defs_simple(
+                    constraints,
+                    &mut env,
+                    declarations,
+                    index + 1..index + 1 + length,
+                    constraint,
+                    cycle_mark,
+                );
+
+                index += length as usize;
             }
         }
 
+        index += 1;
+    }
+
     // this assert make the "root" of the constraint wasn't dropped
     debug_assert!(constraints.contains_save_the_environment(&constraint));
 
@@ -2556,39 +2582,40 @@ fn constrain_recursive_defs_simple(
     constraints: &mut Constraints,
     env: &mut Env,
     declarations: &Declarations,
-    start_index: usize,
+    range: Range<usize>,
-    defs: Slice<Loc<FunctionDef>>,
     body_con: Constraint,
+    cycle_mark: IllegalCycleMark,
 ) -> Constraint {
+    let length = range.end - range.start;
+
     rec_defs_help_simple(
         constraints,
         env,
         declarations,
-        defs,
+        range,
-        start_index,
         body_con,
-        Info::with_capacity(defs.len()),
+        Info::with_capacity(length),
-        Info::with_capacity(defs.len()),
+        Info::with_capacity(length),
+        cycle_mark,
     )
 }
 
-pub fn rec_defs_help_simple(
+#[allow(clippy::too_many_arguments)]
+pub fn rec_defs_help_simple_function(
     constraints: &mut Constraints,
     env: &mut Env,
     declarations: &Declarations,
-    defs: Slice<Loc<FunctionDef>>,
+    index: usize,
-    start_index: usize,
+    function_def_index: Index<Loc<FunctionDef>>,
-    body_con: Constraint,
+    rigid_info: &mut Info,
-    mut rigid_info: Info,
+    flex_info: &mut Info,
-    mut flex_info: Info,
+) {
-) -> Constraint {
-    for (index, function_def_index) in (start_index..).zip(defs.indices()) {
     let loc_expr = &declarations.expressions[index];
     let loc_symbol = declarations.symbols[index];
     let expr_var = declarations.variables[index];
     let opt_annotation = &declarations.annotations[index];
 
-        let loc_function_def = &declarations.function_bodies[function_def_index];
+    let loc_function_def = &declarations.function_bodies[function_def_index.index()];
     let function_def = &loc_function_def.value;
 
     match opt_annotation {
@@ -2610,6 +2637,7 @@ pub fn rec_defs_help_simple(
                 loc_symbol.value,
             );
 
+            let expr_con = attach_resolution_constraints(constraints, env, expr_con);
             let def_con = expr_con;
 
             flex_info.vars = vec![expr_var];
@@ -2724,12 +2752,12 @@ pub fn rec_defs_help_simple(
                 &loc_body_expr.value,
                 NoExpectation(ret_type.clone()),
             );
+            let expr_con = attach_resolution_constraints(constraints, env, expr_con);
 
             vars.push(expr_var);
 
             let signature_index = constraints.push_type(signature);
-                let state_constraints =
+            let state_constraints = constraints.and_constraint(argument_pattern_state.constraints);
-                    constraints.and_constraint(argument_pattern_state.constraints);
             let cons = [
                 constraints.let_constraint(
                     [],
@@ -2766,32 +2794,190 @@ pub fn rec_defs_help_simple(
             rigid_info.def_types.extend(def_pattern_state.headers);
         }
     }
+}
+
+#[allow(clippy::too_many_arguments)]
+pub fn rec_defs_help_simple(
+    constraints: &mut Constraints,
+    env: &mut Env,
+    declarations: &Declarations,
+    range: Range<usize>,
+    body_con: Constraint,
+    mut rigid_info: Info,
+    mut flex_info: Info,
+    cycle_mark: IllegalCycleMark,
+) -> Constraint {
+    let length = range.end - range.start;
+    let mut loc_symbols = Vec::with_capacity(length);
+    let mut expr_regions = Vec::with_capacity(length);
+
+    for index in range {
+        // Clear the rigids from the previous iteration.
+        // rigids are not shared between top-level definitions
+        env.rigids.clear();
+
+        let loc_symbol = declarations.symbols[index];
+        loc_symbols.push((loc_symbol.value, loc_symbol.region));
+
+        match declarations.declarations[index] {
+            DeclarationTag::Value => {
+                let expr_var = declarations.variables[index];
+                let opt_annotation = &declarations.annotations[index];
+
+                let loc_expr = &declarations.expressions[index];
+                expr_regions.push(loc_expr.region);
+
+                match opt_annotation {
+                    None => {
+                        let expr_con = constrain_expr(
+                            constraints,
+                            env,
+                            loc_expr.region,
+                            &loc_expr.value,
+                            NoExpectation(Type::Variable(expr_var)),
+                        );
+                        let expr_con = attach_resolution_constraints(constraints, env, expr_con);
+
+                        let def_con = expr_con;
+
+                        flex_info.vars = vec![expr_var];
+                        flex_info.constraints.push(def_con);
+                        flex_info.def_types.insert(
+                            loc_symbol.value,
+                            Loc::at(loc_symbol.region, Type::Variable(expr_var)),
+                        );
+                    }
+                    Some(annotation) => {
+                        let arity = annotation.signature.arity();
+                        let rigids = &env.rigids;
+                        let mut ftv = rigids.clone();
+
+                        let InstantiateRigids {
+                            signature,
+                            new_rigid_variables,
+                            new_infer_variables,
+                        } = instantiate_rigids_simple(
+                            &annotation.signature,
+                            &annotation.introduced_variables,
+                            &mut ftv,
+                        );
+
+                        let loc_pattern =
+                            Loc::at(loc_symbol.region, Pattern::Identifier(loc_symbol.value));
+
+                        flex_info.vars.extend(new_infer_variables);
+
+                        let annotation_expected = FromAnnotation(
+                            loc_pattern.clone(),
+                            arity,
+                            AnnotationSource::TypedBody {
+                                region: annotation.region,
+                            },
+                            signature.clone(),
+                        );
+
+                        let expected = annotation_expected;
+
+                        let ret_constraint = constrain_expr(
+                            constraints,
+                            env,
+                            loc_expr.region,
+                            &loc_expr.value,
+                            expected,
+                        );
+                        let ret_constraint =
+                            attach_resolution_constraints(constraints, env, ret_constraint);
+
+                        let cons = [
+                            ret_constraint,
+                            // Store type into AST vars. We use Store so errors aren't reported twice
+                            constraints.store(
+                                signature.clone(),
+                                expr_var,
+                                std::file!(),
+                                std::line!(),
+                            ),
+                        ];
+                        let def_con = constraints.and_constraint(cons);
+
+                        rigid_info.vars.extend(&new_rigid_variables);
+
+                        rigid_info.constraints.push(constraints.let_constraint(
+                            new_rigid_variables,
+                            [expr_var],
+                            [], // no headers introduced (at this level)
+                            def_con,
+                            Constraint::True,
+                        ));
+                        rigid_info
+                            .def_types
+                            .insert(loc_symbol.value, Loc::at(loc_symbol.region, signature));
+                    }
+                }
+            }
+            DeclarationTag::Recursive(f_index) | DeclarationTag::TailRecursive(f_index) => {
+                expr_regions.push(declarations.function_bodies[f_index.index()].region);
+
+                rec_defs_help_simple_function(
+                    constraints,
+                    env,
+                    declarations,
+                    index,
+                    f_index,
+                    &mut rigid_info,
+                    &mut flex_info,
+                );
+            }
+            _ => unreachable!(),
+        }
     }
 
-    let flex_constraints = constraints.and_constraint(flex_info.constraints);
+    // Strategy for recursive defs:
-    let inner_inner = constraints.let_constraint(
+    // 1. Let-generalize the type annotations we know; these are the source of truth we'll solve
+    //    everything else with. If there are circular type errors here, they will be caught during
+    //    the let-generalization.
+    // 2. Introduce all symbols of the untyped defs, but don't generalize them yet. Now, solve
+    //    the untyped defs' bodies. This way, when checking something like
+    //      f = \x -> f [ x ]
+    //    we introduce `f: b -> c`, then constrain the call `f [ x ]`,
+    //    forcing `b -> c ~ List b -> c` and correctly picking up a recursion error.
+    //    Had we generalized `b -> c`, the call `f [ x ]` would have been generalized, and this
+    //    error would not be found.
+    // 3. Now properly let-generalize the untyped body defs, since we now know their types and
+    //    that they don't have circular type errors.
+    // 4. Solve the bodies of the typed body defs, and check that they agree the types of the type
+    //    annotation.
+    // 5. Solve the rest of the program that happens after this recursive def block.
+
+    // 2. Solve untyped defs without generalization of their symbols.
+    let untyped_body_constraints = constraints.and_constraint(flex_info.constraints);
+    let untyped_def_symbols_constr = constraints.let_constraint(
         [],
         [],
         flex_info.def_types.clone(),
         Constraint::True,
-        flex_constraints,
+        untyped_body_constraints,
     );
 
-    let rigid_constraints = {
+    // an extra constraint that propagates information to the solver to check for invalid recursion
-        let mut temp = rigid_info.constraints;
+    // and generate a good error message there.
-        temp.push(body_con);
+    let cycle_constraint = constraints.check_cycle(loc_symbols, expr_regions, cycle_mark);
 
-        constraints.and_constraint(temp)
+    let typed_body_constraints = constraints.and_constraint(rigid_info.constraints);
-    };
+    let typed_body_and_final_constr =
+        constraints.and_constraint([typed_body_constraints, cycle_constraint, body_con]);
 
+    // 3. Properly generalize untyped defs after solving them.
     let inner = constraints.let_constraint(
         [],
         flex_info.vars,
         flex_info.def_types,
-        inner_inner,
+        untyped_def_symbols_constr,
-        rigid_constraints,
+        // 4 + 5. Solve the typed body defs, and the rest of the program.
+        typed_body_and_final_constr,
     );
 
+    // 1. Let-generalize annotations we know.
     constraints.let_constraint(
         rigid_info.vars,
         [],

compiler/test_gen/src/gen_primitives.rs

@@ -3491,8 +3491,8 @@ fn mutual_recursion_top_level_defs() {
 
             isOdd = \n ->
                 when n is
-                    0 -> True
+                    0 -> False
-                    1 -> False
+                    1 -> True
                     _ -> isEven (n - 1)
 
             main = isOdd 11

reporting/tests/test_reporting.rs

@@ -10067,6 +10067,44 @@ I need all branches in an `if` to have the same type!
         )
     }
 
+    #[test]
+    fn cycle_through_non_function_top_level() {
+        new_report_problem_as(
+            "cycle_through_non_function",
+            indoc!(
+                r#"
+                app "test" provides [ t2 ] to "./platform"
+
+                force : ({} -> I64) -> I64
+                force = \eval -> eval {}
+
+                t1 = \_ -> force (\_ -> t2)
+
+                t2 = t1 {}
+                "#
+            ),
+            indoc!(
+                r#"
+                ── CIRCULAR DEFINITION ─────────────────────────────────── /code/proj/Main.roc ─
+
+                The `t1` definition is causing a very tricky infinite loop:
+
+                6│  t1 = \_ -> force (\_ -> t2)
+                    ^^
+
+                The `t1` value depends on itself through the following chain of
+                definitions:
+
+                    ┌─────┐
+                    │     t1
+                    │     ↓
+                    │     t2
+                    └─────┘
+                "#
+            ),
+        )
+    }
+
     #[test]
     fn shadowing_top_level_scope() {
         new_report_problem_as(