From c4557a2919385e74d2d33aa516109cd23d6a72c2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 7 Oct 2024 17:22:29 -0300 Subject: [PATCH 1/3] Optimize `Quoted::as_expr` by parsing just once --- .../src/hir/comptime/interpreter/builtin.rs | 35 ++++++------ compiler/noirc_frontend/src/parser/mod.rs | 2 +- compiler/noirc_frontend/src/parser/parser.rs | 27 ++------- .../statement_or_expression_or_lvalue.rs | 55 +++++++++++++++++++ 4 files changed, 78 insertions(+), 41 deletions(-) create mode 100644 compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index c80dbb480f5..8277a6397df 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -32,10 +32,12 @@ use crate::{ def_collector::dc_crate::CollectedItems, def_map::ModuleDefId, }, - hir_def::expr::{HirExpression, HirLiteral}, - hir_def::function::FunctionBody, + hir_def::{ + expr::{HirExpression, HirLiteral}, + function::FunctionBody, + }, node_interner::{DefinitionKind, NodeInterner, TraitImplKind}, - parser::Parser, + parser::{Parser, StatementOrExpressionOrLValue}, token::{Attribute, SecondaryAttribute, Token}, Kind, QuotedType, ResolvedGeneric, Shared, Type, TypeVariable, }; @@ -684,21 +686,20 @@ fn quoted_as_expr( ) -> IResult { let argument = check_one_argument(arguments, location)?; - let result = - parse(interner, argument.clone(), Parser::parse_expression_or_error, "an expression"); - if let Ok(expr) = result { - return option(return_type, Some(Value::expression(expr.kind))); - } - - let result = - parse(interner, argument.clone(), Parser::parse_statement_or_error, "an expression"); - if let Ok(stmt) = result { - return option(return_type, Some(Value::statement(stmt.kind))); - } + let result = parse( + interner, + argument.clone(), + Parser::parse_statement_or_expression_or_lvalue, + "an expression", + ); - let result = parse(interner, argument, Parser::parse_lvalue_or_error, "an expression"); - if let Ok(lvalue) = result { - return option(return_type, Some(Value::lvalue(lvalue))); + if let Ok(statement_or_expression_or_lvalue) = result { + let value = match statement_or_expression_or_lvalue { + StatementOrExpressionOrLValue::Expression(expr) => Value::expression(expr.kind), + StatementOrExpressionOrLValue::Statement(statement) => Value::statement(statement.kind), + StatementOrExpressionOrLValue::LValue(lvalue) => Value::lvalue(lvalue), + }; + return option(return_type, Some(value)); } option(return_type, None) diff --git a/compiler/noirc_frontend/src/parser/mod.rs b/compiler/noirc_frontend/src/parser/mod.rs index 21c182a52cd..17c156476a7 100644 --- a/compiler/noirc_frontend/src/parser/mod.rs +++ b/compiler/noirc_frontend/src/parser/mod.rs @@ -20,7 +20,7 @@ use crate::token::SecondaryAttribute; pub use errors::ParserError; pub use errors::ParserErrorReason; use noirc_errors::Span; -pub use parser::{parse_program, Parser}; +pub use parser::{parse_program, Parser, StatementOrExpressionOrLValue}; #[derive(Clone, Default)] pub struct SortedModule { diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index d0b0579ce24..0030144b5e1 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -3,7 +3,7 @@ use modifiers::Modifiers; use noirc_errors::Span; use crate::{ - ast::{Ident, ItemVisibility, LValue}, + ast::{Ident, ItemVisibility}, lexer::{Lexer, SpannedTokenResult}, token::{IntType, Keyword, SpannedToken, Token, TokenKind, Tokens}, }; @@ -28,6 +28,7 @@ mod parse_many; mod path; mod pattern; mod statement; +mod statement_or_expression_or_lvalue; mod structs; mod tests; mod traits; @@ -37,6 +38,8 @@ mod types; mod use_tree; mod where_clause; +pub use statement_or_expression_or_lvalue::StatementOrExpressionOrLValue; + /// Entry function for the parser - also handles lexing internally. /// /// Given a source_program string, return the ParsedModule Ast representation @@ -124,28 +127,6 @@ impl<'a> Parser<'a> { ParsedModule { items, inner_doc_comments } } - /// Parses an LValue. If an LValue can't be parsed, an error is recorded and a default LValue is returned. - pub(crate) fn parse_lvalue_or_error(&mut self) -> LValue { - let start_span = self.current_token_span; - - if let Some(token) = self.eat_kind(TokenKind::InternedLValue) { - match token.into_token() { - Token::InternedLValue(lvalue) => { - return LValue::Interned(lvalue, self.span_since(start_span)); - } - _ => unreachable!(), - } - } - - let expr = self.parse_expression_or_error(); - if let Some(lvalue) = LValue::from_expression(expr) { - lvalue - } else { - self.expected_label(ParsingRuleLabel::LValue); - LValue::Ident(Ident::default()) - } - } - /// Invokes `parsing_function` (`parsing_function` must be some `parse_*` method of the parser) /// and returns the result if the parser has no errors, and if the parser consumed all tokens. /// Otherwise returns the list of errors. diff --git a/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs b/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs new file mode 100644 index 00000000000..3dbbb936165 --- /dev/null +++ b/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs @@ -0,0 +1,55 @@ +use crate::{ + ast::{AssignStatement, Expression, LValue, Statement, StatementKind}, + token::{Token, TokenKind}, +}; + +use super::Parser; + +#[derive(Debug)] +pub enum StatementOrExpressionOrLValue { + Statement(Statement), + Expression(Expression), + LValue(LValue), +} + +impl<'a> Parser<'a> { + /// Parses either a statement, an expression or an LValue. Returns `StatementKind::Error` + /// if none can be parsed, recording an error if so. + /// + /// This method is only used in `Quoted::as_expr`. + pub(crate) fn parse_statement_or_expression_or_lvalue( + &mut self, + ) -> StatementOrExpressionOrLValue { + let start_span = self.current_token_span; + + // First check if it's an interned LVAlue + if let Some(token) = self.eat_kind(TokenKind::InternedLValue) { + match token.into_token() { + Token::InternedLValue(lvalue) => { + let lvalue = LValue::Interned(lvalue, self.span_since(start_span)); + + // If it is, it could be something like `lvalue = expr`: check that. + if self.eat(Token::Assign) { + let expression = self.parse_expression_or_error(); + let kind = StatementKind::Assign(AssignStatement { lvalue, expression }); + return StatementOrExpressionOrLValue::Statement(Statement { + kind, + span: self.span_since(start_span), + }); + } else { + return StatementOrExpressionOrLValue::LValue(lvalue); + } + } + _ => unreachable!(), + } + } + + // Otherwise, check if it's a statement (which in turn checks if it's an expression) + let statement = self.parse_statement_or_error(); + if let StatementKind::Expression(expr) = statement.kind { + StatementOrExpressionOrLValue::Expression(expr) + } else { + StatementOrExpressionOrLValue::Statement(statement) + } + } +} From 5020df18f021d3be3e0663ead4b876db3c11a4f9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 8 Oct 2024 07:45:09 -0300 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Maxim Vezenov --- compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs | 2 +- .../src/parser/parser/statement_or_expression_or_lvalue.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 8277a6397df..76ecbf964c0 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -688,7 +688,7 @@ fn quoted_as_expr( let result = parse( interner, - argument.clone(), + argument, Parser::parse_statement_or_expression_or_lvalue, "an expression", ); diff --git a/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs b/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs index 3dbbb936165..fdc187f3fb2 100644 --- a/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs +++ b/compiler/noirc_frontend/src/parser/parser/statement_or_expression_or_lvalue.rs @@ -22,7 +22,7 @@ impl<'a> Parser<'a> { ) -> StatementOrExpressionOrLValue { let start_span = self.current_token_span; - // First check if it's an interned LVAlue + // First check if it's an interned LValue if let Some(token) = self.eat_kind(TokenKind::InternedLValue) { match token.into_token() { Token::InternedLValue(lvalue) => { From e5512a3c74012fbd493ed868a18720fb23b2a933 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 8 Oct 2024 07:46:09 -0300 Subject: [PATCH 3/3] Use result ok map --- .../src/hir/comptime/interpreter/builtin.rs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 76ecbf964c0..12648bca8d9 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -686,23 +686,21 @@ fn quoted_as_expr( ) -> IResult { let argument = check_one_argument(arguments, location)?; - let result = parse( - interner, - argument, - Parser::parse_statement_or_expression_or_lvalue, - "an expression", - ); - - if let Ok(statement_or_expression_or_lvalue) = result { - let value = match statement_or_expression_or_lvalue { - StatementOrExpressionOrLValue::Expression(expr) => Value::expression(expr.kind), - StatementOrExpressionOrLValue::Statement(statement) => Value::statement(statement.kind), - StatementOrExpressionOrLValue::LValue(lvalue) => Value::lvalue(lvalue), - }; - return option(return_type, Some(value)); - } + let result = + parse(interner, argument, Parser::parse_statement_or_expression_or_lvalue, "an expression"); + + let value = + result.ok().map( + |statement_or_expression_or_lvalue| match statement_or_expression_or_lvalue { + StatementOrExpressionOrLValue::Expression(expr) => Value::expression(expr.kind), + StatementOrExpressionOrLValue::Statement(statement) => { + Value::statement(statement.kind) + } + StatementOrExpressionOrLValue::LValue(lvalue) => Value::lvalue(lvalue), + }, + ); - option(return_type, None) + option(return_type, value) } // fn as_module(quoted: Quoted) -> Option