From 41d8c5c958006e0dc500ab62510a9a2833eb0483 Mon Sep 17 00:00:00 2001 From: Christoph Burgdorf Date: Fri, 26 Nov 2021 13:55:55 +0100 Subject: [PATCH] Lower BoolOperation expressions Fixes #488 --- crates/lowering/src/ast_utils.rs | 111 ++++++++++- crates/lowering/src/mappers/functions.rs | 45 +++-- crates/lowering/tests/lowering.rs | 1 + .../tests/snapshots/lowering__and_or.snap | 182 ++++++++++++++++++ .../fixtures/features/short_circuit.fe | 18 ++ .../ternary_expression_with_revert.fe | 8 - crates/test-files/fixtures/lowering/and_or.fe | 55 ++++++ .../test-files/fixtures/lowering/ternary.fe | 1 + crates/tests/src/features.rs | 25 ++- crates/yulgen/src/mappers/expressions.rs | 16 +- newsfragments/488.bugfix.md | 2 + 11 files changed, 420 insertions(+), 44 deletions(-) create mode 100644 crates/lowering/tests/snapshots/lowering__and_or.snap create mode 100644 crates/test-files/fixtures/features/short_circuit.fe delete mode 100644 crates/test-files/fixtures/features/ternary_expression_with_revert.fe create mode 100644 crates/test-files/fixtures/lowering/and_or.fe diff --git a/crates/lowering/src/ast_utils.rs b/crates/lowering/src/ast_utils.rs index 03a4307584..f81686e9bb 100644 --- a/crates/lowering/src/ast_utils.rs +++ b/crates/lowering/src/ast_utils.rs @@ -1,5 +1,5 @@ use fe_analyzer::namespace::types::FixedSize; -use fe_parser::ast::{CallArg, Expr, FuncStmt, VarDeclTarget}; +use fe_parser::ast::{BoolOperator, CallArg, Expr, FuncStmt, UnaryOperator, VarDeclTarget}; use fe_parser::node::{Node, NodeId}; use crate::names; @@ -494,6 +494,85 @@ pub fn ternary_to_if( unreachable!() } +/// Turns a boolean expression into a set of statements resembling an if/else block with equal +/// functionality. Expects the type and variable result name to be provided as parameters. +pub fn boolean_expr_to_if( + expr_type: FixedSize, + expr: &Node, + result_name: &str, +) -> Vec> { + if let Expr::BoolOperation { left, op, right } = &expr.kind { + return match op.kind { + BoolOperator::And => { + // from: left && right + + // into: + // res: bool = false + // if left: + // res = right + + let mut stmts = vec![FuncStmt::VarDecl { + target: VarDeclTarget::Name(result_name.to_string()).into_node(), + typ: names::fixed_size_type_desc(&expr_type).into_node(), + value: Some(Expr::Bool(false).into_node()), + } + .into_node()]; + let if_branch = FuncStmt::Assign { + target: Expr::Name(result_name.to_string()).into_node(), + value: right.kind.clone().into_traceable_node(right.original_id), + } + .into_node(); + + stmts.push( + FuncStmt::If { + test: left.kind.clone().into_traceable_node(left.original_id), + body: vec![if_branch], + or_else: vec![], + } + .into_node(), + ); + stmts + } + BoolOperator::Or => { + // from: left || right + // into: + // res: bool = true + // if not left: + // res = right + let mut stmts = vec![FuncStmt::VarDecl { + target: VarDeclTarget::Name(result_name.to_string()).into_node(), + typ: names::fixed_size_type_desc(&expr_type).into_node(), + value: Some(Expr::Bool(true).into_node()), + } + .into_node()]; + let if_branch = FuncStmt::Assign { + target: Expr::Name(result_name.to_string()).into_node(), + value: right.kind.clone().into_traceable_node(right.original_id), + } + .into_node(); + + stmts.push( + FuncStmt::If { + test: Expr::UnaryOperation { + op: UnaryOperator::Not.into_node(), + operand: Box::new( + left.kind.clone().into_traceable_node(left.original_id), + ), + } + .into_node(), + body: vec![if_branch], + or_else: vec![], + } + .into_node(), + ); + stmts + } + }; + } + + unreachable!() +} + /// Returns a vector of expressions with all ternary expressions that are /// contained within the given function statement. The last expression /// in the list is the outermost ternary expression found in the statement. @@ -524,6 +603,36 @@ pub fn get_first_ternary_expressions(nodes: &[Node]) -> Vec vec![] } +/// Returns a vector of expressions with all boolean expressions that are +/// contained within the given function statement. The last expression +/// in the list is the outermost boolean expression found in the statement. +pub fn get_all_boolean_expressions(node: &Node) -> Vec> { + let mut expressions = vec![]; + map_ast_node(node.clone().into(), &mut |exp| { + if let StmtOrExpr::Expr(expr) = &exp { + if let Expr::BoolOperation { .. } = expr.kind { + expressions.push(expr.clone()) + } + } + + exp + }); + + expressions +} + +/// For a given set of nodes returns the first set of boolean expressions that can be found. +/// The last expression in the list is the outermost boolean expression found in the statement. +pub fn get_first_boolean_expressions(nodes: &[Node]) -> Vec> { + for node in nodes { + let result = get_all_boolean_expressions(node); + if !result.is_empty() { + return result; + } + } + vec![] +} + /// In a given set of `nodes replaces a node that matches the `node_id` with a name expression node. pub fn replace_node_with_name_expression( nodes: &[Node], diff --git a/crates/lowering/src/mappers/functions.rs b/crates/lowering/src/mappers/functions.rs index 7667126698..a494d85f45 100644 --- a/crates/lowering/src/mappers/functions.rs +++ b/crates/lowering/src/mappers/functions.rs @@ -1,4 +1,6 @@ -use crate::ast_utils::get_first_ternary_expressions; +use crate::ast_utils::{ + boolean_expr_to_if, get_first_boolean_expressions, get_first_ternary_expressions, +}; use crate::ast_utils::{ inject_before_expression, replace_node_with_name_expression, ternary_to_if, }; @@ -11,7 +13,7 @@ use crate::utils::ZeroSpanNode; use fe_analyzer::namespace::items::FunctionId; use fe_analyzer::namespace::types::{Base, Type}; use fe_analyzer::namespace::types::{FixedSize, TypeDowncast}; -use fe_parser::ast::{self as fe, FuncStmt, RegularFunctionArg}; +use fe_parser::ast::{self as fe, Expr, FuncStmt, RegularFunctionArg}; use fe_parser::node::Node; /// Lowers a function definition. @@ -49,7 +51,20 @@ pub fn func_def(context: &mut ModuleContext, function: FunctionId) -> Node Node>, + result_name: &str, + getter_fn: &dyn Fn(&[Node]) -> Vec>, + mapper_fn: &dyn Fn(FixedSize, &Node, &str) -> Vec>, ) -> Vec> { let mut current_statements = statements; loop { - let terns = get_first_ternary_expressions(¤t_statements); + let expressions = getter_fn(¤t_statements); - if let Some(ternary) = terns.last() { + if let Some(current_expression) = expressions.last() { let expr_attr = context - .expression_attributes(ternary.original_id) + .expression_attributes(current_expression.original_id) .expect("missing attributes"); - let ternary_type = + let expression_type = FixedSize::try_from(expr_attr.typ.clone()).expect("Not a fixed size"); - let unique_name = context.make_unique_name("ternary_result"); - let generated_if_else = ternary_to_if(ternary_type.clone(), ternary, &unique_name); + let unique_name = context.make_unique_name(result_name); + let generated_statements = + mapper_fn(expression_type.clone(), current_expression, &unique_name); current_statements = inject_before_expression( ¤t_statements, - ternary.original_id, - &generated_if_else, + current_expression.original_id, + &generated_statements, ); current_statements = replace_node_with_name_expression( ¤t_statements, - ternary.original_id, + current_expression.original_id, &unique_name, ); } else { diff --git a/crates/lowering/tests/lowering.rs b/crates/lowering/tests/lowering.rs index 5f06e7f273..2b2319e11e 100644 --- a/crates/lowering/tests/lowering.rs +++ b/crates/lowering/tests/lowering.rs @@ -75,5 +75,6 @@ test_file! { module_const, "lowering/module_const.fe" } test_file! { module_fn, "lowering/module_fn.fe" } test_file! { struct_fn, "lowering/struct_fn.fe" } test_file! { ternary, "lowering/ternary.fe" } +test_file! { and_or, "lowering/and_or.fe" } // TODO: the analyzer rejects lowered nested tuples. // test_file!(array_tuple, "lowering/array_tuple.fe"); diff --git a/crates/lowering/tests/snapshots/lowering__and_or.snap b/crates/lowering/tests/snapshots/lowering__and_or.snap new file mode 100644 index 0000000000..1e6ac48be4 --- /dev/null +++ b/crates/lowering/tests/snapshots/lowering__and_or.snap @@ -0,0 +1,182 @@ +--- +source: crates/lowering/tests/lowering.rs +expression: lowered_code + +--- +struct $tuple_bool_bool_: + item0: bool + item1: bool + +contract Foo: + pub fn bar() -> (): + let $boolean_expr_result_0: bool = true + if not false: + $boolean_expr_result_0 = true + + return baz($boolean_expr_result_0) + + pub fn nested() -> (): + if true: + let a: bool = true + let b: bool = false + let x: bool = true + let y: bool = false + let $boolean_expr_result_0: bool = true + if not x: + $boolean_expr_result_0 = y + + let $boolean_expr_result_1: bool = false + if a: + $boolean_expr_result_1 = b + + return double_baz($boolean_expr_result_1, $boolean_expr_result_0) + + return () + + pub fn nested_ternary() -> (): + let a: bool = true + let b: bool = false + let x: bool = true + let y: bool = false + let $boolean_expr_result_0: bool = false + let $boolean_expr_result_1: bool = false + if a: + $boolean_expr_result_1 = b + + if baz($boolean_expr_result_1): + let $boolean_expr_result_2: bool = false + if x: + $boolean_expr_result_2 = y + + $boolean_expr_result_0 = $boolean_expr_result_2 + + return $boolean_expr_result_0 + + pub fn in_dec() -> (): + let a: bool = true + let b: bool = false + let x: bool = true + let y: bool = false + let $boolean_expr_result_0: bool = true + if not x: + $boolean_expr_result_0 = y + + let $boolean_expr_result_1: bool = false + if a: + $boolean_expr_result_1 = b + + let z: $tuple_bool_bool_ = $tuple_bool_bool_(item0=$boolean_expr_result_1, item1=$boolean_expr_result_0) + return () + + pub fn short_or(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = true + let $boolean_expr_result_1: bool = true + let $boolean_expr_result_2: bool = true + if not first: + $boolean_expr_result_2 = second + + if not $boolean_expr_result_2: + $boolean_expr_result_1 = third + + if not $boolean_expr_result_1: + $boolean_expr_result_0 = fourth + + return $boolean_expr_result_0 + + pub fn short_or2(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = true + let $boolean_expr_result_1: bool = true + if not first: + $boolean_expr_result_1 = second + + if not $boolean_expr_result_1: + let $boolean_expr_result_2: bool = true + if not third: + $boolean_expr_result_2 = fourth + + $boolean_expr_result_0 = $boolean_expr_result_2 + + return $boolean_expr_result_0 + + pub fn short_or3(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = true + let $boolean_expr_result_1: bool = true + let $boolean_expr_result_2: bool = true + if not first: + $boolean_expr_result_2 = second + + if not $boolean_expr_result_2: + $boolean_expr_result_1 = third + + if not $boolean_expr_result_1: + $boolean_expr_result_0 = fourth + + return $boolean_expr_result_0 + + pub fn short_and(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = false + let $boolean_expr_result_1: bool = false + let $boolean_expr_result_2: bool = false + if first: + $boolean_expr_result_2 = second + + if $boolean_expr_result_2: + $boolean_expr_result_1 = third + + if $boolean_expr_result_1: + $boolean_expr_result_0 = fourth + + return $boolean_expr_result_0 + + pub fn short_and2(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = false + let $boolean_expr_result_1: bool = false + if first: + $boolean_expr_result_1 = second + + if $boolean_expr_result_1: + let $boolean_expr_result_2: bool = false + if third: + $boolean_expr_result_2 = fourth + + $boolean_expr_result_0 = $boolean_expr_result_2 + + return $boolean_expr_result_0 + + pub fn short_and3(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = false + let $boolean_expr_result_1: bool = false + let $boolean_expr_result_2: bool = false + if first: + $boolean_expr_result_2 = second + + if $boolean_expr_result_2: + $boolean_expr_result_1 = third + + if $boolean_expr_result_1: + $boolean_expr_result_0 = fourth + + return $boolean_expr_result_0 + + pub fn short_mixed(first: bool, second: bool, third: bool, fourth: bool) -> bool: + let $boolean_expr_result_0: bool = true + let $boolean_expr_result_1: bool = false + let $boolean_expr_result_2: bool = true + if not first: + $boolean_expr_result_2 = second + + if $boolean_expr_result_2: + $boolean_expr_result_1 = third + + if not $boolean_expr_result_1: + $boolean_expr_result_0 = fourth + + return $boolean_expr_result_0 + + pub fn baz(val: bool) -> (): + pass + return () + + pub fn double_baz(val: bool, val2: bool) -> (): + pass + return () diff --git a/crates/test-files/fixtures/features/short_circuit.fe b/crates/test-files/fixtures/features/short_circuit.fe new file mode 100644 index 0000000000..18ce29faca --- /dev/null +++ b/crates/test-files/fixtures/features/short_circuit.fe @@ -0,0 +1,18 @@ +contract Foo: + + pub fn bar(input: u256) -> u256: + return 1 if input > 5 else revert_u256() + + fn revert_u256() -> u256: + revert + return 0 + + fn revert_bool() -> bool: + revert + return true + + pub fn short_circuit_and(let_through: bool) -> bool: + return let_through and revert_bool() + + pub fn short_circuit_or(break_early: bool) -> bool: + return break_early or revert_bool() \ No newline at end of file diff --git a/crates/test-files/fixtures/features/ternary_expression_with_revert.fe b/crates/test-files/fixtures/features/ternary_expression_with_revert.fe deleted file mode 100644 index fc55dce392..0000000000 --- a/crates/test-files/fixtures/features/ternary_expression_with_revert.fe +++ /dev/null @@ -1,8 +0,0 @@ -contract Foo: - - pub fn bar(input: u256) -> u256: - return 1 if input > 5 else revert_me() - - fn revert_me() -> u256: - revert - return 0 \ No newline at end of file diff --git a/crates/test-files/fixtures/lowering/and_or.fe b/crates/test-files/fixtures/lowering/and_or.fe new file mode 100644 index 0000000000..cba4cae8fe --- /dev/null +++ b/crates/test-files/fixtures/lowering/and_or.fe @@ -0,0 +1,55 @@ +contract Foo: + + pub fn bar(): + return baz(false or true) + + pub fn nested(): + if true: + let a: bool = true + let b: bool = false + let x: bool = true + let y: bool = false + return double_baz(a and b, x or y) + + pub fn nested_ternary(): + let a: bool = true + let b: bool = false + let x: bool = true + let y: bool = false + return baz(a and b) and (x and y) + + pub fn in_dec(): + let a: bool = true + let b: bool = false + let x: bool = true + let y: bool = false + + let z: (bool, bool) = (a and b, x or y) + + pub fn short_or(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return first or second or third or fourth + + pub fn short_or2(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return (first or second) or (third or fourth) + + pub fn short_or3(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return (first or second) or third or fourth + + pub fn short_and(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return first and second and third and fourth + + pub fn short_and2(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return (first and second) and (third and fourth) + + pub fn short_and3(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return (first and second) and third and fourth + + pub fn short_mixed(first: bool, second: bool, third: bool, fourth: bool) -> bool: + return (first or second) and third or fourth + + pub fn baz(val: bool): + pass + + pub fn double_baz(val: bool, val2: bool): + pass + diff --git a/crates/test-files/fixtures/lowering/ternary.fe b/crates/test-files/fixtures/lowering/ternary.fe index 8645a2b669..dd91317119 100644 --- a/crates/test-files/fixtures/lowering/ternary.fe +++ b/crates/test-files/fixtures/lowering/ternary.fe @@ -31,3 +31,4 @@ contract Foo: pub fn double_baz(val1: u256, val2: u256): pass + diff --git a/crates/tests/src/features.rs b/crates/tests/src/features.rs index f16dc86cee..41d6577e25 100644 --- a/crates/tests/src/features.rs +++ b/crates/tests/src/features.rs @@ -1244,17 +1244,28 @@ fn keccak() { } #[test] -fn ternary() { +fn short_circuit() { with_executor(&|mut executor| { - let harness = deploy_contract( - &mut executor, - "ternary_expression_with_revert.fe", - "Foo", - &[], - ); + let harness = deploy_contract(&mut executor, "short_circuit.fe", "Foo", &[]); harness.test_function(&mut executor, "bar", &[uint_token(6)], Some(&uint_token(1))); harness.test_function_reverts(&mut executor, "bar", &[uint_token(1)], &[]); + + harness.test_function( + &mut executor, + "short_circuit_and", + &[bool_token(false)], + Some(&bool_token(false)), + ); + harness.test_function_reverts(&mut executor, "short_circuit_and", &[bool_token(true)], &[]); + + harness.test_function( + &mut executor, + "short_circuit_or", + &[bool_token(true)], + Some(&bool_token(true)), + ); + harness.test_function_reverts(&mut executor, "short_circuit_or", &[bool_token(false)], &[]); }); } diff --git a/crates/yulgen/src/mappers/expressions.rs b/crates/yulgen/src/mappers/expressions.rs index 5d16a381c2..9845f75ab4 100644 --- a/crates/yulgen/src/mappers/expressions.rs +++ b/crates/yulgen/src/mappers/expressions.rs @@ -31,7 +31,7 @@ pub fn expr(context: &mut FnContext, exp: &Node) -> yul::Expression { fe::Expr::Subscript { .. } => expr_subscript(context, exp), fe::Expr::Attribute { .. } => expr_attribute(context, exp), fe::Expr::Ternary { .. } => panic!("ternary expressions should be lowered"), - fe::Expr::BoolOperation { .. } => expr_bool_operation(context, exp), + fe::Expr::BoolOperation { .. } => panic!("bool operation expressions should be lowered"), fe::Expr::BinOperation { .. } => expr_bin_operation(context, exp), fe::Expr::UnaryOperation { .. } => expr_unary_operation(context, exp), fe::Expr::CompOperation { .. } => expr_comp_operation(context, exp), @@ -508,17 +508,3 @@ pub fn nonce_to_ptr(nonce: usize) -> yul::Expression { let ptr = keccak::partial_right_padded(nonce.to_string().as_bytes(), 31); literal_expression! { (ptr) } } - -fn expr_bool_operation(context: &mut FnContext, exp: &Node) -> yul::Expression { - if let fe::Expr::BoolOperation { left, op, right } = &exp.kind { - let yul_left = expr(context, left); - let yul_right = expr(context, right); - - return match op.kind { - fe::BoolOperator::And => expression! {and([yul_left], [yul_right])}, - fe::BoolOperator::Or => expression! {or([yul_left], [yul_right])}, - }; - } - - unreachable!() -} diff --git a/newsfragments/488.bugfix.md b/newsfragments/488.bugfix.md index 00856e920b..8ae393abcf 100644 --- a/newsfragments/488.bugfix.md +++ b/newsfragments/488.bugfix.md @@ -17,3 +17,5 @@ Previous to this change, the code above would **always** revert no matter which branch of the ternary expressions it would resolve to. That is because both sides were evaluated and then one side was discarded. With this change, only the branch that doesn't get picked won't get evaluated at all. + +The same is true for the boolean operations `and` and `or`.