From 68aa33d8bad546a2d6116d392a8b8db01955189b Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 19:42:41 -0500 Subject: [PATCH 01/10] Introduce Expr::Nested --- src/can/mod.rs | 6 ++++++ src/can/operator.rs | 9 +++++++-- src/fmt/expr.rs | 2 +- src/parse/ast.rs | 4 ++++ src/parse/mod.rs | 2 +- 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/can/mod.rs b/src/can/mod.rs index ff0f315559..984cde086a 100644 --- a/src/can/mod.rs +++ b/src/can/mod.rs @@ -860,6 +860,12 @@ fn canonicalize_expr( local_successors(&References::new(), &env.closures) ); } + ast::Expr::Nested(sub_expr) => { + let (answer, output) = + canonicalize_expr(rigids, env, var_store, scope, region, sub_expr, expected); + + (answer.value, output) + } ast::Expr::BinaryInt(string) => { let (constraint, answer) = int_expr_from_result(var_store, finish_parsing_bin(string), env, expected, region); diff --git a/src/can/operator.rs b/src/can/operator.rs index 62c6e1bf72..37fc71b7d8 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -282,10 +282,15 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca let mut desugared_branches = Vec::with_capacity_in(branches.len(), arena); for (loc_pattern, loc_branch_expr) in branches.into_iter() { + let desugared = desugar(arena, &loc_branch_expr); + // TODO FIXME cloning performance disaster desugared_branches.push(&*arena.alloc(( loc_pattern.clone(), - desugar(arena, &loc_branch_expr).clone(), + Located { + region: desugared.region, + value: Nested(&desugared.value), + }, ))); } @@ -317,7 +322,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca region: loc_expr.region, }) } - SpaceBefore(expr, _) | SpaceAfter(expr, _) | ParensAround(expr) => { + SpaceBefore(expr, _) | SpaceAfter(expr, _) | ParensAround(expr) | Nested(expr) => { // Since we've already begun canonicalization, spaces and parens // are no longer needed and should be dropped. desugar( diff --git a/src/fmt/expr.rs b/src/fmt/expr.rs index beee192ec5..75bae8a69f 100644 --- a/src/fmt/expr.rs +++ b/src/fmt/expr.rs @@ -286,7 +286,7 @@ pub fn is_multiline_expr<'a>(expr: &'a Expr<'a>) -> bool { is_multiline_expr(&loc_subexpr.value) } - ParensAround(subexpr) => is_multiline_expr(&subexpr), + ParensAround(subexpr) | Nested(subexpr) => is_multiline_expr(&subexpr), Closure(loc_patterns, loc_body) => { // check the body first because it's more likely to be multiline diff --git a/src/parse/ast.rs b/src/parse/ast.rs index ca5d3d4e24..009a22046f 100644 --- a/src/parse/ast.rs +++ b/src/parse/ast.rs @@ -168,6 +168,10 @@ pub enum Expr<'a> { SpaceAfter(&'a Expr<'a>, &'a [CommentOrNewline<'a>]), ParensAround(&'a Expr<'a>), + /// This is used only to avoid cloning when reordering expressions (e.g. in desugar()). + /// It lets us take an (&Expr) and create a plain (Expr) from it. + Nested(&'a Expr<'a>), + // Problems MalformedIdent(&'a str), MalformedClosure, diff --git a/src/parse/mod.rs b/src/parse/mod.rs index 0d013efaba..461a2f174d 100644 --- a/src/parse/mod.rs +++ b/src/parse/mod.rs @@ -272,7 +272,7 @@ fn expr_to_pattern<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result, spaces, )), - Expr::ParensAround(sub_expr) => expr_to_pattern(arena, sub_expr), + Expr::ParensAround(sub_expr) | Expr::Nested(sub_expr) => expr_to_pattern(arena, sub_expr), Expr::Record(loc_assigned_fields) => { let mut loc_patterns = Vec::with_capacity_in(loc_assigned_fields.len(), arena); From 7a94a0e576e6483b77f347e65649a851b034f51e Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 19:49:52 -0500 Subject: [PATCH 02/10] Use Expr::Nested without infinite recursion --- src/can/operator.rs | 62 ++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/src/can/operator.rs b/src/can/operator.rs index 37fc71b7d8..ea6527262a 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -39,25 +39,39 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca match &loc_expr.value { Float(_) + | Nested(Float(_)) | Int(_) + | Nested(Int(_)) | HexInt(_) + | Nested(HexInt(_)) | OctalInt(_) + | Nested(OctalInt(_)) | BinaryInt(_) + | Nested(BinaryInt(_)) | Str(_) + | Nested(Str(_)) | BlockStr(_) + | Nested(BlockStr(_)) | QualifiedField(_, _) + | Nested(QualifiedField(_, _)) | AccessorFunction(_) + | Nested(AccessorFunction(_)) | Var(_, _) + | Nested(Var(_, _)) | MalformedIdent(_) + | Nested(MalformedIdent(_)) | MalformedClosure + | Nested(MalformedClosure) | PrecedenceConflict(_, _, _) - | Variant(_, _) => loc_expr, + | Nested(PrecedenceConflict(_, _, _)) + | Variant(_, _) + | Nested(Variant(_, _)) => loc_expr, - Field(sub_expr, paths) => arena.alloc(Located { + Field(sub_expr, paths) | Nested(Field(sub_expr, paths)) => arena.alloc(Located { region: loc_expr.region, value: Field(desugar(arena, sub_expr), paths.clone()), }), - List(elems) => { + List(elems) | Nested(List(elems)) => { let mut new_elems = Vec::with_capacity_in(elems.len(), arena); for elem in elems { @@ -70,7 +84,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca value, }) } - Record(fields) => { + Record(fields) | Nested(Record(fields)) => { let mut new_fields = Vec::with_capacity_in(fields.len(), arena); for field in fields { @@ -87,11 +101,13 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca value: Record(new_fields), }) } - Closure(loc_patterns, loc_ret) => arena.alloc(Located { - region: loc_expr.region, - value: Closure(loc_patterns, desugar(arena, loc_ret)), - }), - BinOp(_) => { + Closure(loc_patterns, loc_ret) | Nested(Closure(loc_patterns, loc_ret)) => { + arena.alloc(Located { + region: loc_expr.region, + value: Closure(loc_patterns, desugar(arena, loc_ret)), + }) + } + BinOp(_) | Nested(BinOp(_)) => { let mut infixes = Infixes::new(arena.alloc(loc_expr)); let mut arg_stack: Vec<&'a Located> = Vec::new_in(arena); let mut op_stack: Vec> = Vec::new_in(arena); @@ -251,7 +267,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca arg_stack.pop().unwrap() } - Defs(defs, loc_ret) => { + Defs(defs, loc_ret) | Nested(Defs(defs, loc_ret)) => { let mut desugared_defs = Vec::with_capacity_in(defs.len(), arena); for loc_def in defs.into_iter() { @@ -265,7 +281,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca region: loc_expr.region, }) } - Apply(loc_fn, loc_args, called_via) => { + Apply(loc_fn, loc_args, called_via) | Nested(Apply(loc_fn, loc_args, called_via)) => { let mut desugared_args = Vec::with_capacity_in(loc_args.len(), arena); for loc_arg in loc_args { @@ -277,7 +293,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca region: loc_expr.region, }) } - Case(loc_cond_expr, branches) => { + Case(loc_cond_expr, branches) | Nested(Case(loc_cond_expr, branches)) => { let loc_desugared_cond = &*arena.alloc(desugar(arena, &loc_cond_expr)); let mut desugared_branches = Vec::with_capacity_in(branches.len(), arena); @@ -299,7 +315,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca region: loc_expr.region, }) } - UnaryOp(loc_arg, loc_op) => { + UnaryOp(loc_arg, loc_op) | Nested(UnaryOp(loc_arg, loc_op)) => { use crate::operator::UnaryOp::*; let region = loc_op.region; @@ -322,25 +338,25 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca region: loc_expr.region, }) } - SpaceBefore(expr, _) | SpaceAfter(expr, _) | ParensAround(expr) | Nested(expr) => { + SpaceBefore(expr, _) + | Nested(SpaceBefore(expr, _)) + | SpaceAfter(expr, _) + | Nested(SpaceAfter(expr, _)) + | ParensAround(expr) + | Nested(ParensAround(expr)) + | Nested(Nested(expr)) => { // Since we've already begun canonicalization, spaces and parens // are no longer needed and should be dropped. desugar( arena, arena.alloc(Located { - // TODO FIXME performance disaster!!! Must remove this clone! - // - // This won't be easy because: - // - // * If this function takes an &'a Expr, then Infixes hits a problem. - // * If SpaceBefore holds a Loc<&'a Expr>, then Spaceable hits a problem. - // * If all the existing &'a Loc values become Loc<&'a Expr>...who knows? - value: (*expr).clone(), + value: Nested(expr), region: loc_expr.region, }), ) } - If((condition, then_branch, else_branch)) => { + If((condition, then_branch, else_branch)) + | Nested(If((condition, then_branch, else_branch))) => { // desugar if into case, meaning that // // if b then x else y From c9ebb03347cbf48bab2ff63d5e34f30168316605 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 19:50:12 -0500 Subject: [PATCH 03/10] Introduce Pattern::Nested --- src/can/mod.rs | 4 ++-- src/can/operator.rs | 6 ++++-- src/can/pattern.rs | 5 +++-- src/fmt/pattern.rs | 4 ++++ src/parse/ast.rs | 4 ++++ 5 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/can/mod.rs b/src/can/mod.rs index 984cde086a..130dcdb761 100644 --- a/src/can/mod.rs +++ b/src/can/mod.rs @@ -1264,7 +1264,7 @@ fn add_idents_from_pattern<'a>( RecordField(_, _) => { panic!("TODO implement RecordField pattern in add_idents_from_pattern."); } - SpaceBefore(pattern, _) | SpaceAfter(pattern, _) => { + SpaceBefore(pattern, _) | SpaceAfter(pattern, _) | Nested(pattern) => { // Ignore the newline/comment info; it doesn't matter in canonicalization. add_idents_from_pattern(region, pattern, scope, answer) } @@ -1306,7 +1306,7 @@ fn remove_idents(pattern: &ast::Pattern, idents: &mut ImMap { panic!("TODO implement RecordField pattern in remove_idents."); } - SpaceBefore(pattern, _) | SpaceAfter(pattern, _) => { + SpaceBefore(pattern, _) | SpaceAfter(pattern, _) | Nested(pattern) => { // Ignore the newline/comment info; it doesn't matter in canonicalization. remove_idents(pattern, idents) } diff --git a/src/can/operator.rs b/src/can/operator.rs index ea6527262a..47460fb40e 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -300,9 +300,11 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca for (loc_pattern, loc_branch_expr) in branches.into_iter() { let desugared = desugar(arena, &loc_branch_expr); - // TODO FIXME cloning performance disaster desugared_branches.push(&*arena.alloc(( - loc_pattern.clone(), + Located { + region: loc_pattern.region, + value: Pattern::Nested(&loc_pattern.value), + }, Located { region: desugared.region, value: Nested(&desugared.value), diff --git a/src/can/pattern.rs b/src/can/pattern.rs index c4639a5cc0..f2118026ba 100644 --- a/src/can/pattern.rs +++ b/src/can/pattern.rs @@ -258,7 +258,7 @@ pub fn canonicalize_pattern<'a>( }, // &EmptyRecordLiteral => Pattern::EmptyRecordLiteral, - &SpaceBefore(sub_pattern, _) | SpaceAfter(sub_pattern, _) => { + &SpaceBefore(sub_pattern, _) | SpaceAfter(sub_pattern, _) | Nested(sub_pattern) => { return canonicalize_pattern( env, state, @@ -271,6 +271,7 @@ pub fn canonicalize_pattern<'a>( expected, ) } + _ => panic!("TODO finish restoring can_pattern branch for {:?}", pattern), }; @@ -356,7 +357,7 @@ fn add_constraints<'a>( )); } - SpaceBefore(pattern, _) | SpaceAfter(pattern, _) => { + SpaceBefore(pattern, _) | SpaceAfter(pattern, _) | Nested(pattern) => { add_constraints(pattern, scope, region, expected, state) } diff --git a/src/fmt/pattern.rs b/src/fmt/pattern.rs index fba3513214..76f67af4ce 100644 --- a/src/fmt/pattern.rs +++ b/src/fmt/pattern.rs @@ -84,6 +84,10 @@ pub fn fmt_pattern<'a>( fmt_spaces(buf, spaces.iter(), indent); } + Nested(sub_pattern) => { + fmt_pattern(buf, sub_pattern, indent, apply_needs_parens); + } + // Malformed Malformed(string) => buf.push_str(string), QualifiedIdentifier(maybe_qualified) => { diff --git a/src/parse/ast.rs b/src/parse/ast.rs index 009a22046f..2133492679 100644 --- a/src/parse/ast.rs +++ b/src/parse/ast.rs @@ -283,6 +283,10 @@ pub enum Pattern<'a> { /// can only occur inside of a RecordDestructure RecordField(&'a str, &'a Loc>), + /// This is used only to avoid cloning when reordering expressions (e.g. in desugar()). + /// It lets us take an (&Expr) and create a plain (Expr) from it. + Nested(&'a Pattern<'a>), + // Literal IntLiteral(&'a str), HexIntLiteral(&'a str), From e687b500d8f029cf829d6278116f257ed61468ea Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 20:16:12 -0500 Subject: [PATCH 04/10] Replace some more clone()s with Nested --- src/can/operator.rs | 70 +++++++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 21 deletions(-) diff --git a/src/can/operator.rs b/src/can/operator.rs index 47460fb40e..4a36a70fe9 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -127,9 +127,15 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca infixes.next_op = Some(next_op); arg_stack.push(arena.alloc(new_op_expr( arena, - left.clone(), + Located { + value: Nested(&left.value), + region: left.region, + }, stack_op, - right.clone(), + Located { + value: Nested(&right.value), + region: right.region, + }, ))); } @@ -152,9 +158,15 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca infixes.next_op = Some(next_op); arg_stack.push(arena.alloc(new_op_expr( arena, - left.clone(), + Located { + value: Nested(&left.value), + region: left.region, + }, stack_op, - right.clone(), + Located { + value: Nested(&right.value), + region: right.region, + }, ))); } @@ -172,9 +184,15 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca let left = arg_stack.pop().unwrap(); let broken_expr = new_op_expr( arena, - left.clone(), + Located { + value: Nested(&left.value), + region: left.region, + }, next_op, - right.clone(), + Located { + value: Nested(&right.value), + region: right.region, + }, ); let region = broken_expr.region; let value = Expr::PrecedenceConflict( @@ -234,7 +252,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca args.push(*arena.alloc(left)); let function = arena.alloc(Located { - value: expr.clone(), + value: Nested(expr), region: right.region, }); @@ -387,7 +405,10 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca value: Pattern::Variant(&[], "False"), region: pattern_region, }, - else_branch.clone(), + Located { + value: Nested(&else_branch.value), + region: else_branch.region, + }, ))); branches.push(&*arena.alloc(( @@ -395,7 +416,10 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca value: Pattern::Underscore, region: pattern_region, }, - then_branch.clone(), + Located { + value: Nested(&then_branch.value), + region: then_branch.region, + }, ))); desugar( @@ -440,16 +464,20 @@ fn desugar_field<'a>( use crate::parse::ast::AssignedField::*; match field { - LabeledValue(ref loc_str, spaces, loc_expr) => { - AssignedField::LabeledValue(loc_str.clone(), spaces, desugar(arena, loc_expr)) - } - LabelOnly(ref loc_str) => LabelOnly(loc_str.clone()), - SpaceBefore(ref field, spaces) => { - SpaceBefore(arena.alloc(desugar_field(arena, field)), spaces) - } - SpaceAfter(ref field, spaces) => { - SpaceAfter(arena.alloc(desugar_field(arena, field)), spaces) - } + LabeledValue(loc_str, spaces, loc_expr) => AssignedField::LabeledValue( + Located { + value: loc_str.value, + region: loc_str.region, + }, + spaces, + desugar(arena, loc_expr), + ), + LabelOnly(loc_str) => LabelOnly(Located { + value: loc_str.value, + region: loc_str.region, + }), + SpaceBefore(field, spaces) => SpaceBefore(arena.alloc(desugar_field(arena, field)), spaces), + SpaceAfter(field, spaces) => SpaceAfter(arena.alloc(desugar_field(arena, field)), spaces), Malformed(string) => Malformed(string), } @@ -584,9 +612,9 @@ impl<'a> Iterator for Infixes<'a> { .remaining_expr .take() .map(|loc_expr| match loc_expr.value { - Expr::BinOp((left, op, right)) => { + Expr::BinOp((left, loc_op, right)) => { self.remaining_expr = Some(right); - self.next_op = Some(op.clone()); + self.next_op = Some(loc_op.clone()); InfixToken::Arg(left) } From eb6ec90cc608fa08e5f46be3730a1eb4365b2793 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 20:22:10 -0500 Subject: [PATCH 05/10] Remove an unnecessary alloc() --- src/can/operator.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/can/operator.rs b/src/can/operator.rs index 4a36a70fe9..fd8fff37ba 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -249,7 +249,8 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca expr => { // e.g. `1 |> (if b then (\a -> a) else (\c -> c))` let mut args = Vec::with_capacity_in(1, arena); - args.push(*arena.alloc(left)); + + args.push(left); let function = arena.alloc(Located { value: Nested(expr), From 540167643274d9e50d3758d8e37b42de8e21fdaa Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 20:23:22 -0500 Subject: [PATCH 06/10] Fix an error message --- src/can/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/can/mod.rs b/src/can/mod.rs index 130dcdb761..3cbe917141 100644 --- a/src/can/mod.rs +++ b/src/can/mod.rs @@ -909,7 +909,7 @@ fn canonicalize_expr( } ast::Expr::UnaryOp(_, loc_op) => { panic!( - "A binary operator did not get desugared somehow: {:?}", + "A unary operator did not get desugared somehow: {:?}", loc_op ); } From 02253883b2582a73dfbef8ffa759e4cba8c4b5f2 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 21:10:57 -0500 Subject: [PATCH 07/10] Drop obsolete comment --- src/can/operator.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/can/operator.rs b/src/can/operator.rs index fd8fff37ba..b82fba2282 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -400,7 +400,6 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca // no type errors will occur here so using this region should be fine let pattern_region = condition.region; - // TODO make False qualified branches.push(&*arena.alloc(( Located { value: Pattern::Variant(&[], "False"), From f069bf252c1df3f53070c5ca3d46cb9506270c33 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 21:11:06 -0500 Subject: [PATCH 08/10] Fix bug with Nested and BinOp --- src/can/operator.rs | 375 +++++++++++++++++++++++--------------------- 1 file changed, 192 insertions(+), 183 deletions(-) diff --git a/src/can/operator.rs b/src/can/operator.rs index b82fba2282..d4ddca6043 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -34,9 +34,6 @@ fn new_op_expr<'a>( /// Reorder the expression tree based on operator precedence and associativity rules, /// then replace the BinOp nodes with Apply nodes. Also drop SpaceBefore and SpaceAfter nodes. pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Located> { - use crate::operator::Associativity::*; - use std::cmp::Ordering; - match &loc_expr.value { Float(_) | Nested(Float(_)) @@ -107,185 +104,14 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca value: Closure(loc_patterns, desugar(arena, loc_ret)), }) } - BinOp(_) | Nested(BinOp(_)) => { - let mut infixes = Infixes::new(arena.alloc(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.clone(); - 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 value = Expr::PrecedenceConflict( - bad_op, - stack_op, - arena.alloc(broken_expr), - ); - - 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(arena, arg_stack.pop().unwrap()); - let left = desugar(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 { - args.push(arg); - } - - 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, - }); - - Apply(function, args, CalledVia::BinOp(Pizza)) - } - } - } - binop => { - // This is a normal binary operator like (+), so desugar it - // into the appropriate function call. - let (module_parts, name) = desugar_binop(binop, arena); - 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_parts, name), - region: loc_op.region, - }); - - 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() - } + BinOp(_) => desugar_bin_op(arena, loc_expr), + Nested(BinOp(op)) => desugar_bin_op( + arena, + arena.alloc(Located { + value: BinOp(op), + region: loc_expr.region, + }), + ), Defs(defs, loc_ret) | Nested(Defs(defs, loc_ret)) => { let mut desugared_defs = Vec::with_capacity_in(defs.len(), arena); @@ -484,7 +310,7 @@ fn desugar_field<'a>( } #[inline(always)] -fn desugar_binop(binop: BinOp, arena: &Bump) -> (&[&str], &str) { +fn binop_to_function(binop: BinOp, arena: &Bump) -> (&[&str], &str) { use self::BinOp::*; match binop { @@ -556,6 +382,189 @@ fn desugar_binop(binop: BinOp, arena: &Bump) -> (&[&str], &str) { } } +fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Located> { + use crate::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.clone(); + 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 value = Expr::PrecedenceConflict( + bad_op, + stack_op, + arena.alloc(broken_expr), + ); + + 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(arena, arg_stack.pop().unwrap()); + let left = desugar(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 { + args.push(arg); + } + + 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, + }); + + Apply(function, args, CalledVia::BinOp(Pizza)) + } + } + } + binop => { + // This is a normal binary operator like (+), so desugar it + // into the appropriate function call. + let (module_parts, name) = binop_to_function(binop, arena); + 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_parts, name), + region: loc_op.region, + }); + + 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>), From 61825d5dab7c6777985fe8ba59d098882cd6d79c Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 21:25:13 -0500 Subject: [PATCH 09/10] Fix Nested BinOp bug in Infixes --- src/can/operator.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/can/operator.rs b/src/can/operator.rs index d4ddca6043..bab913cdb4 100644 --- a/src/can/operator.rs +++ b/src/can/operator.rs @@ -104,14 +104,7 @@ pub fn desugar<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a Loca value: Closure(loc_patterns, desugar(arena, loc_ret)), }) } - BinOp(_) => desugar_bin_op(arena, loc_expr), - Nested(BinOp(op)) => desugar_bin_op( - arena, - arena.alloc(Located { - value: BinOp(op), - region: loc_expr.region, - }), - ), + BinOp(_) | Nested(BinOp(_)) => desugar_bin_op(arena, loc_expr), Defs(defs, loc_ret) | Nested(Defs(defs, loc_ret)) => { let mut desugared_defs = Vec::with_capacity_in(defs.len(), arena); @@ -621,7 +614,8 @@ impl<'a> Iterator for Infixes<'a> { .remaining_expr .take() .map(|loc_expr| match loc_expr.value { - Expr::BinOp((left, loc_op, right)) => { + 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.clone()); From ed7529a9916e338be08419d5efd43534b28631af Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sun, 1 Dec 2019 21:36:55 -0500 Subject: [PATCH 10/10] Use ref to avoid def.clone() --- src/can/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/can/mod.rs b/src/can/mod.rs index 3cbe917141..d6acde075a 100644 --- a/src/can/mod.rs +++ b/src/can/mod.rs @@ -59,7 +59,7 @@ pub fn canonicalize_module_defs<'a>( for loc_def in loc_defs { buf.push_back(canonicalize_def( arena, - loc_def.value, + &loc_def.value, loc_def.region, home.clone(), scope, @@ -72,7 +72,7 @@ pub fn canonicalize_module_defs<'a>( fn canonicalize_def<'a>( arena: &Bump, - def: Def<'a>, + def: &'a Def<'a>, region: Region, home: Box, scope: &mut ImMap, (Symbol, Region)>, @@ -150,8 +150,7 @@ fn canonicalize_def<'a>( // Ignore spaces Def::SpaceBefore(def, _) | Def::SpaceAfter(def, _) => { - // TODO FIXME performance disaster!!! - canonicalize_def(arena, def.clone(), region, home, scope, var_store) + canonicalize_def(arena, def, region, home, scope, var_store) } } }