From b0fde1d9edf85c9d5c90d25d93728eda374bdcdc Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 25 Apr 2024 14:40:25 -0500 Subject: [PATCH 1/3] Implement comptime mut globals --- aztec_macros/src/utils/hir_utils.rs | 1 + compiler/noirc_frontend/src/ast/statement.rs | 7 ++-- .../src/hir/comptime/hir_to_ast.rs | 2 +- .../src/hir/comptime/interpreter.rs | 11 ++++-- .../noirc_frontend/src/hir/comptime/tests.rs | 14 +++++++ .../src/hir/def_collector/dc_mod.rs | 5 ++- .../src/hir/resolution/errors.rs | 9 +++++ .../src/hir/resolution/resolver.rs | 17 ++++++--- compiler/noirc_frontend/src/node_interner.rs | 10 ++++- compiler/noirc_frontend/src/parser/parser.rs | 37 ++++++++++++------- compiler/noirc_frontend/src/tests.rs | 10 +++++ .../comptime_mut_global/Nargo.toml | 7 ++++ .../comptime_mut_global/src/main.nr | 24 ++++++++++++ tooling/nargo_fmt/src/visitor/stmt.rs | 2 +- 14 files changed, 126 insertions(+), 30 deletions(-) create mode 100644 test_programs/compile_success_empty/comptime_mut_global/Nargo.toml create mode 100644 test_programs/compile_success_empty/comptime_mut_global/src/main.nr diff --git a/aztec_macros/src/utils/hir_utils.rs b/aztec_macros/src/utils/hir_utils.rs index 3801ff1cc75..99b02acd606 100644 --- a/aztec_macros/src/utils/hir_utils.rs +++ b/aztec_macros/src/utils/hir_utils.rs @@ -228,6 +228,7 @@ pub fn inject_global( module_id, file_id, global.attributes.clone(), + false, ); // Add the statement to the scope so its path can be looked up later diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 1831a046f5b..0da39edfd85 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. @@ -486,7 +486,8 @@ impl Pattern { pub fn name_ident(&self) -> &Ident { match self { Pattern::Identifier(name_ident) => name_ident, - _ => panic!("only the identifier pattern can return a name"), + Pattern::Mutable(pattern, ..) => pattern.name_ident(), + _ => panic!("Only the Identifier or Mutable patterns can return a name"), } } @@ -685,7 +686,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.kind), StatementKind::Semi(semi) => write!(f, "{semi};"), StatementKind::Error => write!(f, "Error"), } 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 42ee76d29fa..1ab9c13ea25 100644 --- a/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs +++ b/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs @@ -67,7 +67,7 @@ impl StmtId { 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)) + StatementKind::Comptime(Box::new(statement.to_ast(interner))) } }; diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index b8ffb209bc7..89251743d6a 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -332,9 +332,14 @@ impl<'a> Interpreter<'a> { DefinitionKind::Local(_) => self.lookup(&ident), DefinitionKind::Global(global_id) => { // Don't need to check let_.comptime, we can evaluate non-comptime globals too. - let let_ = self.interner.get_global_let_statement(*global_id).unwrap(); - self.evaluate_let(let_)?; - self.lookup(&ident) + // Avoid resetting the value if it is already known + if let Ok(value) = self.lookup(&ident) { + Ok(value) + } else { + let let_ = self.interner.get_global_let_statement(*global_id).unwrap(); + self.evaluate_let(let_)?; + self.lookup(&ident) + } } DefinitionKind::GenericType(type_variable) => { let value = match &*type_variable.borrow() { diff --git a/compiler/noirc_frontend/src/hir/comptime/tests.rs b/compiler/noirc_frontend/src/hir/comptime/tests.rs index 1a84dae4a87..5a12eb7292c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/tests.rs +++ b/compiler/noirc_frontend/src/hir/comptime/tests.rs @@ -76,6 +76,20 @@ fn mutating_arrays() { assert_eq!(result, Value::U8(22)); } +#[test] +fn mutate_in_new_scope() { + let program = "fn main() -> pub u8 { + let mut x = 0; + x += 1; + { + x += 1; + } + x + }"; + let result = interpret(program, vec!["main".into()]); + assert_eq!(result, Value::U8(2)); +} + #[test] fn for_loop() { let program = "fn main() -> pub u8 { diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index baedb109967..b2ec7dbc813 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -6,7 +6,8 @@ use noirc_errors::Location; use crate::ast::{ FunctionDefinition, Ident, ItemVisibility, LetStatement, ModuleDeclaration, NoirFunction, - NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl, + NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Pattern, TraitImplItem, TraitItem, + TypeImpl, }; use crate::{ graph::CrateId, @@ -109,6 +110,7 @@ impl<'a> ModCollector<'a> { self.module_id, self.file_id, global.attributes.clone(), + matches!(global.pattern, Pattern::Mutable { .. }), ); // Add the statement to the scope so its path can be looked up later @@ -463,6 +465,7 @@ impl<'a> ModCollector<'a> { trait_id.0.local_id, self.file_id, vec![], + false, ); if let Err((first_def, second_def)) = self.def_collector.def_map.modules diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index a90a95d0116..a8b73c15814 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -86,6 +86,8 @@ pub enum ResolverError { JumpInConstrainedFn { is_break: bool, span: Span }, #[error("break/continue are only allowed within loops")] JumpOutsideLoop { is_break: bool, span: Span }, + #[error("Only `comptime` globals can be mutable")] + MutableGlobal { span: Span }, } impl ResolverError { @@ -340,6 +342,13 @@ impl<'a> From<&'a ResolverError> for Diagnostic { *span, ) }, + ResolverError::MutableGlobal { span } => { + Diagnostic::simple_error( + "Only `comptime` globals may be mutable".into(), + String::new(), + *span, + ) + }, } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 1dc60bcc010..04f9f2260bf 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -1173,15 +1173,18 @@ impl<'a> Resolver<'a> { ) -> HirStatement { self.current_item = Some(DependencyId::Global(global_id)); let expression = self.resolve_expression(let_stmt.expression); - let global_id = self.interner.next_global_id(); let definition = DefinitionKind::Global(global_id); if !self.in_contract && let_stmt.attributes.iter().any(|attr| matches!(attr, SecondaryAttribute::Abi(_))) { - self.push_err(ResolverError::AbiAttributeOusideContract { - span: let_stmt.pattern.span(), - }); + let span = let_stmt.pattern.span(); + self.push_err(ResolverError::AbiAttributeOusideContract { span }); + } + + if !let_stmt.comptime && matches!(let_stmt.pattern, Pattern::Mutable(..)) { + let span = let_stmt.pattern.span(); + self.push_err(ResolverError::MutableGlobal { span }); } HirStatement::Let(HirLetStatement { @@ -1275,8 +1278,10 @@ impl<'a> Resolver<'a> { } StatementKind::Error => HirStatement::Error, StatementKind::Comptime(statement) => { - let statement = self.resolve_stmt(*statement, span); - HirStatement::Comptime(self.interner.push_stmt(statement)) + let hir_statement = self.resolve_stmt(statement.kind, statement.span); + let statement_id = self.interner.push_stmt(hir_statement); + self.interner.push_statement_location(statement_id, statement.span, self.file); + HirStatement::Comptime(statement_id) } } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 5e4fa3df6c5..7ec32613cb7 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -653,11 +653,13 @@ impl NodeInterner { let_statement: StmtId, file: FileId, attributes: Vec, + mutable: bool, ) -> GlobalId { let id = GlobalId(self.globals.len()); let location = Location::new(ident.span(), file); let name = ident.to_string(); - let definition_id = self.push_definition(name, false, DefinitionKind::Global(id), location); + let definition_id = + self.push_definition(name, mutable, DefinitionKind::Global(id), location); self.globals.push(GlobalInfo { id, @@ -682,9 +684,13 @@ impl NodeInterner { local_id: LocalModuleId, file: FileId, attributes: Vec, + mutable: bool, ) -> GlobalId { let statement = self.push_stmt(HirStatement::Error); - self.push_global(name, local_id, statement, file, attributes) + let span = name.span(); + let id = self.push_global(name, local_id, statement, file, attributes, mutable); + self.push_statement_location(statement, span, file); + id } /// Intern an empty function. diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 5cbab9bde3a..82b848d8095 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -235,16 +235,26 @@ fn implementation() -> impl NoirParser { fn global_declaration() -> impl NoirParser { let p = attributes::attributes() .then(maybe_comp_time()) + .then(spanned(keyword(Keyword::Mut)).or_not()) .then_ignore(keyword(Keyword::Global).labelled(ParsingRuleLabel::Global)) .then(ident().map(Pattern::Identifier)); let p = then_commit(p, optional_type_annotation()); let p = then_commit_ignore(p, just(Token::Assign)); let p = then_commit(p, expression()); - p.validate(|((((attributes, comptime), pattern), r#type), expression), span, emit| { - let global_attributes = attributes::validate_secondary_attributes(attributes, span, emit); - LetStatement { pattern, r#type, comptime, expression, attributes: global_attributes } - }) + p.validate( + |(((((attributes, comptime), mutable), mut pattern), r#type), expression), span, emit| { + let global_attributes = + attributes::validate_secondary_attributes(attributes, span, emit); + + // Only comptime globals are allowed to be mutable, but we always parse the `mut` + if let Some((_, mut_span)) = mutable { + let span = mut_span.merge(pattern.span()); + pattern = Pattern::Mutable(Box::new(pattern), span, false); + } + LetStatement { pattern, r#type, comptime, expression, attributes: global_attributes } + }, + ) .map(TopLevelStatement::Global) } @@ -532,15 +542,16 @@ where P2: ExprParser + 'a, S: NoirParser + 'a, { - keyword(Keyword::Comptime) - .ignore_then(choice(( - declaration(expr), - for_loop(expr_no_constructors, statement.clone()), - block(statement).map_with_span(|block, span| { - StatementKind::Expression(Expression::new(ExpressionKind::Block(block), span)) - }), - ))) - .map(|statement| StatementKind::Comptime(Box::new(statement))) + let comptime_statement = choice(( + declaration(expr), + for_loop(expr_no_constructors, statement.clone()), + block(statement).map_with_span(|block, span| { + StatementKind::Expression(Expression::new(ExpressionKind::Block(block), span)) + }), + )) + .map_with_span(|kind, span| Box::new(Statement { kind, span })); + + keyword(Keyword::Comptime).ignore_then(comptime_statement).map(StatementKind::Comptime) } /// Comptime in an expression position only accepts entire blocks diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c6cef568ed8..7a2c14fbced 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1282,4 +1282,14 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { "#; assert_eq!(get_program_errors(src).len(), 0); } + + #[test] + fn ban_mutable_globals() { + // Mutable globals are only allowed in a comptime context + let src = r#" + mut global FOO: Field = 0; + fn main() {} + "#; + assert_eq!(get_program_errors(src).len(), 1); + } } diff --git a/test_programs/compile_success_empty/comptime_mut_global/Nargo.toml b/test_programs/compile_success_empty/comptime_mut_global/Nargo.toml new file mode 100644 index 00000000000..c29aa8d83b9 --- /dev/null +++ b/test_programs/compile_success_empty/comptime_mut_global/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "comptime_mut_global" +type = "bin" +authors = [""] +compiler_version = ">=0.28.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/comptime_mut_global/src/main.nr b/test_programs/compile_success_empty/comptime_mut_global/src/main.nr new file mode 100644 index 00000000000..9aacf9af8dd --- /dev/null +++ b/test_programs/compile_success_empty/comptime_mut_global/src/main.nr @@ -0,0 +1,24 @@ +comptime mut global COUNTER: Field = 0; + +comptime fn get_unique_id() -> Field { + let id = COUNTER; + COUNTER += 1; + id +} + +fn id1() -> Field { + comptime { get_unique_id() } +} + +fn id2() -> Field { + comptime { get_unique_id() } +} + +fn main() { + // Order of comptime evaluation between functions isn't guaranteed + // so we don't know if (id1 == 0 && id2 == 1) or if (id1 == 1 && id2 == 0). + // we only know they are not equal + let id1 = id1(); + let id2 = id2(); + assert(id1 != id2); +} diff --git a/tooling/nargo_fmt/src/visitor/stmt.rs b/tooling/nargo_fmt/src/visitor/stmt.rs index e41827c94a1..8e05fe3f5c5 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.kind, span, is_last), } } } From 6cc6d0ef29d1349d056e4151005238fd5f8fbf12 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 25 Apr 2024 15:27:28 -0500 Subject: [PATCH 2/3] fmt test program in an objectively worse fashion --- .../comptime_mut_global/src/main.nr | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test_programs/compile_success_empty/comptime_mut_global/src/main.nr b/test_programs/compile_success_empty/comptime_mut_global/src/main.nr index 9aacf9af8dd..fa739289d37 100644 --- a/test_programs/compile_success_empty/comptime_mut_global/src/main.nr +++ b/test_programs/compile_success_empty/comptime_mut_global/src/main.nr @@ -7,11 +7,17 @@ comptime fn get_unique_id() -> Field { } fn id1() -> Field { - comptime { get_unique_id() } + comptime + { + get_unique_id() + } } fn id2() -> Field { - comptime { get_unique_id() } + comptime + { + get_unique_id() + } } fn main() { From 1c5dced74ed69d0aa29b8c5271c77707d82e2e80 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 29 Apr 2024 12:18:55 +0100 Subject: [PATCH 3/3] Update compiler/noirc_frontend/src/parser/parser.rs --- compiler/noirc_frontend/src/parser/parser.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 82b848d8095..4a85f6cb18f 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -248,6 +248,7 @@ fn global_declaration() -> impl NoirParser { attributes::validate_secondary_attributes(attributes, span, emit); // Only comptime globals are allowed to be mutable, but we always parse the `mut` + // and throw the error in name resolution. if let Some((_, mut_span)) = mutable { let span = mut_span.merge(pattern.span()); pattern = Pattern::Mutable(Box::new(pattern), span, false);