From e73cdbb93b0714331fef754f862d89c08c28a9e5 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 29 May 2024 14:44:30 -0500 Subject: [PATCH] fix(experimental elaborator): Fix global values used in the elaborator (#5135) # Description ## Problem\* Resolves an issue where globals would not have their type set correctly in the elaborator. This is because `elaborate_let` was called which creates a fresh DefinitionId rather than the one already defined by the global, and pushes the type to that fresh id. ## Summary\* Adds a `elaborate_global_let` function which copies over the new type to the original DefinitionId defined by the global. This PR also adds a call to `self.type_variables.clear()` after a global is resolved so that its type is not defaulted too early. ## 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\* - [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. --- .../src/elaborator/expressions.rs | 2 + compiler/noirc_frontend/src/elaborator/mod.rs | 11 ++-- .../src/elaborator/statements.rs | 52 +++++++++++++++---- tooling/nargo_cli/tests/stdlib-tests.rs | 17 ++---- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 2ad1598c6a2..8acd1867074 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -348,6 +348,8 @@ impl<'context> Elaborator<'context> { let func_type = self.type_check_variable(function_name, function_id, turbofish_generics); + self.interner.push_expr_type(function_id, func_type.clone()); + // Type check the new call now that it has been changed from a method call // to a function call. This way we avoid duplicating code. let typ = self.type_check_call(&function_call, func_type, function_args, span); diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index c55438193d1..1d66b4b952a 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1212,8 +1212,6 @@ impl<'context> Elaborator<'context> { let global_id = global.global_id; self.current_item = Some(DependencyId::Global(global_id)); - - let definition_kind = DefinitionKind::Global(global_id); let let_stmt = global.stmt_def; if !self.in_contract @@ -1228,11 +1226,12 @@ impl<'context> Elaborator<'context> { self.push_err(ResolverError::MutableGlobal { span }); } - let (let_statement, _typ) = self.elaborate_let(let_stmt); + self.elaborate_global_let(let_stmt, global_id); - let statement_id = self.interner.get_global(global_id).let_statement; - self.interner.get_global_definition_mut(global_id).kind = definition_kind.clone(); - self.interner.replace_statement(statement_id, let_statement); + // Avoid defaulting the types of globals here since they may be used in any function. + // Otherwise we may prematurely default to a Field inside the next function if this + // global was unused there, even if it is consistently used as a u8 everywhere else. + self.type_variables.clear(); } fn define_function_metas( diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index a7a2df4041e..9c50e7096dd 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -15,7 +15,7 @@ use crate::{ macros_api::{ ForLoopStatement, ForRange, HirStatement, LetStatement, Statement, StatementKind, }, - node_interner::{DefinitionId, DefinitionKind, StmtId}, + node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId}, Type, }; @@ -24,7 +24,7 @@ use super::Elaborator; impl<'context> Elaborator<'context> { fn elaborate_statement_value(&mut self, statement: Statement) -> (HirStatement, Type) { match statement.kind { - StatementKind::Let(let_stmt) => self.elaborate_let(let_stmt), + StatementKind::Let(let_stmt) => self.elaborate_local_let(let_stmt), StatementKind::Constrain(constrain) => self.elaborate_constrain(constrain), StatementKind::Assign(assign) => self.elaborate_assign(assign), StatementKind::For(for_stmt) => self.elaborate_for(for_stmt), @@ -51,7 +51,33 @@ impl<'context> Elaborator<'context> { (id, typ) } - pub(super) fn elaborate_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type) { + pub(super) fn elaborate_local_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type) { + let (statement, typ, _) = self.elaborate_let(let_stmt); + (statement, typ) + } + + /// Elaborates a global let statement. Compared to the local version, this + /// version fixes up the result to use the given DefinitionId rather than + /// the fresh one defined by the let pattern. + pub(super) fn elaborate_global_let(&mut self, let_stmt: LetStatement, global_id: GlobalId) { + let (let_statement, _typ, new_ids) = self.elaborate_let(let_stmt); + let statement_id = self.interner.get_global(global_id).let_statement; + + // To apply the changes from the fresh id created in elaborate_let to this global + // we need to change the definition kind and update the type. + let definition_id = self.interner.get_global(global_id).definition_id; + self.interner.definition_mut(definition_id).kind = DefinitionKind::Global(global_id); + + assert_eq!(new_ids.len(), 1, "Globals should only define 1 value"); + let definition_type = self.interner.definition_type(new_ids[0].id); + self.interner.push_definition_type(definition_id, definition_type); + + self.interner.replace_statement(statement_id, let_statement); + } + + /// Elaborate a local or global let statement. In addition to the HirLetStatement and unit + /// type, this also returns each HirIdent defined by this let statement. + fn elaborate_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type, Vec) { let expr_span = let_stmt.expression.span; let (expression, expr_type) = self.elaborate_expression(let_stmt.expression); let definition = DefinitionKind::Local(Some(expression)); @@ -77,14 +103,18 @@ impl<'context> Elaborator<'context> { expr_type }; - let let_ = HirLetStatement { - pattern: self.elaborate_pattern(let_stmt.pattern, r#type.clone(), definition), - r#type, - expression, - attributes: let_stmt.attributes, - comptime: let_stmt.comptime, - }; - (HirStatement::Let(let_), Type::Unit) + let mut created_ids = Vec::new(); + let pattern = self.elaborate_pattern_and_store_ids( + let_stmt.pattern, + r#type.clone(), + definition, + &mut created_ids, + ); + + let attributes = let_stmt.attributes; + let comptime = let_stmt.comptime; + let let_ = HirLetStatement { pattern, r#type, expression, attributes, comptime }; + (HirStatement::Let(let_), Type::Unit, created_ids) } pub(super) fn elaborate_constrain(&mut self, stmt: ConstrainStatement) -> (HirStatement, Type) { diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index 70a9354f50a..0bb967e7502 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -10,7 +10,8 @@ use nargo::{ parse_all, prepare_package, }; -fn run_stdlib_tests(use_elaborator: bool) { +#[test] +fn run_stdlib_tests() { let mut file_manager = file_manager_with_stdlib(&PathBuf::from(".")); file_manager.add_file_with_source_canonical_path(&PathBuf::from("main.nr"), "".to_owned()); let parsed_files = parse_all(&file_manager); @@ -29,7 +30,7 @@ fn run_stdlib_tests(use_elaborator: bool) { let (mut context, dummy_crate_id) = prepare_package(&file_manager, &parsed_files, &dummy_package); - let result = check_crate(&mut context, dummy_crate_id, true, false, use_elaborator); + let result = check_crate(&mut context, dummy_crate_id, true, false, false); report_errors(result, &context.file_manager, true, false) .expect("Error encountered while compiling standard library"); @@ -59,15 +60,3 @@ fn run_stdlib_tests(use_elaborator: bool) { assert!(!test_report.is_empty(), "Could not find any tests within the stdlib"); assert!(test_report.iter().all(|(_, status)| !status.failed())); } - -#[test] -fn stdlib_noir_tests() { - run_stdlib_tests(false) -} - -// Once this no longer panics we can use the elaborator by default and remove the old passes -#[test] -#[should_panic] -fn stdlib_elaborator_tests() { - run_stdlib_tests(true) -}