From 83a86e6dc030bbc42675f5a1e70ba9898cfe89fc Mon Sep 17 00:00:00 2001 From: KtorZ Date: Thu, 9 Feb 2023 13:57:12 +0100 Subject: [PATCH] Fix logical operator precedence in parser. Whoopsie... || and && were treated with the same precedence, causing very surprising behavior down the line. I noticed this because of the auto-formatter adding parenthesis where it really shouldn't. The problem came actually from the parser and how it constructed the AST. --- crates/aiken-lang/src/parser.rs | 72 +++++++----- crates/aiken-lang/src/tests/format.rs | 37 ++++++- crates/aiken-lang/src/tests/parser.rs | 151 +++++++++++++++++++++----- 3 files changed, 200 insertions(+), 60 deletions(-) diff --git a/crates/aiken-lang/src/parser.rs b/crates/aiken-lang/src/parser.rs index 99d4c633..383257cb 100644 --- a/crates/aiken-lang/src/parser.rs +++ b/crates/aiken-lang/src/parser.rs @@ -1301,13 +1301,9 @@ pub fn expr_parser( }) .boxed(); - // Logical - let op = choice(( - just(Token::AmperAmper).to(BinOp::And), - just(Token::VbarVbar).to(BinOp::Or), - )); - - let logical = comparison + // Conjunction + let op = just(Token::AmperAmper).to(BinOp::And); + let conjunction = comparison .clone() .then(op.then(comparison).repeated()) .foldl(|a, (op, b)| expr::UntypedExpr::BinOp { @@ -1318,10 +1314,23 @@ pub fn expr_parser( }) .boxed(); - // Pipeline - logical + // Disjunction + let op = just(Token::VbarVbar).to(BinOp::Or); + let disjunction = conjunction .clone() - .then(just(Token::Pipe).ignore_then(logical).repeated()) + .then(op.then(conjunction).repeated()) + .foldl(|a, (op, b)| expr::UntypedExpr::BinOp { + location: a.location().union(b.location()), + name: op, + left: Box::new(a), + right: Box::new(b), + }) + .boxed(); + + // Pipeline + disjunction + .clone() + .then(just(Token::Pipe).ignore_then(disjunction).repeated()) .foldl(|l, r| { let expressions = if let expr::UntypedExpr::PipeLine { mut expressions } = l { expressions.push(r); @@ -1421,30 +1430,33 @@ pub fn when_clause_guard_parser() -> impl Parser }) .boxed(); - let logical_op = choice(( - just(Token::AmperAmper).to(BinOp::And), - just(Token::VbarVbar).to(BinOp::Or), - )); - - comparison + let and_op = just(Token::AmperAmper); + let conjunction = comparison .clone() - .then(logical_op.then(comparison).repeated()) - .foldl(|left, (op, right)| { + .then(and_op.then(comparison).repeated()) + .foldl(|left, (_tok, right)| { let location = left.location().union(right.location()); let left = Box::new(left); let right = Box::new(right); - match op { - BinOp::And => ast::ClauseGuard::And { - location, - left, - right, - }, - BinOp::Or => ast::ClauseGuard::Or { - location, - left, - right, - }, - _ => unreachable!(), + ast::ClauseGuard::And { + location, + left, + right, + } + }); + + let or_op = just(Token::VbarVbar); + conjunction + .clone() + .then(or_op.then(conjunction).repeated()) + .foldl(|left, (_tok, right)| { + let location = left.location().union(right.location()); + let left = Box::new(left); + let right = Box::new(right); + ast::ClauseGuard::Or { + location, + left, + right, } }) }) diff --git a/crates/aiken-lang/src/tests/format.rs b/crates/aiken-lang/src/tests/format.rs index 5746b747..d5a053f3 100644 --- a/crates/aiken-lang/src/tests/format.rs +++ b/crates/aiken-lang/src/tests/format.rs @@ -14,7 +14,7 @@ fn assert_fmt(src: &str, expected: &str) { let (module2, extra2) = parser::module(&out, ModuleKind::Lib).unwrap(); let mut out2 = String::new(); format::pretty(&mut out2, module2, extra2, &out); - assert!(out == out2, "formatting isn't idempotent"); + assert_eq!(out, out2, "formatting isn't idempotent"); } #[test] @@ -233,7 +233,7 @@ fn test_negate() { } #[test] -fn test_block_expr() { +fn test_block_arithmetic_expr() { let src = indoc! {r#" fn foo() { ( 14 + 42 ) * 1337 @@ -257,6 +257,39 @@ fn test_block_expr() { assert_fmt(src, expected); } +#[test] +fn test_block_logical_expr() { + let src = indoc! {r#" + fn foo() { + !(a && b) + } + + fn bar() { + (a || b) && (c || d) + } + + fn baz() { + a || (b && c) || d + } + "#}; + + let expected = indoc! {r#" + fn foo() { + !(a && b) + } + + fn bar() { + ( a || b ) && ( c || d ) + } + + fn baz() { + a || b && c || d + } + "#}; + + assert_fmt(src, expected); +} + #[test] fn test_format_bytearray_literals() { let src = indoc! {r#" diff --git a/crates/aiken-lang/src/tests/parser.rs b/crates/aiken-lang/src/tests/parser.rs index 845c9393..a3b042a6 100644 --- a/crates/aiken-lang/src/tests/parser.rs +++ b/crates/aiken-lang/src/tests/parser.rs @@ -2166,7 +2166,7 @@ fn clause_guards() { _ if bar -> Void _ if True -> Void _ if a || b && c -> Void - _ if a || (b && c) -> Void + _ if (a || b) && c -> Void _ if a <= 42 || b > 14 || "str" -> Void _ if a == 14 && !b -> Void _ if !!True -> Void @@ -2241,25 +2241,25 @@ fn clause_guards() { location: Span::new((), 92..93), }], alternative_patterns: vec![], - guard: Some(ast::ClauseGuard::And { + guard: Some(ast::ClauseGuard::Or { location: Span::new((), 97..108), - left: Box::new(ast::ClauseGuard::Or { - location: Span::new((), 97..103), + left: Box::new(ast::ClauseGuard::Var { + location: Span::new((), 97..98), + name: "a".to_string(), + tipo: (), + }), + right: Box::new(ast::ClauseGuard::And { + location: Span::new((), 102..108), left: Box::new(ast::ClauseGuard::Var { - location: Span::new((), 97..98), - name: "a".to_string(), - tipo: (), - }), - right: Box::new(ast::ClauseGuard::Var { location: Span::new((), 102..103), name: "b".to_string(), tipo: (), }), - }), - right: Box::new(ast::ClauseGuard::Var { - location: Span::new((), 107..108), - name: "c".to_string(), - tipo: (), + right: Box::new(ast::ClauseGuard::Var { + location: Span::new((), 107..108), + name: "c".to_string(), + tipo: (), + }), }), }), then: expr::UntypedExpr::Var { @@ -2274,25 +2274,25 @@ fn clause_guards() { location: Span::new((), 121..122), }], alternative_patterns: vec![], - guard: Some(ast::ClauseGuard::Or { - location: Span::new((), 126..138), - left: Box::new(ast::ClauseGuard::Var { - location: Span::new((), 126..127), - name: "a".to_string(), - tipo: (), - }), - right: Box::new(ast::ClauseGuard::And { - location: Span::new((), 132..138), + guard: Some(ast::ClauseGuard::And { + location: Span::new((), 127..139), + left: Box::new(ast::ClauseGuard::Or { + location: Span::new((), 127..133), left: Box::new(ast::ClauseGuard::Var { + location: Span::new((), 127..128), + name: "a".to_string(), + tipo: (), + }), + right: Box::new(ast::ClauseGuard::Var { location: Span::new((), 132..133), name: "b".to_string(), tipo: (), }), - right: Box::new(ast::ClauseGuard::Var { - location: Span::new((), 137..138), - name: "c".to_string(), - tipo: (), - }), + }), + right: Box::new(ast::ClauseGuard::Var { + location: Span::new((), 138..139), + name: "c".to_string(), + tipo: (), }), }), then: expr::UntypedExpr::Var { @@ -2420,3 +2420,98 @@ fn clause_guards() { })], ); } + +#[test] +fn scope_logical_expression() { + let code = indoc! {r#" + fn foo() { + let x = !(a && b) + let y = a || b && c || d + x + } + "#}; + assert_definitions( + code, + vec![ast::Definition::Fn(Function { + arguments: vec![], + body: expr::UntypedExpr::Sequence { + location: Span::new((), 13..61), + expressions: vec![ + expr::UntypedExpr::Assignment { + location: Span::new((), 13..30), + value: Box::new(expr::UntypedExpr::UnOp { + op: ast::UnOp::Not, + location: Span::new((), 21..29), + value: Box::new(expr::UntypedExpr::BinOp { + location: Span::new((), 23..29), + name: ast::BinOp::And, + left: Box::new(expr::UntypedExpr::Var { + location: Span::new((), 23..24), + name: "a".to_string(), + }), + right: Box::new(expr::UntypedExpr::Var { + location: Span::new((), 28..29), + name: "b".to_string(), + }), + }), + }), + pattern: ast::Pattern::Var { + location: Span::new((), 17..18), + name: "x".to_string(), + }, + kind: ast::AssignmentKind::Let, + annotation: None, + }, + expr::UntypedExpr::Assignment { + location: Span::new((), 33..57), + value: Box::new(expr::UntypedExpr::BinOp { + location: Span::new((), 41..57), + name: ast::BinOp::Or, + left: Box::new(expr::UntypedExpr::BinOp { + location: Span::new((), 41..52), + name: ast::BinOp::Or, + left: Box::new(expr::UntypedExpr::Var { + location: Span::new((), 41..42), + name: "a".to_string(), + }), + right: Box::new(expr::UntypedExpr::BinOp { + location: Span::new((), 46..52), + name: ast::BinOp::And, + left: Box::new(expr::UntypedExpr::Var { + location: Span::new((), 46..47), + name: "b".to_string(), + }), + right: Box::new(expr::UntypedExpr::Var { + location: Span::new((), 51..52), + name: "c".to_string(), + }), + }), + }), + right: Box::new(expr::UntypedExpr::Var { + location: Span::new((), 56..57), + name: "d".to_string(), + }), + }), + pattern: ast::Pattern::Var { + location: Span::new((), 37..38), + name: "y".to_string(), + }, + kind: ast::AssignmentKind::Let, + annotation: None, + }, + expr::UntypedExpr::Var { + location: Span::new((), 60..61), + name: "x".to_string(), + }, + ], + }, + doc: None, + location: Span::new((), 0..8), + name: "foo".to_string(), + public: false, + return_annotation: None, + return_type: (), + end_position: 62, + })], + ) +}