From 59ea775445ebb548d3ec457721421de0b0399728 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 25 Mar 2024 15:04:57 +0000 Subject: [PATCH] chore: convert `BlockExpression` into a standard struct (#4623) # Description ## Problem\* Resolves ## Summary\* This PR splits out some more mundane changes from #4429. `BlockExpression` is now a standard rather than tuple struct to make it easier for us to add an `unsafe` property (and potentially `unchecked` in future) ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher --- aztec_macros/src/transforms/events.rs | 12 ++++-- aztec_macros/src/transforms/functions.rs | 29 +++++++------- aztec_macros/src/transforms/storage.rs | 2 +- compiler/noirc_frontend/src/ast/expression.rs | 32 +++++++++------- compiler/noirc_frontend/src/ast/function.rs | 2 +- compiler/noirc_frontend/src/ast/statement.rs | 14 ++++--- compiler/noirc_frontend/src/debug/mod.rs | 38 +++++++++++-------- .../src/hir/resolution/resolver.rs | 6 +-- .../noirc_frontend/src/hir/type_check/mod.rs | 3 +- compiler/noirc_frontend/src/hir_def/expr.rs | 8 ++-- .../src/monomorphization/mod.rs | 2 +- compiler/noirc_frontend/src/parser/parser.rs | 20 +++++----- tooling/nargo_fmt/src/visitor/expr.rs | 4 +- 13 files changed, 98 insertions(+), 74 deletions(-) diff --git a/aztec_macros/src/transforms/events.rs b/aztec_macros/src/transforms/events.rs index b02709efacb..e7e39ed29ba 100644 --- a/aztec_macros/src/transforms/events.rs +++ b/aztec_macros/src/transforms/events.rs @@ -47,10 +47,14 @@ pub fn generate_selector_impl(structure: &NoirStruct) -> TypeImpl { let mut from_signature_path = selector_path.clone(); from_signature_path.segments.push(ident("from_signature")); - let selector_fun_body = BlockExpression(vec![make_statement(StatementKind::Expression(call( - variable_path(from_signature_path), - vec![expression(ExpressionKind::Literal(Literal::Str(SIGNATURE_PLACEHOLDER.to_string())))], - )))]); + let selector_fun_body = BlockExpression { + statements: vec![make_statement(StatementKind::Expression(call( + variable_path(from_signature_path), + vec![expression(ExpressionKind::Literal(Literal::Str( + SIGNATURE_PLACEHOLDER.to_string(), + )))], + )))], + }; // Define `FunctionSelector` return type let return_type = diff --git a/aztec_macros/src/transforms/functions.rs b/aztec_macros/src/transforms/functions.rs index c719651e10e..855969732d0 100644 --- a/aztec_macros/src/transforms/functions.rs +++ b/aztec_macros/src/transforms/functions.rs @@ -39,29 +39,29 @@ pub fn transform_function( // Add check that msg sender equals this address and flag function as internal if is_internal { let is_internal_check = create_internal_check(func.name()); - func.def.body.0.insert(0, is_internal_check); + func.def.body.statements.insert(0, is_internal_check); } // Add initialization check if insert_init_check { let init_check = create_init_check(); - func.def.body.0.insert(0, init_check); + func.def.body.statements.insert(0, init_check); } // Add assertion for initialization arguments and sender if is_initializer { - func.def.body.0.insert(0, create_assert_initializer()); + func.def.body.statements.insert(0, create_assert_initializer()); } // Add access to the storage struct if storage_defined { let storage_def = abstract_storage(&ty.to_lowercase(), false); - func.def.body.0.insert(0, storage_def); + func.def.body.statements.insert(0, storage_def); } // Insert the context creation as the first action let create_context = create_context(&context_name, &func.def.parameters)?; - func.def.body.0.splice(0..0, (create_context).iter().cloned()); + func.def.body.statements.splice(0..0, (create_context).iter().cloned()); // Add the inputs to the params let input = create_inputs(&inputs_name); @@ -71,20 +71,20 @@ pub fn transform_function( if let Some(return_values) = abstract_return_values(func) { // In case we are pushing return values to the context, we remove the statement that originated it // This avoids running duplicate code, since blocks like if/else can be value returning statements - func.def.body.0.pop(); + func.def.body.statements.pop(); // Add the new return statement - func.def.body.0.push(return_values); + func.def.body.statements.push(return_values); } // Before returning mark the contract as initialized if is_initializer { let mark_initialized = create_mark_as_initialized(); - func.def.body.0.push(mark_initialized); + func.def.body.statements.push(mark_initialized); } // Push the finish method call to the end of the function let finish_def = create_context_finish(); - func.def.body.0.push(finish_def); + func.def.body.statements.push(finish_def); let return_type = create_return_type(&return_type_name); func.def.return_type = return_type; @@ -109,12 +109,12 @@ pub fn transform_vm_function( // Create access to storage if storage_defined { let storage = abstract_storage("public_vm", true); - func.def.body.0.insert(0, storage); + func.def.body.statements.insert(0, storage); } // Push Avm context creation to the beginning of the function let create_context = create_avm_context()?; - func.def.body.0.insert(0, create_context); + func.def.body.statements.insert(0, create_context); // We want the function to be seen as a public function func.def.is_unconstrained = true; @@ -131,7 +131,7 @@ pub fn transform_vm_function( /// /// This will allow developers to access their contract' storage struct in unconstrained functions pub fn transform_unconstrained(func: &mut NoirFunction) { - func.def.body.0.insert(0, abstract_storage("Unconstrained", true)); + func.def.body.statements.insert(0, abstract_storage("Unconstrained", true)); } /// Helper function that returns what the private context would look like in the ast @@ -393,7 +393,7 @@ fn create_avm_context() -> Result { /// Any primitive type that can be cast will be casted to a field and pushed to the context. fn abstract_return_values(func: &NoirFunction) -> Option { let current_return_type = func.return_type().typ; - let last_statement = func.def.body.0.last()?; + let last_statement = func.def.body.statements.last()?; // TODO: (length, type) => We can limit the size of the array returned to be limited by kernel size // Doesn't need done until we have settled on a kernel size @@ -645,7 +645,8 @@ fn create_loop_over(var: Expression, loop_body: Vec) -> Statement { // What will be looped over // - `hasher.add({ident}[i] as Field)` - let for_loop_block = expression(ExpressionKind::Block(BlockExpression(loop_body))); + let for_loop_block = + expression(ExpressionKind::Block(BlockExpression { statements: loop_body })); // `for i in 0..{ident}.len()` make_statement(StatementKind::For(ForLoopStatement { diff --git a/aztec_macros/src/transforms/storage.rs b/aztec_macros/src/transforms/storage.rs index 40a094f78e3..10f44d01bb4 100644 --- a/aztec_macros/src/transforms/storage.rs +++ b/aztec_macros/src/transforms/storage.rs @@ -150,7 +150,7 @@ pub fn generate_storage_implementation(module: &mut SortedModule) -> Result<(), true, )), )], - &BlockExpression(vec![storage_constructor_statement]), + &BlockExpression { statements: vec![storage_constructor_statement] }, &[], &return_type(chained_path!("Self")), )); diff --git a/compiler/noirc_frontend/src/ast/expression.rs b/compiler/noirc_frontend/src/ast/expression.rs index d646a6ca98a..0e5919bf7db 100644 --- a/compiler/noirc_frontend/src/ast/expression.rs +++ b/compiler/noirc_frontend/src/ast/expression.rs @@ -192,16 +192,18 @@ impl Expression { // with tuples without calling them. E.g. `if c { t } else { e }(a, b)` is interpreted // as a sequence of { if, tuple } rather than a function call. This behavior matches rust. let kind = if matches!(&lhs.kind, ExpressionKind::If(..)) { - ExpressionKind::Block(BlockExpression(vec![ - Statement { kind: StatementKind::Expression(lhs), span }, - Statement { - kind: StatementKind::Expression(Expression::new( - ExpressionKind::Tuple(arguments), + ExpressionKind::Block(BlockExpression { + statements: vec![ + Statement { kind: StatementKind::Expression(lhs), span }, + Statement { + kind: StatementKind::Expression(Expression::new( + ExpressionKind::Tuple(arguments), + span, + )), span, - )), - span, - }, - ])) + }, + ], + }) } else { ExpressionKind::Call(Box::new(CallExpression { func: Box::new(lhs), arguments })) }; @@ -452,19 +454,21 @@ pub struct IndexExpression { } #[derive(Debug, PartialEq, Eq, Clone)] -pub struct BlockExpression(pub Vec); +pub struct BlockExpression { + pub statements: Vec, +} impl BlockExpression { pub fn pop(&mut self) -> Option { - self.0.pop().map(|stmt| stmt.kind) + self.statements.pop().map(|stmt| stmt.kind) } pub fn len(&self) -> usize { - self.0.len() + self.statements.len() } pub fn is_empty(&self) -> bool { - self.0.is_empty() + self.statements.is_empty() } } @@ -542,7 +546,7 @@ impl Display for Literal { impl Display for BlockExpression { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, "{{")?; - for statement in &self.0 { + for statement in &self.statements { let statement = statement.kind.to_string(); for line in statement.lines() { writeln!(f, " {line}")?; diff --git a/compiler/noirc_frontend/src/ast/function.rs b/compiler/noirc_frontend/src/ast/function.rs index 46f0ac0fa0f..3e8b78c1312 100644 --- a/compiler/noirc_frontend/src/ast/function.rs +++ b/compiler/noirc_frontend/src/ast/function.rs @@ -83,7 +83,7 @@ impl NoirFunction { &mut self.def } pub fn number_of_statements(&self) -> usize { - self.def.body.0.len() + self.def.body.statements.len() } pub fn span(&self) -> Span { self.def.span diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index fb7f520ee71..b14ead8ad42 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -603,10 +603,12 @@ impl ForRange { }; let block_span = block.span; - let new_block = BlockExpression(vec![ - let_elem, - Statement { kind: StatementKind::Expression(block), span: block_span }, - ]); + let new_block = BlockExpression { + statements: vec![ + let_elem, + Statement { kind: StatementKind::Expression(block), span: block_span }, + ], + }; let new_block = Expression::new(ExpressionKind::Block(new_block), block_span); let for_loop = Statement { kind: StatementKind::For(ForLoopStatement { @@ -618,7 +620,9 @@ impl ForRange { span: for_loop_span, }; - let block = ExpressionKind::Block(BlockExpression(vec![let_array, for_loop])); + let block = ExpressionKind::Block(BlockExpression { + statements: vec![let_array, for_loop], + }); StatementKind::Expression(Expression::new(block, for_loop_span)) } } diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index 05916502d73..8e5c174d270 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -113,7 +113,7 @@ impl DebugInstrumenter { }) .collect(); - let func_body = &mut func.body.0; + let func_body = &mut func.body.statements; let mut statements = take(func_body); self.walk_scope(&mut statements, func.span); @@ -243,7 +243,9 @@ impl DebugInstrumenter { pattern: ast::Pattern::Tuple(vars_pattern, let_stmt.pattern.span()), r#type: ast::UnresolvedType::unspecified(), expression: ast::Expression { - kind: ast::ExpressionKind::Block(ast::BlockExpression(block_stmts)), + kind: ast::ExpressionKind::Block(ast::BlockExpression { + statements: block_stmts, + }), span: let_stmt.expression.span, }, }), @@ -330,11 +332,13 @@ impl DebugInstrumenter { kind: ast::StatementKind::Assign(ast::AssignStatement { lvalue: assign_stmt.lvalue.clone(), expression: ast::Expression { - kind: ast::ExpressionKind::Block(ast::BlockExpression(vec![ - ast::Statement { kind: let_kind, span: expression_span }, - new_assign_stmt, - ast::Statement { kind: ret_kind, span: expression_span }, - ])), + kind: ast::ExpressionKind::Block(ast::BlockExpression { + statements: vec![ + ast::Statement { kind: let_kind, span: expression_span }, + new_assign_stmt, + ast::Statement { kind: ret_kind, span: expression_span }, + ], + }), span: expression_span, }, }), @@ -344,7 +348,7 @@ impl DebugInstrumenter { fn walk_expr(&mut self, expr: &mut ast::Expression) { match &mut expr.kind { - ast::ExpressionKind::Block(ast::BlockExpression(ref mut statements)) => { + ast::ExpressionKind::Block(ast::BlockExpression { ref mut statements, .. }) => { self.scope.push(HashMap::default()); self.walk_scope(statements, expr.span); } @@ -415,14 +419,16 @@ impl DebugInstrumenter { self.walk_expr(&mut for_stmt.block); for_stmt.block = ast::Expression { - kind: ast::ExpressionKind::Block(ast::BlockExpression(vec![ - set_stmt, - ast::Statement { - kind: ast::StatementKind::Semi(for_stmt.block.clone()), - span: for_stmt.block.span, - }, - drop_stmt, - ])), + kind: ast::ExpressionKind::Block(ast::BlockExpression { + statements: vec![ + set_stmt, + ast::Statement { + kind: ast::StatementKind::Semi(for_stmt.block.clone()), + span: for_stmt.block.span, + }, + drop_stmt, + ], + }), span: for_stmt.span, }; } diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 3fbde8a890b..f8e3c4cab60 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -252,7 +252,7 @@ impl<'a> Resolver<'a> { typ: typ.clone(), span: name.span(), }), - body: BlockExpression(Vec::new()), + body: BlockExpression { statements: Vec::new() }, span: name.span(), where_clause: where_clause.to_vec(), return_type: return_type.clone(), @@ -1952,8 +1952,8 @@ impl<'a> Resolver<'a> { fn resolve_block(&mut self, block_expr: BlockExpression) -> HirExpression { let statements = - self.in_new_scope(|this| vecmap(block_expr.0, |stmt| this.intern_stmt(stmt))); - HirExpression::Block(HirBlockExpression(statements)) + self.in_new_scope(|this| vecmap(block_expr.statements, |stmt| this.intern_stmt(stmt))); + HirExpression::Block(HirBlockExpression { statements }) } pub fn intern_block(&mut self, block: BlockExpression) -> ExprId { diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index ab759f454e5..137608f8037 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -468,7 +468,8 @@ mod test { expression: expr_id, }; let stmt_id = interner.push_stmt(HirStatement::Let(let_stmt)); - let expr_id = interner.push_expr(HirExpression::Block(HirBlockExpression(vec![stmt_id]))); + let expr_id = interner + .push_expr(HirExpression::Block(HirBlockExpression { statements: vec![stmt_id] })); interner.push_expr_location(expr_id, Span::single_char(0), file); // Create function to enclose the let statement diff --git a/compiler/noirc_frontend/src/hir_def/expr.rs b/compiler/noirc_frontend/src/hir_def/expr.rs index 61743d2cdc7..c2f6031bf6d 100644 --- a/compiler/noirc_frontend/src/hir_def/expr.rs +++ b/compiler/noirc_frontend/src/hir_def/expr.rs @@ -37,7 +37,7 @@ pub enum HirExpression { impl HirExpression { /// Returns an empty block expression pub const fn empty_block() -> HirExpression { - HirExpression::Block(HirBlockExpression(vec![])) + HirExpression::Block(HirBlockExpression { statements: vec![] }) } } @@ -249,11 +249,13 @@ pub struct HirIndexExpression { } #[derive(Debug, Clone)] -pub struct HirBlockExpression(pub Vec); +pub struct HirBlockExpression { + pub statements: Vec, +} impl HirBlockExpression { pub fn statements(&self) -> &[StmtId] { - &self.0 + &self.statements } } diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 9b0f57a7d39..618eba8f190 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -430,7 +430,7 @@ impl<'interner> Monomorphizer<'interner> { } }, HirExpression::Literal(HirLiteral::Unit) => ast::Expression::Block(vec![]), - HirExpression::Block(block) => self.block(block.0)?, + HirExpression::Block(block) => self.block(block.statements)?, HirExpression::Prefix(prefix) => { let location = self.interner.expr_location(&expr); diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index dec1c7aa9ce..a40355be8aa 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -347,7 +347,7 @@ fn block<'a>( [(LeftParen, RightParen), (LeftBracket, RightBracket)], |span| vec![Statement { kind: StatementKind::Error, span }], )) - .map(BlockExpression) + .map(|statements| BlockExpression { statements }) } fn check_statements_require_semicolon( @@ -1015,10 +1015,12 @@ where // Wrap the inner `if` expression in a block expression. // i.e. rewrite the sugared form `if cond1 {} else if cond2 {}` as `if cond1 {} else { if cond2 {} }`. let if_expression = Expression::new(kind, span); - let desugared_else = BlockExpression(vec![Statement { - kind: StatementKind::Expression(if_expression), - span, - }]); + let desugared_else = BlockExpression { + statements: vec![Statement { + kind: StatementKind::Expression(if_expression), + span, + }], + }; Expression::new(ExpressionKind::Block(desugared_else), span) })); @@ -1399,13 +1401,13 @@ mod test { // Regression for #1310: this should be parsed as a block and not a function call let res = parse_with(block(fresh_statement()), "{ if true { 1 } else { 2 } (3, 4) }").unwrap(); - match unwrap_expr(&res.0.last().unwrap().kind) { + match unwrap_expr(&res.statements.last().unwrap().kind) { // The `if` followed by a tuple is currently creates a block around both in case // there was none to start with, so there is an extra block here. ExpressionKind::Block(block) => { - assert_eq!(block.0.len(), 2); - assert!(matches!(unwrap_expr(&block.0[0].kind), ExpressionKind::If(_))); - assert!(matches!(unwrap_expr(&block.0[1].kind), ExpressionKind::Tuple(_))); + assert_eq!(block.statements.len(), 2); + assert!(matches!(unwrap_expr(&block.statements[0].kind), ExpressionKind::If(_))); + assert!(matches!(unwrap_expr(&block.statements[1].kind), ExpressionKind::Tuple(_))); } _ => unreachable!(), } diff --git a/tooling/nargo_fmt/src/visitor/expr.rs b/tooling/nargo_fmt/src/visitor/expr.rs index 2cd0e881e84..f9836adda18 100644 --- a/tooling/nargo_fmt/src/visitor/expr.rs +++ b/tooling/nargo_fmt/src/visitor/expr.rs @@ -119,11 +119,11 @@ impl FmtVisitor<'_> { self.last_position = block_span.start() + 1; // `{` self.push_str("{"); - self.trim_spaces_after_opening_brace(&block.0); + self.trim_spaces_after_opening_brace(&block.statements); self.indent.block_indent(self.config); - self.visit_stmts(block.0); + self.visit_stmts(block.statements); let span = (self.last_position..block_span.end() - 1).into(); self.close_block(span);