From d22acb521efaa5ea94bbfcb2844981dd7e0ee93b Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 15:07:15 +0100 Subject: [PATCH 01/14] use record for PrecedenceConflict --- compiler/can/src/expr.rs | 7 ++++++- compiler/can/src/operator.rs | 16 ++++++++-------- compiler/fmt/src/expr.rs | 9 +++++---- compiler/parse/src/ast.rs | 7 ++++++- compiler/parse/src/expr.rs | 2 +- editor/src/lang/expr.rs | 2 +- 6 files changed, 27 insertions(+), 16 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index f3787ccce8..71c16026b9 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -720,7 +720,12 @@ pub fn canonicalize_expr<'a>( ) } - ast::Expr::PrecedenceConflict(whole_region, binop1, binop2, _expr) => { + ast::Expr::PrecedenceConflict { + whole_region, + binop1, + binop2, + expr: _, + } => { use roc_problem::can::RuntimeError::*; let problem = PrecedenceProblem::BothNonAssociative(*whole_region, *binop1, *binop2); diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 2d03a4dccd..7749ba25d5 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -117,8 +117,8 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a | Nested(MalformedIdent(_, _)) | MalformedClosure | Nested(MalformedClosure) - | PrecedenceConflict(_, _, _, _) - | Nested(PrecedenceConflict(_, _, _, _)) + | PrecedenceConflict { .. } + | Nested(PrecedenceConflict { .. }) | GlobalTag(_) | Nested(GlobalTag(_)) | PrivateTag(_) @@ -517,12 +517,12 @@ fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a L }, ); let region = broken_expr.region; - let value = Expr::PrecedenceConflict( - loc_expr.region, - stack_op, - bad_op, - arena.alloc(broken_expr), - ); + let value = Expr::PrecedenceConflict { + whole_region: loc_expr.region, + binop1: stack_op, + binop2: bad_op, + expr: arena.alloc(broken_expr), + }; return arena.alloc(Located { region, value }); } diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index 77d6f668f1..b721c43253 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -74,9 +74,10 @@ impl<'a> Formattable<'a> for Expr<'a> { next_is_multiline_bin_op || loc_left.is_multiline() || loc_right.is_multiline() } - UnaryOp(loc_subexpr, _) | PrecedenceConflict(_, _, _, loc_subexpr) => { - loc_subexpr.is_multiline() - } + UnaryOp(loc_subexpr, _) + | PrecedenceConflict { + expr: loc_subexpr, .. + } => loc_subexpr.is_multiline(), ParensAround(subexpr) | Nested(subexpr) => subexpr.is_multiline(), @@ -316,7 +317,7 @@ impl<'a> Formattable<'a> for Expr<'a> { } MalformedIdent(_, _) => {} MalformedClosure => {} - PrecedenceConflict(_, _, _, _) => {} + PrecedenceConflict { .. } => {} } } } diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index bc0e0d1259..60d528e8df 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -155,7 +155,12 @@ pub enum Expr<'a> { MalformedClosure, // Both operators were non-associative, e.g. (True == False == False). // We should tell the author to disambiguate by grouping them with parens. - PrecedenceConflict(Region, Loc, Loc, &'a Loc>), + PrecedenceConflict { + whole_region: Region, + binop1: Loc, + binop2: Loc, + expr: &'a Loc>, + }, } #[derive(Debug, Clone, PartialEq)] diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index 6e34bfdf70..d201da574c 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -1389,7 +1389,7 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result( (expr, output) } - PrecedenceConflict(_whole_region, _binop1, _binop2, _expr) => { + PrecedenceConflict { .. } => { // use roc_problem::can::RuntimeError::*; // // let problem = PrecedenceProblem::BothNonAssociative( From 9208000316359fd8c827c4c836942ca91dd98aca Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 15:34:08 +0100 Subject: [PATCH 02/14] only store start position for PrecedenceConflict --- compiler/can/src/expr.rs | 21 ++++++++++++++++++++- compiler/can/src/operator.rs | 6 ++++-- compiler/module/src/operator.rs | 12 ++++++++++++ compiler/parse/src/ast.rs | 8 +++++--- 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 71c16026b9..f73fa43095 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -722,13 +722,32 @@ pub fn canonicalize_expr<'a>( ast::Expr::PrecedenceConflict { whole_region, + binop1_position, + binop2_position, binop1, binop2, expr: _, } => { use roc_problem::can::RuntimeError::*; - let problem = PrecedenceProblem::BothNonAssociative(*whole_region, *binop1, *binop2); + let region1 = Region::new( + binop1_position.row, + binop1_position.row, + binop1_position.col, + binop1_position.col + binop1.width(), + ); + let loc_binop1 = Located::at(region1, *binop1); + + let region2 = Region::new( + binop2_position.row, + binop2_position.row, + binop2_position.col, + binop2_position.col + binop2.width(), + ); + let loc_binop2 = Located::at(region2, *binop2); + + let problem = + PrecedenceProblem::BothNonAssociative(*whole_region, loc_binop1, loc_binop2); env.problem(Problem::PrecedenceProblem(problem.clone())); diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 7749ba25d5..ba9c1be2e2 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -519,8 +519,10 @@ fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a L let region = broken_expr.region; let value = Expr::PrecedenceConflict { whole_region: loc_expr.region, - binop1: stack_op, - binop2: bad_op, + binop1_position: stack_op.region.start(), + binop1: stack_op.value, + binop2_position: bad_op.region.start(), + binop2: bad_op.value, expr: arena.alloc(broken_expr), }; diff --git a/compiler/module/src/operator.rs b/compiler/module/src/operator.rs index 4aa93f6620..14ba2de17b 100644 --- a/compiler/module/src/operator.rs +++ b/compiler/module/src/operator.rs @@ -47,6 +47,18 @@ pub enum BinOp { Backpassing, } +impl BinOp { + /// how wide this operator is when typed out + pub fn width(self) -> u16 { + match self { + Caret | Star | Slash | Percent | Plus | Minus | LessThan | GreaterThan => 1, + DoubleSlash | DoublePercent | Equals | NotEquals | LessThanOrEq | GreaterThanOrEq + | And | Or | Pizza => 2, + Assignment | HasType | Backpassing => unreachable!(), + } + } +} + #[derive(Clone, Debug, PartialEq, Eq)] pub enum ArgSide { Left, diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 60d528e8df..7e058afc9a 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -3,7 +3,7 @@ use crate::ident::Ident; use bumpalo::collections::String; use bumpalo::Bump; use roc_module::operator::{BinOp, CalledVia, UnaryOp}; -use roc_region::all::{Loc, Region}; +use roc_region::all::{Loc, Position, Region}; #[derive(Clone, Debug, PartialEq)] pub enum Module<'a> { @@ -157,8 +157,10 @@ pub enum Expr<'a> { // We should tell the author to disambiguate by grouping them with parens. PrecedenceConflict { whole_region: Region, - binop1: Loc, - binop2: Loc, + binop1_position: Position, + binop2_position: Position, + binop1: BinOp, + binop2: BinOp, expr: &'a Loc>, }, } From 0e7106280c3a93158d654554ea4a5c30ba7d8128 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 15:59:06 +0100 Subject: [PATCH 03/14] bump allocate precedence conflict --- compiler/can/src/expr.rs | 4 ++-- compiler/can/src/operator.rs | 3 ++- compiler/fmt/src/expr.rs | 4 ++-- compiler/parse/src/ast.rs | 19 +++++++++++-------- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index f73fa43095..bba15ddc79 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -720,14 +720,14 @@ pub fn canonicalize_expr<'a>( ) } - ast::Expr::PrecedenceConflict { + ast::Expr::PrecedenceConflict(ast::PrecedenceConflict { whole_region, binop1_position, binop2_position, binop1, binop2, expr: _, - } => { + }) => { use roc_problem::can::RuntimeError::*; let region1 = Region::new( diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index ba9c1be2e2..7625eb8efa 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -517,7 +517,7 @@ fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a L }, ); let region = broken_expr.region; - let value = Expr::PrecedenceConflict { + let data = roc_parse::ast::PrecedenceConflict { whole_region: loc_expr.region, binop1_position: stack_op.region.start(), binop1: stack_op.value, @@ -525,6 +525,7 @@ fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a L binop2: bad_op.value, expr: arena.alloc(broken_expr), }; + let value = Expr::PrecedenceConflict(arena.alloc(data)); return arena.alloc(Located { region, value }); } diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index b721c43253..752d5ef8cf 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -75,9 +75,9 @@ impl<'a> Formattable<'a> for Expr<'a> { } UnaryOp(loc_subexpr, _) - | PrecedenceConflict { + | PrecedenceConflict(roc_parse::ast::PrecedenceConflict { expr: loc_subexpr, .. - } => loc_subexpr.is_multiline(), + }) => loc_subexpr.is_multiline(), ParensAround(subexpr) | Nested(subexpr) => subexpr.is_multiline(), diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 7e058afc9a..d5ed150c00 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -155,14 +155,17 @@ pub enum Expr<'a> { MalformedClosure, // Both operators were non-associative, e.g. (True == False == False). // We should tell the author to disambiguate by grouping them with parens. - PrecedenceConflict { - whole_region: Region, - binop1_position: Position, - binop2_position: Position, - binop1: BinOp, - binop2: BinOp, - expr: &'a Loc>, - }, + PrecedenceConflict(&'a PrecedenceConflict<'a>), +} + +#[derive(Clone, Debug, PartialEq)] +pub struct PrecedenceConflict<'a> { + pub whole_region: Region, + pub binop1_position: Position, + pub binop2_position: Position, + pub binop1: BinOp, + pub binop2: BinOp, + pub expr: &'a Loc>, } #[derive(Debug, Clone, PartialEq)] From 60265b5d2afe62b8b8d451f6c41d17060262a9c9 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 16:06:07 +0100 Subject: [PATCH 04/14] add dedicated RecordUpdate tag to parse ast --- compiler/can/src/expr.rs | 48 ++++++++++++++++++++++++++++++++++++ compiler/can/src/operator.rs | 34 +++++++++++++++++++++++++ compiler/fmt/src/expr.rs | 8 ++++++ compiler/parse/src/ast.rs | 6 +++++ compiler/parse/src/expr.rs | 1 + 5 files changed, 97 insertions(+) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index bba15ddc79..400f021ec3 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -284,6 +284,54 @@ pub fn canonicalize_expr<'a>( } } } + ast::Expr::RecordUpdate { + fields, + update: loc_update, + final_comments: _, + } => { + let (can_update, update_out) = + canonicalize_expr(env, var_store, scope, loc_update.region, &loc_update.value); + if let Var(symbol) = &can_update.value { + match canonicalize_fields(env, var_store, scope, region, fields) { + Ok((can_fields, mut output)) => { + output.references = output.references.union(update_out.references); + + let answer = Update { + record_var: var_store.fresh(), + ext_var: var_store.fresh(), + symbol: *symbol, + updates: can_fields, + }; + + (answer, output) + } + Err(CanonicalizeRecordProblem::InvalidOptionalValue { + field_name, + field_region, + record_region, + }) => ( + Expr::RuntimeError(roc_problem::can::RuntimeError::InvalidOptionalValue { + field_name, + field_region, + record_region, + }), + Output::default(), + ), + } + } else { + // only (optionally qualified) variables can be updated, not arbitrary expressions + + let error = roc_problem::can::RuntimeError::InvalidRecordUpdate { + region: can_update.region, + }; + + let answer = Expr::RuntimeError(error.clone()); + + env.problems.push(Problem::RuntimeError(error)); + + (answer, Output::default()) + } + } ast::Expr::Str(literal) => flatten_str_literal(env, var_store, scope, literal), ast::Expr::List { items: loc_elems, .. diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 7625eb8efa..2bec12616d 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -190,6 +190,40 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a }, }) } + + RecordUpdate { + fields, + update, + final_comments, + } + | Nested(RecordUpdate { + fields, + update, + final_comments, + }) => { + // NOTE the `update` field is always a `Var { .. }` and does not need to be desugared + let mut new_fields = Vec::with_capacity_in(fields.len(), arena); + + for field in fields.iter() { + let value = desugar_field(arena, &field.value); + + new_fields.push(Located { + value, + region: field.region, + }); + } + + let new_fields = new_fields.into_bump_slice(); + + arena.alloc(Located { + region: loc_expr.region, + value: RecordUpdate { + update: *update, + fields: new_fields, + final_comments, + }, + }) + } Closure(loc_patterns, loc_ret) | Nested(Closure(loc_patterns, loc_ret)) => { arena.alloc(Located { region: loc_expr.region, diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index 752d5ef8cf..f1274daed8 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -98,6 +98,7 @@ impl<'a> Formattable<'a> for Expr<'a> { } Record { fields, .. } => fields.iter().any(|loc_field| loc_field.is_multiline()), + RecordUpdate { fields, .. } => fields.iter().any(|loc_field| loc_field.is_multiline()), } } @@ -247,6 +248,13 @@ impl<'a> Formattable<'a> for Expr<'a> { } => { fmt_record(buf, *update, fields, final_comments, indent); } + RecordUpdate { + fields, + update, + final_comments, + } => { + fmt_record(buf, Some(*update), fields, final_comments, indent); + } Closure(loc_patterns, loc_ret) => { fmt_closure(buf, loc_patterns, loc_ret, indent); } diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index d5ed150c00..fffde1d816 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -98,6 +98,12 @@ pub enum Expr<'a> { final_comments: &'a [CommentOrNewline<'a>], }, + RecordUpdate { + update: &'a Loc>, + fields: &'a [Loc>>], + final_comments: &'a [CommentOrNewline<'a>], + }, + Record { update: Option<&'a Loc>>, fields: &'a [Loc>>], diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index d201da574c..c5bdb93e1c 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -1393,6 +1393,7 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result Err(()), Expr::Str(string) => Ok(Pattern::StrLiteral(string.clone())), From e0c211081a34542ecf2ae76b58274b1d8d7101e8 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 16:49:14 +0100 Subject: [PATCH 05/14] remove update field from normal Record constructor --- cli/src/repl/eval.rs | 6 ---- compiler/can/src/expr.rs | 49 ------------------------------ compiler/can/src/operator.rs | 3 -- compiler/fmt/src/expr.rs | 3 +- compiler/parse/src/ast.rs | 1 - compiler/parse/src/expr.rs | 18 ++++++----- compiler/parse/tests/test_parse.rs | 7 ++--- editor/src/lang/expr.rs | 5 ++- 8 files changed, 15 insertions(+), 77 deletions(-) diff --git a/cli/src/repl/eval.rs b/cli/src/repl/eval.rs index ee95a9bf4b..22f608df70 100644 --- a/cli/src/repl/eval.rs +++ b/cli/src/repl/eval.rs @@ -121,7 +121,6 @@ fn jit_to_ast_help<'a>( } Layout::PhantomEmptyStruct => Ok(run_jit_function!(lib, main_fn_name, &u8, |_| { Expr::Record { - update: None, fields: &[], final_comments: env.arena.alloc([]), } @@ -474,7 +473,6 @@ fn struct_to_ast<'a>( let output = env.arena.alloc([loc_field]); Expr::Record { - update: None, fields: output, final_comments: &[], } @@ -510,7 +508,6 @@ fn struct_to_ast<'a>( let output = output.into_bump_slice(); Expr::Record { - update: None, fields: output, final_comments: &[], } @@ -554,7 +551,6 @@ fn bool_to_ast<'a>(env: &Env<'a, '_>, value: bool, content: &Content) -> Expr<'a }; Expr::Record { - update: None, fields: arena.alloc([loc_assigned_field]), final_comments: arena.alloc([]), } @@ -667,7 +663,6 @@ fn byte_to_ast<'a>(env: &Env<'a, '_>, value: u8, content: &Content) -> Expr<'a> }; Expr::Record { - update: None, fields: arena.alloc([loc_assigned_field]), final_comments: &[], } @@ -783,7 +778,6 @@ fn num_to_ast<'a>(env: &Env<'a, '_>, num_expr: Expr<'a>, content: &Content) -> E }; Expr::Record { - update: None, fields: arena.alloc([loc_assigned_field]), final_comments: arena.alloc([]), } diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 400f021ec3..72cfe5f95f 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -207,55 +207,6 @@ pub fn canonicalize_expr<'a>( } ast::Expr::Record { fields, - update: Some(loc_update), - final_comments: _, - } => { - let (can_update, update_out) = - canonicalize_expr(env, var_store, scope, loc_update.region, &loc_update.value); - if let Var(symbol) = &can_update.value { - match canonicalize_fields(env, var_store, scope, region, fields) { - Ok((can_fields, mut output)) => { - output.references = output.references.union(update_out.references); - - let answer = Update { - record_var: var_store.fresh(), - ext_var: var_store.fresh(), - symbol: *symbol, - updates: can_fields, - }; - - (answer, output) - } - Err(CanonicalizeRecordProblem::InvalidOptionalValue { - field_name, - field_region, - record_region, - }) => ( - Expr::RuntimeError(roc_problem::can::RuntimeError::InvalidOptionalValue { - field_name, - field_region, - record_region, - }), - Output::default(), - ), - } - } else { - // only (optionally qualified) variables can be updated, not arbitrary expressions - - let error = roc_problem::can::RuntimeError::InvalidRecordUpdate { - region: can_update.region, - }; - - let answer = Expr::RuntimeError(error.clone()); - - env.problems.push(Problem::RuntimeError(error)); - - (answer, Output::default()) - } - } - ast::Expr::Record { - fields, - update: None, final_comments: _, } => { if fields.is_empty() { diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 2bec12616d..3283e2a139 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -160,12 +160,10 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a } Record { fields, - update, final_comments, } | Nested(Record { fields, - update, final_comments, }) => { let mut new_fields = Vec::with_capacity_in(fields.len(), arena); @@ -184,7 +182,6 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a arena.alloc(Located { region: loc_expr.region, value: Record { - update: *update, fields: new_fields, final_comments, }, diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index f1274daed8..716a5bdbd3 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -243,10 +243,9 @@ impl<'a> Formattable<'a> for Expr<'a> { } Record { fields, - update, final_comments, } => { - fmt_record(buf, *update, fields, final_comments, indent); + fmt_record(buf, None, fields, final_comments, indent); } RecordUpdate { fields, diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index fffde1d816..f825b35f93 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -105,7 +105,6 @@ pub enum Expr<'a> { }, Record { - update: Option<&'a Loc>>, fields: &'a [Loc>>], final_comments: &'a [CommentOrNewline<'a>], }, diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index c5bdb93e1c..2ef384e8db 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -1352,7 +1352,6 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result { let mut loc_patterns = Vec::with_capacity_in(fields.len(), arena); @@ -1390,9 +1389,6 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result Err(()), @@ -2086,10 +2082,16 @@ fn record_literal_help<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>, EExpr<' let (opt_update, loc_assigned_fields_with_comments) = loc_record.value; // This is a record literal, not a destructure. - let mut value = Expr::Record { - update: opt_update.map(|loc_expr| &*arena.alloc(loc_expr)), - fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), - final_comments: loc_assigned_fields_with_comments.value.1, + let mut value = match opt_update { + Some(update) => Expr::RecordUpdate { + update: &*arena.alloc(update), + fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), + final_comments: loc_assigned_fields_with_comments.value.1, + }, + None => Expr::Record { + fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), + final_comments: loc_assigned_fields_with_comments.value.1, + }, }; // there can be field access, e.g. `{ x : 4 }.x` diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index c650f5f121..7a5629b3de 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -414,7 +414,6 @@ mod test_parse { let arena = Bump::new(); let expected = Record { fields: &[], - update: None, final_comments: &[], }; let actual = parse_expr_with(&arena, "{}"); @@ -444,8 +443,8 @@ mod test_parse { ident: "baz", }; let update_target = Located::new(0, 0, 2, 13, var); - let expected = Record { - update: Some(&*arena.alloc(update_target)), + let expected = RecordUpdate { + update: &*arena.alloc(update_target), fields, final_comments: &[], }; @@ -480,7 +479,6 @@ mod test_parse { Located::new(0, 0, 28, 32, label2), ]; let expected = Record { - update: None, fields, final_comments: &[], }; @@ -1860,7 +1858,6 @@ mod test_parse { let identifier_z = Located::new(2, 2, 0, 1, Identifier("z")); let empty_record = Record { - update: None, fields: &[], final_comments: &[], }; diff --git a/editor/src/lang/expr.rs b/editor/src/lang/expr.rs index db4941f752..198e4e4b0f 100644 --- a/editor/src/lang/expr.rs +++ b/editor/src/lang/expr.rs @@ -379,9 +379,9 @@ pub fn to_expr2<'a>( ) } - Record { + RecordUpdate { fields, - update: Some(loc_update), + update: loc_update, final_comments: _, } => { let (can_update, update_out) = @@ -435,7 +435,6 @@ pub fn to_expr2<'a>( Record { fields, - update: None, final_comments: _, } => { if fields.is_empty() { From d5ec66244f2904121237a50680db44856b723f65 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 21:19:10 +0100 Subject: [PATCH 06/14] use new binops --- compiler/can/src/expr.rs | 6 + compiler/can/src/operator.rs | 270 ++++++++++++++++++++++++++++++++++- compiler/fmt/src/expr.rs | 6 + compiler/parse/src/ast.rs | 1 + compiler/parse/src/expr.rs | 27 ++-- 5 files changed, 295 insertions(+), 15 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 72cfe5f95f..621670fb57 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -809,6 +809,12 @@ pub fn canonicalize_expr<'a>( bad_expr ); } + bad_expr @ ast::Expr::BinOps { .. } => { + panic!( + "A binary operator chain did not get desugared somehow: {:#?}", + bad_expr + ); + } bad_expr @ ast::Expr::UnaryOp(_, _) => { panic!( "A unary operator did not get desugared somehow: {:#?}", diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 3283e2a139..7ec20f9a09 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -31,6 +31,63 @@ fn new_op_expr<'a>( } } +fn new_op_call_expr<'a>( + arena: &'a Bump, + left: &'a Located>, + loc_op: Located, + right: &'a Located>, +) -> Located> { + let region = Region::span_across(&left.region, &right.region); + + let value = match loc_op.value { + Pizza => { + // Rewrite the Pizza operator into an Apply + + match &right.value { + Apply(function, arguments, _called_via) => { + let mut args = Vec::with_capacity_in(1 + arguments.len(), arena); + + args.push(left); + + for arg in arguments.iter() { + args.push(arg); + } + + let args = args.into_bump_slice(); + + Apply(function, args, CalledVia::BinOp(Pizza)) + } + _ => { + // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` + let mut args = Vec::with_capacity_in(1, arena); + + args.push(left); + + let args = args.into_bump_slice(); + + Apply(right, args, CalledVia::BinOp(Pizza)) + } + } + } + binop => { + // This is a normal binary operator like (+), so desugar it + // into the appropriate function call. + let (module_name, ident) = binop_to_function(binop); + + let args = arena.alloc([left, right]); + + let loc_expr = arena.alloc(Located { + value: Expr::Var { module_name, ident }, + region: loc_op.region, + }); + + Apply(loc_expr, args, CalledVia::BinOp(binop)) + } + }; + + Located { value, region } +} + fn desugar_defs<'a>( arena: &'a Bump, region: Region, @@ -255,7 +312,10 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a _ => panic!(), } } - BinOp(_) | Nested(BinOp(_)) => desugar_bin_op(arena, loc_expr), + BinOp(_) | Nested(BinOp(_)) => todo!(), // desugar_bin_op(arena, loc_expr), + BinOps(lefts, right) | Nested(BinOps(lefts, right)) => { + desugar_bin_ops(arena, loc_expr.region, lefts, right) + } Defs(defs, loc_ret) | Nested(Defs(defs, loc_ret)) => { desugar_defs(arena, loc_expr.region, *defs, loc_ret) } @@ -457,6 +517,214 @@ fn binop_to_function(binop: BinOp) -> (&'static str, &'static str) { } } +fn desugar_bin_ops<'a>( + arena: &'a Bump, + whole_region: Region, + lefts: &'a [(Located>, Located)], + right: &'a Located>, +) -> &'a Located> { + let mut arg_stack: Vec<&'a Located> = Vec::new_in(arena); + let mut op_stack: Vec> = Vec::new_in(arena); + + for (loc_expr, loc_op) in lefts { + arg_stack.push(desugar_expr(arena, loc_expr)); + match run_binop_step(arena, whole_region, &mut arg_stack, &mut op_stack, *loc_op) { + Err(problem) => return problem, + Ok(()) => continue, + } + } + + arg_stack.push(desugar_expr(arena, right)); + + for loc_op in op_stack.into_iter().rev() { + let right = arg_stack.pop().unwrap(); + let left = arg_stack.pop().unwrap(); + + let region = Region::span_across(&left.region, &right.region); + let value = match loc_op.value { + Pizza => { + // Rewrite the Pizza operator into an Apply + + match &right.value { + Apply(function, arguments, _called_via) => { + let mut args = Vec::with_capacity_in(1 + arguments.len(), arena); + + args.push(left); + + for arg in arguments.iter() { + args.push(arg); + } + + let args = args.into_bump_slice(); + + Apply(function, args, CalledVia::BinOp(Pizza)) + } + expr => { + // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` + let mut args = Vec::with_capacity_in(1, arena); + + args.push(left); + + let function = arena.alloc(Located { + value: Nested(expr), + region: right.region, + }); + + let args = args.into_bump_slice(); + + Apply(function, args, CalledVia::BinOp(Pizza)) + } + } + } + binop => { + // This is a normal binary operator like (+), so desugar it + // into the appropriate function call. + let (module_name, ident) = binop_to_function(binop); + let mut args = Vec::with_capacity_in(2, arena); + + args.push(left); + args.push(right); + + let loc_expr = arena.alloc(Located { + value: Expr::Var { module_name, ident }, + region: loc_op.region, + }); + + let args = args.into_bump_slice(); + + Apply(loc_expr, args, CalledVia::BinOp(binop)) + } + }; + + arg_stack.push(arena.alloc(Located { region, value })); + } + + assert_eq!(arg_stack.len(), 1); + + arg_stack.pop().unwrap() +} + +enum Step<'a> { + Error(&'a Located>), + Push(Located), + Skip, +} + +fn run_binop_step<'a>( + arena: &'a Bump, + whole_region: Region, + arg_stack: &mut Vec<&'a Located>>, + op_stack: &mut Vec>, + next_op: Located, +) -> Result<(), &'a Located>> { + use Step::*; + + match binop_step(arena, whole_region, arg_stack, op_stack, next_op) { + Error(problem) => Err(problem), + Push(loc_op) => run_binop_step(arena, whole_region, arg_stack, op_stack, loc_op), + Skip => Ok(()), + } +} + +fn binop_step<'a>( + arena: &'a Bump, + whole_region: Region, + arg_stack: &mut Vec<&'a Located>>, + op_stack: &mut Vec>, + next_op: Located, +) -> Step<'a> { + use roc_module::operator::Associativity::*; + use std::cmp::Ordering; + + match op_stack.pop() { + Some(stack_op) => { + match next_op.value.cmp(&stack_op.value) { + Ordering::Less => { + // Inline + let right = arg_stack.pop().unwrap(); + let left = arg_stack.pop().unwrap(); + + arg_stack.push(arena.alloc(new_op_call_expr(arena, left, stack_op, right))); + + Step::Push(next_op) + } + + Ordering::Greater => { + // Swap + op_stack.push(stack_op); + op_stack.push(next_op); + + Step::Skip + } + + Ordering::Equal => { + match ( + next_op.value.associativity(), + stack_op.value.associativity(), + ) { + (LeftAssociative, LeftAssociative) => { + // Inline + let right = arg_stack.pop().unwrap(); + let left = arg_stack.pop().unwrap(); + + arg_stack + .push(arena.alloc(new_op_call_expr(arena, left, stack_op, right))); + + Step::Push(next_op) + } + + (RightAssociative, RightAssociative) => { + // Swap + op_stack.push(stack_op); + op_stack.push(next_op); + + Step::Skip + } + + (NonAssociative, NonAssociative) => { + // Both operators were non-associative, e.g. (True == False == False). + // We should tell the author to disambiguate by grouping them with parens. + let bad_op = next_op; + let right = arg_stack.pop().unwrap(); + let left = arg_stack.pop().unwrap(); + let broken_expr = + arena.alloc(new_op_call_expr(arena, left, stack_op, right)); + let region = broken_expr.region; + let data = roc_parse::ast::PrecedenceConflict { + whole_region, + binop1_position: stack_op.region.start(), + binop1: stack_op.value, + binop2_position: bad_op.region.start(), + binop2: bad_op.value, + expr: arena.alloc(broken_expr), + }; + let value = Expr::PrecedenceConflict(arena.alloc(data)); + + Step::Error(arena.alloc(Located { region, value })) + } + + _ => { + // The operators had the same precedence but different associativity. + // + // In many languages, this case can happen due to (for example) <| and |> having the same + // precedence but different associativity. Languages which support custom operators with + // (e.g. Haskell) can potentially have arbitrarily many of these cases. + // + // By design, Roc neither allows custom operators nor has any built-in operators with + // the same precedence and different associativity, so this should never happen! + panic!("BinOps had the same associativity, but different precedence. This should never happen!"); + } + } + } + } + } + None => { + op_stack.push(next_op); + Step::Skip + } + } +} + fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Located> { use roc_module::operator::Associativity::*; use std::cmp::Ordering; diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index 716a5bdbd3..ebd7d64321 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -74,6 +74,11 @@ impl<'a> Formattable<'a> for Expr<'a> { next_is_multiline_bin_op || loc_left.is_multiline() || loc_right.is_multiline() } + BinOps(lefts, loc_right) => { + lefts.iter().any(|(loc_expr, _)| loc_expr.is_multiline()) + || loc_right.is_multiline() + } + UnaryOp(loc_subexpr, _) | PrecedenceConflict(roc_parse::ast::PrecedenceConflict { expr: loc_subexpr, .. @@ -298,6 +303,7 @@ impl<'a> Formattable<'a> for Expr<'a> { parens, indent, ), + BinOps(_, _) => todo!(), UnaryOp(sub_expr, unary_op) => { match &unary_op.value { operator::UnaryOp::Negate => { diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index f825b35f93..e3d6ced645 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -130,6 +130,7 @@ pub enum Expr<'a> { /// To apply a tag by name, do Apply(Tag(...), ...) Apply(&'a Loc>, &'a [&'a Loc>], CalledVia), BinOp(&'a (Loc>, Loc, Loc>)), + BinOps(&'a [(Loc>, Loc)], &'a Loc>), UnaryOp(&'a Loc>, Loc), // Conditionals diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index 2ef384e8db..6a6806b37e 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -416,15 +416,14 @@ fn parse_expr_final<'a>( arena: &'a Bump, state: State<'a>, ) -> ParseResult<'a, Expr<'a>, EExpr<'a>> { - let mut expr = to_call(arena, expr_state.arguments, expr_state.expr); + let right_arg = to_call(arena, expr_state.arguments, expr_state.expr); - for (left_arg, op) in expr_state.operators.into_iter().rev() { - let region = Region::span_across(&left_arg.region, &expr.region); - let new = Expr::BinOp(arena.alloc((left_arg, op, expr))); - expr = Located::at(region, new); - } + let expr = Expr::BinOps( + expr_state.operators.into_bump_slice(), + arena.alloc(right_arg), + ); - Ok((MadeProgress, expr.value, state)) + Ok((MadeProgress, expr, state)) } fn to_call<'a>( @@ -1262,15 +1261,14 @@ fn parse_expr_end<'a>( Ok((MadeProgress, expr.value, state)) } else { - let mut expr = to_call(arena, expr_state.arguments, expr_state.expr); + let right_arg = to_call(arena, expr_state.arguments, expr_state.expr); - for (left_arg, op) in expr_state.operators.into_iter().rev() { - let region = Region::span_across(&left_arg.region, &expr.region); - let new = Expr::BinOp(arena.alloc((left_arg, op, expr))); - expr = Located::at(region, new); - } + let expr = Expr::BinOps( + expr_state.operators.into_bump_slice(), + arena.alloc(right_arg), + ); - Ok((MadeProgress, expr.value, state)) + Ok((MadeProgress, expr, state)) } } } @@ -1384,6 +1382,7 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result Date: Sat, 20 Mar 2021 21:56:22 +0100 Subject: [PATCH 07/14] no empty binops --- compiler/parse/src/expr.rs | 27 ++++++++-------------- compiler/reporting/tests/test_reporting.rs | 1 - 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index 6a6806b37e..3b97de791a 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -418,10 +418,14 @@ fn parse_expr_final<'a>( ) -> ParseResult<'a, Expr<'a>, EExpr<'a>> { let right_arg = to_call(arena, expr_state.arguments, expr_state.expr); - let expr = Expr::BinOps( - expr_state.operators.into_bump_slice(), - arena.alloc(right_arg), - ); + let expr = if expr_state.operators.is_empty() { + right_arg.value + } else { + Expr::BinOps( + expr_state.operators.into_bump_slice(), + arena.alloc(right_arg), + ) + }; Ok((MadeProgress, expr, state)) } @@ -1256,20 +1260,7 @@ fn parse_expr_end<'a>( // roll back space parsing let state = expr_state.initial; - if expr_state.operators.is_empty() { - let expr = to_call(arena, expr_state.arguments, expr_state.expr); - - Ok((MadeProgress, expr.value, state)) - } else { - let right_arg = to_call(arena, expr_state.arguments, expr_state.expr); - - let expr = Expr::BinOps( - expr_state.operators.into_bump_slice(), - arena.alloc(right_arg), - ); - - Ok((MadeProgress, expr, state)) - } + parse_expr_final(expr_state, arena, state) } } } diff --git a/compiler/reporting/tests/test_reporting.rs b/compiler/reporting/tests/test_reporting.rs index f3fef89294..f86d7909d4 100644 --- a/compiler/reporting/tests/test_reporting.rs +++ b/compiler/reporting/tests/test_reporting.rs @@ -65,7 +65,6 @@ mod test_reporting { problems: can_problems, .. } = can_expr(arena, expr_src)?; - dbg!(&loc_expr); let mut subs = Subs::new(var_store.into()); for (var, name) in output.introduced_variables.name_by_var { From dd5e13ae2532f402e26166ab0253453feb087024 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 22:12:35 +0100 Subject: [PATCH 08/14] update tests --- compiler/parse/tests/test_parse.rs | 179 ++++++++++++++++------------- 1 file changed, 99 insertions(+), 80 deletions(-) diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 7a5629b3de..5df58fa08d 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -82,6 +82,19 @@ mod test_parse { } } + fn single_binop<'a>( + arena: &'a Bump, + args: ( + Located>, + Located, + Located>, + ), + ) -> Expr<'a> { + let (left, op, right) = args; + + Expr::BinOps(arena.alloc([(left, op)]), arena.alloc(right)) + } + // STRING LITERALS fn expect_parsed_str(input: &str, expected: &str) { @@ -492,12 +505,12 @@ mod test_parse { #[test] fn one_plus_two() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, Num("1")), Located::new(0, 0, 1, 2, Plus), Located::new(0, 0, 2, 3, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "1+2"); assert_eq!(Ok(expected), actual); @@ -506,12 +519,12 @@ mod test_parse { #[test] fn one_minus_two() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, Num("1")), Located::new(0, 0, 1, 2, Minus), Located::new(0, 0, 2, 3, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "1-2"); assert_eq!(Ok(expected), actual); @@ -520,7 +533,7 @@ mod test_parse { #[test] fn var_minus_two() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new( 0, 0, @@ -533,8 +546,8 @@ mod test_parse { ), Located::new(0, 0, 1, 2, Minus), Located::new(0, 0, 2, 3, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x-2"); assert_eq!(Ok(expected), actual); @@ -543,12 +556,12 @@ mod test_parse { #[test] fn add_with_spaces() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, Num("1")), Located::new(0, 0, 3, 4, Plus), Located::new(0, 0, 7, 8, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "1 + 2"); assert_eq!(Ok(expected), actual); @@ -557,12 +570,12 @@ mod test_parse { #[test] fn sub_with_spaces() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, Num("1")), Located::new(0, 0, 3, 4, Minus), Located::new(0, 0, 7, 8, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "1 - 2"); assert_eq!(Ok(expected), actual); @@ -580,12 +593,12 @@ mod test_parse { module_name: "", ident: "x", }; - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, var), Located::new(0, 0, 2, 3, Plus), Located::new(0, 0, 4, 5, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x + 2"); assert_eq!(Ok(expected), actual); @@ -602,12 +615,12 @@ mod test_parse { module_name: "", ident: "x", }; - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, var), Located::new(0, 0, 2, 3, Minus), Located::new(0, 0, 4, 5, Num("2")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x - 2"); assert_eq!(Ok(expected), actual); @@ -617,12 +630,12 @@ mod test_parse { fn newline_before_add() { let arena = Bump::new(); let spaced_int = Expr::SpaceAfter(arena.alloc(Num("3")), &[Newline]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, spaced_int), Located::new(1, 1, 0, 1, Plus), Located::new(1, 1, 2, 3, Num("4")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 \n+ 4"); assert_eq!(Ok(expected), actual); @@ -632,12 +645,12 @@ mod test_parse { fn newline_before_sub() { let arena = Bump::new(); let spaced_int = Expr::SpaceAfter(arena.alloc(Num("3")), &[Newline]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, spaced_int), Located::new(1, 1, 0, 1, Minus), Located::new(1, 1, 2, 3, Num("4")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 \n- 4"); assert_eq!(Ok(expected), actual); @@ -647,12 +660,12 @@ mod test_parse { fn newline_after_mul() { let arena = Bump::new(); let spaced_int = arena.alloc(Num("4")).before(&[Newline]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, Num("3")), Located::new(0, 0, 3, 4, Star), Located::new(1, 1, 2, 3, spaced_int), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 *\n 4"); assert_eq!(Ok(expected), actual); @@ -662,12 +675,12 @@ mod test_parse { fn newline_after_sub() { let arena = Bump::new(); let spaced_int = arena.alloc(Num("4")).before(&[Newline]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, Num("3")), Located::new(0, 0, 3, 4, Minus), Located::new(1, 1, 2, 3, spaced_int), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 -\n 4"); assert_eq!(Ok(expected), actual); @@ -677,16 +690,16 @@ mod test_parse { fn newline_and_spaces_before_less_than() { let arena = Bump::new(); let spaced_int = arena.alloc(Num("1")).after(&[Newline]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 4, 5, spaced_int), Located::new(1, 1, 4, 5, LessThan), Located::new(1, 1, 6, 7, Num("2")), - )); + ); let newlines = bumpalo::vec![in &arena; Newline, Newline]; let def = Def::Body( arena.alloc(Located::new(0, 0, 0, 1, Identifier("x"))), - arena.alloc(Located::new(0, 1, 4, 7, BinOp(tuple))), + arena.alloc(Located::new(0, 1, 4, 7, single_binop(&arena, tuple))), ); let loc_def = &*arena.alloc(Located::new(0, 1, 0, 7, def)); let defs = &[loc_def]; @@ -694,7 +707,7 @@ mod test_parse { let loc_ret = Located::new(3, 3, 0, 2, ret); let expected = Defs(defs, arena.alloc(loc_ret)); - // let expected = BinOp(tuple); + // let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x = 1\n < 2\n\n42"); assert_eq!(Ok(expected), actual); @@ -704,12 +717,12 @@ mod test_parse { fn comment_with_non_ascii() { let arena = Bump::new(); let spaced_int = arena.alloc(Num("3")).after(&[LineComment(" 2 × 2")]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, spaced_int), Located::new(1, 1, 0, 1, Plus), Located::new(1, 1, 2, 3, Num("4")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 # 2 × 2\n+ 4"); assert_eq!(Ok(expected), actual); @@ -719,12 +732,12 @@ mod test_parse { fn comment_before_op() { let arena = Bump::new(); let spaced_int = arena.alloc(Num("3")).after(&[LineComment(" test!")]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, spaced_int), Located::new(1, 1, 0, 1, Plus), Located::new(1, 1, 2, 3, Num("4")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 # test!\n+ 4"); assert_eq!(Ok(expected), actual); @@ -734,12 +747,12 @@ mod test_parse { fn comment_after_op() { let arena = Bump::new(); let spaced_int = arena.alloc(Num("92")).before(&[LineComment(" test!")]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 2, Num("12")), Located::new(0, 0, 4, 5, Star), Located::new(1, 1, 1, 3, spaced_int), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "12 * # test!\n 92"); assert_eq!(Ok(expected), actual); @@ -750,12 +763,12 @@ mod test_parse { let arena = Bump::new(); let spaced_int1 = arena.alloc(Num("3")).after(&[Newline]); let spaced_int2 = arena.alloc(Num("4")).before(&[Newline, Newline]); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, spaced_int1), Located::new(1, 1, 0, 1, Plus), Located::new(3, 3, 2, 3, spaced_int2), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "3 \n+ \n\n 4"); assert_eq!(Ok(expected), actual); @@ -774,12 +787,12 @@ mod test_parse { module_name: "", ident: "y", }; - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, var1), Located::new(0, 0, 1, 2, Minus), Located::new(0, 0, 3, 4, var2), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x- y"); assert_eq!(Ok(expected), actual); @@ -788,12 +801,12 @@ mod test_parse { #[test] fn minus_twelve_minus_five() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 3, Num("-12")), Located::new(0, 0, 3, 4, Minus), Located::new(0, 0, 4, 5, Num("5")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "-12-5"); assert_eq!(Ok(expected), actual); @@ -802,12 +815,12 @@ mod test_parse { #[test] fn ten_times_eleven() { let arena = Bump::new(); - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 2, Num("10")), Located::new(0, 0, 2, 3, Star), Located::new(0, 0, 3, 5, Num("11")), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "10*11"); assert_eq!(Ok(expected), actual); @@ -816,17 +829,20 @@ mod test_parse { #[test] fn multiple_operators() { let arena = Bump::new(); - let inner = arena.alloc(( - Located::new(0, 0, 3, 5, Num("42")), - Located::new(0, 0, 5, 6, Plus), - Located::new(0, 0, 6, 9, Num("534")), - )); - let outer = arena.alloc(( - Located::new(0, 0, 0, 2, Num("31")), - Located::new(0, 0, 2, 3, Star), - Located::new(0, 0, 3, 9, BinOp(inner)), - )); - let expected = BinOp(outer); + + let lefts = [ + ( + Located::new(0, 0, 0, 2, Num("31")), + Located::new(0, 0, 2, 3, Star), + ), + ( + Located::new(0, 0, 3, 5, Num("42")), + Located::new(0, 0, 5, 6, Plus), + ), + ]; + let right = Located::new(0, 0, 6, 9, Num("534")); + + let expected = BinOps(&lefts, &right); let actual = parse_expr_with(&arena, "31*42+534"); assert_eq!(Ok(expected), actual); @@ -843,12 +859,12 @@ mod test_parse { module_name: "", ident: "y", }; - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, var1), Located::new(0, 0, 1, 3, Equals), Located::new(0, 0, 3, 4, var2), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x==y"); assert_eq!(Ok(expected), actual); @@ -865,12 +881,12 @@ mod test_parse { module_name: "", ident: "y", }; - let tuple = arena.alloc(( + let tuple = ( Located::new(0, 0, 0, 1, var1), Located::new(0, 0, 2, 4, Equals), Located::new(0, 0, 5, 6, var2), - )); - let expected = BinOp(tuple); + ); + let expected = single_binop(&arena, tuple); let actual = parse_expr_with(&arena, "x == y"); assert_eq!(Ok(expected), actual); @@ -1953,11 +1969,14 @@ mod test_parse { let loc_closure = Located::new(0, 0, 8, 23, apply); - let binop = Expr::BinOp(arena.alloc(( - loc_var_x, - Located::new(2, 2, 2, 3, roc_module::operator::BinOp::Plus), - loc_var_y, - ))); + let binop = single_binop( + &arena, + ( + loc_var_x, + Located::new(2, 2, 2, 3, roc_module::operator::BinOp::Plus), + loc_var_y, + ), + ); let spaced_binop = Expr::SpaceBefore(arena.alloc(binop), &[Newline, Newline]); From 6122b0650bd2f07678aae0bff1143537d269534c Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 22:29:02 +0100 Subject: [PATCH 09/14] format binops --- compiler/fmt/src/expr.rs | 79 ++++++++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index ebd7d64321..ee76fe7995 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -303,7 +303,7 @@ impl<'a> Formattable<'a> for Expr<'a> { parens, indent, ), - BinOps(_, _) => todo!(), + BinOps(lefts, right) => fmt_bin_ops(buf, lefts, right, false, parens, indent), UnaryOp(sub_expr, unary_op) => { match &unary_op.value { operator::UnaryOp::Negate => { @@ -365,28 +365,8 @@ fn format_str_segment<'a>(seg: &StrSegment<'a>, buf: &mut String<'a>, indent: u1 } } -fn fmt_bin_op<'a>( - buf: &mut String<'a>, - loc_left_side: &'a Located>, - loc_bin_op: &'a Located, - loc_right_side: &'a Located>, - part_of_multi_line_bin_ops: bool, - apply_needs_parens: Parens, - indent: u16, -) { - loc_left_side.format_with_options(buf, apply_needs_parens, Newlines::No, indent); - - let is_multiline = (&loc_right_side.value).is_multiline() - || (&loc_left_side.value).is_multiline() - || part_of_multi_line_bin_ops; - - if is_multiline { - newline(buf, indent + INDENT) - } else { - buf.push(' '); - } - - match &loc_bin_op.value { +fn push_op(buf: &mut String, op: BinOp) { + match op { operator::BinOp::Caret => buf.push('^'), operator::BinOp::Star => buf.push('*'), operator::BinOp::Slash => buf.push('/'), @@ -408,6 +388,30 @@ fn fmt_bin_op<'a>( operator::BinOp::HasType => unreachable!(), operator::BinOp::Backpassing => unreachable!(), } +} + +fn fmt_bin_op<'a>( + buf: &mut String<'a>, + loc_left_side: &'a Located>, + loc_bin_op: &'a Located, + loc_right_side: &'a Located>, + part_of_multi_line_bin_ops: bool, + apply_needs_parens: Parens, + indent: u16, +) { + loc_left_side.format_with_options(buf, apply_needs_parens, Newlines::No, indent); + + let is_multiline = (&loc_right_side.value).is_multiline() + || (&loc_left_side.value).is_multiline() + || part_of_multi_line_bin_ops; + + if is_multiline { + newline(buf, indent + INDENT) + } else { + buf.push(' '); + } + + push_op(buf, loc_bin_op.value); buf.push(' '); @@ -430,6 +434,35 @@ fn fmt_bin_op<'a>( } } +fn fmt_bin_ops<'a>( + buf: &mut String<'a>, + lefts: &'a [(Located>, Located)], + loc_right_side: &'a Located>, + part_of_multi_line_bin_ops: bool, + apply_needs_parens: Parens, + indent: u16, +) { + let is_multiline = part_of_multi_line_bin_ops + || (&loc_right_side.value).is_multiline() + || lefts.iter().any(|(expr, _)| expr.value.is_multiline()); + + for (loc_left_side, loc_bin_op) in lefts { + loc_left_side.format_with_options(buf, apply_needs_parens, Newlines::No, indent); + + if is_multiline { + newline(buf, indent + INDENT) + } else { + buf.push(' '); + } + + push_op(buf, loc_bin_op.value); + + buf.push(' '); + } + + loc_right_side.format_with_options(buf, apply_needs_parens, Newlines::Yes, indent); +} + fn fmt_list<'a>( buf: &mut String<'a>, loc_items: &[&Located>], From ad40609d44f90cb9b5bbc503b0f8514608c97c31 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 22:36:08 +0100 Subject: [PATCH 10/14] cleanup --- compiler/can/src/operator.rs | 282 ----------------------------------- 1 file changed, 282 deletions(-) diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 7ec20f9a09..bb24c55b37 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -10,26 +10,6 @@ use roc_region::all::{Located, Region}; // BinOp precedence logic adapted from Gluon by Markus Westerlind, MIT licensed // https://github.com/gluon-lang/gluon // Thank you, Markus! -fn new_op_expr<'a>( - arena: &'a Bump, - left: Located>, - op: Located, - right: Located>, -) -> Located> { - let new_region = Region { - start_line: left.region.start_line, - start_col: left.region.start_col, - - end_line: right.region.end_line, - end_col: right.region.end_col, - }; - let new_expr = Expr::BinOp(arena.alloc((left, op, right))); - - Located { - value: new_expr, - region: new_region, - } -} fn new_op_call_expr<'a>( arena: &'a Bump, @@ -724,265 +704,3 @@ fn binop_step<'a>( } } } - -fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Located> { - use roc_module::operator::Associativity::*; - use std::cmp::Ordering; - - let mut infixes = Infixes::new(loc_expr); - let mut arg_stack: Vec<&'a Located> = Vec::new_in(arena); - let mut op_stack: Vec> = Vec::new_in(arena); - - while let Some(token) = infixes.next() { - match token { - InfixToken::Arg(next_expr) => arg_stack.push(next_expr), - InfixToken::Op(next_op) => { - match op_stack.pop() { - Some(stack_op) => { - match next_op.value.cmp(&stack_op.value) { - Ordering::Less => { - // Inline - let right = arg_stack.pop().unwrap(); - let left = arg_stack.pop().unwrap(); - - infixes.next_op = Some(next_op); - arg_stack.push(arena.alloc(new_op_expr( - arena, - Located { - value: Nested(&left.value), - region: left.region, - }, - stack_op, - Located { - value: Nested(&right.value), - region: right.region, - }, - ))); - } - - Ordering::Greater => { - // Swap - op_stack.push(stack_op); - op_stack.push(next_op); - } - - Ordering::Equal => { - match ( - next_op.value.associativity(), - stack_op.value.associativity(), - ) { - (LeftAssociative, LeftAssociative) => { - // Inline - let right = arg_stack.pop().unwrap(); - let left = arg_stack.pop().unwrap(); - - infixes.next_op = Some(next_op); - arg_stack.push(arena.alloc(new_op_expr( - arena, - Located { - value: Nested(&left.value), - region: left.region, - }, - stack_op, - Located { - value: Nested(&right.value), - region: right.region, - }, - ))); - } - - (RightAssociative, RightAssociative) => { - // Swap - op_stack.push(stack_op); - op_stack.push(next_op); - } - - (NonAssociative, NonAssociative) => { - // Both operators were non-associative, e.g. (True == False == False). - // We should tell the author to disambiguate by grouping them with parens. - let bad_op = next_op; - let right = arg_stack.pop().unwrap(); - let left = arg_stack.pop().unwrap(); - let broken_expr = new_op_expr( - arena, - Located { - value: Nested(&left.value), - region: left.region, - }, - next_op, - Located { - value: Nested(&right.value), - region: right.region, - }, - ); - let region = broken_expr.region; - let data = roc_parse::ast::PrecedenceConflict { - whole_region: loc_expr.region, - binop1_position: stack_op.region.start(), - binop1: stack_op.value, - binop2_position: bad_op.region.start(), - binop2: bad_op.value, - expr: arena.alloc(broken_expr), - }; - let value = Expr::PrecedenceConflict(arena.alloc(data)); - - return arena.alloc(Located { region, value }); - } - - _ => { - // The operators had the same precedence but different associativity. - // - // In many languages, this case can happen due to (for example) <| and |> having the same - // precedence but different associativity. Languages which support custom operators with - // (e.g. Haskell) can potentially have arbitrarily many of these cases. - // - // By design, Roc neither allows custom operators nor has any built-in operators with - // the same precedence and different associativity, so this should never happen! - panic!("BinOps had the same associativity, but different precedence. This should never happen!"); - } - } - } - } - } - None => op_stack.push(next_op), - }; - } - } - } - - for loc_op in op_stack.into_iter().rev() { - let right = desugar_expr(arena, arg_stack.pop().unwrap()); - let left = desugar_expr(arena, arg_stack.pop().unwrap()); - - let region = Region::span_across(&left.region, &right.region); - let value = match loc_op.value { - Pizza => { - // Rewrite the Pizza operator into an Apply - - match &right.value { - Apply(function, arguments, _called_via) => { - let mut args = Vec::with_capacity_in(1 + arguments.len(), arena); - - args.push(left); - - for arg in arguments.iter() { - args.push(arg); - } - - let args = args.into_bump_slice(); - - Apply(function, args, CalledVia::BinOp(Pizza)) - } - expr => { - // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` - let mut args = Vec::with_capacity_in(1, arena); - - args.push(left); - - let function = arena.alloc(Located { - value: Nested(expr), - region: right.region, - }); - - let args = args.into_bump_slice(); - - Apply(function, args, CalledVia::BinOp(Pizza)) - } - } - } - binop => { - // This is a normal binary operator like (+), so desugar it - // into the appropriate function call. - let (module_name, ident) = binop_to_function(binop); - let mut args = Vec::with_capacity_in(2, arena); - - args.push(left); - args.push(right); - - let loc_expr = arena.alloc(Located { - value: Expr::Var { module_name, ident }, - region: loc_op.region, - }); - - let args = args.into_bump_slice(); - - Apply(loc_expr, args, CalledVia::BinOp(binop)) - } - }; - - arg_stack.push(arena.alloc(Located { region, value })); - } - - assert_eq!(arg_stack.len(), 1); - - arg_stack.pop().unwrap() -} - -#[derive(Debug, Clone, PartialEq)] -enum InfixToken<'a> { - Arg(&'a Located>), - Op(Located), -} - -/// An iterator that takes an expression that has had its operators grouped -/// with _right associativity_, and yeilds a sequence of `InfixToken`s. This -/// is useful for reparsing the operators with their correct associativies -/// and precedences. -/// -/// For example, the expression: -/// -/// ```text -/// (1 + (2 ^ (4 * (6 - 8)))) -/// ``` -/// -/// Will result in the following iterations: -/// -/// ```text -/// Arg: 1 -/// Op: + -/// Arg: 2 -/// Op: ^ -/// Arg: 4 -/// Op: * -/// Arg: 6 -/// Op: - -/// Arg: 8 -/// ``` -struct Infixes<'a> { - /// The next part of the expression that we need to flatten - remaining_expr: Option<&'a Located>>, - /// Cached operator from a previous iteration - next_op: Option>, -} - -impl<'a> Infixes<'a> { - fn new(expr: &'a Located>) -> Infixes<'a> { - Infixes { - remaining_expr: Some(expr), - next_op: None, - } - } -} - -impl<'a> Iterator for Infixes<'a> { - type Item = InfixToken<'a>; - - fn next(&mut self) -> Option> { - match self.next_op.take() { - Some(op) => Some(InfixToken::Op(op)), - None => self - .remaining_expr - .take() - .map(|loc_expr| match loc_expr.value { - Expr::BinOp((left, loc_op, right)) - | Expr::Nested(Expr::BinOp((left, loc_op, right))) => { - self.remaining_expr = Some(right); - self.next_op = Some(*loc_op); - - InfixToken::Arg(left) - } - _ => InfixToken::Arg(loc_expr), - }), - } - } -} From 1871697b0f2515b622556f4d9fefffcb24c68e1a Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 23:01:12 +0100 Subject: [PATCH 11/14] remove BinOp variant --- compiler/can/src/expr.rs | 6 ---- compiler/can/src/operator.rs | 1 - compiler/fmt/src/expr.rs | 62 ------------------------------------ compiler/parse/src/ast.rs | 1 - compiler/parse/src/expr.rs | 1 - editor/src/lang/expr.rs | 4 +-- 6 files changed, 2 insertions(+), 73 deletions(-) diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 621670fb57..3eaffe6e55 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -803,12 +803,6 @@ pub fn canonicalize_expr<'a>( bad_expr ); } - bad_expr @ ast::Expr::BinOp(_) => { - panic!( - "A binary operator did not get desugared somehow: {:#?}", - bad_expr - ); - } bad_expr @ ast::Expr::BinOps { .. } => { panic!( "A binary operator chain did not get desugared somehow: {:#?}", diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index bb24c55b37..62a7dcf82c 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -292,7 +292,6 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a _ => panic!(), } } - BinOp(_) | Nested(BinOp(_)) => todo!(), // desugar_bin_op(arena, loc_expr), BinOps(lefts, right) | Nested(BinOps(lefts, right)) => { desugar_bin_ops(arena, loc_expr.region, lefts, right) } diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index ee76fe7995..f04cfdbe35 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -65,15 +65,6 @@ impl<'a> Formattable<'a> for Expr<'a> { .any(|(c, t)| c.is_multiline() || t.is_multiline()) } - BinOp((loc_left, _, loc_right)) => { - let next_is_multiline_bin_op: bool = match &loc_right.value { - Expr::BinOp((_, _, nested_loc_right)) => nested_loc_right.is_multiline(), - _ => false, - }; - - next_is_multiline_bin_op || loc_left.is_multiline() || loc_right.is_multiline() - } - BinOps(lefts, loc_right) => { lefts.iter().any(|(loc_expr, _)| loc_expr.is_multiline()) || loc_right.is_multiline() @@ -294,15 +285,6 @@ impl<'a> Formattable<'a> for Expr<'a> { } => { fmt_list(buf, &items, final_comments, indent); } - BinOp((loc_left_side, bin_op, loc_right_side)) => fmt_bin_op( - buf, - loc_left_side, - bin_op, - loc_right_side, - false, - parens, - indent, - ), BinOps(lefts, right) => fmt_bin_ops(buf, lefts, right, false, parens, indent), UnaryOp(sub_expr, unary_op) => { match &unary_op.value { @@ -390,50 +372,6 @@ fn push_op(buf: &mut String, op: BinOp) { } } -fn fmt_bin_op<'a>( - buf: &mut String<'a>, - loc_left_side: &'a Located>, - loc_bin_op: &'a Located, - loc_right_side: &'a Located>, - part_of_multi_line_bin_ops: bool, - apply_needs_parens: Parens, - indent: u16, -) { - loc_left_side.format_with_options(buf, apply_needs_parens, Newlines::No, indent); - - let is_multiline = (&loc_right_side.value).is_multiline() - || (&loc_left_side.value).is_multiline() - || part_of_multi_line_bin_ops; - - if is_multiline { - newline(buf, indent + INDENT) - } else { - buf.push(' '); - } - - push_op(buf, loc_bin_op.value); - - buf.push(' '); - - match &loc_right_side.value { - Expr::BinOp((nested_left_side, nested_bin_op, nested_right_side)) => { - fmt_bin_op( - buf, - nested_left_side, - nested_bin_op, - nested_right_side, - is_multiline, - apply_needs_parens, - indent, - ); - } - - _ => { - loc_right_side.format_with_options(buf, apply_needs_parens, Newlines::Yes, indent); - } - } -} - fn fmt_bin_ops<'a>( buf: &mut String<'a>, lefts: &'a [(Located>, Located)], diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index e3d6ced645..1217561349 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -129,7 +129,6 @@ pub enum Expr<'a> { /// To apply by name, do Apply(Var(...), ...) /// To apply a tag by name, do Apply(Tag(...), ...) Apply(&'a Loc>, &'a [&'a Loc>], CalledVia), - BinOp(&'a (Loc>, Loc, Loc>)), BinOps(&'a [(Loc>, Loc)], &'a Loc>), UnaryOp(&'a Loc>, Loc), diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index 3b97de791a..da5716d630 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -1372,7 +1372,6 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result( bad_expr ); } - bad_expr @ BinOp(_) => { + bad_expr @ BinOps { .. } => { panic!( - "A binary operator did not get desugared somehow: {:#?}", + "A binary operator chain did not get desugared somehow: {:#?}", bad_expr ); } From 03fab9297c0ba410f753694fc592babe18d70668 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 23:37:36 +0100 Subject: [PATCH 12/14] shrink stack size usage for parse Expr --- compiler/parse/src/ast.rs | 2 +- compiler/parse/src/expr.rs | 2 +- compiler/parse/tests/test_parse.rs | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 1217561349..b92e3debc3 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -101,7 +101,7 @@ pub enum Expr<'a> { RecordUpdate { update: &'a Loc>, fields: &'a [Loc>>], - final_comments: &'a [CommentOrNewline<'a>], + final_comments: &'a &'a [CommentOrNewline<'a>], }, Record { diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index da5716d630..bc9075eced 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -2075,7 +2075,7 @@ fn record_literal_help<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>, EExpr<' Some(update) => Expr::RecordUpdate { update: &*arena.alloc(update), fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), - final_comments: loc_assigned_fields_with_comments.value.1, + final_comments: arena.alloc(loc_assigned_fields_with_comments.value.1), }, None => Expr::Record { fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 5df58fa08d..1aa5d0cba9 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -447,7 +447,7 @@ mod test_parse { &[], arena.alloc(Located::new(0, 0, 25, 26, Num("0"))), ); - let fields = &[ + let fields: &[_] = &[ Located::new(0, 0, 16, 20, label1), Located::new(0, 0, 22, 26, label2), ]; @@ -459,7 +459,7 @@ mod test_parse { let expected = RecordUpdate { update: &*arena.alloc(update_target), fields, - final_comments: &[], + final_comments: &(&[] as &[_]), }; let actual = parse_expr_with(&arena, "{ Foo.Bar.baz & x: 5, y: 0 }"); @@ -3379,6 +3379,11 @@ mod test_parse { assert!(actual.is_ok()); } + #[test] + fn parse_expr_size() { + assert_eq!(std::mem::size_of::(), 40); + } + // PARSE ERROR // TODO this should be parse error, but isn't! From 8b8afec50e08e596dd0c0041275d6db33bbf03b6 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 20 Mar 2021 23:51:55 +0100 Subject: [PATCH 13/14] remove allocation --- compiler/can/src/operator.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 62a7dcf82c..c046e6b2b5 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -502,8 +502,8 @@ fn desugar_bin_ops<'a>( lefts: &'a [(Located>, Located)], right: &'a Located>, ) -> &'a Located> { - let mut arg_stack: Vec<&'a Located> = Vec::new_in(arena); - let mut op_stack: Vec> = Vec::new_in(arena); + let mut arg_stack: Vec<&'a Located> = Vec::with_capacity_in(lefts.len() + 1, arena); + let mut op_stack: Vec> = Vec::with_capacity_in(lefts.len(), arena); for (loc_expr, loc_op) in lefts { arg_stack.push(desugar_expr(arena, loc_expr)); @@ -524,7 +524,7 @@ fn desugar_bin_ops<'a>( Pizza => { // Rewrite the Pizza operator into an Apply - match &right.value { + match right.value { Apply(function, arguments, _called_via) => { let mut args = Vec::with_capacity_in(1 + arguments.len(), arena); @@ -538,20 +538,15 @@ fn desugar_bin_ops<'a>( Apply(function, args, CalledVia::BinOp(Pizza)) } - expr => { + _ => { // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` let mut args = Vec::with_capacity_in(1, arena); args.push(left); - let function = arena.alloc(Located { - value: Nested(expr), - region: right.region, - }); - let args = args.into_bump_slice(); - Apply(function, args, CalledVia::BinOp(Pizza)) + Apply(right, args, CalledVia::BinOp(Pizza)) } } } From 165a2d83ff3cc063f271cc725525263b4b2e4925 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 21 Mar 2021 00:24:58 +0100 Subject: [PATCH 14/14] cleanup --- compiler/can/src/operator.rs | 101 ++++++++--------------------------- 1 file changed, 22 insertions(+), 79 deletions(-) diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index c046e6b2b5..520e67e2f1 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -28,10 +28,7 @@ fn new_op_call_expr<'a>( let mut args = Vec::with_capacity_in(1 + arguments.len(), arena); args.push(left); - - for arg in arguments.iter() { - args.push(arg); - } + args.extend(arguments.iter()); let args = args.into_bump_slice(); @@ -39,13 +36,7 @@ fn new_op_call_expr<'a>( } _ => { // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` - let mut args = Vec::with_capacity_in(1, arena); - - args.push(left); - - let args = args.into_bump_slice(); - - Apply(right, args, CalledVia::BinOp(Pizza)) + Apply(right, arena.alloc([left]), CalledVia::BinOp(Pizza)) } } } @@ -273,12 +264,12 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a // first desugar the body, because it may contain |> let desugared_body = desugar_expr(arena, loc_body); + let desugared_ret = desugar_expr(arena, loc_ret); + let closure = Expr::Closure(loc_patterns, desugared_ret); + let loc_closure = Located::at_zero(closure); + match &desugared_body.value { Expr::Apply(function, arguments, called_via) => { - let desugared_ret = desugar_expr(arena, loc_ret); - let closure = Expr::Closure(loc_patterns, desugared_ret); - let loc_closure = Located::at_zero(closure); - let mut new_arguments: Vec<'a, &'a Located>> = Vec::with_capacity_in(arguments.len() + 1, arena); new_arguments.extend(arguments.iter()); @@ -289,7 +280,17 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a arena.alloc(loc_call) } - _ => panic!(), + _ => { + // e.g. `x <- (if b then (\a -> a) else (\c -> c))` + let call = Expr::Apply( + desugared_body, + arena.alloc([&*arena.alloc(loc_closure)]), + CalledVia::Space, + ); + let loc_call = Located::at(loc_expr.region, call); + + arena.alloc(loc_call) + } } } BinOps(lefts, right) | Nested(BinOps(lefts, right)) => { @@ -371,9 +372,7 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a }, }; let loc_fn_var = arena.alloc(Located { region, value }); - let desugared_args = bumpalo::vec![in arena; desugar_expr(arena, loc_arg)]; - - let desugared_args = desugared_args.into_bump_slice(); + let desugared_args = arena.alloc([desugar_expr(arena, loc_arg)]); arena.alloc(Located { value: Apply(loc_fn_var, desugared_args, CalledVia::UnaryOp(op)), @@ -513,69 +512,13 @@ fn desugar_bin_ops<'a>( } } - arg_stack.push(desugar_expr(arena, right)); + let mut expr = desugar_expr(arena, right); - for loc_op in op_stack.into_iter().rev() { - let right = arg_stack.pop().unwrap(); - let left = arg_stack.pop().unwrap(); - - let region = Region::span_across(&left.region, &right.region); - let value = match loc_op.value { - Pizza => { - // Rewrite the Pizza operator into an Apply - - match right.value { - Apply(function, arguments, _called_via) => { - let mut args = Vec::with_capacity_in(1 + arguments.len(), arena); - - args.push(left); - - for arg in arguments.iter() { - args.push(arg); - } - - let args = args.into_bump_slice(); - - Apply(function, args, CalledVia::BinOp(Pizza)) - } - _ => { - // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` - let mut args = Vec::with_capacity_in(1, arena); - - args.push(left); - - let args = args.into_bump_slice(); - - Apply(right, args, CalledVia::BinOp(Pizza)) - } - } - } - binop => { - // This is a normal binary operator like (+), so desugar it - // into the appropriate function call. - let (module_name, ident) = binop_to_function(binop); - let mut args = Vec::with_capacity_in(2, arena); - - args.push(left); - args.push(right); - - let loc_expr = arena.alloc(Located { - value: Expr::Var { module_name, ident }, - region: loc_op.region, - }); - - let args = args.into_bump_slice(); - - Apply(loc_expr, args, CalledVia::BinOp(binop)) - } - }; - - arg_stack.push(arena.alloc(Located { region, value })); + for (left, loc_op) in arg_stack.into_iter().zip(op_stack.into_iter()).rev() { + expr = arena.alloc(new_op_call_expr(arena, left, loc_op, expr)); } - assert_eq!(arg_stack.len(), 1); - - arg_stack.pop().unwrap() + expr } enum Step<'a> {