From b3e60f9d3a59b4b15c2ece222c673dd82bd94448 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Sun, 20 Oct 2024 04:50:12 -0700 Subject: [PATCH 01/16] Implement return keyword --- crates/compiler/can/src/builtins.rs | 1 + crates/compiler/can/src/constraint.rs | 2 + crates/compiler/can/src/copy.rs | 13 +++ crates/compiler/can/src/debug/pretty_print.rs | 1 + crates/compiler/can/src/def.rs | 100 ++++++++++++++---- crates/compiler/can/src/desugar.rs | 26 ++++- crates/compiler/can/src/expr.rs | 86 ++++++++++++--- crates/compiler/can/src/module.rs | 16 ++- crates/compiler/can/src/scope.rs | 17 ++- crates/compiler/can/src/suffixed.rs | 2 +- crates/compiler/can/src/task_module.rs | 3 + crates/compiler/can/src/traverse.rs | 15 +++ crates/compiler/constrain/src/expr.rs | 53 +++++++++- crates/compiler/derive/src/decoding.rs | 1 + crates/compiler/derive/src/decoding/record.rs | 3 + crates/compiler/derive/src/decoding/tuple.rs | 3 + crates/compiler/derive/src/encoding.rs | 6 ++ crates/compiler/derive/src/hash.rs | 1 + crates/compiler/derive/src/inspect.rs | 6 ++ crates/compiler/fmt/src/def.rs | 2 + crates/compiler/fmt/src/expr.rs | 28 +++++ crates/compiler/load_internal/src/docs.rs | 3 + crates/compiler/lower_params/src/lower.rs | 8 ++ .../compiler/lower_params/src/type_error.rs | 3 +- crates/compiler/mono/src/ir.rs | 22 ++++ crates/compiler/parse/src/ast.rs | 25 +++++ crates/compiler/parse/src/expr.rs | 46 +++++++- crates/compiler/parse/src/keyword.rs | 5 +- crates/compiler/parse/src/lib.rs | 1 - crates/compiler/parse/src/normalize.rs | 23 ++++ crates/compiler/parse/src/parser.rs | 10 ++ crates/compiler/parse/src/problems.rs | 25 ----- crates/compiler/problem/src/can.rs | 18 +++- crates/compiler/types/src/types.rs | 2 + .../src/analysis/completion/visitor.rs | 1 + crates/language_server/src/analysis/tokens.rs | 6 ++ crates/repl_ui/src/repl_state.rs | 3 + crates/reporting/src/error/canonicalize.rs | 38 +++++++ crates/reporting/src/error/type.rs | 50 ++++++++- 39 files changed, 594 insertions(+), 80 deletions(-) delete mode 100644 crates/compiler/parse/src/problems.rs diff --git a/crates/compiler/can/src/builtins.rs b/crates/compiler/can/src/builtins.rs index 60f01975841..64e7d9d6697 100644 --- a/crates/compiler/can/src/builtins.rs +++ b/crates/compiler/can/src/builtins.rs @@ -446,6 +446,7 @@ fn defn_help( function_type: var_store.fresh(), closure_type: var_store.fresh(), return_type: ret_var, + early_returns: vec![], name: fn_name, captured_symbols: Vec::new(), recursive: Recursive::NotRecursive, diff --git a/crates/compiler/can/src/constraint.rs b/crates/compiler/can/src/constraint.rs index e10815bc874..04ef75b6e21 100644 --- a/crates/compiler/can/src/constraint.rs +++ b/crates/compiler/can/src/constraint.rs @@ -96,6 +96,7 @@ impl Constraints { Category::List, Category::Str, Category::Character, + Category::Return, ]); pattern_categories.extend([ @@ -149,6 +150,7 @@ impl Constraints { pub const CATEGORY_LIST: Index = Index::new(11); pub const CATEGORY_STR: Index = Index::new(12); pub const CATEGORY_CHARACTER: Index = Index::new(13); + pub const CATEGORY_RETURN: Index = Index::new(14); pub const PCATEGORY_RECORD: Index = Index::new(0); pub const PCATEGORY_EMPTYRECORD: Index = Index::new(1); diff --git a/crates/compiler/can/src/copy.rs b/crates/compiler/can/src/copy.rs index 00064c7893c..15dae6ac32a 100644 --- a/crates/compiler/can/src/copy.rs +++ b/crates/compiler/can/src/copy.rs @@ -455,6 +455,7 @@ fn deep_copy_expr_help(env: &mut C, copied: &mut Vec, expr function_type, closure_type, return_type, + early_returns, name, captured_symbols, recursive, @@ -464,6 +465,10 @@ fn deep_copy_expr_help(env: &mut C, copied: &mut Vec, expr function_type: sub!(*function_type), closure_type: sub!(*closure_type), return_type: sub!(*return_type), + early_returns: early_returns + .iter() + .map(|(var, region)| (sub!(*var), *region)) + .collect(), name: *name, captured_symbols: captured_symbols .iter() @@ -687,6 +692,14 @@ fn deep_copy_expr_help(env: &mut C, copied: &mut Vec, expr lookups_in_cond: lookups_in_cond.to_vec(), }, + Return { + return_value, + return_var, + } => Return { + return_value: Box::new(return_value.map(|e| go_help!(e))), + return_var: sub!(*return_var), + }, + Dbg { source_location, source, diff --git a/crates/compiler/can/src/debug/pretty_print.rs b/crates/compiler/can/src/debug/pretty_print.rs index b842d6076ef..1ab6dec4774 100644 --- a/crates/compiler/can/src/debug/pretty_print.rs +++ b/crates/compiler/can/src/debug/pretty_print.rs @@ -449,6 +449,7 @@ fn expr<'a>(c: &Ctx, p: EPrec, f: &'a Arena<'a>, e: &'a Expr) -> DocBuilder<'a, Dbg { .. } => todo!(), Expect { .. } => todo!(), ExpectFx { .. } => todo!(), + Return { .. } => todo!(), TypedHole(_) => todo!(), RuntimeError(_) => todo!(), } diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index c95b62bf8dc..1864c6bb4fa 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -134,31 +134,32 @@ impl Annotation { #[derive(Debug)] pub(crate) struct CanDefs { defs: Vec>, - dbgs: ExpectsOrDbgs, - expects: ExpectsOrDbgs, - expects_fx: ExpectsOrDbgs, + dbgs: OrderDependentStatements, + expects: OrderDependentStatements, + expects_fx: OrderDependentStatements, + returns: OrderDependentStatements, def_ordering: DefOrdering, aliases: VecMap, } #[derive(Clone, Debug)] -pub struct ExpectsOrDbgs { - pub conditions: Vec, +pub struct OrderDependentStatements { + pub expressions: Vec, pub regions: Vec, pub preceding_comment: Vec, } -impl ExpectsOrDbgs { +impl OrderDependentStatements { fn with_capacity(capacity: usize) -> Self { Self { - conditions: Vec::with_capacity(capacity), + expressions: Vec::with_capacity(capacity), regions: Vec::with_capacity(capacity), preceding_comment: Vec::with_capacity(capacity), } } fn push(&mut self, loc_can_condition: Loc, preceding_comment: Region) { - self.conditions.push(loc_can_condition.value); + self.expressions.push(loc_can_condition.value); self.regions.push(loc_can_condition.region); self.preceding_comment.push(preceding_comment); } @@ -303,9 +304,10 @@ impl PendingTypeDef<'_> { pub enum Declaration { Declare(Def), DeclareRec(Vec, IllegalCycleMark), + Return(Expr, Region), Builtin(Def), - Expects(ExpectsOrDbgs), - ExpectsFx(ExpectsOrDbgs), + Expects(OrderDependentStatements), + ExpectsFx(OrderDependentStatements), /// If we know a cycle is illegal during canonicalization. /// Otherwise we will try to detect this during solving; see [`IllegalCycleMark`]. InvalidCycle(Vec), @@ -317,6 +319,7 @@ impl Declaration { match self { Declare(_) => 1, DeclareRec(defs, _) => defs.len(), + Return(_, _) => 0, InvalidCycle { .. } => 0, Builtin(_) => 0, Expects(_) => 0, @@ -340,6 +343,7 @@ impl Declaration { expects.regions.first().unwrap(), expects.regions.last().unwrap(), ), + Declaration::Return(_return_expr, return_region) => *return_region, } } } @@ -1137,6 +1141,7 @@ fn canonicalize_value_defs<'a>( let mut pending_dbgs = Vec::with_capacity(value_defs.len()); let mut pending_expects = Vec::with_capacity(value_defs.len()); let mut pending_expect_fx = Vec::with_capacity(value_defs.len()); + let mut pending_returns = Vec::with_capacity(value_defs.len()); let mut imports_introduced = Vec::with_capacity(value_defs.len()); @@ -1159,6 +1164,9 @@ fn canonicalize_value_defs<'a>( PendingValue::ExpectFx(pending_expect) => { pending_expect_fx.push(pending_expect); } + PendingValue::Return(pending_return) => { + pending_returns.push(pending_return); + } PendingValue::ModuleImport(PendingModuleImport { module_id, region, @@ -1235,9 +1243,10 @@ fn canonicalize_value_defs<'a>( def_ordering.insert_symbol_references(def_id as u32, &temp_output.references) } - let mut dbgs = ExpectsOrDbgs::with_capacity(pending_dbgs.len()); - let mut expects = ExpectsOrDbgs::with_capacity(pending_expects.len()); - let mut expects_fx = ExpectsOrDbgs::with_capacity(pending_expects.len()); + let mut dbgs = OrderDependentStatements::with_capacity(pending_dbgs.len()); + let mut expects = OrderDependentStatements::with_capacity(pending_expects.len()); + let mut expects_fx = OrderDependentStatements::with_capacity(pending_expects.len()); + let mut returns = OrderDependentStatements::with_capacity(pending_returns.len()); for pending in pending_dbgs { let (loc_can_condition, can_output) = canonicalize_expr( @@ -1281,11 +1290,21 @@ fn canonicalize_value_defs<'a>( output.union(can_output); } + for pending in pending_returns { + let (loc_return_expr, can_output) = + canonicalize_expr(env, var_store, scope, pending.region, &pending.value); + + returns.push(loc_return_expr, Region::zero()); + + output.union(can_output); + } + let can_defs = CanDefs { defs, dbgs, expects, expects_fx, + returns, def_ordering, aliases, }; @@ -1681,7 +1700,7 @@ impl DefOrdering { } #[inline(always)] -pub(crate) fn sort_can_defs_new( +pub(crate) fn sort_top_level_can_defs( env: &mut Env<'_>, scope: &mut Scope, var_store: &mut VarStore, @@ -1694,10 +1713,17 @@ pub(crate) fn sort_can_defs_new( dbgs: _, expects, expects_fx, + returns, def_ordering, aliases, } = defs; + for return_region in returns.regions { + env.problem(Problem::ReturnOutsideOfFunction { + region: return_region, + }); + } + // TODO: inefficient, but I want to make this what CanDefs contains in the future let mut defs: Vec<_> = defs.into_iter().map(|x| x.unwrap()).collect(); @@ -1712,7 +1738,7 @@ pub(crate) fn sort_can_defs_new( // because of the ordering of declarations, expects should come first because they are // independent, but can rely on all other top-level symbols in the module let it = expects - .conditions + .expressions .into_iter() .zip(expects.regions) .zip(expects.preceding_comment); @@ -1725,7 +1751,7 @@ pub(crate) fn sort_can_defs_new( } let it = expects_fx - .conditions + .expressions .into_iter() .zip(expects_fx.regions) .zip(expects_fx.preceding_comment); @@ -1829,6 +1855,7 @@ pub(crate) fn sort_can_defs_new( None, ); } + // TODO: do I need to handle returns here? _ => { declarations.push_value_def( Loc::at(def.loc_pattern.region, symbol), @@ -1975,6 +2002,7 @@ pub(crate) fn sort_can_defs( dbgs, expects, expects_fx, + returns, def_ordering, aliases, } = defs; @@ -2100,18 +2128,24 @@ pub(crate) fn sort_can_defs( } } - if !dbgs.conditions.is_empty() { + if !dbgs.expressions.is_empty() { declarations.push(Declaration::Expects(dbgs)); } - if !expects.conditions.is_empty() { + if !expects.expressions.is_empty() { declarations.push(Declaration::Expects(expects)); } - if !expects_fx.conditions.is_empty() { + if !expects_fx.expressions.is_empty() { declarations.push(Declaration::ExpectsFx(expects_fx)); } + if !returns.expressions.is_empty() { + for (return_expr, return_region) in returns.expressions.into_iter().zip(returns.regions) { + declarations.push(Declaration::Return(return_expr, return_region)); + } + } + (declarations, output) } @@ -2354,6 +2388,7 @@ fn canonicalize_pending_value_def<'a>( function_type: var_store.fresh(), closure_type: var_store.fresh(), return_type: var_store.fresh(), + early_returns: scope.early_returns.clone(), name: symbol, captured_symbols: Vec::new(), recursive: Recursive::NotRecursive, @@ -2571,6 +2606,8 @@ fn canonicalize_pending_body<'a>( loc_value = value; } + let expr_var = var_store.fresh(); + // We treat closure definitions `foo = \a, b -> ...` differently from other body expressions, // because they need more bookkeeping (for tail calls, closure captures, etc.) // @@ -2664,7 +2701,6 @@ fn canonicalize_pending_body<'a>( } }; - let expr_var = var_store.fresh(); let mut vars_by_symbol = SendMap::default(); pattern_to_vars_by_symbol(&mut vars_by_symbol, &loc_can_pattern.value, expr_var); @@ -2731,7 +2767,7 @@ pub fn can_defs_with_return<'a>( let mut loc_expr: Loc = ret_expr; for declaration in declarations.into_iter().rev() { - loc_expr = decl_to_let(declaration, loc_expr); + loc_expr = decl_to_let_or_return(declaration, loc_expr, var_store); } (loc_expr.value, output) @@ -2758,7 +2794,11 @@ pub fn report_unused_imports( } } -fn decl_to_let(decl: Declaration, loc_ret: Loc) -> Loc { +fn decl_to_let_or_return<'a>( + decl: Declaration, + loc_ret: Loc, + var_store: &mut VarStore, +) -> Loc { match decl { Declaration::Declare(def) => { let region = Region::span_across(&def.loc_pattern.region, &loc_ret.region); @@ -2770,6 +2810,17 @@ fn decl_to_let(decl: Declaration, loc_ret: Loc) -> Loc { let expr = Expr::LetRec(defs, Box::new(loc_ret), cycle_mark); Loc::at(region, expr) } + Declaration::Return(return_expr, return_region) => { + let region = Region::span_across(&return_region, &loc_ret.region); + + let return_var = var_store.fresh(); + let expr = Expr::Return { + return_value: Box::new(Loc::at(return_region, return_expr)), + return_var, + }; + + Loc::at(region, expr) + } Declaration::InvalidCycle(entries) => { Loc::at_zero(Expr::RuntimeError(RuntimeError::CircularDef(entries))) } @@ -2780,7 +2831,7 @@ fn decl_to_let(decl: Declaration, loc_ret: Loc) -> Loc { Declaration::Expects(expects) => { let mut loc_ret = loc_ret; - let conditions = expects.conditions.into_iter().rev(); + let conditions = expects.expressions.into_iter().rev(); let condition_regions = expects.regions.into_iter().rev(); let expect_regions = expects.preceding_comment.into_iter().rev(); @@ -3005,6 +3056,7 @@ enum PendingValue<'a> { Dbg(PendingExpectOrDbg<'a>), Expect(PendingExpectOrDbg<'a>), ExpectFx(PendingExpectOrDbg<'a>), + Return(&'a Loc>), ModuleImport(PendingModuleImport<'a>), SignatureDefMismatch, InvalidIngestedFile, @@ -3154,6 +3206,8 @@ fn to_pending_value_def<'a>( preceding_comment: *preceding_comment, }), + Return(return_expr) => PendingValue::Return(return_expr), + ModuleImport(module_import) => { let qualified_module_name: QualifiedModuleName = module_import.name.value.into(); let module_name = qualified_module_name.module.clone(); diff --git a/crates/compiler/can/src/desugar.rs b/crates/compiler/can/src/desugar.rs index 4d06abf270e..653da47b1c7 100644 --- a/crates/compiler/can/src/desugar.rs +++ b/crates/compiler/can/src/desugar.rs @@ -177,6 +177,12 @@ fn desugar_value_def<'a>( body_expr: desugar_expr(env, scope, stmt_expr), } } + + Return(return_expr) => { + let desugared_return_expr = &*env.arena.alloc(desugar_expr(env, scope, return_expr)); + + Return(&desugared_return_expr) + } } } @@ -315,9 +321,15 @@ pub fn desugar_value_def_suffixed<'a>(arena: &'a Bump, value_def: ValueDef<'a>) } }, - // TODO support desugaring of Dbg, Expect, and ExpectFx + // TODO support desugaring of Dbg and ExpectFx Dbg { .. } | ExpectFx { .. } => value_def, ModuleImport { .. } | IngestedFileImport(_) => value_def, + Return(ret_expr) => match unwrap_suffixed_expression(arena, ret_expr, None) { + Ok(new_ret_expr) => ValueDef::Return(new_ret_expr), + Err(..) => { + internal_error!("Unable to desugar the suffix inside a Return value def"); + } + }, Stmt(..) => { internal_error!( @@ -1008,6 +1020,7 @@ pub fn desugar_expr<'a>( Expect(condition, continuation) => { let desugared_condition = &*env.arena.alloc(desugar_expr(env, scope, condition)); let desugared_continuation = &*env.arena.alloc(desugar_expr(env, scope, continuation)); + env.arena.alloc(Loc { value: Expect(desugared_condition, desugared_continuation), region: loc_expr.region, @@ -1019,7 +1032,6 @@ pub fn desugar_expr<'a>( } DbgStmt(condition, continuation) => { let desugared_condition = &*env.arena.alloc(desugar_expr(env, scope, condition)); - let desugared_continuation = &*env.arena.alloc(desugar_expr(env, scope, continuation)); env.arena.alloc(Loc { @@ -1027,6 +1039,16 @@ pub fn desugar_expr<'a>( region: loc_expr.region, }) } + Return(return_value, after_return) => { + let desugared_return_value = &*env.arena.alloc(desugar_expr(env, scope, return_value)); + let desugared_after_return = + after_return.map(|ar| *env.arena.alloc(desugar_expr(env, scope, ar))); + + env.arena.alloc(Loc { + value: Return(desugared_return_value, desugared_after_return), + region: loc_expr.region, + }) + } // note this only exists after desugaring LowLevelDbg(_, _, _) => loc_expr, diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index 890370382a5..7a8b8a5c74b 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -286,6 +286,11 @@ pub enum Expr { symbol: Symbol, }, + Return { + return_value: Box>, + return_var: Variable, + }, + /// Rendered as empty box in editor TypedHole(Variable), @@ -360,6 +365,7 @@ impl Expr { Self::Expect { .. } => Category::Expect, Self::ExpectFx { .. } => Category::Expect, Self::Crash { .. } => Category::Crash, + Self::Return { .. } => Category::Return, Self::Dbg { .. } => Category::Expect, @@ -400,6 +406,7 @@ pub struct ClosureData { pub function_type: Variable, pub closure_type: Variable, pub return_type: Variable, + pub early_returns: Vec<(Variable, Region)>, pub name: Symbol, pub captured_symbols: Vec<(Symbol, Variable)>, pub recursive: Recursive, @@ -476,6 +483,7 @@ impl StructAccessorData { function_type: function_var, closure_type: closure_var, return_type: field_var, + early_returns: vec![], name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -549,6 +557,7 @@ impl OpaqueWrapFunctionData { function_type: function_var, closure_type: closure_var, return_type: opaque_var, + early_returns: vec![], name: function_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -1008,7 +1017,7 @@ pub fn canonicalize_expr<'a>( } ast::Expr::Defs(loc_defs, loc_ret) => { // The body expression gets a new scope for canonicalization, - scope.inner_scope(|inner_scope| { + scope.inner_scope(false, |inner_scope| { let defs: Defs = (*loc_defs).clone(); can_defs_with_return(env, var_store, inner_scope, env.arena.alloc(defs), loc_ret) }) @@ -1040,16 +1049,17 @@ pub fn canonicalize_expr<'a>( let mut can_branches = Vec::with_capacity(branches.len()); for branch in branches.iter() { - let (can_when_branch, branch_references) = scope.inner_scope(|inner_scope| { - canonicalize_when_branch( - env, - var_store, - inner_scope, - region, - branch, - &mut output, - ) - }); + let (can_when_branch, branch_references) = + scope.inner_scope(false, |inner_scope| { + canonicalize_when_branch( + env, + var_store, + inner_scope, + region, + branch, + &mut output, + ) + }); output.references.union_mut(&branch_references); @@ -1258,6 +1268,37 @@ pub fn canonicalize_expr<'a>( output, ) } + ast::Expr::Return(return_expr, after_return) => { + let mut output = Output::default(); + + let (loc_return_expr, output1) = canonicalize_expr( + env, + var_store, + scope, + return_expr.region, + &return_expr.value, + ); + + if let Some(after_return) = after_return { + env.problem(Problem::StatementsAfterReturn { + region: after_return.region, + }); + } + + output.union(output1); + + let return_var = var_store.fresh(); + + scope.early_returns.push((return_var, return_expr.region)); + + ( + Return { + return_value: Box::new(loc_return_expr), + return_var, + }, + output, + ) + } ast::Expr::If { if_thens, final_else: final_else_branch, @@ -1493,7 +1534,7 @@ pub fn canonicalize_closure<'a>( loc_body_expr: &'a Loc>, opt_def_name: Option, ) -> (ClosureData, Output) { - scope.inner_scope(|inner_scope| { + scope.inner_scope(true, |inner_scope| { canonicalize_closure_body( env, var_store, @@ -1621,10 +1662,13 @@ fn canonicalize_closure_body<'a>( output.non_closures.insert(symbol); } + let return_type_var = var_store.fresh(); + let closure_data = ClosureData { function_type: var_store.fresh(), closure_type: var_store.fresh(), - return_type: var_store.fresh(), + return_type: return_type_var, + early_returns: scope.early_returns.clone(), name: symbol, captured_symbols, recursive: Recursive::NotRecursive, @@ -2027,7 +2071,8 @@ pub fn inline_calls(var_store: &mut VarStore, expr: Expr) -> Expr { | other @ TypedHole { .. } | other @ ForeignCall { .. } | other @ OpaqueWrapFunction(_) - | other @ Crash { .. } => other, + | other @ Crash { .. } + | other @ Return { .. } => other, List { elem_var, @@ -2253,6 +2298,7 @@ pub fn inline_calls(var_store: &mut VarStore, expr: Expr) -> Expr { function_type, closure_type, return_type, + early_returns, recursive, name, captured_symbols, @@ -2269,6 +2315,7 @@ pub fn inline_calls(var_store: &mut VarStore, expr: Expr) -> Expr { function_type, closure_type, return_type, + early_returns, recursive, name, captured_symbols, @@ -2495,6 +2542,7 @@ pub fn is_valid_interpolation(expr: &ast::Expr<'_>) -> bool { ast::Expr::DbgStmt(_, _) | ast::Expr::LowLevelDbg(_, _, _) | ast::Expr::Expect(_, _) + | ast::Expr::Return(_, _) | ast::Expr::When(_, _) | ast::Expr::Backpassing(_, _, _) | ast::Expr::SpaceBefore(_, _) @@ -2795,6 +2843,7 @@ impl Declarations { pub fn new() -> Self { Self::with_capacity(0) } + pub fn with_capacity(capacity: usize) -> Self { Self { declarations: Vec::with_capacity(capacity), @@ -2841,6 +2890,7 @@ impl Declarations { let function_def = FunctionDef { closure_type: loc_closure_data.value.closure_type, return_type: loc_closure_data.value.return_type, + early_returns: loc_closure_data.value.early_returns, captured_symbols: loc_closure_data.value.captured_symbols, arguments: loc_closure_data.value.arguments, }; @@ -2892,6 +2942,7 @@ impl Declarations { let function_def = FunctionDef { closure_type: loc_closure_data.value.closure_type, return_type: loc_closure_data.value.return_type, + early_returns: loc_closure_data.value.early_returns, captured_symbols: loc_closure_data.value.captured_symbols, arguments: loc_closure_data.value.arguments, }; @@ -3072,6 +3123,7 @@ impl Declarations { let function_def = FunctionDef { closure_type: closure_data.closure_type, return_type: closure_data.return_type, + early_returns: closure_data.early_returns, captured_symbols: closure_data.captured_symbols, arguments: closure_data.arguments, }; @@ -3112,6 +3164,7 @@ impl Declarations { function_type: var_store.fresh(), closure_type: var_store.fresh(), return_type: var_store.fresh(), + early_returns: vec![], name: self.symbols[index].value, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -3124,6 +3177,7 @@ impl Declarations { let function_def = FunctionDef { closure_type: loc_closure_data.value.closure_type, return_type: loc_closure_data.value.return_type, + early_returns: loc_closure_data.value.early_returns, captured_symbols: loc_closure_data.value.captured_symbols, arguments: loc_closure_data.value.arguments, }; @@ -3258,6 +3312,7 @@ impl DeclarationTag { pub struct FunctionDef { pub closure_type: Variable, pub return_type: Variable, + pub early_returns: Vec<(Variable, Region)>, pub captured_symbols: Vec<(Symbol, Variable)>, pub arguments: Vec<(Variable, AnnotatedMark, Loc)>, } @@ -3402,6 +3457,9 @@ pub(crate) fn get_lookup_symbols(expr: &Expr) -> Vec { // Intentionally ignore the lookups in the nested `expect` condition itself, // because they couldn't possibly influence the outcome of this `expect`! } + Expr::Return { return_value, .. } => { + stack.push(&return_value.value); + } Expr::Crash { msg, .. } => stack.push(&msg.value), Expr::Num(_, _, _, _) | Expr::Float(_, _, _, _, _) diff --git a/crates/compiler/can/src/module.rs b/crates/compiler/can/src/module.rs index 3a45b6ba912..38276a9fd3a 100644 --- a/crates/compiler/can/src/module.rs +++ b/crates/compiler/can/src/module.rs @@ -369,6 +369,12 @@ pub fn canonicalize_module_defs<'a>( PatternType::TopLevelDef, ); + for (_early_return_var, early_return_region) in &scope.early_returns { + env.problem(Problem::ReturnOutsideOfFunction { + region: *early_return_region, + }); + } + let pending_derives = output.pending_derives; // See if any of the new idents we defined went unused. @@ -425,7 +431,7 @@ pub fn canonicalize_module_defs<'a>( ..Default::default() }; - let (mut declarations, mut output) = crate::def::sort_can_defs_new( + let (mut declarations, mut output) = crate::def::sort_top_level_can_defs( &mut env, &mut scope, var_store, @@ -969,6 +975,14 @@ fn fix_values_captured_in_closure_expr( ); } + Return { return_value, .. } => { + fix_values_captured_in_closure_expr( + &mut return_value.value, + no_capture_symbols, + closure_captures, + ); + } + Crash { msg, ret_var: _ } => { fix_values_captured_in_closure_expr( &mut msg.value, diff --git a/crates/compiler/can/src/scope.rs b/crates/compiler/can/src/scope.rs index 33127f7dd76..47c5fad7746 100644 --- a/crates/compiler/can/src/scope.rs +++ b/crates/compiler/can/src/scope.rs @@ -48,6 +48,8 @@ pub struct Scope { /// Ignored variables (variables that start with an underscore). /// We won't intern them because they're only used during canonicalization for error reporting. ignored_locals: VecMap, + + pub early_returns: Vec<(Variable, Region)>, } impl Scope { @@ -73,6 +75,7 @@ impl Scope { modules: ScopeModules::new(home, module_name), imported_symbols: default_imports, ignored_locals: VecMap::default(), + early_returns: Vec::default(), } } @@ -429,7 +432,7 @@ impl Scope { self.aliases.contains_key(&name) } - pub fn inner_scope(&mut self, f: F) -> T + pub fn inner_scope(&mut self, entering_function: bool, f: F) -> T where F: FnOnce(&mut Scope) -> T, { @@ -446,6 +449,11 @@ impl Scope { let locals_snapshot = self.locals.in_scope.len(); let imported_symbols_snapshot = self.imported_symbols.len(); let imported_modules_snapshot = self.modules.len(); + let early_returns_snapshot = if entering_function { + std::mem::replace(&mut self.early_returns, Vec::new()) + } else { + Vec::new() + }; let result = f(self); @@ -453,6 +461,9 @@ impl Scope { self.ignored_locals.truncate(ignored_locals_count); self.imported_symbols.truncate(imported_symbols_snapshot); self.modules.truncate(imported_modules_snapshot); + if entering_function { + self.early_returns = early_returns_snapshot; + } // anything added in the inner scope is no longer in scope now for i in locals_snapshot..self.locals.in_scope.len() { @@ -882,7 +893,7 @@ mod test { assert!(scope.lookup(&ident, region).is_err()); - scope.inner_scope(|inner| { + scope.inner_scope(false, |inner| { assert!(inner.introduce(ident.clone(), region).is_ok()); }); @@ -943,7 +954,7 @@ mod test { &[ident1.clone(), ident2.clone(), ident3.clone(),] ); - scope.inner_scope(|inner| { + scope.inner_scope(false, |inner| { let ident4 = Ident::from("Ångström"); let ident5 = Ident::from("Sirály"); diff --git a/crates/compiler/can/src/suffixed.rs b/crates/compiler/can/src/suffixed.rs index 8a2900a385d..49a5d82d2f1 100644 --- a/crates/compiler/can/src/suffixed.rs +++ b/crates/compiler/can/src/suffixed.rs @@ -680,7 +680,7 @@ pub fn unwrap_suffixed_expression_defs_help<'a>( }; let maybe_suffixed_value_def = match current_value_def { - Annotation(..) | Dbg{..} | Expect{..} | ExpectFx{..} | Stmt(..) | ModuleImport{..} | IngestedFileImport(_) => None, + Annotation(..) | Dbg{..} | Expect{..} | ExpectFx{..} | Return(_) | Stmt(..) | ModuleImport{..} | IngestedFileImport(_) => None, AnnotatedBody { body_pattern, body_expr, ann_type, ann_pattern, .. } => Some((body_pattern, body_expr, Some((ann_pattern, ann_type)))), Body (def_pattern, def_expr) => Some((def_pattern, def_expr, None)), }; diff --git a/crates/compiler/can/src/task_module.rs b/crates/compiler/can/src/task_module.rs index c494b78f9eb..f762527a478 100644 --- a/crates/compiler/can/src/task_module.rs +++ b/crates/compiler/can/src/task_module.rs @@ -72,6 +72,7 @@ pub fn build_host_exposed_def( function_type: var_store.fresh(), closure_type: var_store.fresh(), return_type: var_store.fresh(), + early_returns: vec![], name: task_closure_symbol, captured_symbols, recursive: Recursive::NotRecursive, @@ -98,6 +99,7 @@ pub fn build_host_exposed_def( function_type: var_store.fresh(), closure_type: var_store.fresh(), return_type: var_store.fresh(), + early_returns: vec![], name: symbol, captured_symbols: std::vec::Vec::new(), recursive: Recursive::NotRecursive, @@ -126,6 +128,7 @@ pub fn build_host_exposed_def( function_type: var_store.fresh(), closure_type: var_store.fresh(), return_type: var_store.fresh(), + early_returns: vec![], name: task_closure_symbol, captured_symbols, recursive: Recursive::NotRecursive, diff --git a/crates/compiler/can/src/traverse.rs b/crates/compiler/can/src/traverse.rs index 9293f457abf..744e1963b7e 100644 --- a/crates/compiler/can/src/traverse.rs +++ b/crates/compiler/can/src/traverse.rs @@ -22,6 +22,10 @@ pub enum DeclarationInfo<'a> { pattern: Pattern, annotation: Option<&'a Annotation>, }, + Return { + loc_expr: &'a Loc, + expr_var: Variable, + }, Expectation { loc_condition: &'a Loc, }, @@ -50,6 +54,7 @@ impl<'a> DeclarationInfo<'a> { loc_expr, .. } => Region::span_across(&loc_symbol.region, &loc_expr.region), + Return { loc_expr, .. } => loc_expr.region, Expectation { loc_condition } => loc_condition.region, Function { loc_symbol, @@ -67,6 +72,7 @@ impl<'a> DeclarationInfo<'a> { fn var(&self) -> Variable { match self { DeclarationInfo::Value { expr_var, .. } => *expr_var, + DeclarationInfo::Return { expr_var, .. } => *expr_var, DeclarationInfo::Expectation { .. } => Variable::BOOL, DeclarationInfo::Function { expr_var, .. } => *expr_var, DeclarationInfo::Destructure { expr_var, .. } => *expr_var, @@ -185,6 +191,9 @@ pub fn walk_decl(visitor: &mut V, decl: DeclarationInfo<'_>) { Expectation { loc_condition } => { visitor.visit_expr(&loc_condition.value, loc_condition.region, Variable::BOOL); } + Return { loc_expr, expr_var } => { + visitor.visit_expr(&loc_expr.value, loc_expr.region, expr_var); + } Function { loc_symbol, loc_body, @@ -403,6 +412,12 @@ pub fn walk_expr(visitor: &mut V, expr: &Expr, var: Variable) { Variable::NULL, ); } + Expr::Return { + return_value, + return_var, + } => { + visitor.visit_expr(&return_value.value, return_value.region, *return_var); + } Expr::TypedHole(_) => { /* terminal */ } Expr::RuntimeError(..) => { /* terminal */ } } diff --git a/crates/compiler/constrain/src/expr.rs b/crates/compiler/constrain/src/expr.rs index 4961cb43db9..48b80189a9b 100644 --- a/crates/compiler/constrain/src/expr.rs +++ b/crates/compiler/constrain/src/expr.rs @@ -113,6 +113,7 @@ fn constrain_untyped_closure( fn_var: Variable, closure_var: Variable, ret_var: Variable, + early_returns: &[(Variable, Region)], arguments: &[(Variable, AnnotatedMark, Loc)], loc_body_expr: &Loc, captured_symbols: &[(Symbol, Variable)], @@ -134,7 +135,12 @@ fn constrain_untyped_closure( vars.push(closure_var); vars.push(fn_var); - let body_type = constraints.push_expected_type(NoExpectation(return_type_index)); + let body_type = constraints.push_expected_type(ForReason( + Reason::Return, + return_type_index, + loc_body_expr.region, + )); + let ret_constraint = constrain_expr( types, constraints, @@ -144,6 +150,21 @@ fn constrain_untyped_closure( body_type, ); + let mut early_return_constraints = Vec::with_capacity(early_returns.len()); + for (early_return_variable, early_return_region) in early_returns { + let early_return_var = constraints.push_variable(*early_return_variable); + let early_return_con = constraints.equal_types( + early_return_var, + body_type, + Category::Return, + *early_return_region, + ); + + early_return_constraints.push(early_return_con); + } + + let early_returns_constraint = constraints.and_constraint(early_return_constraints); + // make sure the captured symbols are sorted! debug_assert_eq!(captured_symbols.to_vec(), { let mut copy = captured_symbols.to_vec(); @@ -185,6 +206,7 @@ fn constrain_untyped_closure( region, fn_var, ), + early_returns_constraint, closure_constraint, ]; @@ -624,6 +646,7 @@ pub fn constrain_expr( function_type: fn_var, closure_type: closure_var, return_type: ret_var, + early_returns, arguments, loc_body: boxed, captured_symbols, @@ -640,6 +663,7 @@ pub fn constrain_expr( *fn_var, *closure_var, *ret_var, + early_returns, arguments, boxed, captured_symbols, @@ -1378,6 +1402,29 @@ pub fn constrain_expr( body_con } + Return { + return_value, + return_var, + } => { + let return_type_index = constraints.push_variable(*return_var); + + let expected_return_value = constraints.push_expected_type(ForReason( + Reason::Return, + return_type_index, + return_value.region, + )); + + let return_con = constrain_expr( + types, + constraints, + env, + return_value.region, + &return_value.value, + expected_return_value, + ); + + constraints.exists([*return_var], return_con) + } Tag { tag_union_var: variant_var, ext_var, @@ -1870,6 +1917,7 @@ fn constrain_function_def( expr_var, function_def.closure_type, function_def.return_type, + &function_def.early_returns, &function_def.arguments, loc_body_expr, &function_def.captured_symbols, @@ -2071,6 +2119,7 @@ fn constrain_function_def( expr_var, function_def.closure_type, function_def.return_type, + &function_def.early_returns, &function_def.arguments, loc_expr, &function_def.captured_symbols, @@ -3651,6 +3700,7 @@ fn constraint_recursive_function( expr_var, function_def.closure_type, function_def.return_type, + &function_def.early_returns, &function_def.arguments, loc_expr, &function_def.captured_symbols, @@ -4133,6 +4183,7 @@ fn is_generalizable_expr(mut expr: &Expr) -> bool { | Expect { .. } | ExpectFx { .. } | Dbg { .. } + | Return { .. } | TypedHole(_) | RuntimeError(..) | ZeroArgumentTag { .. } diff --git a/crates/compiler/derive/src/decoding.rs b/crates/compiler/derive/src/decoding.rs index 500e129c1f6..16d0f8b638f 100644 --- a/crates/compiler/derive/src/decoding.rs +++ b/crates/compiler/derive/src/decoding.rs @@ -147,6 +147,7 @@ fn wrap_in_decode_custom_decode_with( function_type: fn_var, closure_type: fn_clos_var, return_type: decode_with_result_var, + early_returns: vec![], name: fn_name, captured_symbols: sorted_inner_decoder_captures, recursive: Recursive::NotRecursive, diff --git a/crates/compiler/derive/src/decoding/record.rs b/crates/compiler/derive/src/decoding/record.rs index ddca4e80ed5..12540abac3e 100644 --- a/crates/compiler/derive/src/decoding/record.rs +++ b/crates/compiler/derive/src/decoding/record.rs @@ -350,6 +350,7 @@ pub(super) fn step_field( function_type, closure_type, return_type: keep_or_skip_var, + early_returns: vec![], name: step_field_closure, captured_symbols: Vec::new(), recursive: Recursive::NotRecursive, @@ -586,6 +587,7 @@ fn custom_decoder_lambda(env: &mut Env<'_>, args: DecodingFieldArgs) -> (Variabl function_type: this_custom_callback_var, closure_type: custom_callback_lambda_set_var, return_type: custom_callback_ret_var, + early_returns: vec![], name: custom_closure_symbol, captured_symbols: vec![(state_arg_symbol, state_record_var)], recursive: Recursive::NotRecursive, @@ -993,6 +995,7 @@ pub(super) fn finalizer( function_type: function_var, closure_type, return_type: return_type_var, + early_returns: vec![], name: function_symbol, captured_symbols: Vec::new(), recursive: Recursive::NotRecursive, diff --git a/crates/compiler/derive/src/decoding/tuple.rs b/crates/compiler/derive/src/decoding/tuple.rs index 3bfe4be72bc..e9a2ed2a780 100644 --- a/crates/compiler/derive/src/decoding/tuple.rs +++ b/crates/compiler/derive/src/decoding/tuple.rs @@ -556,6 +556,7 @@ fn step_elem( function_type: this_custom_callback_var, closure_type: custom_callback_lambda_set_var, return_type: custom_callback_ret_var, + early_returns: vec![], name: custom_closure_symbol, captured_symbols: vec![(state_arg_symbol, state_record_var)], recursive: Recursive::NotRecursive, @@ -710,6 +711,7 @@ fn step_elem( function_type, closure_type, return_type: keep_or_skip_var, + early_returns: vec![], name: step_elem_closure, captured_symbols: Vec::new(), recursive: Recursive::NotRecursive, @@ -896,6 +898,7 @@ fn finalizer( function_type: function_var, closure_type, return_type: return_type_var, + early_returns: vec![], name: function_symbol, captured_symbols: Vec::new(), recursive: Recursive::NotRecursive, diff --git a/crates/compiler/derive/src/encoding.rs b/crates/compiler/derive/src/encoding.rs index 07102010552..3e711b3ae3f 100644 --- a/crates/compiler/derive/src/encoding.rs +++ b/crates/compiler/derive/src/encoding.rs @@ -188,6 +188,7 @@ fn to_encoder_list(env: &mut Env<'_>, fn_name: Symbol) -> (Expr, Variable) { function_type: to_elem_encoder_fn_var, closure_type: to_elem_encoder_lset, return_type: elem_encoder_var, + early_returns: vec![], name: to_elem_encoder_sym, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -281,6 +282,7 @@ fn to_encoder_list(env: &mut Env<'_>, fn_name: Symbol) -> (Expr, Variable) { function_type: fn_var, closure_type: fn_clos_var, return_type: this_encoder_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -490,6 +492,7 @@ fn to_encoder_record( function_type: fn_var, closure_type: fn_clos_var, return_type: this_encoder_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -672,6 +675,7 @@ fn to_encoder_tuple( function_type: fn_var, closure_type: fn_clos_var, return_type: this_encoder_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -914,6 +918,7 @@ fn to_encoder_tag_union( function_type: fn_var, closure_type: fn_clos_var, return_type: this_encoder_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -1025,6 +1030,7 @@ fn wrap_in_encode_custom( function_type: fn_var, closure_type: fn_clos_var, return_type: Variable::LIST_U8, + early_returns: vec![], name: fn_name, captured_symbols: vec![(captured_symbol, captured_var)], recursive: Recursive::NotRecursive, diff --git a/crates/compiler/derive/src/hash.rs b/crates/compiler/derive/src/hash.rs index ded752e5549..73f2ffcbe04 100644 --- a/crates/compiler/derive/src/hash.rs +++ b/crates/compiler/derive/src/hash.rs @@ -542,6 +542,7 @@ fn build_outer_derived_closure( function_type: fn_var, closure_type: fn_clos_var, return_type: body_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, diff --git a/crates/compiler/derive/src/inspect.rs b/crates/compiler/derive/src/inspect.rs index d51342053c1..cb02dde3cf5 100644 --- a/crates/compiler/derive/src/inspect.rs +++ b/crates/compiler/derive/src/inspect.rs @@ -194,6 +194,7 @@ fn to_inspector_list(env: &mut Env<'_>, fn_name: Symbol) -> (Expr, Variable) { function_type: to_elem_inspector_fn_var, closure_type: to_elem_inspector_lset, return_type: elem_inspector_var, + early_returns: vec![], name: to_elem_inspector_sym, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -292,6 +293,7 @@ fn to_inspector_list(env: &mut Env<'_>, fn_name: Symbol) -> (Expr, Variable) { function_type: fn_var, closure_type: fn_clos_var, return_type: this_inspector_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -502,6 +504,7 @@ fn to_inspector_record( function_type: fn_var, closure_type: fn_clos_var, return_type: this_inspector_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -685,6 +688,7 @@ fn to_inspector_tuple( function_type: fn_var, closure_type: fn_clos_var, return_type: this_inspector_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -931,6 +935,7 @@ fn to_inspector_tag_union( function_type: fn_var, closure_type: fn_clos_var, return_type: this_inspector_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![], recursive: Recursive::NotRecursive, @@ -1029,6 +1034,7 @@ fn wrap_in_inspect_custom( function_type: fn_var, closure_type: fn_clos_var, return_type: fmt_var, + early_returns: vec![], name: fn_name, captured_symbols: vec![(captured_symbol, captured_var)], recursive: Recursive::NotRecursive, diff --git a/crates/compiler/fmt/src/def.rs b/crates/compiler/fmt/src/def.rs index 642dab3f542..4b14a88f3fc 100644 --- a/crates/compiler/fmt/src/def.rs +++ b/crates/compiler/fmt/src/def.rs @@ -423,6 +423,7 @@ impl<'a> Formattable for ValueDef<'a> { ModuleImport(module_import) => module_import.is_multiline(), IngestedFileImport(ingested_file_import) => ingested_file_import.is_multiline(), Stmt(loc_expr) => loc_expr.is_multiline(), + Return(loc_expr) => loc_expr.is_multiline(), } } @@ -464,6 +465,7 @@ impl<'a> Formattable for ValueDef<'a> { ModuleImport(module_import) => module_import.format(buf, indent), IngestedFileImport(ingested_file_import) => ingested_file_import.format(buf, indent), Stmt(loc_expr) => loc_expr.format_with_options(buf, parens, newlines, indent), + Return(loc_expr) => loc_expr.format_with_options(buf, parens, newlines, indent), } } } diff --git a/crates/compiler/fmt/src/expr.rs b/crates/compiler/fmt/src/expr.rs index c3deed750eb..8e4bb031ce5 100644 --- a/crates/compiler/fmt/src/expr.rs +++ b/crates/compiler/fmt/src/expr.rs @@ -14,6 +14,7 @@ use roc_parse::ast::{ }; use roc_parse::ast::{StrLiteral, StrSegment}; use roc_parse::ident::Accessor; +use roc_parse::keyword; use roc_region::all::Loc; impl<'a> Formattable for Expr<'a> { @@ -70,6 +71,9 @@ impl<'a> Formattable for Expr<'a> { LowLevelDbg(_, _, _) => unreachable!( "LowLevelDbg should only exist after desugaring, not during formatting" ), + Return(return_value, after_return) => { + return_value.is_multiline() || after_return.is_some() + } If { if_thens: branches, @@ -452,6 +456,9 @@ impl<'a> Formattable for Expr<'a> { LowLevelDbg(_, _, _) => unreachable!( "LowLevelDbg should only exist after desugaring, not during formatting" ), + Return(return_value, after_return) => { + fmt_return(buf, return_value, &after_return, parens, newlines, indent); + } If { if_thens: branches, final_else, @@ -1064,6 +1071,27 @@ fn fmt_expect<'a>( continuation.format(buf, indent); } +fn fmt_return<'a>( + buf: &mut Buf, + return_value: &'a Loc>, + after_return: &Option<&'a Loc>>, + parens: Parens, + newlines: Newlines, + indent: u16, +) { + buf.ensure_ends_with_newline(); + buf.indent(indent); + buf.push_str(keyword::RETURN); + + buf.spaces(1); + + return_value.format(buf, indent); + + if let Some(after_return) = after_return { + after_return.format_with_options(buf, parens, newlines, indent); + } +} + fn fmt_if<'a>( buf: &mut Buf, branches: &'a [(Loc>, Loc>)], diff --git a/crates/compiler/load_internal/src/docs.rs b/crates/compiler/load_internal/src/docs.rs index 3b4e3c8362b..0c7bb84fde2 100644 --- a/crates/compiler/load_internal/src/docs.rs +++ b/crates/compiler/load_internal/src/docs.rs @@ -281,6 +281,9 @@ fn generate_entry_docs( ValueDef::IngestedFileImport { .. } => { // Don't generate docs for ingested file imports } + ValueDef::Return { .. } => { + // Don't generate docs for `return`s + } ValueDef::Stmt(loc_expr) => { if let roc_parse::ast::Expr::Var { diff --git a/crates/compiler/lower_params/src/lower.rs b/crates/compiler/lower_params/src/lower.rs index 43f312ce733..6ef675eadea 100644 --- a/crates/compiler/lower_params/src/lower.rs +++ b/crates/compiler/lower_params/src/lower.rs @@ -220,6 +220,7 @@ impl<'a> LowerParams<'a> { function_type: _, closure_type: _, return_type: _, + early_returns: _, recursive: _, arguments: _, }) => { @@ -380,6 +381,12 @@ impl<'a> LowerParams<'a> { expr_stack.push(&mut loc_message.value); expr_stack.push(&mut loc_continuation.value); } + Return { + return_value, + return_var: _, + } => { + expr_stack.push(&mut return_value.value); + } RecordAccessor(_) | ImportParams(_, _, None) | ZeroArgumentTag { @@ -532,6 +539,7 @@ impl<'a> LowerParams<'a> { function_type: self.var_store.fresh(), closure_type: self.var_store.fresh(), return_type: self.var_store.fresh(), + early_returns: vec![], name: self.unique_symbol(), captured_symbols, recursive: roc_can::expr::Recursive::NotRecursive, diff --git a/crates/compiler/lower_params/src/type_error.rs b/crates/compiler/lower_params/src/type_error.rs index da16882d7e0..249b5e18275 100644 --- a/crates/compiler/lower_params/src/type_error.rs +++ b/crates/compiler/lower_params/src/type_error.rs @@ -181,7 +181,8 @@ fn remove_for_reason( def_region: _, } | Reason::CrashArg - | Reason::ImportParams(_) => {} + | Reason::ImportParams(_) + | Reason::Return => {} } } diff --git a/crates/compiler/mono/src/ir.rs b/crates/compiler/mono/src/ir.rs index d8b23ad23db..2022cb7f28c 100644 --- a/crates/compiler/mono/src/ir.rs +++ b/crates/compiler/mono/src/ir.rs @@ -5878,6 +5878,28 @@ pub fn with_hole<'a>( } } } + Return { + return_value, + return_var, + } => { + let return_symbol = possible_reuse_symbol_or_specialize( + env, + procs, + layout_cache, + &return_value.value, + return_var, + ); + + assign_to_symbol( + env, + procs, + layout_cache, + return_var, + *return_value, + return_symbol, + Stmt::Ret(return_symbol), + ) + } TypedHole(_) => runtime_error(env, "Hit a blank"), RuntimeError(e) => runtime_error(env, env.arena.alloc(e.runtime_message())), Crash { msg, ret_var: _ } => { diff --git a/crates/compiler/parse/src/ast.rs b/crates/compiler/parse/src/ast.rs index f1860100470..500a8ecaacc 100644 --- a/crates/compiler/parse/src/ast.rs +++ b/crates/compiler/parse/src/ast.rs @@ -519,6 +519,13 @@ pub enum Expr<'a> { &'a [&'a WhenBranch<'a>], ), + Return( + /// The return value + &'a Loc>, + /// The unused code after the return statement + Option<&'a Loc>>, + ), + // Blank Space (e.g. comments, spaces, newlines) before or after an expression. // We preserve this for the formatter; canonicalization ignores it. SpaceBefore(&'a Expr<'a>, &'a [CommentOrNewline<'a>]), @@ -668,6 +675,9 @@ pub fn is_expr_suffixed(expr: &Expr) -> bool { Expr::When(cond, branches) => { is_expr_suffixed(&cond.value) || branches.iter().any(|x| is_when_branch_suffixed(x)) } + Expr::Return(a, b) => { + is_expr_suffixed(&a.value) || b.is_some_and(|loc_b| is_expr_suffixed(&loc_b.value)) + } Expr::SpaceBefore(a, _) => is_expr_suffixed(a), Expr::SpaceAfter(a, _) => is_expr_suffixed(a), Expr::MalformedIdent(_, _) => false, @@ -826,6 +836,8 @@ pub enum ValueDef<'a> { IngestedFileImport(IngestedFileImport<'a>), Stmt(&'a Loc>), + + Return(&'a Loc>), } impl<'a> ValueDef<'a> { @@ -937,6 +949,16 @@ impl<'a, 'b> RecursiveValueDefIter<'a, 'b> { expr_stack.push(&condition.value); expr_stack.push(&cont.value); } + Return(return_value, after_return) => { + if let Some(after_return) = after_return { + expr_stack.reserve(2); + expr_stack.push(&return_value.value); + expr_stack.push(&after_return.value); + } else { + expr_stack.reserve(1); + expr_stack.push(&return_value.value); + } + } Apply(fun, args, _) => { expr_stack.reserve(args.len() + 1); expr_stack.push(&fun.value); @@ -1068,6 +1090,7 @@ impl<'a, 'b> Iterator for RecursiveValueDefIter<'a, 'b> { } } ValueDef::Stmt(loc_expr) => self.push_pending_from_expr(&loc_expr.value), + ValueDef::Return(loc_expr) => self.push_pending_from_expr(&loc_expr.value), ValueDef::Annotation(_, _) | ValueDef::IngestedFileImport(_) => {} } @@ -2463,6 +2486,7 @@ impl<'a> Malformed for Expr<'a> { Dbg => false, DbgStmt(condition, continuation) => condition.is_malformed() || continuation.is_malformed(), LowLevelDbg(_, condition, continuation) => condition.is_malformed() || continuation.is_malformed(), + Return(return_value, after_return) => return_value.is_malformed() || after_return.is_some_and(|ar| ar.is_malformed()), Apply(func, args, _) => func.is_malformed() || args.iter().any(|arg| arg.is_malformed()), BinOps(firsts, last) => firsts.iter().any(|(expr, _)| expr.is_malformed()) || last.is_malformed(), UnaryOp(expr, _) => expr.is_malformed(), @@ -2713,6 +2737,7 @@ impl<'a> Malformed for ValueDef<'a> { annotation, }) => path.is_malformed() || annotation.is_malformed(), ValueDef::Stmt(loc_expr) => loc_expr.is_malformed(), + ValueDef::Return(loc_expr) => loc_expr.is_malformed(), } } } diff --git a/crates/compiler/parse/src/expr.rs b/crates/compiler/parse/src/expr.rs index 20a00f3daa0..a1ffd397353 100644 --- a/crates/compiler/parse/src/expr.rs +++ b/crates/compiler/parse/src/expr.rs @@ -19,7 +19,7 @@ use crate::parser::{ map_with_arena, optional, reset_min_indent, sep_by1, sep_by1_e, set_min_indent, skip_first, skip_second, specialize_err, specialize_err_ref, then, two_bytes, zero_or_more, EClosure, EExpect, EExpr, EIf, EImport, EImportParams, EInParens, EList, ENumber, EPattern, ERecord, - EString, EType, EWhen, Either, ParseResult, Parser, SpaceProblem, + EReturn, EString, EType, EWhen, Either, ParseResult, Parser, SpaceProblem, }; use crate::pattern::closure_param; use crate::state::State; @@ -546,6 +546,7 @@ fn stmt_start<'a>( EExpr::Dbg, dbg_stmt_help(options, preceding_comment) )), + loc(specialize_err(EExpr::Return, return_help(options))), loc(specialize_err(EExpr::Import, map(import(), Stmt::ValueDef))), map( loc(specialize_err(EExpr::Closure, closure_help(options))), @@ -1443,6 +1444,7 @@ fn parse_stmt_operator<'a>( let op_start = loc_op.region.start(); let op_end = loc_op.region.end(); let new_start = state.pos(); + match op { OperatorOrDef::BinOp(BinOp::Minus) if expr_state.end != op_start && op_end == new_start => { parse_negated_term( @@ -2172,6 +2174,7 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result( } } +fn return_help<'a>(options: ExprParseOptions) -> impl Parser<'a, Stmt<'a>, EReturn<'a>> { + (move |arena: &'a Bump, state: State<'a>, min_indent| { + let (_, return_kw, state) = loc(parser::keyword(keyword::RETURN, EReturn::Return)) + .parse(arena, state, min_indent)?; + + let (_, return_value, state) = parse_block( + options, + arena, + state, + true, + EReturn::IndentReturnValue, + EReturn::ReturnValue, + ) + .map_err(|(_, f)| (MadeProgress, f))?; + + let region = Region::span_across(&return_kw.region, &return_value.region); + + let stmt = Stmt::ValueDef(ValueDef::Return( + arena.alloc(Loc::at(region, return_value.value)), + )); + + Ok((MadeProgress, stmt, state)) + }) + .trace("return_help") +} + fn dbg_stmt_help<'a>( options: ExprParseOptions, preceding_comment: Region, @@ -3028,6 +3057,7 @@ fn stmts_to_expr<'a>( CalledVia::Space, ) } + Stmt::ValueDef(ValueDef::Return(return_value)) => Expr::Return(return_value, None), Stmt::ValueDef(ValueDef::Expect { .. }) => { return Err(EExpr::Expect( EExpect::Continuation( @@ -3082,6 +3112,20 @@ fn stmts_to_defs<'a>( last_expr = Some(sp_stmt.item.with_value(e)); } } + Stmt::ValueDef(ValueDef::Return(return_value)) => { + if i == stmts.len() - 1 { + last_expr = Some(Loc::at_zero(Expr::Return(return_value, None))); + } else { + let rest = stmts_to_expr(&stmts[i + 1..], arena)?; + last_expr = Some(Loc::at_zero(Expr::Return( + return_value, + Some(arena.alloc(rest)), + ))); + } + + // don't re-process the rest of the statements, they got consumed by the early return + break; + } Stmt::Backpassing(pats, call) => { if last_expr.is_some() { return Err(EExpr::StmtAfterExpr(sp_stmt.item.region.start())); diff --git a/crates/compiler/parse/src/keyword.rs b/crates/compiler/parse/src/keyword.rs index 88a13875039..056dd2e8e79 100644 --- a/crates/compiler/parse/src/keyword.rs +++ b/crates/compiler/parse/src/keyword.rs @@ -9,6 +9,7 @@ pub const DBG: &str = "dbg"; pub const IMPORT: &str = "import"; pub const EXPECT: &str = "expect"; pub const EXPECT_FX: &str = "expect-fx"; +pub const RETURN: &str = "return"; pub const CRASH: &str = "crash"; // These keywords are valid in imports @@ -21,6 +22,6 @@ pub const WHERE: &str = "where"; // These keywords are valid in headers pub const PLATFORM: &str = "platform"; -pub const KEYWORDS: [&str; 11] = [ - IF, THEN, ELSE, WHEN, AS, IS, DBG, IMPORT, EXPECT, EXPECT_FX, CRASH, +pub const KEYWORDS: [&str; 12] = [ + IF, THEN, ELSE, WHEN, AS, IS, DBG, IMPORT, EXPECT, EXPECT_FX, RETURN, CRASH, ]; diff --git a/crates/compiler/parse/src/lib.rs b/crates/compiler/parse/src/lib.rs index f92899aa89a..0a80414cacb 100644 --- a/crates/compiler/parse/src/lib.rs +++ b/crates/compiler/parse/src/lib.rs @@ -16,7 +16,6 @@ pub mod keyword; pub mod normalize; pub mod number_literal; pub mod pattern; -pub mod problems; pub mod src64; pub mod state; pub mod string_literal; diff --git a/crates/compiler/parse/src/normalize.rs b/crates/compiler/parse/src/normalize.rs index 833aeb13bb0..0bbf337bacf 100644 --- a/crates/compiler/parse/src/normalize.rs +++ b/crates/compiler/parse/src/normalize.rs @@ -3,6 +3,7 @@ use bumpalo::Bump; use roc_module::called_via::{BinOp, UnaryOp}; use roc_region::all::{Loc, Position, Region}; +use crate::parser::EReturn; use crate::{ ast::{ AbilityImpls, AbilityMember, AssignedField, Collection, Defs, Expr, FullAst, Header, @@ -439,6 +440,7 @@ impl<'a> Normalize<'a> for ValueDef<'a> { IngestedFileImport(ingested_file_import.normalize(arena)) } Stmt(loc_expr) => Stmt(arena.alloc(loc_expr.normalize(arena))), + Return(loc_expr) => Return(arena.alloc(loc_expr.normalize(arena))), } } } @@ -756,6 +758,10 @@ impl<'a> Normalize<'a> for Expr<'a> { arena.alloc(a.normalize(arena)), arena.alloc(b.normalize(arena)), ), + Expr::Return(a, b) => Expr::Return( + arena.alloc(a.normalize(arena)), + b.map(|loc_b| &*arena.alloc(loc_b.normalize(arena))), + ), Expr::Apply(a, b, c) => { Expr::Apply(arena.alloc(a.normalize(arena)), b.normalize(arena), c) } @@ -1038,6 +1044,9 @@ impl<'a> Normalize<'a> for EExpr<'a> { EExpr::Expect(inner_err, _pos) => { EExpr::Expect(inner_err.normalize(arena), Position::zero()) } + EExpr::Return(inner_err, _pos) => { + EExpr::Return(inner_err.normalize(arena), Position::zero()) + } EExpr::Dbg(inner_err, _pos) => EExpr::Dbg(inner_err.normalize(arena), Position::zero()), EExpr::Import(inner_err, _pos) => { EExpr::Import(inner_err.normalize(arena), Position::zero()) @@ -1472,6 +1481,20 @@ impl<'a> Normalize<'a> for EExpect<'a> { } } } + +impl<'a> Normalize<'a> for EReturn<'a> { + fn normalize(&self, arena: &'a Bump) -> Self { + match self { + EReturn::Space(inner_err, _) => EReturn::Space(*inner_err, Position::zero()), + EReturn::Return(_) => EReturn::Return(Position::zero()), + EReturn::ReturnValue(inner_err, _) => { + EReturn::ReturnValue(arena.alloc(inner_err.normalize(arena)), Position::zero()) + } + EReturn::IndentReturnValue(_) => EReturn::IndentReturnValue(Position::zero()), + } + } +} + impl<'a> Normalize<'a> for EIf<'a> { fn normalize(&self, arena: &'a Bump) -> Self { match self { diff --git a/crates/compiler/parse/src/parser.rs b/crates/compiler/parse/src/parser.rs index 71ebcf647b5..7d34903666e 100644 --- a/crates/compiler/parse/src/parser.rs +++ b/crates/compiler/parse/src/parser.rs @@ -97,6 +97,7 @@ impl_space_problem! { EPattern<'a>, EProvides<'a>, ERecord<'a>, + EReturn<'a>, ERequires<'a>, EString<'a>, EType<'a>, @@ -337,6 +338,7 @@ pub enum EExpr<'a> { Expect(EExpect<'a>, Position), Dbg(EExpect<'a>, Position), Import(EImport<'a>, Position), + Return(EReturn<'a>, Position), Closure(EClosure<'a>, Position), Underscore(Position), @@ -513,6 +515,14 @@ pub enum EExpect<'a> { IndentCondition(Position), } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum EReturn<'a> { + Space(BadInputError, Position), + Return(Position), + ReturnValue(&'a EExpr<'a>, Position), + IndentReturnValue(Position), +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum EImport<'a> { Import(Position), diff --git a/crates/compiler/parse/src/problems.rs b/crates/compiler/parse/src/problems.rs deleted file mode 100644 index 28c3318e31f..00000000000 --- a/crates/compiler/parse/src/problems.rs +++ /dev/null @@ -1,25 +0,0 @@ -use roc_region::all::Loc; - -pub type Problems = Vec>; - -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum Problem { - // UNICODE CODE POINT - /// TODO Invalid hex code - Unicode code points must be specified using hexadecimal characters (the numbers 0-9 and letters A-F) - NonHexCharsInUnicodeCodePt, - /// TODO Invalid Unicode code point. It must be no more than \\u{10FFFF}. - UnicodeCodePtTooLarge, - InvalidUnicodeCodePt, - MalformedEscapedUnicode, - NoUnicodeDigits, - - // STRING LITERAL - NewlineInLiteral, - Tab, - CarriageReturn, - NullChar, - UnsupportedEscapedChar, - - // NUMBER LITERAL - OutsideSupportedRange, -} diff --git a/crates/compiler/problem/src/can.rs b/crates/compiler/problem/src/can.rs index 31af6a39aa9..92057bd27c8 100644 --- a/crates/compiler/problem/src/can.rs +++ b/crates/compiler/problem/src/can.rs @@ -242,6 +242,12 @@ pub enum Problem { one_occurrence: Region, kind: AliasKind, }, + ReturnOutsideOfFunction { + region: Region, + }, + StatementsAfterReturn { + region: Region, + }, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -323,6 +329,8 @@ impl Problem { Problem::OverAppliedDbg { .. } => RuntimeError, Problem::DefsOnlyUsedInRecursion(_, _) => Warning, Problem::FileProblem { .. } => Fatal, + Problem::ReturnOutsideOfFunction { .. } => Warning, + Problem::StatementsAfterReturn { .. } => Warning, } } @@ -434,6 +442,7 @@ impl Problem { field: region, }) | Problem::RuntimeError(RuntimeError::ReadIngestedFileError { region, .. }) + | Problem::RuntimeError(RuntimeError::ReturnOutsideOfFunction(region)) | Problem::InvalidAliasRigid { region, .. } | Problem::InvalidInterpolation(region) | Problem::InvalidHexadecimal(region) @@ -485,7 +494,9 @@ impl Problem { | Problem::UnappliedCrash { region } | Problem::OverAppliedDbg { region } | Problem::UnappliedDbg { region } - | Problem::DefsOnlyUsedInRecursion(_, region) => Some(*region), + | Problem::DefsOnlyUsedInRecursion(_, region) + | Problem::ReturnOutsideOfFunction { region } + | Problem::StatementsAfterReturn { region } => Some(*region), Problem::RuntimeError(RuntimeError::CircularDef(cycle_entries)) | Problem::BadRecursion(cycle_entries) => { cycle_entries.first().map(|entry| entry.expr_region) @@ -692,6 +703,8 @@ pub enum RuntimeError { }, MalformedSuffixed(Region), + + ReturnOutsideOfFunction(Region), } impl RuntimeError { @@ -740,7 +753,8 @@ impl RuntimeError { record: _, field: region, } - | RuntimeError::ReadIngestedFileError { region, .. } => *region, + | RuntimeError::ReadIngestedFileError { region, .. } + | RuntimeError::ReturnOutsideOfFunction(region) => *region, RuntimeError::InvalidUnicodeCodePt(region) => *region, RuntimeError::UnresolvedTypeVar | RuntimeError::ErroneousType => Region::zero(), RuntimeError::LookupNotInScope { loc_name, .. } => loc_name.region, diff --git a/crates/compiler/types/src/types.rs b/crates/compiler/types/src/types.rs index f6757247c9e..ab5754a82f9 100644 --- a/crates/compiler/types/src/types.rs +++ b/crates/compiler/types/src/types.rs @@ -3425,6 +3425,7 @@ pub enum Reason { }, CrashArg, ImportParams(ModuleId), + Return, } #[derive(PartialEq, Eq, Debug, Clone)] @@ -3474,6 +3475,7 @@ pub enum Category { Expect, Dbg, + Return, Unknown, } diff --git a/crates/language_server/src/analysis/completion/visitor.rs b/crates/language_server/src/analysis/completion/visitor.rs index ad0fb8905c5..83e5a188ba7 100644 --- a/crates/language_server/src/analysis/completion/visitor.rs +++ b/crates/language_server/src/analysis/completion/visitor.rs @@ -214,6 +214,7 @@ impl CompletionVisitor<'_> { DeclarationInfo::Value { expr_var, pattern, .. } => self.patterns(pattern, expr_var), + DeclarationInfo::Return { .. } => vec![], DeclarationInfo::Function { expr_var, pattern, diff --git a/crates/language_server/src/analysis/tokens.rs b/crates/language_server/src/analysis/tokens.rs index 8f645484f61..6c1335405f3 100644 --- a/crates/language_server/src/analysis/tokens.rs +++ b/crates/language_server/src/analysis/tokens.rs @@ -641,6 +641,7 @@ impl IterTokens for ValueDef<'_> { onetoken(Token::Import, import.name.item.region, arena) } ValueDef::Stmt(loc_expr) => loc_expr.iter_tokens(arena), + ValueDef::Return(loc_expr) => loc_expr.iter_tokens(arena), } } } @@ -718,6 +719,11 @@ impl IterTokens for Loc> { Expr::When(e, branches) => (e.iter_tokens(arena).into_iter()) .chain(branches.iter_tokens(arena)) .collect_in(arena), + Expr::Return(ret_expr, after_ret) => ret_expr + .iter_tokens(arena) + .into_iter() + .chain(after_ret.iter_tokens(arena)) + .collect_in(arena), Expr::SpaceBefore(e, _) | Expr::SpaceAfter(e, _) => { Loc::at(region, *e).iter_tokens(arena) } diff --git a/crates/repl_ui/src/repl_state.rs b/crates/repl_ui/src/repl_state.rs index 2b612d832c2..2dee1fb2574 100644 --- a/crates/repl_ui/src/repl_state.rs +++ b/crates/repl_ui/src/repl_state.rs @@ -199,6 +199,9 @@ impl ReplState { ValueDef::ExpectFx { .. } => { todo!("handle receiving an `expect-fx` - what should the repl do for that?") } + ValueDef::Return(_) => { + todo!("handle receiving an `return` - what should the repl do for that?") + } ValueDef::ModuleImport(import) => match import.name.value.package { Some(_) => { todo!("handle importing a module from a package") diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index 7e60ea07294..86f4d0a8af9 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/crates/reporting/src/error/canonicalize.rs @@ -1346,6 +1346,32 @@ pub fn can_problem<'b>( doc = report.doc; title = report.title; } + + Problem::ReturnOutsideOfFunction { region } => { + doc = alloc.stack([ + alloc.concat([ + alloc.reflow("This "), + alloc.keyword("return"), + alloc.reflow(" statement doesn't belong to a function:"), + ]), + alloc.region(lines.convert_region(region), severity), + ]); + + title = "RETURN OUTSIDE OF FUNCTION".to_string(); + } + + Problem::StatementsAfterReturn { region } => { + doc = alloc.stack([ + alloc.concat([ + alloc.reflow("This code won't run because it follows a "), + alloc.keyword("return"), + alloc.reflow(" statement:"), + ]), + alloc.region(lines.convert_region(region), severity), + ]); + + title = "UNREACHABLE CODE".to_string(); + } }; Report { @@ -2522,6 +2548,18 @@ fn pretty_runtime_error<'b>( title = "OPTIONAL FIELD IN RECORD BUILDER"; } + RuntimeError::ReturnOutsideOfFunction(region) => { + doc = alloc.stack([ + alloc.concat([ + alloc.reflow("The "), + alloc.keyword("return"), + alloc.reflow(" keyword can only be used in functions."), + ]), + alloc.region(lines.convert_region(region), severity), + ]); + + title = "RETURN OUTSIDE OF FUNCTION"; + } } (doc, title) diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index 23fed78a231..13e65c416a2 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -918,7 +918,6 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("expect"), alloc.reflow(" condition to evaluate to a "), - alloc.type_str("Bool"), alloc.reflow("—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), @@ -958,7 +957,6 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("if"), alloc.reflow(" condition to evaluate to a "), - alloc.type_str("Bool"), alloc.reflow("—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), @@ -997,7 +995,6 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("if"), alloc.reflow(" guard condition to evaluate to a "), - alloc.type_str("Bool"), alloc.reflow("—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), @@ -1645,6 +1642,44 @@ fn to_expr_report<'b>( unimplemented!("record default field is not implemented yet") } Reason::ImportParams(_) => unreachable!(), + Reason::Return => { + let problem = alloc.concat([ + alloc.text("This "), + alloc.keyword("return"), + alloc.reflow( + " statement doesn't match the return type of its enclosing function:", + ), + ]); + + let comparison = type_comparison( + alloc, + found, + expected_type, + ExpectationContext::Arbitrary, + add_category(alloc, alloc.text("It is"), &category), + alloc.concat([ + alloc.reflow("But I need every "), + alloc.keyword("return"), + alloc.reflow(" statement in that function to return:"), + ]), + None, + ); + + Report { + title: "TYPE MISMATCH".to_string(), + filename, + doc: alloc.stack([ + problem, + alloc.region_with_subregion( + lines.convert_region(region), + lines.convert_region(expr_region), + severity, + ), + comparison, + ]), + severity, + } + } }, } } @@ -1983,6 +2018,15 @@ fn format_category<'b>( alloc.concat([this_is, alloc.text(" a dbg statement")]), alloc.text(" of type:"), ), + Return => ( + alloc.concat([ + this_is, + alloc.reflow(" a "), + alloc.keyword("return"), + alloc.reflow(" statement"), + ]), + alloc.text(" of type:"), + ), } } From 7518a2c5ab0a13a69f357ba85a65f68c87b909f1 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Mon, 21 Oct 2024 03:06:43 -0700 Subject: [PATCH 02/16] Address PR comments, add syntax tests --- crates/compiler/can/src/def.rs | 8 +- crates/compiler/can/src/expr.rs | 25 ++- crates/compiler/can/src/scope.rs | 33 ++-- crates/compiler/constrain/src/expr.rs | 4 +- crates/compiler/fmt/src/expr.rs | 8 +- .../compiler/lower_params/src/type_error.rs | 2 +- .../fail/empty_return.expr.result-ast | 1 + .../snapshots/fail/empty_return.expr.roc | 1 + ...return_as_single_line_expr.expr.result-ast | 1 + .../fail/return_as_single_line_expr.expr.roc | 1 + .../pass/return_in_if.expr.formatted.roc | 10 + .../pass/return_in_if.expr.result-ast | 167 +++++++++++++++++ .../snapshots/pass/return_in_if.expr.roc | 10 + .../return_in_static_def.expr.formatted.roc | 11 ++ .../pass/return_in_static_def.expr.result-ast | 169 +++++++++++++++++ .../pass/return_in_static_def.expr.roc | 12 ++ .../pass/return_in_when.expr.formatted.roc | 11 ++ .../pass/return_in_when.expr.result-ast | 177 ++++++++++++++++++ .../snapshots/pass/return_in_when.expr.roc | 13 ++ .../pass/return_multiline.expr.formatted.roc | 3 + .../pass/return_multiline.expr.result-ast | 45 +++++ .../snapshots/pass/return_multiline.expr.roc | 4 + .../return_only_statement.expr.formatted.roc | 4 + .../return_only_statement.expr.result-ast | 68 +++++++ .../pass/return_only_statement.expr.roc | 5 + .../test_syntax/tests/test_snapshots.rs | 7 + crates/compiler/types/src/types.rs | 2 +- crates/reporting/src/error/type.rs | 4 +- 28 files changed, 772 insertions(+), 34 deletions(-) create mode 100644 crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.formatted.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.formatted.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.formatted.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.formatted.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.formatted.roc create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast create mode 100644 crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.roc diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index 1864c6bb4fa..b740013e4ce 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -2640,7 +2640,13 @@ fn canonicalize_pending_body<'a>( // The closure is self tail recursive iff it tail calls itself (by defined name). let is_recursive = match can_output.tail_call { - Some(tail_symbol) if tail_symbol == *defined_symbol => Recursive::TailRecursive, + Some(tail_symbol) if tail_symbol == *defined_symbol => { + if closure_data.early_returns.is_empty() { + Recursive::TailRecursive + } else { + Recursive::Recursive + } + } _ => Recursive::NotRecursive, }; diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index 7a8b8a5c74b..b3f787e7a66 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -1017,7 +1017,7 @@ pub fn canonicalize_expr<'a>( } ast::Expr::Defs(loc_defs, loc_ret) => { // The body expression gets a new scope for canonicalization, - scope.inner_scope(false, |inner_scope| { + scope.inner_def_scope(|inner_scope| { let defs: Defs = (*loc_defs).clone(); can_defs_with_return(env, var_store, inner_scope, env.arena.alloc(defs), loc_ret) }) @@ -1049,17 +1049,16 @@ pub fn canonicalize_expr<'a>( let mut can_branches = Vec::with_capacity(branches.len()); for branch in branches.iter() { - let (can_when_branch, branch_references) = - scope.inner_scope(false, |inner_scope| { - canonicalize_when_branch( - env, - var_store, - inner_scope, - region, - branch, - &mut output, - ) - }); + let (can_when_branch, branch_references) = scope.inner_def_scope(|inner_scope| { + canonicalize_when_branch( + env, + var_store, + inner_scope, + region, + branch, + &mut output, + ) + }); output.references.union_mut(&branch_references); @@ -1534,7 +1533,7 @@ pub fn canonicalize_closure<'a>( loc_body_expr: &'a Loc>, opt_def_name: Option, ) -> (ClosureData, Output) { - scope.inner_scope(true, |inner_scope| { + scope.inner_function_scope(|inner_scope| { canonicalize_closure_body( env, var_store, diff --git a/crates/compiler/can/src/scope.rs b/crates/compiler/can/src/scope.rs index 47c5fad7746..0119b5fdd5a 100644 --- a/crates/compiler/can/src/scope.rs +++ b/crates/compiler/can/src/scope.rs @@ -432,7 +432,8 @@ impl Scope { self.aliases.contains_key(&name) } - pub fn inner_scope(&mut self, entering_function: bool, f: F) -> T + /// Enter an inner scope within a definition, e.g. a def or when block. + pub fn inner_def_scope(&mut self, f: F) -> T where F: FnOnce(&mut Scope) -> T, { @@ -449,11 +450,6 @@ impl Scope { let locals_snapshot = self.locals.in_scope.len(); let imported_symbols_snapshot = self.imported_symbols.len(); let imported_modules_snapshot = self.modules.len(); - let early_returns_snapshot = if entering_function { - std::mem::replace(&mut self.early_returns, Vec::new()) - } else { - Vec::new() - }; let result = f(self); @@ -461,9 +457,6 @@ impl Scope { self.ignored_locals.truncate(ignored_locals_count); self.imported_symbols.truncate(imported_symbols_snapshot); self.modules.truncate(imported_modules_snapshot); - if entering_function { - self.early_returns = early_returns_snapshot; - } // anything added in the inner scope is no longer in scope now for i in locals_snapshot..self.locals.in_scope.len() { @@ -473,6 +466,20 @@ impl Scope { result } + /// Enter an inner scope within a child function, e.g. a closure body. + pub fn inner_function_scope(&mut self, f: F) -> T + where + F: FnOnce(&mut Scope) -> T, + { + let early_returns_snapshot = std::mem::replace(&mut self.early_returns, Vec::new()); + + let result = self.inner_def_scope(f); + + self.early_returns = early_returns_snapshot; + + result + } + pub fn register_debug_idents(&self) { self.home.register_debug_idents(&self.locals.ident_ids) } @@ -879,7 +886,7 @@ mod test { } #[test] - fn inner_scope_does_not_influence_outer() { + fn inner_def_scope_does_not_influence_outer() { let _register_module_debug_names = ModuleIds::default(); let mut scope = Scope::new( ModuleId::ATTR, @@ -893,7 +900,7 @@ mod test { assert!(scope.lookup(&ident, region).is_err()); - scope.inner_scope(false, |inner| { + scope.inner_def_scope(|inner| { assert!(inner.introduce(ident.clone(), region).is_ok()); }); @@ -919,7 +926,7 @@ mod test { } #[test] - fn idents_with_inner_scope() { + fn idents_with_inner_def_scope() { let _register_module_debug_names = ModuleIds::default(); let mut scope = Scope::new( ModuleId::ATTR, @@ -954,7 +961,7 @@ mod test { &[ident1.clone(), ident2.clone(), ident3.clone(),] ); - scope.inner_scope(false, |inner| { + scope.inner_def_scope(|inner| { let ident4 = Ident::from("Ångström"); let ident5 = Ident::from("Sirály"); diff --git a/crates/compiler/constrain/src/expr.rs b/crates/compiler/constrain/src/expr.rs index 48b80189a9b..d5bcd360087 100644 --- a/crates/compiler/constrain/src/expr.rs +++ b/crates/compiler/constrain/src/expr.rs @@ -136,7 +136,7 @@ fn constrain_untyped_closure( vars.push(fn_var); let body_type = constraints.push_expected_type(ForReason( - Reason::Return, + Reason::FunctionOutput, return_type_index, loc_body_expr.region, )); @@ -1409,7 +1409,7 @@ pub fn constrain_expr( let return_type_index = constraints.push_variable(*return_var); let expected_return_value = constraints.push_expected_type(ForReason( - Reason::Return, + Reason::FunctionOutput, return_type_index, return_value.region, )); diff --git a/crates/compiler/fmt/src/expr.rs b/crates/compiler/fmt/src/expr.rs index 8e4bb031ce5..29257695cb8 100644 --- a/crates/compiler/fmt/src/expr.rs +++ b/crates/compiler/fmt/src/expr.rs @@ -1085,7 +1085,13 @@ fn fmt_return<'a>( buf.spaces(1); - return_value.format(buf, indent); + let return_indent = if return_value.is_multiline() { + indent + INDENT + } else { + indent + }; + + return_value.format(buf, return_indent); if let Some(after_return) = after_return { after_return.format_with_options(buf, parens, newlines, indent); diff --git a/crates/compiler/lower_params/src/type_error.rs b/crates/compiler/lower_params/src/type_error.rs index 249b5e18275..7e2e78479c6 100644 --- a/crates/compiler/lower_params/src/type_error.rs +++ b/crates/compiler/lower_params/src/type_error.rs @@ -182,7 +182,7 @@ fn remove_for_reason( } | Reason::CrashArg | Reason::ImportParams(_) - | Reason::Return => {} + | Reason::FunctionOutput => {} } } diff --git a/crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.result-ast new file mode 100644 index 00000000000..0c853950355 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.result-ast @@ -0,0 +1 @@ +Expr(Return(IndentReturnValue(@6), @0), @0) \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.roc b/crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.roc new file mode 100644 index 00000000000..a09c86384fa --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/fail/empty_return.expr.roc @@ -0,0 +1 @@ +return diff --git a/crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.result-ast new file mode 100644 index 00000000000..83572402651 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.result-ast @@ -0,0 +1 @@ +Expr(Start(@0), @0) \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.roc b/crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.roc new file mode 100644 index 00000000000..0a65218cf81 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/fail/return_as_single_line_expr.expr.roc @@ -0,0 +1 @@ +x = return 5 diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.formatted.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.formatted.roc new file mode 100644 index 00000000000..037e118de30 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.formatted.roc @@ -0,0 +1,10 @@ +maybeEarlyReturn = \x -> + y = + if x > 5 then + return "abc" + else + x + 2 + + Num.toStr y + +maybeEarlyReturn 10 \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast new file mode 100644 index 00000000000..175979cf674 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast @@ -0,0 +1,167 @@ +SpaceAfter( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @0-127, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @0-16 Identifier { + ident: "maybeEarlyReturn", + }, + @19-127 Closure( + [ + @20-21 Identifier { + ident: "x", + }, + ], + @29-127 SpaceBefore( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @29-110, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @29-30 Identifier { + ident: "y", + }, + @41-110 SpaceBefore( + If { + if_thens: [ + ( + @44-49 BinOps( + [ + ( + @44-45 Var { + module_name: "", + ident: "x", + }, + @46-47 GreaterThan, + ), + ], + @48-49 Num( + "5", + ), + ), + @67-79 SpaceBefore( + SpaceAfter( + Return( + @67-79 Str( + PlainLine( + "abc", + ), + ), + None, + ), + [ + Newline, + ], + ), + [ + Newline, + ], + ), + ), + ], + final_else: @105-110 SpaceBefore( + BinOps( + [ + ( + @105-106 Var { + module_name: "", + ident: "x", + }, + @107-108 Plus, + ), + ], + @109-110 Num( + "2", + ), + ), + [ + Newline, + ], + ), + indented_else: false, + }, + [ + Newline, + ], + ), + ), + ], + }, + @116-127 SpaceBefore( + Apply( + @116-125 Var { + module_name: "Num", + ident: "toStr", + }, + [ + @126-127 Var { + module_name: "", + ident: "y", + }, + ], + Space, + ), + [ + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], + ), + ), + ), + ], + }, + @129-148 SpaceBefore( + Apply( + @129-145 Var { + module_name: "", + ident: "maybeEarlyReturn", + }, + [ + @146-148 Num( + "10", + ), + ], + Space, + ), + [ + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], +) diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.roc new file mode 100644 index 00000000000..18a7c88d61b --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.roc @@ -0,0 +1,10 @@ +maybeEarlyReturn = \x -> + y = + if x > 5 then + return "abc" + else + x + 2 + + Num.toStr y + +maybeEarlyReturn 10 diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.formatted.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.formatted.roc new file mode 100644 index 00000000000..abd83147f43 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.formatted.roc @@ -0,0 +1,11 @@ +staticValueDef = + someVal = + if 10 > 5 then + x = 5 + return x + else + 6 + + someVal + 2 + +staticValueDef \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast new file mode 100644 index 00000000000..002f52d75db --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast @@ -0,0 +1,169 @@ +SpaceAfter( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @0-142, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @0-14 Identifier { + ident: "staticValueDef", + }, + @21-142 SpaceBefore( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @21-125, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @21-28 Identifier { + ident: "someVal", + }, + @39-125 SpaceBefore( + If { + if_thens: [ + ( + @42-48 BinOps( + [ + ( + @42-44 Num( + "10", + ), + @45-46 GreaterThan, + ), + ], + @47-48 Num( + "5", + ), + ), + @67-97 SpaceBefore( + SpaceAfter( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @67-72, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @67-68 Identifier { + ident: "x", + }, + @71-72 Num( + "5", + ), + ), + ], + }, + Return( + @86-97 Var { + module_name: "", + ident: "x", + }, + None, + ), + ), + [ + Newline, + ], + ), + [ + Newline, + ], + ), + ), + ], + final_else: @124-125 SpaceBefore( + Num( + "6", + ), + [ + Newline, + ], + ), + indented_else: false, + }, + [ + Newline, + ], + ), + ), + ], + }, + @131-142 SpaceBefore( + BinOps( + [ + ( + @131-138 Var { + module_name: "", + ident: "someVal", + }, + @139-140 Plus, + ), + ], + @141-142 Num( + "2", + ), + ), + [ + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], + ), + ), + ], + }, + @145-159 SpaceBefore( + Var { + module_name: "", + ident: "staticValueDef", + }, + [ + Newline, + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], +) diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.roc new file mode 100644 index 00000000000..142207a63a6 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.roc @@ -0,0 +1,12 @@ +staticValueDef = + someVal = + if 10 > 5 then + x = 5 + return x + else + 6 + + someVal + 2 + + +staticValueDef diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.formatted.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.formatted.roc new file mode 100644 index 00000000000..2fde95e9cf6 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.formatted.roc @@ -0,0 +1,11 @@ +maybeEarlyReturn = \x -> + y = + when x is + 5 -> + return "abc" + + _ -> x + 2 + + Num.toStr y + +maybeEarlyRetun 3 \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast new file mode 100644 index 00000000000..2840fda14aa --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast @@ -0,0 +1,177 @@ +SpaceAfter( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @0-154, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @0-16 Identifier { + ident: "maybeEarlyReturn", + }, + @19-154 Closure( + [ + @20-21 Identifier { + ident: "x", + }, + ], + @29-154 SpaceBefore( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @29-136, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @29-30 Identifier { + ident: "y", + }, + @37-136 SpaceBefore( + When( + @42-43 Var { + module_name: "", + ident: "x", + }, + [ + WhenBranch { + patterns: [ + @55-56 SpaceBefore( + NumLiteral( + "5", + ), + [ + Newline, + ], + ), + ], + value: @80-116 SpaceBefore( + Return( + @80-116 SpaceBefore( + Str( + PlainLine( + "abc", + ), + ), + [ + Newline, + ], + ), + None, + ), + [ + Newline, + ], + ), + guard: None, + }, + WhenBranch { + patterns: [ + @126-127 SpaceBefore( + Underscore( + "", + ), + [ + Newline, + Newline, + ], + ), + ], + value: @131-136 BinOps( + [ + ( + @131-132 Var { + module_name: "", + ident: "x", + }, + @133-134 Plus, + ), + ], + @135-136 Num( + "2", + ), + ), + guard: None, + }, + ], + ), + [ + Newline, + ], + ), + ), + ], + }, + @143-154 SpaceBefore( + Apply( + @143-152 Var { + module_name: "Num", + ident: "toStr", + }, + [ + @153-154 Var { + module_name: "", + ident: "y", + }, + ], + Space, + ), + [ + Newline, + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], + ), + ), + ), + ], + }, + @156-173 SpaceBefore( + Apply( + @156-171 Var { + module_name: "", + ident: "maybeEarlyRetun", + }, + [ + @172-173 Num( + "3", + ), + ], + Space, + ), + [ + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], +) diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.roc new file mode 100644 index 00000000000..5f00323c6c1 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.roc @@ -0,0 +1,13 @@ +maybeEarlyReturn = \x -> + y = + when x is + 5 -> + return + "abc" + + _ -> x + 2 + + + Num.toStr y + +maybeEarlyRetun 3 diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.formatted.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.formatted.roc new file mode 100644 index 00000000000..d617d94c5d8 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.formatted.roc @@ -0,0 +1,3 @@ +return something + |> pipeToFunction + |> andAnother \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.result-ast new file mode 100644 index 00000000000..2d1dc21b698 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.result-ast @@ -0,0 +1,45 @@ +SpaceAfter( + Return( + @0-84 SpaceBefore( + BinOps( + [ + ( + @15-24 SpaceAfter( + Var { + module_name: "", + ident: "something", + }, + [ + Newline, + ], + ), + @37-39 Pizza, + ), + ( + @40-54 SpaceAfter( + Var { + module_name: "", + ident: "pipeToFunction", + }, + [ + Newline, + ], + ), + @71-73 Pizza, + ), + ], + @74-84 Var { + module_name: "", + ident: "andAnother", + }, + ), + [ + Newline, + ], + ), + None, + ), + [ + Newline, + ], +) diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.roc new file mode 100644 index 00000000000..0d137b50fd9 --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_multiline.expr.roc @@ -0,0 +1,4 @@ +return + something + |> pipeToFunction + |> andAnother diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.formatted.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.formatted.roc new file mode 100644 index 00000000000..7c4a392345b --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.formatted.roc @@ -0,0 +1,4 @@ +identityFn = \x -> + return x + +identityFn 45 \ No newline at end of file diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast new file mode 100644 index 00000000000..4cd4c7ee95b --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast @@ -0,0 +1,68 @@ +SpaceAfter( + Defs( + Defs { + tags: [ + Index(2147483648), + ], + regions: [ + @0-33, + ], + space_before: [ + Slice(start = 0, length = 0), + ], + space_after: [ + Slice(start = 0, length = 0), + ], + spaces: [], + type_defs: [], + value_defs: [ + Body( + @0-10 Identifier { + ident: "identityFn", + }, + @13-33 Closure( + [ + @14-15 Identifier { + ident: "x", + }, + ], + @21-33 SpaceBefore( + Return( + @21-33 Var { + module_name: "", + ident: "x", + }, + None, + ), + [ + Newline, + ], + ), + ), + ), + ], + }, + @36-49 SpaceBefore( + Apply( + @36-46 Var { + module_name: "", + ident: "identityFn", + }, + [ + @47-49 Num( + "45", + ), + ], + Space, + ), + [ + Newline, + Newline, + Newline, + ], + ), + ), + [ + Newline, + ], +) diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.roc b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.roc new file mode 100644 index 00000000000..f305aa09b8b --- /dev/null +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.roc @@ -0,0 +1,5 @@ +identityFn = \x -> + return x + + +identityFn 45 diff --git a/crates/compiler/test_syntax/tests/test_snapshots.rs b/crates/compiler/test_syntax/tests/test_snapshots.rs index 05bf4d7e6a9..a24855ee090 100644 --- a/crates/compiler/test_syntax/tests/test_snapshots.rs +++ b/crates/compiler/test_syntax/tests/test_snapshots.rs @@ -196,6 +196,7 @@ mod test_snapshots { fail/double_plus.expr, fail/elm_function_syntax.expr, fail/empty_or_pattern.expr, + fail/empty_return.expr, fail/error_inline_alias_argument_uppercase.expr, fail/error_inline_alias_not_an_alias.expr, fail/error_inline_alias_qualified.expr, @@ -233,6 +234,7 @@ mod test_snapshots { fail/record_type_open.expr, fail/record_type_open_indent.expr, fail/record_type_tab.expr, + fail/return_as_single_line_expr.expr, fail/single_no_end.expr, fail/tab_crash.header, fail/tag_union_end.expr, @@ -463,6 +465,11 @@ mod test_snapshots { pass/record_updater_var_apply.expr, pass/record_with_if.expr, pass/requires_type.header, + pass/return_in_if.expr, + pass/return_in_static_def.expr, + pass/return_in_when.expr, + pass/return_multiline.expr, + pass/return_only_statement.expr, pass/separate_defs.moduledefs, pass/single_arg_closure.expr, pass/single_underscore_closure.expr, diff --git a/crates/compiler/types/src/types.rs b/crates/compiler/types/src/types.rs index ab5754a82f9..8ddf5969211 100644 --- a/crates/compiler/types/src/types.rs +++ b/crates/compiler/types/src/types.rs @@ -3425,7 +3425,7 @@ pub enum Reason { }, CrashArg, ImportParams(ModuleId), - Return, + FunctionOutput, } #[derive(PartialEq, Eq, Debug, Clone)] diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index 13e65c416a2..2a57b3470de 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -918,7 +918,7 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("expect"), alloc.reflow(" condition to evaluate to a "), - alloc.reflow("—either "), + alloc.reflow("Bool—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), alloc.tag("Bool.false".into()), @@ -1642,7 +1642,7 @@ fn to_expr_report<'b>( unimplemented!("record default field is not implemented yet") } Reason::ImportParams(_) => unreachable!(), - Reason::Return => { + Reason::FunctionOutput => { let problem = alloc.concat([ alloc.text("This "), alloc.keyword("return"), From 01369dc6d8a20f6014ed0e415c95bdc451118f40 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Thu, 24 Oct 2024 22:56:03 -0700 Subject: [PATCH 03/16] Add mono tests to validate refcounts with early return --- ...unt_for_usage_after_early_return_in_if.txt | 53 +++++++++++++++++++ ...t_for_usage_after_early_return_in_when.txt | 49 +++++++++++++++++ crates/compiler/test_mono/src/tests.rs | 50 +++++++++++++++++ 3 files changed, 152 insertions(+) create mode 100644 crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_if.txt create mode 100644 crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt diff --git a/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_if.txt b/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_if.txt new file mode 100644 index 00000000000..9486070c43d --- /dev/null +++ b/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_if.txt @@ -0,0 +1,53 @@ +procedure Bool.11 (#Attr.2, #Attr.3): + let Bool.24 : Int1 = lowlevel Eq #Attr.2 #Attr.3; + ret Bool.24; + +procedure Num.19 (#Attr.2, #Attr.3): + let Num.283 : I64 = lowlevel NumAdd #Attr.2 #Attr.3; + ret Num.283; + +procedure Num.96 (#Attr.2): + let Num.282 : Str = lowlevel NumToStr #Attr.2; + ret Num.282; + +procedure Str.3 (#Attr.2, #Attr.3): + let Str.247 : Str = lowlevel StrConcat #Attr.2 #Attr.3; + ret Str.247; + +procedure Test.1 (Test.2): + let Test.3 : Str = CallByName Num.96 Test.2; + joinpoint Test.12 Test.4: + let Test.10 : Str = ", "; + let Test.9 : Str = CallByName Str.3 Test.10 Test.4; + dec Test.4; + let Test.8 : Str = CallByName Str.3 Test.3 Test.9; + dec Test.9; + ret Test.8; + in + let Test.22 : I64 = 1i64; + let Test.20 : Int1 = CallByName Bool.11 Test.2 Test.22; + if Test.20 then + dec Test.3; + let Test.21 : Str = "early 1"; + ret Test.21; + else + let Test.19 : I64 = 1i64; + let Test.18 : I64 = CallByName Num.19 Test.2 Test.19; + let Test.5 : Str = CallByName Num.96 Test.18; + joinpoint Test.14 Test.11: + jump Test.12 Test.11; + in + let Test.17 : I64 = 2i64; + let Test.15 : Int1 = CallByName Bool.11 Test.2 Test.17; + if Test.15 then + dec Test.3; + dec Test.5; + let Test.16 : Str = "early 2"; + ret Test.16; + else + jump Test.14 Test.5; + +procedure Test.0 (): + let Test.7 : I64 = 3i64; + let Test.6 : Str = CallByName Test.1 Test.7; + ret Test.6; diff --git a/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt b/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt new file mode 100644 index 00000000000..31d4e6d9af2 --- /dev/null +++ b/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt @@ -0,0 +1,49 @@ +procedure Num.19 (#Attr.2, #Attr.3): + let Num.283 : I64 = lowlevel NumAdd #Attr.2 #Attr.3; + ret Num.283; + +procedure Num.96 (#Attr.2): + let Num.282 : Str = lowlevel NumToStr #Attr.2; + ret Num.282; + +procedure Str.3 (#Attr.2, #Attr.3): + let Str.247 : Str = lowlevel StrConcat #Attr.2 #Attr.3; + ret Str.247; + +procedure Test.1 (Test.2): + let Test.3 : Str = CallByName Num.96 Test.2; + joinpoint Test.11 Test.4: + let Test.10 : Str = ", "; + let Test.9 : Str = CallByName Str.3 Test.10 Test.4; + dec Test.4; + let Test.8 : Str = CallByName Str.3 Test.3 Test.9; + dec Test.9; + ret Test.8; + in + let Test.23 : I64 = 1i64; + let Test.24 : Int1 = lowlevel Eq Test.23 Test.2; + if Test.24 then + dec Test.3; + let Test.13 : Str = "early 1"; + ret Test.13; + else + let Test.22 : I64 = 1i64; + let Test.21 : I64 = CallByName Num.19 Test.2 Test.22; + let Test.5 : Str = CallByName Num.96 Test.21; + joinpoint Test.15 Test.14: + jump Test.11 Test.14; + in + let Test.19 : I64 = 2i64; + let Test.20 : Int1 = lowlevel Eq Test.19 Test.2; + if Test.20 then + dec Test.3; + dec Test.5; + let Test.17 : Str = "early 2"; + ret Test.17; + else + jump Test.15 Test.5; + +procedure Test.0 (): + let Test.7 : I64 = 3i64; + let Test.6 : Str = CallByName Test.1 Test.7; + ret Test.6; diff --git a/crates/compiler/test_mono/src/tests.rs b/crates/compiler/test_mono/src/tests.rs index 148bb4e417f..19dbd0be593 100644 --- a/crates/compiler/test_mono/src/tests.rs +++ b/crates/compiler/test_mono/src/tests.rs @@ -3672,3 +3672,53 @@ fn issue_6606_2() { " ) } + +#[mono_test] +fn dec_refcount_for_usage_after_early_return_in_if() { + indoc!( + r#" + displayN = \n -> + first = Num.toStr n + second = + if n == 1 then + return "early 1" + else + third = Num.toStr (n + 1) + if n == 2 then + return "early 2" + else + third + + "$(first), $(second)" + + displayN 3 + "# + ) +} + +#[mono_test] +fn dec_refcount_for_usage_after_early_return_in_when() { + indoc!( + r#" + displayN = \n -> + first = Num.toStr n + second = + when n is + 1 -> + return "early 1" + + _ -> + third = Num.toStr (n + 1) + when n is + 2 -> + return "early 2" + + _ -> + third + + "$(first), $(second)" + + displayN 3 + "# + ) +} From e9c096088a08f5423478006e64092bcf30a7a1ba Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Thu, 24 Oct 2024 23:00:37 -0700 Subject: [PATCH 04/16] Remove unnecessary vec size reservation --- crates/compiler/parse/src/ast.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/compiler/parse/src/ast.rs b/crates/compiler/parse/src/ast.rs index 500a8ecaacc..14f2edb6cde 100644 --- a/crates/compiler/parse/src/ast.rs +++ b/crates/compiler/parse/src/ast.rs @@ -955,7 +955,6 @@ impl<'a, 'b> RecursiveValueDefIter<'a, 'b> { expr_stack.push(&return_value.value); expr_stack.push(&after_return.value); } else { - expr_stack.reserve(1); expr_stack.push(&return_value.value); } } From aae173d4ac3f3edacbf9b6397df21c95c00b7e32 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Thu, 24 Oct 2024 23:05:15 -0700 Subject: [PATCH 05/16] Add accidentally removed Bool name in reporting --- crates/reporting/src/error/type.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index 2a57b3470de..f729a7f18d5 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -957,7 +957,7 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("if"), alloc.reflow(" condition to evaluate to a "), - alloc.reflow("—either "), + alloc.reflow("Bool—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), alloc.tag("Bool.false".into()), From ca762127e5be0c146fa25996dea10f205e115c01 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Thu, 24 Oct 2024 23:11:15 -0700 Subject: [PATCH 06/16] Fix formatting and clippy errors --- crates/cli/src/main.rs | 2 +- crates/compiler/can/src/def.rs | 2 +- crates/compiler/can/src/desugar.rs | 2 +- crates/compiler/can/src/scope.rs | 2 +- crates/compiler/fmt/src/expr.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 42df5ee6fbb..a85f4d5d8c1 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -238,7 +238,7 @@ fn main() -> io::Result<()> { current_block = String::new(); } else if in_roc_block { current_block.push_str(&line); - current_block.push_str("\n"); + current_block.push('\n'); } } diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index b740013e4ce..2b3ea550113 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -2800,7 +2800,7 @@ pub fn report_unused_imports( } } -fn decl_to_let_or_return<'a>( +fn decl_to_let_or_return( decl: Declaration, loc_ret: Loc, var_store: &mut VarStore, diff --git a/crates/compiler/can/src/desugar.rs b/crates/compiler/can/src/desugar.rs index 653da47b1c7..bcedc212a6d 100644 --- a/crates/compiler/can/src/desugar.rs +++ b/crates/compiler/can/src/desugar.rs @@ -181,7 +181,7 @@ fn desugar_value_def<'a>( Return(return_expr) => { let desugared_return_expr = &*env.arena.alloc(desugar_expr(env, scope, return_expr)); - Return(&desugared_return_expr) + Return(desugared_return_expr) } } } diff --git a/crates/compiler/can/src/scope.rs b/crates/compiler/can/src/scope.rs index 0119b5fdd5a..fe6608f4ffe 100644 --- a/crates/compiler/can/src/scope.rs +++ b/crates/compiler/can/src/scope.rs @@ -471,7 +471,7 @@ impl Scope { where F: FnOnce(&mut Scope) -> T, { - let early_returns_snapshot = std::mem::replace(&mut self.early_returns, Vec::new()); + let early_returns_snapshot = std::mem::take(&mut self.early_returns); let result = self.inner_def_scope(f); diff --git a/crates/compiler/fmt/src/expr.rs b/crates/compiler/fmt/src/expr.rs index 29257695cb8..74912b5e8bc 100644 --- a/crates/compiler/fmt/src/expr.rs +++ b/crates/compiler/fmt/src/expr.rs @@ -457,7 +457,7 @@ impl<'a> Formattable for Expr<'a> { "LowLevelDbg should only exist after desugaring, not during formatting" ), Return(return_value, after_return) => { - fmt_return(buf, return_value, &after_return, parens, newlines, indent); + fmt_return(buf, return_value, after_return, parens, newlines, indent); } If { if_thens: branches, From 8a0cc10c934e447a8b5e6785b7140bc7c59a80bf Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Thu, 24 Oct 2024 23:31:34 -0700 Subject: [PATCH 07/16] Add test_gen tests for early returns --- crates/compiler/test_gen/src/gen_return.rs | 112 +++++++++++++++++++++ crates/compiler/test_gen/src/tests.rs | 1 + crates/reporting/src/error/type.rs | 7 +- 3 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 crates/compiler/test_gen/src/gen_return.rs diff --git a/crates/compiler/test_gen/src/gen_return.rs b/crates/compiler/test_gen/src/gen_return.rs new file mode 100644 index 00000000000..4fce9e5325f --- /dev/null +++ b/crates/compiler/test_gen/src/gen_return.rs @@ -0,0 +1,112 @@ +#![cfg(not(feature = "gen-wasm"))] + +#[cfg(feature = "gen-llvm")] +use crate::helpers::llvm::assert_evals_to; + +#[cfg(feature = "gen-dev")] +use crate::helpers::dev::assert_evals_to; + +#[allow(unused_imports)] +use indoc::indoc; +#[allow(unused_imports)] +use roc_std::{RocList, RocResult, RocStr, I128, U128}; + +#[test] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))] +fn early_return_nested_ifs() { + assert_evals_to!( + indoc!( + r#" + app "test" provides [main] to "./platform" + + displayN = \n -> + first = Num.toStr n + second = + if n == 1 then + return "early 1" + else + third = Num.toStr (n + 1) + if n == 2 then + return "early 2" + else + third + + "$(first), $(second)" + + main : List Str + main = List.map [1, 2, 3] displayN + "# + ), + RocList::from_slice(&[ + RocStr::from("early 1"), + RocStr::from("early 2"), + RocStr::from("3, 4") + ]), + RocList + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))] +fn early_return_nested_whens() { + assert_evals_to!( + indoc!( + r#" + app "test" provides [main] to "./platform" + + displayN = \n -> + first = Num.toStr n + second = + when n is + 1 -> + return "early 1" + + _ -> + third = Num.toStr (n + 1) + when n is + 2 -> + return "early 2" + + _ -> + third + + "$(first), $(second)" + + main : List Str + main = List.map [1, 2, 3] displayN + "# + ), + RocList::from_slice(&[ + RocStr::from("early 1"), + RocStr::from("early 2"), + RocStr::from("3, 4") + ]), + RocList + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] +fn early_return_solo() { + assert_evals_to!( + r#" + identity = \x -> + return x + + identity "abc" + "#, + RocStr::from("abc"), + RocStr + ); + + assert_evals_to!( + r#" + identity = \x -> + return x + + identity 123 + "#, + 123, + i64 + ); +} diff --git a/crates/compiler/test_gen/src/tests.rs b/crates/compiler/test_gen/src/tests.rs index befc7de4457..41fd4988943 100644 --- a/crates/compiler/test_gen/src/tests.rs +++ b/crates/compiler/test_gen/src/tests.rs @@ -16,6 +16,7 @@ pub mod gen_primitives; pub mod gen_records; pub mod gen_refcount; pub mod gen_result; +pub mod gen_return; pub mod gen_set; pub mod gen_str; pub mod gen_tags; diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index f729a7f18d5..5ecb7271ed2 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -918,7 +918,8 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("expect"), alloc.reflow(" condition to evaluate to a "), - alloc.reflow("Bool—either "), + alloc.type_str("Bool"), + alloc.reflow("—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), alloc.tag("Bool.false".into()), @@ -957,7 +958,8 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("if"), alloc.reflow(" condition to evaluate to a "), - alloc.reflow("Bool—either "), + alloc.type_str("Bool"), + alloc.reflow("—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), alloc.tag("Bool.false".into()), @@ -995,6 +997,7 @@ fn to_expr_report<'b>( alloc.reflow("But I need every "), alloc.keyword("if"), alloc.reflow(" guard condition to evaluate to a "), + alloc.type_str("Bool"), alloc.reflow("—either "), alloc.tag("Bool.true".into()), alloc.reflow(" or "), From 012387b1fffa0872247d259a220f7c451183bf67 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Fri, 25 Oct 2024 04:23:48 -0700 Subject: [PATCH 08/16] Fix tests broken by recent internal syntax changes --- .../pass/return_in_if.expr.result-ast | 12 ++++++------ .../pass/return_in_static_def.expr.result-ast | 18 +++++++++--------- .../pass/return_in_when.expr.result-ast | 12 ++++++------ .../pass/return_only_statement.expr.result-ast | 6 +++--- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast index 175979cf674..410e650571c 100644 --- a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_if.expr.result-ast @@ -2,16 +2,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @0-127, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], @@ -30,16 +30,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @29-110, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast index 002f52d75db..94e49d5f7fa 100644 --- a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_static_def.expr.result-ast @@ -2,16 +2,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @0-142, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], @@ -24,16 +24,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @21-125, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], @@ -64,16 +64,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @67-72, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast index 2840fda14aa..ffca41041bd 100644 --- a/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_in_when.expr.result-ast @@ -2,16 +2,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @0-154, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], @@ -30,16 +30,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @29-136, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], diff --git a/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast index 4cd4c7ee95b..0041535b2d1 100644 --- a/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast +++ b/crates/compiler/test_syntax/tests/snapshots/pass/return_only_statement.expr.result-ast @@ -2,16 +2,16 @@ SpaceAfter( Defs( Defs { tags: [ - Index(2147483648), + EitherIndex(2147483648), ], regions: [ @0-33, ], space_before: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], space_after: [ - Slice(start = 0, length = 0), + Slice { start: 0, length: 0 }, ], spaces: [], type_defs: [], From 03f83a0ba8bef025b0f6c02ecb8c93f5fdecbca2 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Fri, 25 Oct 2024 04:58:33 -0700 Subject: [PATCH 09/16] Update uitest tests --- .../generalization_among_large_recursive_group.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/compiler/uitest/tests/recursion/generalization_among_large_recursive_group.txt b/crates/compiler/uitest/tests/recursion/generalization_among_large_recursive_group.txt index 2b36a017b16..c446d872225 100644 --- a/crates/compiler/uitest/tests/recursion/generalization_among_large_recursive_group.txt +++ b/crates/compiler/uitest/tests/recursion/generalization_among_large_recursive_group.txt @@ -3,22 +3,22 @@ app "test" provides [main] to "./platform" f = \{} -> -#^{-1} <2897><113>{} -<116>[[f(1)]]-> <112>[Ok <2905>{}]<76>* +#^{-1} <2897><114>{} -<117>[[f(1)]]-> <113>[Ok <2905>{}]<77>* when g {} is -# ^ <2887><2905>{} -<2895>[[g(2)]]-> <68>[Ok <2905>{}]<98>* +# ^ <2887><2905>{} -<2895>[[g(2)]]-> <69>[Ok <2905>{}]<99>* _ -> Ok {} g = \{} -> -#^{-1} <2887><2905>{} -<2895>[[g(2)]]-> <68>[Ok <2905>{}]<98>* +#^{-1} <2887><2905>{} -<2895>[[g(2)]]-> <69>[Ok <2905>{}]<99>* when h {} is -# ^ <2892><2905>{} -<2900>[[h(3)]]-> <90>[Ok <2905>{}]<120>* +# ^ <2892><2905>{} -<2900>[[h(3)]]-> <91>[Ok <2905>{}]<121>* _ -> Ok {} h = \{} -> -#^{-1} <2892><2905>{} -<2900>[[h(3)]]-> <90>[Ok <2905>{}]<120>* +#^{-1} <2892><2905>{} -<2900>[[h(3)]]-> <91>[Ok <2905>{}]<121>* when f {} is -# ^ <2897><113>{} -<116>[[f(1)]]-> <112>[Ok <2905>{}]<76>* +# ^ <2897><114>{} -<117>[[f(1)]]-> <113>[Ok <2905>{}]<77>* _ -> Ok {} main = f {} -# ^ <2907><129>{} -<132>[[f(1)]]-> <134>[Ok <2905>{}]<2906>w_a +# ^ <2907><130>{} -<133>[[f(1)]]-> <129>[Ok <2905>{}]<2906>w_a From 6a2ffb2f5a6a81dbe5cb4069d080ce15dc85dfce Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Sat, 26 Oct 2024 04:17:50 -0700 Subject: [PATCH 10/16] Implement most of the recent round of PR feedback --- crates/compiler/can/src/def.rs | 56 +------------------ crates/compiler/can/src/desugar.rs | 12 ---- crates/compiler/can/src/expr.rs | 16 +++++- crates/compiler/can/src/suffixed.rs | 2 +- crates/compiler/fmt/src/def.rs | 2 - crates/compiler/load_internal/src/docs.rs | 3 - crates/compiler/parse/src/ast.rs | 4 -- crates/compiler/parse/src/expr.rs | 32 +++++------ crates/compiler/parse/src/normalize.rs | 1 - crates/compiler/problem/src/can.rs | 13 +++-- crates/language_server/src/analysis/tokens.rs | 1 - crates/repl_ui/src/repl_state.rs | 3 - crates/reporting/src/error/canonicalize.rs | 38 +++++++++---- 13 files changed, 67 insertions(+), 116 deletions(-) diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index 2b3ea550113..260d97241db 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -137,7 +137,6 @@ pub(crate) struct CanDefs { dbgs: OrderDependentStatements, expects: OrderDependentStatements, expects_fx: OrderDependentStatements, - returns: OrderDependentStatements, def_ordering: DefOrdering, aliases: VecMap, } @@ -304,7 +303,6 @@ impl PendingTypeDef<'_> { pub enum Declaration { Declare(Def), DeclareRec(Vec, IllegalCycleMark), - Return(Expr, Region), Builtin(Def), Expects(OrderDependentStatements), ExpectsFx(OrderDependentStatements), @@ -319,7 +317,6 @@ impl Declaration { match self { Declare(_) => 1, DeclareRec(defs, _) => defs.len(), - Return(_, _) => 0, InvalidCycle { .. } => 0, Builtin(_) => 0, Expects(_) => 0, @@ -343,7 +340,6 @@ impl Declaration { expects.regions.first().unwrap(), expects.regions.last().unwrap(), ), - Declaration::Return(_return_expr, return_region) => *return_region, } } } @@ -1141,7 +1137,6 @@ fn canonicalize_value_defs<'a>( let mut pending_dbgs = Vec::with_capacity(value_defs.len()); let mut pending_expects = Vec::with_capacity(value_defs.len()); let mut pending_expect_fx = Vec::with_capacity(value_defs.len()); - let mut pending_returns = Vec::with_capacity(value_defs.len()); let mut imports_introduced = Vec::with_capacity(value_defs.len()); @@ -1164,9 +1159,6 @@ fn canonicalize_value_defs<'a>( PendingValue::ExpectFx(pending_expect) => { pending_expect_fx.push(pending_expect); } - PendingValue::Return(pending_return) => { - pending_returns.push(pending_return); - } PendingValue::ModuleImport(PendingModuleImport { module_id, region, @@ -1246,7 +1238,6 @@ fn canonicalize_value_defs<'a>( let mut dbgs = OrderDependentStatements::with_capacity(pending_dbgs.len()); let mut expects = OrderDependentStatements::with_capacity(pending_expects.len()); let mut expects_fx = OrderDependentStatements::with_capacity(pending_expects.len()); - let mut returns = OrderDependentStatements::with_capacity(pending_returns.len()); for pending in pending_dbgs { let (loc_can_condition, can_output) = canonicalize_expr( @@ -1290,21 +1281,11 @@ fn canonicalize_value_defs<'a>( output.union(can_output); } - for pending in pending_returns { - let (loc_return_expr, can_output) = - canonicalize_expr(env, var_store, scope, pending.region, &pending.value); - - returns.push(loc_return_expr, Region::zero()); - - output.union(can_output); - } - let can_defs = CanDefs { defs, dbgs, expects, expects_fx, - returns, def_ordering, aliases, }; @@ -1713,17 +1694,10 @@ pub(crate) fn sort_top_level_can_defs( dbgs: _, expects, expects_fx, - returns, def_ordering, aliases, } = defs; - for return_region in returns.regions { - env.problem(Problem::ReturnOutsideOfFunction { - region: return_region, - }); - } - // TODO: inefficient, but I want to make this what CanDefs contains in the future let mut defs: Vec<_> = defs.into_iter().map(|x| x.unwrap()).collect(); @@ -1855,7 +1829,6 @@ pub(crate) fn sort_top_level_can_defs( None, ); } - // TODO: do I need to handle returns here? _ => { declarations.push_value_def( Loc::at(def.loc_pattern.region, symbol), @@ -2002,7 +1975,6 @@ pub(crate) fn sort_can_defs( dbgs, expects, expects_fx, - returns, def_ordering, aliases, } = defs; @@ -2140,12 +2112,6 @@ pub(crate) fn sort_can_defs( declarations.push(Declaration::ExpectsFx(expects_fx)); } - if !returns.expressions.is_empty() { - for (return_expr, return_region) in returns.expressions.into_iter().zip(returns.regions) { - declarations.push(Declaration::Return(return_expr, return_region)); - } - } - (declarations, output) } @@ -2773,7 +2739,7 @@ pub fn can_defs_with_return<'a>( let mut loc_expr: Loc = ret_expr; for declaration in declarations.into_iter().rev() { - loc_expr = decl_to_let_or_return(declaration, loc_expr, var_store); + loc_expr = decl_to_let(declaration, loc_expr); } (loc_expr.value, output) @@ -2800,11 +2766,7 @@ pub fn report_unused_imports( } } -fn decl_to_let_or_return( - decl: Declaration, - loc_ret: Loc, - var_store: &mut VarStore, -) -> Loc { +fn decl_to_let<'a>(decl: Declaration, loc_ret: Loc) -> Loc { match decl { Declaration::Declare(def) => { let region = Region::span_across(&def.loc_pattern.region, &loc_ret.region); @@ -2816,17 +2778,6 @@ fn decl_to_let_or_return( let expr = Expr::LetRec(defs, Box::new(loc_ret), cycle_mark); Loc::at(region, expr) } - Declaration::Return(return_expr, return_region) => { - let region = Region::span_across(&return_region, &loc_ret.region); - - let return_var = var_store.fresh(); - let expr = Expr::Return { - return_value: Box::new(Loc::at(return_region, return_expr)), - return_var, - }; - - Loc::at(region, expr) - } Declaration::InvalidCycle(entries) => { Loc::at_zero(Expr::RuntimeError(RuntimeError::CircularDef(entries))) } @@ -3062,7 +3013,6 @@ enum PendingValue<'a> { Dbg(PendingExpectOrDbg<'a>), Expect(PendingExpectOrDbg<'a>), ExpectFx(PendingExpectOrDbg<'a>), - Return(&'a Loc>), ModuleImport(PendingModuleImport<'a>), SignatureDefMismatch, InvalidIngestedFile, @@ -3212,8 +3162,6 @@ fn to_pending_value_def<'a>( preceding_comment: *preceding_comment, }), - Return(return_expr) => PendingValue::Return(return_expr), - ModuleImport(module_import) => { let qualified_module_name: QualifiedModuleName = module_import.name.value.into(); let module_name = qualified_module_name.module.clone(); diff --git a/crates/compiler/can/src/desugar.rs b/crates/compiler/can/src/desugar.rs index bcedc212a6d..e2c75636c71 100644 --- a/crates/compiler/can/src/desugar.rs +++ b/crates/compiler/can/src/desugar.rs @@ -177,12 +177,6 @@ fn desugar_value_def<'a>( body_expr: desugar_expr(env, scope, stmt_expr), } } - - Return(return_expr) => { - let desugared_return_expr = &*env.arena.alloc(desugar_expr(env, scope, return_expr)); - - Return(desugared_return_expr) - } } } @@ -324,12 +318,6 @@ pub fn desugar_value_def_suffixed<'a>(arena: &'a Bump, value_def: ValueDef<'a>) // TODO support desugaring of Dbg and ExpectFx Dbg { .. } | ExpectFx { .. } => value_def, ModuleImport { .. } | IngestedFileImport(_) => value_def, - Return(ret_expr) => match unwrap_suffixed_expression(arena, ret_expr, None) { - Ok(new_ret_expr) => ValueDef::Return(new_ret_expr), - Err(..) => { - internal_error!("Unable to desugar the suffix inside a Return value def"); - } - }, Stmt(..) => { internal_error!( diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index fb5f61add0c..f242d10a5c6 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -1280,8 +1280,11 @@ pub fn canonicalize_expr<'a>( ); if let Some(after_return) = after_return { + let region_with_return = + Region::span_across(&return_expr.region, &after_return.region); + env.problem(Problem::StatementsAfterReturn { - region: after_return.region, + region: region_with_return, }); } @@ -1649,6 +1652,17 @@ fn canonicalize_closure_body<'a>( } } + let final_expr = match &loc_body_expr.value { + Expr::LetRec(_, final_expr, _) | Expr::LetNonRec(_, final_expr) => &final_expr.value, + _ => &loc_body_expr.value, + }; + + if let Expr::Return { return_value, .. } = final_expr { + env.problem(Problem::ReturnAtEndOfFunction { + region: return_value.region, + }); + } + // store the references of this function in the Env. This information is used // when we canonicalize a surrounding def (if it exists) env.closures.insert(symbol, output.references.clone()); diff --git a/crates/compiler/can/src/suffixed.rs b/crates/compiler/can/src/suffixed.rs index 49a5d82d2f1..8a2900a385d 100644 --- a/crates/compiler/can/src/suffixed.rs +++ b/crates/compiler/can/src/suffixed.rs @@ -680,7 +680,7 @@ pub fn unwrap_suffixed_expression_defs_help<'a>( }; let maybe_suffixed_value_def = match current_value_def { - Annotation(..) | Dbg{..} | Expect{..} | ExpectFx{..} | Return(_) | Stmt(..) | ModuleImport{..} | IngestedFileImport(_) => None, + Annotation(..) | Dbg{..} | Expect{..} | ExpectFx{..} | Stmt(..) | ModuleImport{..} | IngestedFileImport(_) => None, AnnotatedBody { body_pattern, body_expr, ann_type, ann_pattern, .. } => Some((body_pattern, body_expr, Some((ann_pattern, ann_type)))), Body (def_pattern, def_expr) => Some((def_pattern, def_expr, None)), }; diff --git a/crates/compiler/fmt/src/def.rs b/crates/compiler/fmt/src/def.rs index 4b14a88f3fc..642dab3f542 100644 --- a/crates/compiler/fmt/src/def.rs +++ b/crates/compiler/fmt/src/def.rs @@ -423,7 +423,6 @@ impl<'a> Formattable for ValueDef<'a> { ModuleImport(module_import) => module_import.is_multiline(), IngestedFileImport(ingested_file_import) => ingested_file_import.is_multiline(), Stmt(loc_expr) => loc_expr.is_multiline(), - Return(loc_expr) => loc_expr.is_multiline(), } } @@ -465,7 +464,6 @@ impl<'a> Formattable for ValueDef<'a> { ModuleImport(module_import) => module_import.format(buf, indent), IngestedFileImport(ingested_file_import) => ingested_file_import.format(buf, indent), Stmt(loc_expr) => loc_expr.format_with_options(buf, parens, newlines, indent), - Return(loc_expr) => loc_expr.format_with_options(buf, parens, newlines, indent), } } } diff --git a/crates/compiler/load_internal/src/docs.rs b/crates/compiler/load_internal/src/docs.rs index 0c7bb84fde2..3b4e3c8362b 100644 --- a/crates/compiler/load_internal/src/docs.rs +++ b/crates/compiler/load_internal/src/docs.rs @@ -281,9 +281,6 @@ fn generate_entry_docs( ValueDef::IngestedFileImport { .. } => { // Don't generate docs for ingested file imports } - ValueDef::Return { .. } => { - // Don't generate docs for `return`s - } ValueDef::Stmt(loc_expr) => { if let roc_parse::ast::Expr::Var { diff --git a/crates/compiler/parse/src/ast.rs b/crates/compiler/parse/src/ast.rs index 3ae8f5603b1..58af74a1ba6 100644 --- a/crates/compiler/parse/src/ast.rs +++ b/crates/compiler/parse/src/ast.rs @@ -837,8 +837,6 @@ pub enum ValueDef<'a> { IngestedFileImport(IngestedFileImport<'a>), Stmt(&'a Loc>), - - Return(&'a Loc>), } impl<'a> ValueDef<'a> { @@ -1090,7 +1088,6 @@ impl<'a, 'b> Iterator for RecursiveValueDefIter<'a, 'b> { } } ValueDef::Stmt(loc_expr) => self.push_pending_from_expr(&loc_expr.value), - ValueDef::Return(loc_expr) => self.push_pending_from_expr(&loc_expr.value), ValueDef::Annotation(_, _) | ValueDef::IngestedFileImport(_) => {} } @@ -2737,7 +2734,6 @@ impl<'a> Malformed for ValueDef<'a> { annotation, }) => path.is_malformed() || annotation.is_malformed(), ValueDef::Stmt(loc_expr) => loc_expr.is_malformed(), - ValueDef::Return(loc_expr) => loc_expr.is_malformed(), } } } diff --git a/crates/compiler/parse/src/expr.rs b/crates/compiler/parse/src/expr.rs index 140c3fe667f..b1868a9c4da 100644 --- a/crates/compiler/parse/src/expr.rs +++ b/crates/compiler/parse/src/expr.rs @@ -2664,8 +2664,9 @@ fn return_help<'a>(options: ExprParseOptions) -> impl Parser<'a, Stmt<'a>, ERetu let region = Region::span_across(&return_kw.region, &return_value.region); - let stmt = Stmt::ValueDef(ValueDef::Return( + let stmt = Stmt::Expr(Expr::Return( arena.alloc(Loc::at(region, return_value.value)), + None, )); Ok((MadeProgress, stmt, state)) @@ -3057,7 +3058,6 @@ fn stmts_to_expr<'a>( CalledVia::Space, ) } - Stmt::ValueDef(ValueDef::Return(return_value)) => Expr::Return(return_value, None), Stmt::ValueDef(ValueDef::Expect { .. }) => { return Err(EExpr::Expect( EExpect::Continuation( @@ -3090,6 +3090,20 @@ fn stmts_to_defs<'a>( while i < stmts.len() { let sp_stmt = stmts[i]; match sp_stmt.item.value { + Stmt::Expr(Expr::Return(return_value, _after_return)) => { + if i == stmts.len() - 1 { + last_expr = Some(Loc::at_zero(Expr::Return(return_value, None))); + } else { + let rest = stmts_to_expr(&stmts[i + 1..], arena)?; + last_expr = Some(Loc::at_zero(Expr::Return( + return_value, + Some(arena.alloc(rest)), + ))); + } + + // don't re-process the rest of the statements, they got consumed by the early return + break; + } Stmt::Expr(e) => { if is_expr_suffixed(&e) && i + 1 < stmts.len() { defs.push_value_def( @@ -3112,20 +3126,6 @@ fn stmts_to_defs<'a>( last_expr = Some(sp_stmt.item.with_value(e)); } } - Stmt::ValueDef(ValueDef::Return(return_value)) => { - if i == stmts.len() - 1 { - last_expr = Some(Loc::at_zero(Expr::Return(return_value, None))); - } else { - let rest = stmts_to_expr(&stmts[i + 1..], arena)?; - last_expr = Some(Loc::at_zero(Expr::Return( - return_value, - Some(arena.alloc(rest)), - ))); - } - - // don't re-process the rest of the statements, they got consumed by the early return - break; - } Stmt::Backpassing(pats, call) => { if last_expr.is_some() { return Err(EExpr::StmtAfterExpr(sp_stmt.item.region.start())); diff --git a/crates/compiler/parse/src/normalize.rs b/crates/compiler/parse/src/normalize.rs index 0bbf337bacf..3ce3ac6dea2 100644 --- a/crates/compiler/parse/src/normalize.rs +++ b/crates/compiler/parse/src/normalize.rs @@ -440,7 +440,6 @@ impl<'a> Normalize<'a> for ValueDef<'a> { IngestedFileImport(ingested_file_import.normalize(arena)) } Stmt(loc_expr) => Stmt(arena.alloc(loc_expr.normalize(arena))), - Return(loc_expr) => Return(arena.alloc(loc_expr.normalize(arena))), } } } diff --git a/crates/compiler/problem/src/can.rs b/crates/compiler/problem/src/can.rs index 92057bd27c8..8591c640b8d 100644 --- a/crates/compiler/problem/src/can.rs +++ b/crates/compiler/problem/src/can.rs @@ -248,6 +248,9 @@ pub enum Problem { StatementsAfterReturn { region: Region, }, + ReturnAtEndOfFunction { + region: Region, + }, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -331,6 +334,7 @@ impl Problem { Problem::FileProblem { .. } => Fatal, Problem::ReturnOutsideOfFunction { .. } => Warning, Problem::StatementsAfterReturn { .. } => Warning, + Problem::ReturnAtEndOfFunction { .. } => Warning, } } @@ -442,7 +446,6 @@ impl Problem { field: region, }) | Problem::RuntimeError(RuntimeError::ReadIngestedFileError { region, .. }) - | Problem::RuntimeError(RuntimeError::ReturnOutsideOfFunction(region)) | Problem::InvalidAliasRigid { region, .. } | Problem::InvalidInterpolation(region) | Problem::InvalidHexadecimal(region) @@ -496,7 +499,8 @@ impl Problem { | Problem::UnappliedDbg { region } | Problem::DefsOnlyUsedInRecursion(_, region) | Problem::ReturnOutsideOfFunction { region } - | Problem::StatementsAfterReturn { region } => Some(*region), + | Problem::StatementsAfterReturn { region } + | Problem::ReturnAtEndOfFunction { region } => Some(*region), Problem::RuntimeError(RuntimeError::CircularDef(cycle_entries)) | Problem::BadRecursion(cycle_entries) => { cycle_entries.first().map(|entry| entry.expr_region) @@ -703,8 +707,6 @@ pub enum RuntimeError { }, MalformedSuffixed(Region), - - ReturnOutsideOfFunction(Region), } impl RuntimeError { @@ -753,8 +755,7 @@ impl RuntimeError { record: _, field: region, } - | RuntimeError::ReadIngestedFileError { region, .. } - | RuntimeError::ReturnOutsideOfFunction(region) => *region, + | RuntimeError::ReadIngestedFileError { region, .. } => *region, RuntimeError::InvalidUnicodeCodePt(region) => *region, RuntimeError::UnresolvedTypeVar | RuntimeError::ErroneousType => Region::zero(), RuntimeError::LookupNotInScope { loc_name, .. } => loc_name.region, diff --git a/crates/language_server/src/analysis/tokens.rs b/crates/language_server/src/analysis/tokens.rs index 6c1335405f3..4c1ed316a41 100644 --- a/crates/language_server/src/analysis/tokens.rs +++ b/crates/language_server/src/analysis/tokens.rs @@ -641,7 +641,6 @@ impl IterTokens for ValueDef<'_> { onetoken(Token::Import, import.name.item.region, arena) } ValueDef::Stmt(loc_expr) => loc_expr.iter_tokens(arena), - ValueDef::Return(loc_expr) => loc_expr.iter_tokens(arena), } } } diff --git a/crates/repl_ui/src/repl_state.rs b/crates/repl_ui/src/repl_state.rs index 2dee1fb2574..2b612d832c2 100644 --- a/crates/repl_ui/src/repl_state.rs +++ b/crates/repl_ui/src/repl_state.rs @@ -199,9 +199,6 @@ impl ReplState { ValueDef::ExpectFx { .. } => { todo!("handle receiving an `expect-fx` - what should the repl do for that?") } - ValueDef::Return(_) => { - todo!("handle receiving an `return` - what should the repl do for that?") - } ValueDef::ModuleImport(import) => match import.name.value.package { Some(_) => { todo!("handle importing a module from a package") diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index 86f4d0a8af9..c606c02308a 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/crates/reporting/src/error/canonicalize.rs @@ -1355,6 +1355,7 @@ pub fn can_problem<'b>( alloc.reflow(" statement doesn't belong to a function:"), ]), alloc.region(lines.convert_region(region), severity), + alloc.reflow("I wouldn't know where to return to if I used it!"), ]); title = "RETURN OUTSIDE OF FUNCTION".to_string(); @@ -1368,10 +1369,35 @@ pub fn can_problem<'b>( alloc.reflow(" statement:"), ]), alloc.region(lines.convert_region(region), severity), + alloc.concat([ + alloc.hint("you can move the "), + alloc.keyword("return"), + alloc.reflow(" statement below this block to make the block run."), + ]), ]); title = "UNREACHABLE CODE".to_string(); } + + Problem::ReturnAtEndOfFunction { region } => { + doc = alloc.stack([ + alloc.concat([ + alloc.reflow("This "), + alloc.keyword("return"), + alloc.reflow(" statement should be an expression instead:"), + ]), + alloc.region(lines.convert_region(region), severity), + alloc.concat([ + alloc.reflow("In expression-based languages like Roc, the last expression in a function is treated like a "), + alloc.keyword("return"), + alloc.reflow(" statement. Even though "), + alloc.keyword("return"), + alloc.reflow(" would work here, just writing an expression is more elegant."), + ]), + ]); + + title = "UNNECESSARY RETURN".to_string(); + } }; Report { @@ -2548,18 +2574,6 @@ fn pretty_runtime_error<'b>( title = "OPTIONAL FIELD IN RECORD BUILDER"; } - RuntimeError::ReturnOutsideOfFunction(region) => { - doc = alloc.stack([ - alloc.concat([ - alloc.reflow("The "), - alloc.keyword("return"), - alloc.reflow(" keyword can only be used in functions."), - ]), - alloc.region(lines.convert_region(region), severity), - ]); - - title = "RETURN OUTSIDE OF FUNCTION"; - } } (doc, title) From a9cd6ac5fad05cf39291ed091605e36766b76c57 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Sat, 26 Oct 2024 06:48:01 -0700 Subject: [PATCH 11/16] Fix tailcalling --- crates/compiler/can/src/def.rs | 48 ++++---- crates/compiler/can/src/desugar.rs | 5 +- crates/compiler/can/src/expr.rs | 87 ++++++++++---- crates/compiler/load/tests/test_reporting.rs | 117 +++++++++++++++++++ 4 files changed, 208 insertions(+), 49 deletions(-) diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index 260d97241db..65e060713aa 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -17,6 +17,7 @@ use crate::expr::ClosureData; use crate::expr::Declarations; use crate::expr::Expr::{self, *}; use crate::expr::StructAccessorData; +use crate::expr::TailCall; use crate::expr::{canonicalize_expr, Output, Recursive}; use crate::pattern::{canonicalize_def_header_pattern, BindingsFromPattern, Pattern}; use crate::procedure::QualifiedReference; @@ -134,31 +135,31 @@ impl Annotation { #[derive(Debug)] pub(crate) struct CanDefs { defs: Vec>, - dbgs: OrderDependentStatements, - expects: OrderDependentStatements, - expects_fx: OrderDependentStatements, + dbgs: ExpectsOrDbgs, + expects: ExpectsOrDbgs, + expects_fx: ExpectsOrDbgs, def_ordering: DefOrdering, aliases: VecMap, } #[derive(Clone, Debug)] -pub struct OrderDependentStatements { - pub expressions: Vec, +pub struct ExpectsOrDbgs { + pub conditions: Vec, pub regions: Vec, pub preceding_comment: Vec, } -impl OrderDependentStatements { +impl ExpectsOrDbgs { fn with_capacity(capacity: usize) -> Self { Self { - expressions: Vec::with_capacity(capacity), + conditions: Vec::with_capacity(capacity), regions: Vec::with_capacity(capacity), preceding_comment: Vec::with_capacity(capacity), } } fn push(&mut self, loc_can_condition: Loc, preceding_comment: Region) { - self.expressions.push(loc_can_condition.value); + self.conditions.push(loc_can_condition.value); self.regions.push(loc_can_condition.region); self.preceding_comment.push(preceding_comment); } @@ -304,8 +305,8 @@ pub enum Declaration { Declare(Def), DeclareRec(Vec, IllegalCycleMark), Builtin(Def), - Expects(OrderDependentStatements), - ExpectsFx(OrderDependentStatements), + Expects(ExpectsOrDbgs), + ExpectsFx(ExpectsOrDbgs), /// If we know a cycle is illegal during canonicalization. /// Otherwise we will try to detect this during solving; see [`IllegalCycleMark`]. InvalidCycle(Vec), @@ -1235,9 +1236,9 @@ fn canonicalize_value_defs<'a>( def_ordering.insert_symbol_references(def_id as u32, &temp_output.references) } - let mut dbgs = OrderDependentStatements::with_capacity(pending_dbgs.len()); - let mut expects = OrderDependentStatements::with_capacity(pending_expects.len()); - let mut expects_fx = OrderDependentStatements::with_capacity(pending_expects.len()); + let mut dbgs = ExpectsOrDbgs::with_capacity(pending_dbgs.len()); + let mut expects = ExpectsOrDbgs::with_capacity(pending_expects.len()); + let mut expects_fx = ExpectsOrDbgs::with_capacity(pending_expects.len()); for pending in pending_dbgs { let (loc_can_condition, can_output) = canonicalize_expr( @@ -1712,7 +1713,7 @@ pub(crate) fn sort_top_level_can_defs( // because of the ordering of declarations, expects should come first because they are // independent, but can rely on all other top-level symbols in the module let it = expects - .expressions + .conditions .into_iter() .zip(expects.regions) .zip(expects.preceding_comment); @@ -1725,7 +1726,7 @@ pub(crate) fn sort_top_level_can_defs( } let it = expects_fx - .expressions + .conditions .into_iter() .zip(expects_fx.regions) .zip(expects_fx.preceding_comment); @@ -2100,15 +2101,15 @@ pub(crate) fn sort_can_defs( } } - if !dbgs.expressions.is_empty() { + if !dbgs.conditions.is_empty() { declarations.push(Declaration::Expects(dbgs)); } - if !expects.expressions.is_empty() { + if !expects.conditions.is_empty() { declarations.push(Declaration::Expects(expects)); } - if !expects_fx.expressions.is_empty() { + if !expects_fx.conditions.is_empty() { declarations.push(Declaration::ExpectsFx(expects_fx)); } @@ -2606,14 +2607,15 @@ fn canonicalize_pending_body<'a>( // The closure is self tail recursive iff it tail calls itself (by defined name). let is_recursive = match can_output.tail_call { - Some(tail_symbol) if tail_symbol == *defined_symbol => { - if closure_data.early_returns.is_empty() { + TailCall::NoneMade => Recursive::NotRecursive, + TailCall::Inconsistent => Recursive::Recursive, + TailCall::CallsTo(tail_symbol) => { + if tail_symbol == *defined_symbol { Recursive::TailRecursive } else { Recursive::Recursive } } - _ => Recursive::NotRecursive, }; closure_data.recursive = is_recursive; @@ -2766,7 +2768,7 @@ pub fn report_unused_imports( } } -fn decl_to_let<'a>(decl: Declaration, loc_ret: Loc) -> Loc { +fn decl_to_let(decl: Declaration, loc_ret: Loc) -> Loc { match decl { Declaration::Declare(def) => { let region = Region::span_across(&def.loc_pattern.region, &loc_ret.region); @@ -2788,7 +2790,7 @@ fn decl_to_let<'a>(decl: Declaration, loc_ret: Loc) -> Loc { Declaration::Expects(expects) => { let mut loc_ret = loc_ret; - let conditions = expects.expressions.into_iter().rev(); + let conditions = expects.conditions.into_iter().rev(); let condition_regions = expects.regions.into_iter().rev(); let expect_regions = expects.preceding_comment.into_iter().rev(); diff --git a/crates/compiler/can/src/desugar.rs b/crates/compiler/can/src/desugar.rs index e2c75636c71..aca84c3ca49 100644 --- a/crates/compiler/can/src/desugar.rs +++ b/crates/compiler/can/src/desugar.rs @@ -1029,11 +1029,10 @@ pub fn desugar_expr<'a>( } Return(return_value, after_return) => { let desugared_return_value = &*env.arena.alloc(desugar_expr(env, scope, return_value)); - let desugared_after_return = - after_return.map(|ar| *env.arena.alloc(desugar_expr(env, scope, ar))); env.arena.alloc(Loc { - value: Return(desugared_return_value, desugared_after_return), + // Do not desugar after_return since it isn't run anyway + value: Return(desugared_return_value, after_return.clone()), region: loc_expr.region, }) } diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index f242d10a5c6..3bccb253988 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -39,20 +39,64 @@ pub type PendingDerives = VecMap>)>; #[derive(Clone, Default, Debug)] pub struct Output { pub references: References, - pub tail_call: Option, + pub tail_call: TailCall, pub introduced_variables: IntroducedVariables, pub aliases: VecMap, pub non_closures: VecSet, pub pending_derives: PendingDerives, } +#[derive(Clone, Copy, Default, Debug)] +pub enum TailCall { + #[default] + NoneMade, + CallsTo(Symbol), + Inconsistent, +} + +impl TailCall { + pub fn for_expr(expr: &Expr) -> Self { + match expr { + Expr::Call(fn_expr, _, _) => match **fn_expr { + ( + _, + Loc { + value: Expr::Var(symbol, _), + .. + }, + _, + _, + ) => Self::CallsTo(symbol), + _ => Self::NoneMade, + }, + _ => Self::NoneMade, + } + } + + pub fn merge(self, other: Self) -> Self { + match self { + TailCall::NoneMade => other, + TailCall::Inconsistent => TailCall::Inconsistent, + TailCall::CallsTo(our_symbol) => match other { + TailCall::NoneMade => TailCall::CallsTo(our_symbol), + TailCall::Inconsistent => TailCall::Inconsistent, + TailCall::CallsTo(other_symbol) => { + if our_symbol == other_symbol { + TailCall::CallsTo(our_symbol) + } else { + TailCall::Inconsistent + } + } + }, + } + } +} + impl Output { pub fn union(&mut self, other: Self) { self.references.union_mut(&other.references); - if let (None, Some(later)) = (self.tail_call, other.tail_call) { - self.tail_call = Some(later); - } + self.tail_call = self.tail_call.merge(other.tail_call); self.introduced_variables .union_owned(other.introduced_variables); @@ -724,7 +768,7 @@ pub fn canonicalize_expr<'a>( let output = Output { references, - tail_call: None, + tail_call: TailCall::NoneMade, ..Default::default() }; @@ -792,7 +836,7 @@ pub fn canonicalize_expr<'a>( let output = Output { references, - tail_call: None, + tail_call: TailCall::NoneMade, ..Default::default() }; @@ -908,18 +952,13 @@ pub fn canonicalize_expr<'a>( output.union(fn_expr_output); - // Default: We're not tail-calling a symbol (by name), we're tail-calling a function value. - output.tail_call = None; + output.tail_call = TailCall::NoneMade; let expr = match fn_expr.value { Var(symbol, _) => { output.references.insert_call(symbol); - // we're tail-calling a symbol by name, check if it's the tail-callable symbol - output.tail_call = match &env.tailcallable_symbol { - Some(tc_sym) if *tc_sym == symbol => Some(symbol), - Some(_) | None => None, - }; + output.tail_call = TailCall::CallsTo(symbol); Call( Box::new(( @@ -1045,7 +1084,7 @@ pub fn canonicalize_expr<'a>( canonicalize_expr(env, var_store, scope, loc_cond.region, &loc_cond.value); // the condition can never be a tail-call - output.tail_call = None; + output.tail_call = TailCall::NoneMade; let mut can_branches = Vec::with_capacity(branches.len()); @@ -1070,7 +1109,7 @@ pub fn canonicalize_expr<'a>( // if code gen mistakenly thinks this is a tail call just because its condition // happened to be one. (The condition gave us our initial output value.) if branches.is_empty() { - output.tail_call = None; + output.tail_call = TailCall::NoneMade; } // Incorporate all three expressions into a combined Output value. @@ -1271,14 +1310,6 @@ pub fn canonicalize_expr<'a>( ast::Expr::Return(return_expr, after_return) => { let mut output = Output::default(); - let (loc_return_expr, output1) = canonicalize_expr( - env, - var_store, - scope, - return_expr.region, - &return_expr.value, - ); - if let Some(after_return) = after_return { let region_with_return = Region::span_across(&return_expr.region, &after_return.region); @@ -1288,8 +1319,18 @@ pub fn canonicalize_expr<'a>( }); } + let (loc_return_expr, output1) = canonicalize_expr( + env, + var_store, + scope, + return_expr.region, + &return_expr.value, + ); + output.union(output1); + output.tail_call = TailCall::for_expr(&loc_return_expr.value); + let return_var = var_store.fresh(); scope.early_returns.push((return_var, return_expr.region)); diff --git a/crates/compiler/load/tests/test_reporting.rs b/crates/compiler/load/tests/test_reporting.rs index ebcbed64a1e..e3fc7a01284 100644 --- a/crates/compiler/load/tests/test_reporting.rs +++ b/crates/compiler/load/tests/test_reporting.rs @@ -14498,4 +14498,121 @@ All branches in an `if` must have the same type! (*)b "### ); + + test_report!( + return_outside_of_function, + indoc!( + r" + someVal = + if 10 > 5 then + x = 5 + return x + else + 6 + + someVal + 2 + " + ), + @r###" + ── RETURN OUTSIDE OF FUNCTION in /code/proj/Main.roc ─────────────────────────── + + This `return` statement doesn't belong to a function: + + 7│ return x + ^^^^^^^^ + + I wouldn't know where to return to if I used it! + "### + ); + + test_report!( + statements_after_return, + indoc!( + r#" + myFunction = \x -> + if x == 2 then + return x + + log! "someData" + useX x 123 + else + x + 5 + + myFunction 2 + "# + ), + @r###" + ── UNREACHABLE CODE in /code/proj/Main.roc ───────────────────────────────────── + + This code won't run because it follows a `return` statement: + + 6│> return x + 7│> + 8│> log! "someData" + 9│> useX x 123 + + Hint: you can move the `return` statement below this block to make the + block run. + "### + ); + + test_report!( + return_at_end_of_function, + indoc!( + r#" + myFunction = \x -> + y = Num.toStr x + + return y + + myFunction 3 + "# + ), + @r###" + ── UNNECESSARY RETURN in /code/proj/Main.roc ─────────────────────────────────── + + This `return` statement should be an expression instead: + + 7│ return y + ^^^^^^^^ + + In expression-based languages like Roc, the last expression in a + function is treated like a `return` statement. Even though `return` would + work here, just writing an expression is more elegant. + "### + ); + + test_report!( + mismatch_early_return_with_function_output, + indoc!( + r#" + myFunction = \x -> + if x == 5 then + return "abc" + else + x + + myFunction 3 + "# + ), + @r###" + ── TYPE MISMATCH in /code/proj/Main.roc ──────────────────────────────────────── + + This `return` statement doesn't match the return type of its enclosing + function: + + 5│ if x == 5 then + 6│> return "abc" + 7│ else + 8│ x + + It is a `return` statement of type: + + Str + + But I need every `return` statement in that function to return: + + Num * + "### + ); } From 33d868117feb96b78f1095d6045136e8608cee29 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Sat, 26 Oct 2024 06:56:32 -0700 Subject: [PATCH 12/16] Fix clippy issue --- crates/compiler/can/src/desugar.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/compiler/can/src/desugar.rs b/crates/compiler/can/src/desugar.rs index aca84c3ca49..c462af089b6 100644 --- a/crates/compiler/can/src/desugar.rs +++ b/crates/compiler/can/src/desugar.rs @@ -1032,7 +1032,7 @@ pub fn desugar_expr<'a>( env.arena.alloc(Loc { // Do not desugar after_return since it isn't run anyway - value: Return(desugared_return_value, after_return.clone()), + value: Return(desugared_return_value, *after_return), region: loc_expr.region, }) } From 346a2d9467b67ac44f57b019dfaa080aa8decaba Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Sat, 26 Oct 2024 07:17:52 -0700 Subject: [PATCH 13/16] Simplify tail-call checks --- crates/compiler/can/src/def.rs | 22 +++---- crates/compiler/can/src/expr.rs | 75 ++++++---------------- crates/compiler/test_gen/src/gen_return.rs | 6 +- 3 files changed, 33 insertions(+), 70 deletions(-) diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index 65e060713aa..bca22c68dc4 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -17,7 +17,6 @@ use crate::expr::ClosureData; use crate::expr::Declarations; use crate::expr::Expr::{self, *}; use crate::expr::StructAccessorData; -use crate::expr::TailCall; use crate::expr::{canonicalize_expr, Output, Recursive}; use crate::pattern::{canonicalize_def_header_pattern, BindingsFromPattern, Pattern}; use crate::procedure::QualifiedReference; @@ -2606,16 +2605,17 @@ fn canonicalize_pending_body<'a>( env.tailcallable_symbol = outer_tailcallable; // The closure is self tail recursive iff it tail calls itself (by defined name). - let is_recursive = match can_output.tail_call { - TailCall::NoneMade => Recursive::NotRecursive, - TailCall::Inconsistent => Recursive::Recursive, - TailCall::CallsTo(tail_symbol) => { - if tail_symbol == *defined_symbol { - Recursive::TailRecursive - } else { - Recursive::Recursive - } - } + let is_recursive = if can_output + .early_tail_calls + .iter() + .chain(std::iter::once(&can_output.final_tail_call)) + .all(|tail_call| { + matches!(tail_call, Some(tail_symbol) if tail_symbol == defined_symbol) + }) + { + Recursive::TailRecursive + } else { + Recursive::NotRecursive }; closure_data.recursive = is_recursive; diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index 3bccb253988..60b4a3cbc89 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -39,64 +39,22 @@ pub type PendingDerives = VecMap>)>; #[derive(Clone, Default, Debug)] pub struct Output { pub references: References, - pub tail_call: TailCall, + pub early_tail_calls: Vec>, + pub final_tail_call: Option, pub introduced_variables: IntroducedVariables, pub aliases: VecMap, pub non_closures: VecSet, pub pending_derives: PendingDerives, } -#[derive(Clone, Copy, Default, Debug)] -pub enum TailCall { - #[default] - NoneMade, - CallsTo(Symbol), - Inconsistent, -} - -impl TailCall { - pub fn for_expr(expr: &Expr) -> Self { - match expr { - Expr::Call(fn_expr, _, _) => match **fn_expr { - ( - _, - Loc { - value: Expr::Var(symbol, _), - .. - }, - _, - _, - ) => Self::CallsTo(symbol), - _ => Self::NoneMade, - }, - _ => Self::NoneMade, - } - } - - pub fn merge(self, other: Self) -> Self { - match self { - TailCall::NoneMade => other, - TailCall::Inconsistent => TailCall::Inconsistent, - TailCall::CallsTo(our_symbol) => match other { - TailCall::NoneMade => TailCall::CallsTo(our_symbol), - TailCall::Inconsistent => TailCall::Inconsistent, - TailCall::CallsTo(other_symbol) => { - if our_symbol == other_symbol { - TailCall::CallsTo(our_symbol) - } else { - TailCall::Inconsistent - } - } - }, - } - } -} - impl Output { pub fn union(&mut self, other: Self) { self.references.union_mut(&other.references); - self.tail_call = self.tail_call.merge(other.tail_call); + self.early_tail_calls.extend(other.early_tail_calls); + if let (None, Some(later)) = (self.final_tail_call, other.final_tail_call) { + self.final_tail_call = Some(later); + } self.introduced_variables .union_owned(other.introduced_variables); @@ -768,7 +726,6 @@ pub fn canonicalize_expr<'a>( let output = Output { references, - tail_call: TailCall::NoneMade, ..Default::default() }; @@ -836,7 +793,6 @@ pub fn canonicalize_expr<'a>( let output = Output { references, - tail_call: TailCall::NoneMade, ..Default::default() }; @@ -952,13 +908,18 @@ pub fn canonicalize_expr<'a>( output.union(fn_expr_output); - output.tail_call = TailCall::NoneMade; + // Default: We're not tail-calling a symbol (by name), we're tail-calling a function value. + output.final_tail_call = None; let expr = match fn_expr.value { Var(symbol, _) => { output.references.insert_call(symbol); - output.tail_call = TailCall::CallsTo(symbol); + // we're tail-calling a symbol by name, check if it's the tail-callable symbol + output.final_tail_call = match &env.tailcallable_symbol { + Some(tc_sym) if *tc_sym == symbol => Some(symbol), + Some(_) | None => None, + }; Call( Box::new(( @@ -1084,7 +1045,7 @@ pub fn canonicalize_expr<'a>( canonicalize_expr(env, var_store, scope, loc_cond.region, &loc_cond.value); // the condition can never be a tail-call - output.tail_call = TailCall::NoneMade; + output.final_tail_call = None; let mut can_branches = Vec::with_capacity(branches.len()); @@ -1109,7 +1070,7 @@ pub fn canonicalize_expr<'a>( // if code gen mistakenly thinks this is a tail call just because its condition // happened to be one. (The condition gave us our initial output value.) if branches.is_empty() { - output.tail_call = TailCall::NoneMade; + output.final_tail_call = None; } // Incorporate all three expressions into a combined Output value. @@ -1319,7 +1280,7 @@ pub fn canonicalize_expr<'a>( }); } - let (loc_return_expr, output1) = canonicalize_expr( + let (loc_return_expr, mut output1) = canonicalize_expr( env, var_store, scope, @@ -1327,9 +1288,9 @@ pub fn canonicalize_expr<'a>( &return_expr.value, ); - output.union(output1); + output1.early_tail_calls.push(output1.final_tail_call); - output.tail_call = TailCall::for_expr(&loc_return_expr.value); + output.union(output1); let return_var = var_store.fresh(); diff --git a/crates/compiler/test_gen/src/gen_return.rs b/crates/compiler/test_gen/src/gen_return.rs index 4fce9e5325f..ce9365e8daf 100644 --- a/crates/compiler/test_gen/src/gen_return.rs +++ b/crates/compiler/test_gen/src/gen_return.rs @@ -96,7 +96,8 @@ fn early_return_solo() { identity "abc" "#, RocStr::from("abc"), - RocStr + RocStr, + true ); assert_evals_to!( @@ -107,6 +108,7 @@ fn early_return_solo() { identity 123 "#, 123, - i64 + i64, + true ); } From c19cfb0a3411293517126df93e684d96bbac1f85 Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Sat, 26 Oct 2024 16:58:21 -0700 Subject: [PATCH 14/16] Fix test_gen incorrect macro invocations --- crates/compiler/test_gen/src/gen_return.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/compiler/test_gen/src/gen_return.rs b/crates/compiler/test_gen/src/gen_return.rs index ce9365e8daf..335423908d3 100644 --- a/crates/compiler/test_gen/src/gen_return.rs +++ b/crates/compiler/test_gen/src/gen_return.rs @@ -6,6 +6,12 @@ use crate::helpers::llvm::assert_evals_to; #[cfg(feature = "gen-dev")] use crate::helpers::dev::assert_evals_to; +#[cfg(feature = "gen-llvm")] +use crate::helpers::llvm::identity; + +#[cfg(feature = "gen-dev")] +use crate::helpers::dev::identity; + #[allow(unused_imports)] use indoc::indoc; #[allow(unused_imports)] @@ -86,7 +92,7 @@ fn early_return_nested_whens() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))] fn early_return_solo() { assert_evals_to!( r#" @@ -97,6 +103,7 @@ fn early_return_solo() { "#, RocStr::from("abc"), RocStr, + identity, true ); @@ -109,6 +116,7 @@ fn early_return_solo() { "#, 123, i64, + identity, true ); } From 66cc96edbbc7d3a91e503c5de569d5a71c1f9c9f Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Tue, 29 Oct 2024 15:40:43 -0700 Subject: [PATCH 15/16] Address PR comments from @ayazhafiz --- crates/compiler/can/src/def.rs | 12 ++--- crates/compiler/can/src/expr.rs | 38 +++++++------- crates/compiler/load/tests/test_reporting.rs | 13 +++-- crates/compiler/test_gen/src/gen_return.rs | 13 ----- ...t_for_usage_after_early_return_in_when.txt | 49 ------------------- crates/compiler/test_mono/src/tests.rs | 27 ---------- crates/reporting/src/error/canonicalize.rs | 12 +++-- crates/reporting/src/error/type.rs | 17 ++----- 8 files changed, 40 insertions(+), 141 deletions(-) delete mode 100644 crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index bca22c68dc4..31e9c1330f8 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -2605,13 +2605,11 @@ fn canonicalize_pending_body<'a>( env.tailcallable_symbol = outer_tailcallable; // The closure is self tail recursive iff it tail calls itself (by defined name). - let is_recursive = if can_output - .early_tail_calls - .iter() - .chain(std::iter::once(&can_output.final_tail_call)) - .all(|tail_call| { - matches!(tail_call, Some(tail_symbol) if tail_symbol == defined_symbol) - }) + let is_recursive = if !can_output.tail_calls.is_empty() + && can_output + .tail_calls + .iter() + .all(|tail_symbol| tail_symbol == defined_symbol) { Recursive::TailRecursive } else { diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index 60b4a3cbc89..349f6339c54 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -39,8 +39,7 @@ pub type PendingDerives = VecMap>)>; #[derive(Clone, Default, Debug)] pub struct Output { pub references: References, - pub early_tail_calls: Vec>, - pub final_tail_call: Option, + pub tail_calls: Vec, pub introduced_variables: IntroducedVariables, pub aliases: VecMap, pub non_closures: VecSet, @@ -51,9 +50,8 @@ impl Output { pub fn union(&mut self, other: Self) { self.references.union_mut(&other.references); - self.early_tail_calls.extend(other.early_tail_calls); - if let (None, Some(later)) = (self.final_tail_call, other.final_tail_call) { - self.final_tail_call = Some(later); + if self.tail_calls.is_empty() && !other.tail_calls.is_empty() { + self.tail_calls = other.tail_calls; } self.introduced_variables @@ -909,17 +907,19 @@ pub fn canonicalize_expr<'a>( output.union(fn_expr_output); // Default: We're not tail-calling a symbol (by name), we're tail-calling a function value. - output.final_tail_call = None; + output.tail_calls = vec![]; let expr = match fn_expr.value { Var(symbol, _) => { output.references.insert_call(symbol); // we're tail-calling a symbol by name, check if it's the tail-callable symbol - output.final_tail_call = match &env.tailcallable_symbol { - Some(tc_sym) if *tc_sym == symbol => Some(symbol), - Some(_) | None => None, - }; + if env + .tailcallable_symbol + .is_some_and(|tc_sym| tc_sym == symbol) + { + output.tail_calls.push(symbol); + } Call( Box::new(( @@ -1045,7 +1045,7 @@ pub fn canonicalize_expr<'a>( canonicalize_expr(env, var_store, scope, loc_cond.region, &loc_cond.value); // the condition can never be a tail-call - output.final_tail_call = None; + output.tail_calls = vec![]; let mut can_branches = Vec::with_capacity(branches.len()); @@ -1070,7 +1070,7 @@ pub fn canonicalize_expr<'a>( // if code gen mistakenly thinks this is a tail call just because its condition // happened to be one. (The condition gave us our initial output value.) if branches.is_empty() { - output.final_tail_call = None; + output.tail_calls = vec![]; } // Incorporate all three expressions into a combined Output value. @@ -1280,7 +1280,7 @@ pub fn canonicalize_expr<'a>( }); } - let (loc_return_expr, mut output1) = canonicalize_expr( + let (loc_return_expr, output1) = canonicalize_expr( env, var_store, scope, @@ -1288,8 +1288,6 @@ pub fn canonicalize_expr<'a>( &return_expr.value, ); - output1.early_tail_calls.push(output1.final_tail_call); - output.union(output1); let return_var = var_store.fresh(); @@ -1654,12 +1652,12 @@ fn canonicalize_closure_body<'a>( } } - let final_expr = match &loc_body_expr.value { - Expr::LetRec(_, final_expr, _) | Expr::LetNonRec(_, final_expr) => &final_expr.value, - _ => &loc_body_expr.value, - }; + let mut final_expr = &loc_body_expr; + while let Expr::LetRec(_, inner, _) | Expr::LetNonRec(_, inner) = &final_expr.value { + final_expr = &*inner; + } - if let Expr::Return { return_value, .. } = final_expr { + if let Expr::Return { return_value, .. } = &final_expr.value { env.problem(Problem::ReturnAtEndOfFunction { region: return_value.region, }); diff --git a/crates/compiler/load/tests/test_reporting.rs b/crates/compiler/load/tests/test_reporting.rs index e3fc7a01284..de94fb3ec13 100644 --- a/crates/compiler/load/tests/test_reporting.rs +++ b/crates/compiler/load/tests/test_reporting.rs @@ -14552,7 +14552,7 @@ All branches in an `if` must have the same type! 9│> useX x 123 Hint: you can move the `return` statement below this block to make the - block run. + code that follows it run. "### ); @@ -14571,14 +14571,13 @@ All branches in an `if` must have the same type! @r###" ── UNNECESSARY RETURN in /code/proj/Main.roc ─────────────────────────────────── - This `return` statement should be an expression instead: + This `return` keyword is redundant: 7│ return y ^^^^^^^^ - In expression-based languages like Roc, the last expression in a - function is treated like a `return` statement. Even though `return` would - work here, just writing an expression is more elegant. + The last expression in a function is treated like a `return` statement. + You can safely remove `return` here. "### ); @@ -14606,11 +14605,11 @@ All branches in an `if` must have the same type! 7│ else 8│ x - It is a `return` statement of type: + This returns a value of type: Str - But I need every `return` statement in that function to return: + But I expected the function to have return type: Num * "### diff --git a/crates/compiler/test_gen/src/gen_return.rs b/crates/compiler/test_gen/src/gen_return.rs index 335423908d3..c2cb6d4c1cc 100644 --- a/crates/compiler/test_gen/src/gen_return.rs +++ b/crates/compiler/test_gen/src/gen_return.rs @@ -106,17 +106,4 @@ fn early_return_solo() { identity, true ); - - assert_evals_to!( - r#" - identity = \x -> - return x - - identity 123 - "#, - 123, - i64, - identity, - true - ); } diff --git a/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt b/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt deleted file mode 100644 index 31d4e6d9af2..00000000000 --- a/crates/compiler/test_mono/generated/dec_refcount_for_usage_after_early_return_in_when.txt +++ /dev/null @@ -1,49 +0,0 @@ -procedure Num.19 (#Attr.2, #Attr.3): - let Num.283 : I64 = lowlevel NumAdd #Attr.2 #Attr.3; - ret Num.283; - -procedure Num.96 (#Attr.2): - let Num.282 : Str = lowlevel NumToStr #Attr.2; - ret Num.282; - -procedure Str.3 (#Attr.2, #Attr.3): - let Str.247 : Str = lowlevel StrConcat #Attr.2 #Attr.3; - ret Str.247; - -procedure Test.1 (Test.2): - let Test.3 : Str = CallByName Num.96 Test.2; - joinpoint Test.11 Test.4: - let Test.10 : Str = ", "; - let Test.9 : Str = CallByName Str.3 Test.10 Test.4; - dec Test.4; - let Test.8 : Str = CallByName Str.3 Test.3 Test.9; - dec Test.9; - ret Test.8; - in - let Test.23 : I64 = 1i64; - let Test.24 : Int1 = lowlevel Eq Test.23 Test.2; - if Test.24 then - dec Test.3; - let Test.13 : Str = "early 1"; - ret Test.13; - else - let Test.22 : I64 = 1i64; - let Test.21 : I64 = CallByName Num.19 Test.2 Test.22; - let Test.5 : Str = CallByName Num.96 Test.21; - joinpoint Test.15 Test.14: - jump Test.11 Test.14; - in - let Test.19 : I64 = 2i64; - let Test.20 : Int1 = lowlevel Eq Test.19 Test.2; - if Test.20 then - dec Test.3; - dec Test.5; - let Test.17 : Str = "early 2"; - ret Test.17; - else - jump Test.15 Test.5; - -procedure Test.0 (): - let Test.7 : I64 = 3i64; - let Test.6 : Str = CallByName Test.1 Test.7; - ret Test.6; diff --git a/crates/compiler/test_mono/src/tests.rs b/crates/compiler/test_mono/src/tests.rs index 19dbd0be593..87d5ed80181 100644 --- a/crates/compiler/test_mono/src/tests.rs +++ b/crates/compiler/test_mono/src/tests.rs @@ -3695,30 +3695,3 @@ fn dec_refcount_for_usage_after_early_return_in_if() { "# ) } - -#[mono_test] -fn dec_refcount_for_usage_after_early_return_in_when() { - indoc!( - r#" - displayN = \n -> - first = Num.toStr n - second = - when n is - 1 -> - return "early 1" - - _ -> - third = Num.toStr (n + 1) - when n is - 2 -> - return "early 2" - - _ -> - third - - "$(first), $(second)" - - displayN 3 - "# - ) -} diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index c606c02308a..f8e7865d31e 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/crates/reporting/src/error/canonicalize.rs @@ -1372,7 +1372,9 @@ pub fn can_problem<'b>( alloc.concat([ alloc.hint("you can move the "), alloc.keyword("return"), - alloc.reflow(" statement below this block to make the block run."), + alloc.reflow( + " statement below this block to make the code that follows it run.", + ), ]), ]); @@ -1384,15 +1386,15 @@ pub fn can_problem<'b>( alloc.concat([ alloc.reflow("This "), alloc.keyword("return"), - alloc.reflow(" statement should be an expression instead:"), + alloc.reflow(" keyword is redundant:"), ]), alloc.region(lines.convert_region(region), severity), alloc.concat([ - alloc.reflow("In expression-based languages like Roc, the last expression in a function is treated like a "), + alloc.reflow("The last expression in a function is treated like a "), alloc.keyword("return"), - alloc.reflow(" statement. Even though "), + alloc.reflow(" statement. You can safely remove "), alloc.keyword("return"), - alloc.reflow(" would work here, just writing an expression is more elegant."), + alloc.reflow(" here."), ]), ]); diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index 5ecb7271ed2..33a04f48105 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -1659,12 +1659,8 @@ fn to_expr_report<'b>( found, expected_type, ExpectationContext::Arbitrary, - add_category(alloc, alloc.text("It is"), &category), - alloc.concat([ - alloc.reflow("But I need every "), - alloc.keyword("return"), - alloc.reflow(" statement in that function to return:"), - ]), + add_category(alloc, alloc.text("It"), &category), + alloc.reflow("But I expected the function to have return type:"), None, ); @@ -1994,7 +1990,7 @@ fn format_category<'b>( } Uniqueness => ( - alloc.concat([this_is, alloc.text(" an uniqueness attribute")]), + alloc.concat([this_is, alloc.text(" a uniqueness attribute")]), alloc.text(" of type:"), ), Crash => { @@ -2022,12 +2018,7 @@ fn format_category<'b>( alloc.text(" of type:"), ), Return => ( - alloc.concat([ - this_is, - alloc.reflow(" a "), - alloc.keyword("return"), - alloc.reflow(" statement"), - ]), + alloc.concat([text!(alloc, "{}his", t), alloc.reflow(" returns a value")]), alloc.text(" of type:"), ), } From facad9943d349220789073a906b5251e023c4f2a Mon Sep 17 00:00:00 2001 From: Sam Mohr Date: Tue, 29 Oct 2024 20:30:27 -0700 Subject: [PATCH 16/16] Fix clippy issue --- crates/compiler/can/src/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index 349f6339c54..50cd40253a1 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -1654,7 +1654,7 @@ fn canonicalize_closure_body<'a>( let mut final_expr = &loc_body_expr; while let Expr::LetRec(_, inner, _) | Expr::LetNonRec(_, inner) = &final_expr.value { - final_expr = &*inner; + final_expr = inner; } if let Expr::Return { return_value, .. } = &final_expr.value {