Skip to content

Commit 6a66138

Browse files
author
jfecher
authored
chore(experimental): Improve variable not defined error message in comptime interpreter (#4889)
# Description ## Problem\* Resolves <!-- Link to GitHub Issue --> ## 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.
1 parent ce5e1a5 commit 6a66138

16 files changed

Lines changed: 40 additions & 47 deletions

File tree

compiler/noirc_frontend/src/ast/statement.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ pub enum StatementKind {
4040
Break,
4141
Continue,
4242
/// This statement should be executed at compile-time
43-
CompTime(Box<StatementKind>),
43+
Comptime(Box<StatementKind>),
4444
// This is an expression with a trailing semi-colon
4545
Semi(Expression),
4646
// This statement is the result of a recovered parse error.
@@ -87,10 +87,10 @@ impl StatementKind {
8787
}
8888
self
8989
}
90-
StatementKind::CompTime(mut statement) => {
90+
StatementKind::Comptime(mut statement) => {
9191
*statement =
9292
statement.add_semicolon(semi, span, last_statement_in_block, emit_error);
93-
StatementKind::CompTime(statement)
93+
StatementKind::Comptime(statement)
9494
}
9595
// A semicolon on a for loop is optional and does nothing
9696
StatementKind::For(_) => self,
@@ -685,7 +685,7 @@ impl Display for StatementKind {
685685
StatementKind::For(for_loop) => for_loop.fmt(f),
686686
StatementKind::Break => write!(f, "break"),
687687
StatementKind::Continue => write!(f, "continue"),
688-
StatementKind::CompTime(statement) => write!(f, "comptime {statement}"),
688+
StatementKind::Comptime(statement) => write!(f, "comptime {statement}"),
689689
StatementKind::Semi(semi) => write!(f, "{semi};"),
690690
StatementKind::Error => write!(f, "Error"),
691691
}

compiler/noirc_frontend/src/hir/comptime/errors.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::{node_interner::DefinitionId, Type};
1+
use crate::Type;
22
use acvm::FieldElement;
33
use noirc_errors::Location;
44

@@ -9,7 +9,7 @@ use super::value::Value;
99
pub enum InterpreterError {
1010
ArgumentCountMismatch { expected: usize, actual: usize, call_location: Location },
1111
TypeMismatch { expected: Type, value: Value, location: Location },
12-
NoValueForId { id: DefinitionId, location: Location },
12+
NonComptimeVarReferenced { name: String, location: Location },
1313
IntegerOutOfRangeForType { value: FieldElement, typ: Type, location: Location },
1414
ErrorNodeEncountered { location: Location },
1515
NonFunctionCalled { value: Value, location: Location },

compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,8 @@ impl StmtId {
6666
HirStatement::Expression(expr) => StatementKind::Expression(expr.to_ast(interner)),
6767
HirStatement::Semi(expr) => StatementKind::Semi(expr.to_ast(interner)),
6868
HirStatement::Error => StatementKind::Error,
69-
HirStatement::CompTime(statement) => {
70-
StatementKind::CompTime(Box::new(statement.to_ast(interner).kind))
69+
HirStatement::Comptime(statement) => {
70+
StatementKind::Comptime(Box::new(statement.to_ast(interner).kind))
7171
}
7272
};
7373

compiler/noirc_frontend/src/hir/comptime/interpreter.rs

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -246,13 +246,7 @@ impl<'a> Interpreter<'a> {
246246
location: Location,
247247
) -> IResult<()> {
248248
self.type_check(typ, &argument, location)?;
249-
for scope in self.scopes.iter_mut().rev() {
250-
if let Entry::Occupied(mut entry) = scope.entry(id) {
251-
entry.insert(argument);
252-
return Ok(());
253-
}
254-
}
255-
Err(InterpreterError::NoValueForId { id, location })
249+
self.mutate(id, argument, location)
256250
}
257251

258252
/// Mutate an existing variable, potentially from a prior scope
@@ -263,17 +257,12 @@ impl<'a> Interpreter<'a> {
263257
return Ok(());
264258
}
265259
}
266-
Err(InterpreterError::NoValueForId { id, location })
260+
let name = self.interner.definition(id).name.clone();
261+
Err(InterpreterError::NonComptimeVarReferenced { name, location })
267262
}
268263

269264
fn lookup(&self, ident: &HirIdent) -> IResult<Value> {
270-
for scope in self.scopes.iter().rev() {
271-
if let Some(value) = scope.get(&ident.id) {
272-
return Ok(value.clone());
273-
}
274-
}
275-
276-
Err(InterpreterError::NoValueForId { id: ident.id, location: ident.location })
265+
self.lookup_id(ident.id, ident.location)
277266
}
278267

279268
fn lookup_id(&self, id: DefinitionId, location: Location) -> IResult<Value> {
@@ -283,7 +272,12 @@ impl<'a> Interpreter<'a> {
283272
}
284273
}
285274

286-
Err(InterpreterError::NoValueForId { id, location })
275+
// Justification for `NonComptimeVarReferenced`:
276+
// If we have an id to lookup at all that means name resolution successfully
277+
// found another variable in scope for this name. If the name is in scope
278+
// but unknown by the interpreter it must be because it was not a comptime variable.
279+
let name = self.interner.definition(id).name.clone();
280+
Err(InterpreterError::NonComptimeVarReferenced { name, location })
287281
}
288282

289283
fn type_check(&self, typ: &Type, value: &Value, location: Location) -> IResult<()> {
@@ -1023,7 +1017,7 @@ impl<'a> Interpreter<'a> {
10231017
HirStatement::Break => self.evaluate_break(statement),
10241018
HirStatement::Continue => self.evaluate_continue(statement),
10251019
HirStatement::Expression(expression) => self.evaluate(expression),
1026-
HirStatement::CompTime(statement) => self.evaluate_comptime(statement),
1020+
HirStatement::Comptime(statement) => self.evaluate_comptime(statement),
10271021
HirStatement::Semi(expression) => {
10281022
self.evaluate(expression)?;
10291023
Ok(Value::Unit)

compiler/noirc_frontend/src/hir/comptime/scan.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
//! This module is for the scanning of the Hir by the interpreter.
2-
//! In this initial step, the Hir is scanned for `CompTime` nodes
2+
//! In this initial step, the Hir is scanned for `Comptime` nodes
33
//! without actually executing anything until such a node is found.
44
//! Once such a node is found, the interpreter will call the relevant
55
//! evaluate method on that node type, insert the result into the Ast,
66
//! and continue scanning the rest of the program.
77
//!
8-
//! Since it mostly just needs to recur on the Hir looking for CompTime
8+
//! Since it mostly just needs to recur on the Hir looking for Comptime
99
//! nodes, this pass is fairly simple. The only thing it really needs to
1010
//! ensure to do is to push and pop scopes on the interpreter as needed
1111
//! so that any variables defined within e.g. an `if` statement containing
12-
//! a `CompTime` block aren't accessible outside of the `if`.
12+
//! a `Comptime` block aren't accessible outside of the `if`.
1313
use crate::{
1414
hir_def::{
1515
expr::{
@@ -30,7 +30,7 @@ use super::{
3030

3131
#[allow(dead_code)]
3232
impl<'interner> Interpreter<'interner> {
33-
/// Scan through a function, evaluating any CompTime nodes found.
33+
/// Scan through a function, evaluating any Comptime nodes found.
3434
/// These nodes will be modified in place, replaced with the
3535
/// result of their evaluation.
3636
pub fn scan_function(&mut self, function: FuncId) -> IResult<()> {
@@ -185,7 +185,7 @@ impl<'interner> Interpreter<'interner> {
185185
HirStatement::Expression(expression) => self.scan_expression(expression),
186186
HirStatement::Semi(semi) => self.scan_expression(semi),
187187
HirStatement::Error => Ok(()),
188-
HirStatement::CompTime(comptime) => {
188+
HirStatement::Comptime(comptime) => {
189189
let location = self.interner.statement_location(comptime);
190190
let new_expr =
191191
self.evaluate_comptime(comptime)?.into_expression(self.interner, location)?;

compiler/noirc_frontend/src/hir/comptime/value.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ use rustc_hash::FxHashMap as HashMap;
1616

1717
use super::errors::{IResult, InterpreterError};
1818

19-
#[allow(unused)]
2019
#[derive(Debug, Clone, PartialEq, Eq)]
2120
pub enum Value {
2221
Unit,

compiler/noirc_frontend/src/hir/resolution/resolver.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1272,9 +1272,9 @@ impl<'a> Resolver<'a> {
12721272
HirStatement::Continue
12731273
}
12741274
StatementKind::Error => HirStatement::Error,
1275-
StatementKind::CompTime(statement) => {
1275+
StatementKind::Comptime(statement) => {
12761276
let statement = self.resolve_stmt(*statement, span);
1277-
HirStatement::CompTime(self.interner.push_stmt(statement))
1277+
HirStatement::Comptime(self.interner.push_stmt(statement))
12781278
}
12791279
}
12801280
}

compiler/noirc_frontend/src/hir/type_check/stmt.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ impl<'interner> TypeChecker<'interner> {
5151
HirStatement::Constrain(constrain_stmt) => self.check_constrain_stmt(constrain_stmt),
5252
HirStatement::Assign(assign_stmt) => self.check_assign_stmt(assign_stmt, stmt_id),
5353
HirStatement::For(for_loop) => self.check_for_loop(for_loop),
54-
HirStatement::CompTime(statement) => return self.check_statement(&statement),
54+
HirStatement::Comptime(statement) => return self.check_statement(&statement),
5555
HirStatement::Break | HirStatement::Continue | HirStatement::Error => (),
5656
}
5757
Type::Unit

compiler/noirc_frontend/src/hir_def/stmt.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ pub enum HirStatement {
2020
Continue,
2121
Expression(ExprId),
2222
Semi(ExprId),
23-
CompTime(StmtId),
23+
Comptime(StmtId),
2424
Error,
2525
}
2626

compiler/noirc_frontend/src/lexer/token.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -811,7 +811,7 @@ pub enum Keyword {
811811
Break,
812812
CallData,
813813
Char,
814-
CompTime,
814+
Comptime,
815815
Constrain,
816816
Continue,
817817
Contract,
@@ -856,7 +856,7 @@ impl fmt::Display for Keyword {
856856
Keyword::Break => write!(f, "break"),
857857
Keyword::Char => write!(f, "char"),
858858
Keyword::CallData => write!(f, "call_data"),
859-
Keyword::CompTime => write!(f, "comptime"),
859+
Keyword::Comptime => write!(f, "comptime"),
860860
Keyword::Constrain => write!(f, "constrain"),
861861
Keyword::Continue => write!(f, "continue"),
862862
Keyword::Contract => write!(f, "contract"),
@@ -904,7 +904,7 @@ impl Keyword {
904904
"break" => Keyword::Break,
905905
"call_data" => Keyword::CallData,
906906
"char" => Keyword::Char,
907-
"comptime" => Keyword::CompTime,
907+
"comptime" => Keyword::Comptime,
908908
"constrain" => Keyword::Constrain,
909909
"continue" => Keyword::Continue,
910910
"contract" => Keyword::Contract,

0 commit comments

Comments
 (0)