From 6a661388c6bbb1018a51b45325e741bdf0bb3635 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 23 Apr 2024 13:39:42 -0500 Subject: [PATCH] chore(experimental): Improve variable not defined error message in comptime interpreter (#4889) # Description ## Problem\* Resolves ## Summary\* Slightly improves the error message for when a variable in the interpreter isn't found. Since all ids are already given by name resolution, the only time one cannot be found is if it is not a comptime variable, so I've narrowed the error message a bit. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/ast/statement.rs | 8 +++--- .../noirc_frontend/src/hir/comptime/errors.rs | 4 +-- .../src/hir/comptime/hir_to_ast.rs | 4 +-- .../src/hir/comptime/interpreter.rs | 28 ++++++++----------- .../noirc_frontend/src/hir/comptime/scan.rs | 10 +++---- .../noirc_frontend/src/hir/comptime/value.rs | 1 - .../src/hir/resolution/resolver.rs | 4 +-- .../noirc_frontend/src/hir/type_check/stmt.rs | 2 +- compiler/noirc_frontend/src/hir_def/stmt.rs | 2 +- compiler/noirc_frontend/src/lexer/token.rs | 6 ++-- .../src/monomorphization/mod.rs | 2 +- .../noirc_frontend/src/noir_parser.lalrpop | 2 +- compiler/noirc_frontend/src/parser/parser.rs | 8 +++--- .../noirc_frontend/src/parser/parser/types.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 2 +- tooling/nargo_fmt/src/visitor/stmt.rs | 2 +- 16 files changed, 40 insertions(+), 47 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index f37c7adc983..1831a046f5b 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -40,7 +40,7 @@ pub enum StatementKind { Break, Continue, /// This statement should be executed at compile-time - CompTime(Box), + Comptime(Box), // This is an expression with a trailing semi-colon Semi(Expression), // This statement is the result of a recovered parse error. @@ -87,10 +87,10 @@ impl StatementKind { } self } - StatementKind::CompTime(mut statement) => { + StatementKind::Comptime(mut statement) => { *statement = statement.add_semicolon(semi, span, last_statement_in_block, emit_error); - StatementKind::CompTime(statement) + StatementKind::Comptime(statement) } // A semicolon on a for loop is optional and does nothing StatementKind::For(_) => self, @@ -685,7 +685,7 @@ impl Display for StatementKind { StatementKind::For(for_loop) => for_loop.fmt(f), StatementKind::Break => write!(f, "break"), StatementKind::Continue => write!(f, "continue"), - StatementKind::CompTime(statement) => write!(f, "comptime {statement}"), + StatementKind::Comptime(statement) => write!(f, "comptime {statement}"), StatementKind::Semi(semi) => write!(f, "{semi};"), StatementKind::Error => write!(f, "Error"), } diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index d3db7fcaee9..67d9a006b22 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -1,4 +1,4 @@ -use crate::{node_interner::DefinitionId, Type}; +use crate::Type; use acvm::FieldElement; use noirc_errors::Location; @@ -9,7 +9,7 @@ use super::value::Value; pub enum InterpreterError { ArgumentCountMismatch { expected: usize, actual: usize, call_location: Location }, TypeMismatch { expected: Type, value: Value, location: Location }, - NoValueForId { id: DefinitionId, location: Location }, + NonComptimeVarReferenced { name: String, location: Location }, IntegerOutOfRangeForType { value: FieldElement, typ: Type, location: Location }, ErrorNodeEncountered { location: Location }, NonFunctionCalled { value: Value, location: Location }, diff --git a/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs b/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs index dd23edf0004..42ee76d29fa 100644 --- a/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs +++ b/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs @@ -66,8 +66,8 @@ impl StmtId { HirStatement::Expression(expr) => StatementKind::Expression(expr.to_ast(interner)), HirStatement::Semi(expr) => StatementKind::Semi(expr.to_ast(interner)), HirStatement::Error => StatementKind::Error, - HirStatement::CompTime(statement) => { - StatementKind::CompTime(Box::new(statement.to_ast(interner).kind)) + HirStatement::Comptime(statement) => { + StatementKind::Comptime(Box::new(statement.to_ast(interner).kind)) } }; diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index c6d508a581e..c01c985a40c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -246,13 +246,7 @@ impl<'a> Interpreter<'a> { location: Location, ) -> IResult<()> { self.type_check(typ, &argument, location)?; - for scope in self.scopes.iter_mut().rev() { - if let Entry::Occupied(mut entry) = scope.entry(id) { - entry.insert(argument); - return Ok(()); - } - } - Err(InterpreterError::NoValueForId { id, location }) + self.mutate(id, argument, location) } /// Mutate an existing variable, potentially from a prior scope @@ -263,17 +257,12 @@ impl<'a> Interpreter<'a> { return Ok(()); } } - Err(InterpreterError::NoValueForId { id, location }) + let name = self.interner.definition(id).name.clone(); + Err(InterpreterError::NonComptimeVarReferenced { name, location }) } fn lookup(&self, ident: &HirIdent) -> IResult { - for scope in self.scopes.iter().rev() { - if let Some(value) = scope.get(&ident.id) { - return Ok(value.clone()); - } - } - - Err(InterpreterError::NoValueForId { id: ident.id, location: ident.location }) + self.lookup_id(ident.id, ident.location) } fn lookup_id(&self, id: DefinitionId, location: Location) -> IResult { @@ -283,7 +272,12 @@ impl<'a> Interpreter<'a> { } } - Err(InterpreterError::NoValueForId { id, location }) + // Justification for `NonComptimeVarReferenced`: + // If we have an id to lookup at all that means name resolution successfully + // found another variable in scope for this name. If the name is in scope + // but unknown by the interpreter it must be because it was not a comptime variable. + let name = self.interner.definition(id).name.clone(); + Err(InterpreterError::NonComptimeVarReferenced { name, location }) } fn type_check(&self, typ: &Type, value: &Value, location: Location) -> IResult<()> { @@ -1023,7 +1017,7 @@ impl<'a> Interpreter<'a> { HirStatement::Break => self.evaluate_break(statement), HirStatement::Continue => self.evaluate_continue(statement), HirStatement::Expression(expression) => self.evaluate(expression), - HirStatement::CompTime(statement) => self.evaluate_comptime(statement), + HirStatement::Comptime(statement) => self.evaluate_comptime(statement), HirStatement::Semi(expression) => { self.evaluate(expression)?; Ok(Value::Unit) diff --git a/compiler/noirc_frontend/src/hir/comptime/scan.rs b/compiler/noirc_frontend/src/hir/comptime/scan.rs index d4fa355627f..2e13daea527 100644 --- a/compiler/noirc_frontend/src/hir/comptime/scan.rs +++ b/compiler/noirc_frontend/src/hir/comptime/scan.rs @@ -1,15 +1,15 @@ //! This module is for the scanning of the Hir by the interpreter. -//! In this initial step, the Hir is scanned for `CompTime` nodes +//! In this initial step, the Hir is scanned for `Comptime` nodes //! without actually executing anything until such a node is found. //! Once such a node is found, the interpreter will call the relevant //! evaluate method on that node type, insert the result into the Ast, //! and continue scanning the rest of the program. //! -//! Since it mostly just needs to recur on the Hir looking for CompTime +//! Since it mostly just needs to recur on the Hir looking for Comptime //! nodes, this pass is fairly simple. The only thing it really needs to //! ensure to do is to push and pop scopes on the interpreter as needed //! so that any variables defined within e.g. an `if` statement containing -//! a `CompTime` block aren't accessible outside of the `if`. +//! a `Comptime` block aren't accessible outside of the `if`. use crate::{ hir_def::{ expr::{ @@ -30,7 +30,7 @@ use super::{ #[allow(dead_code)] impl<'interner> Interpreter<'interner> { - /// Scan through a function, evaluating any CompTime nodes found. + /// Scan through a function, evaluating any Comptime nodes found. /// These nodes will be modified in place, replaced with the /// result of their evaluation. pub fn scan_function(&mut self, function: FuncId) -> IResult<()> { @@ -185,7 +185,7 @@ impl<'interner> Interpreter<'interner> { HirStatement::Expression(expression) => self.scan_expression(expression), HirStatement::Semi(semi) => self.scan_expression(semi), HirStatement::Error => Ok(()), - HirStatement::CompTime(comptime) => { + HirStatement::Comptime(comptime) => { let location = self.interner.statement_location(comptime); let new_expr = self.evaluate_comptime(comptime)?.into_expression(self.interner, location)?; diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index 89102344b09..d7af4643192 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -16,7 +16,6 @@ use rustc_hash::FxHashMap as HashMap; use super::errors::{IResult, InterpreterError}; -#[allow(unused)] #[derive(Debug, Clone, PartialEq, Eq)] pub enum Value { Unit, diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 8d2cb17189b..8e29f8f9fce 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -1272,9 +1272,9 @@ impl<'a> Resolver<'a> { HirStatement::Continue } StatementKind::Error => HirStatement::Error, - StatementKind::CompTime(statement) => { + StatementKind::Comptime(statement) => { let statement = self.resolve_stmt(*statement, span); - HirStatement::CompTime(self.interner.push_stmt(statement)) + HirStatement::Comptime(self.interner.push_stmt(statement)) } } } diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs index 064fefc8ae9..f5f6e1e8180 100644 --- a/compiler/noirc_frontend/src/hir/type_check/stmt.rs +++ b/compiler/noirc_frontend/src/hir/type_check/stmt.rs @@ -51,7 +51,7 @@ impl<'interner> TypeChecker<'interner> { HirStatement::Constrain(constrain_stmt) => self.check_constrain_stmt(constrain_stmt), HirStatement::Assign(assign_stmt) => self.check_assign_stmt(assign_stmt, stmt_id), HirStatement::For(for_loop) => self.check_for_loop(for_loop), - HirStatement::CompTime(statement) => return self.check_statement(&statement), + HirStatement::Comptime(statement) => return self.check_statement(&statement), HirStatement::Break | HirStatement::Continue | HirStatement::Error => (), } Type::Unit diff --git a/compiler/noirc_frontend/src/hir_def/stmt.rs b/compiler/noirc_frontend/src/hir_def/stmt.rs index 48e7d7344e3..7e22e5ee9c0 100644 --- a/compiler/noirc_frontend/src/hir_def/stmt.rs +++ b/compiler/noirc_frontend/src/hir_def/stmt.rs @@ -20,7 +20,7 @@ pub enum HirStatement { Continue, Expression(ExprId), Semi(ExprId), - CompTime(StmtId), + Comptime(StmtId), Error, } diff --git a/compiler/noirc_frontend/src/lexer/token.rs b/compiler/noirc_frontend/src/lexer/token.rs index ec513d55299..0242fc7e7ff 100644 --- a/compiler/noirc_frontend/src/lexer/token.rs +++ b/compiler/noirc_frontend/src/lexer/token.rs @@ -811,7 +811,7 @@ pub enum Keyword { Break, CallData, Char, - CompTime, + Comptime, Constrain, Continue, Contract, @@ -856,7 +856,7 @@ impl fmt::Display for Keyword { Keyword::Break => write!(f, "break"), Keyword::Char => write!(f, "char"), Keyword::CallData => write!(f, "call_data"), - Keyword::CompTime => write!(f, "comptime"), + Keyword::Comptime => write!(f, "comptime"), Keyword::Constrain => write!(f, "constrain"), Keyword::Continue => write!(f, "continue"), Keyword::Contract => write!(f, "contract"), @@ -904,7 +904,7 @@ impl Keyword { "break" => Keyword::Break, "call_data" => Keyword::CallData, "char" => Keyword::Char, - "comptime" => Keyword::CompTime, + "comptime" => Keyword::Comptime, "constrain" => Keyword::Constrain, "continue" => Keyword::Continue, "contract" => Keyword::Contract, diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index d92b6c65d7a..b130af9d125 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -632,7 +632,7 @@ impl<'interner> Monomorphizer<'interner> { HirStatement::Error => unreachable!(), // All `comptime` statements & expressions should be removed before runtime. - HirStatement::CompTime(_) => unreachable!("comptime statement in runtime code"), + HirStatement::Comptime(_) => unreachable!("comptime statement in runtime code"), } } diff --git a/compiler/noirc_frontend/src/noir_parser.lalrpop b/compiler/noirc_frontend/src/noir_parser.lalrpop index ec2b4c8ab46..9acb5ef8b58 100644 --- a/compiler/noirc_frontend/src/noir_parser.lalrpop +++ b/compiler/noirc_frontend/src/noir_parser.lalrpop @@ -61,7 +61,7 @@ extern { "break" => BorrowedToken::Keyword(noir_token::Keyword::Break), "call_data" => BorrowedToken::Keyword(noir_token::Keyword::CallData), "char" => BorrowedToken::Keyword(noir_token::Keyword::Char), - "comptime" => BorrowedToken::Keyword(noir_token::Keyword::CompTime), + "comptime" => BorrowedToken::Keyword(noir_token::Keyword::Comptime), "constrain" => BorrowedToken::Keyword(noir_token::Keyword::Constrain), "continue" => BorrowedToken::Keyword(noir_token::Keyword::Continue), "contract" => BorrowedToken::Keyword(noir_token::Keyword::Contract), diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 858e5c4838c..6e9f3969297 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -532,7 +532,7 @@ where P2: ExprParser + 'a, S: NoirParser + 'a, { - keyword(Keyword::CompTime) + keyword(Keyword::Comptime) .ignore_then(choice(( declaration(expr), for_loop(expr_no_constructors, statement.clone()), @@ -540,7 +540,7 @@ where StatementKind::Expression(Expression::new(ExpressionKind::Block(block), span)) }), ))) - .map(|statement| StatementKind::CompTime(Box::new(statement))) + .map(|statement| StatementKind::Comptime(Box::new(statement))) } /// Comptime in an expression position only accepts entire blocks @@ -548,7 +548,7 @@ fn comptime_expr<'a, S>(statement: S) -> impl NoirParser + 'a where S: NoirParser + 'a, { - keyword(Keyword::CompTime).ignore_then(block(statement)).map(ExpressionKind::Comptime) + keyword(Keyword::Comptime).ignore_then(block(statement)).map(ExpressionKind::Comptime) } fn declaration<'a, P>(expr_parser: P) -> impl NoirParser + 'a @@ -733,7 +733,7 @@ fn optional_distinctness() -> impl NoirParser { } fn maybe_comp_time() -> impl NoirParser { - keyword(Keyword::CompTime).or_not().validate(|opt, span, emit| { + keyword(Keyword::Comptime).or_not().validate(|opt, span, emit| { if opt.is_some() { emit(ParserError::with_reason( ParserErrorReason::ExperimentalFeature("comptime"), diff --git a/compiler/noirc_frontend/src/parser/parser/types.rs b/compiler/noirc_frontend/src/parser/parser/types.rs index d8a63d161b9..82dd3dad681 100644 --- a/compiler/noirc_frontend/src/parser/parser/types.rs +++ b/compiler/noirc_frontend/src/parser/parser/types.rs @@ -12,7 +12,7 @@ use chumsky::prelude::*; use noirc_errors::Span; fn maybe_comp_time() -> impl NoirParser<()> { - keyword(Keyword::CompTime).or_not().validate(|opt, span, emit| { + keyword(Keyword::Comptime).or_not().validate(|opt, span, emit| { if opt.is_some() { emit(ParserError::with_reason(ParserErrorReason::ComptimeDeprecated, span)); } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index ac3d7bbc4cc..31bf2245b1f 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -780,7 +780,7 @@ mod test { HirStatement::Error => panic!("Invalid HirStatement!"), HirStatement::Break => panic!("Unexpected break"), HirStatement::Continue => panic!("Unexpected continue"), - HirStatement::CompTime(_) => panic!("Unexpected comptime"), + HirStatement::Comptime(_) => panic!("Unexpected comptime"), }; let expr = interner.expression(&expr_id); diff --git a/tooling/nargo_fmt/src/visitor/stmt.rs b/tooling/nargo_fmt/src/visitor/stmt.rs index 869977d5f3c..e41827c94a1 100644 --- a/tooling/nargo_fmt/src/visitor/stmt.rs +++ b/tooling/nargo_fmt/src/visitor/stmt.rs @@ -103,7 +103,7 @@ impl super::FmtVisitor<'_> { StatementKind::Error => unreachable!(), StatementKind::Break => self.push_rewrite("break;".into(), span), StatementKind::Continue => self.push_rewrite("continue;".into(), span), - StatementKind::CompTime(statement) => self.visit_stmt(*statement, span, is_last), + StatementKind::Comptime(statement) => self.visit_stmt(*statement, span, is_last), } } }