From d2ea5ce85fbc17ba121b1b428f3707c36e98d2b7 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 14:21:56 +0100 Subject: [PATCH 1/7] Maintain a reverse index from (LocalModuleId, Ident) to GlobalId --- compiler/noirc_frontend/src/elaborator/patterns.rs | 9 ++------- compiler/noirc_frontend/src/node_interner.rs | 13 ++++++++++++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 0f6cb78fae7..cb692cacf37 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -386,13 +386,8 @@ impl<'context> Elaborator<'context> { // This check is necessary to maintain the same definition ids in the interner. Currently, each function uses a new resolver that has its own ScopeForest and thus global scope. // We must first check whether an existing definition ID has been inserted as otherwise there will be multiple definitions for the same global statement. // This leads to an error in evaluation where the wrong definition ID is selected when evaluating a statement using the global. The check below prevents this error. - let mut global_id = None; - let global = self.interner.get_all_globals(); - for global_info in global { - if global_info.local_id == self.local_module && global_info.ident == name { - global_id = Some(global_info.id); - } - } + let global_id = + self.interner.find_global_by_local_and_ident(self.local_module, name.clone()); let (ident, resolver_meta) = if let Some(id) = global_id { let global = self.interner.get_global(id); diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index b80c37c2ce4..eee74dc6bae 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -181,6 +181,7 @@ pub struct NodeInterner { // NOTE: currently only used for checking repeat globals and restricting their scope to a module globals: Vec, global_attributes: HashMap>, + globals_by_local_and_ident: HashMap<(LocalModuleId, Ident), GlobalId>, next_type_variable_id: std::cell::Cell, @@ -660,6 +661,7 @@ impl Default for NodeInterner { next_type_variable_id: std::cell::Cell::new(0), globals: Vec::new(), global_attributes: HashMap::default(), + globals_by_local_and_ident: HashMap::default(), struct_methods: HashMap::default(), primitive_methods: HashMap::default(), type_alias_ref: Vec::new(), @@ -855,7 +857,7 @@ impl NodeInterner { self.globals.push(GlobalInfo { id, definition_id, - ident, + ident: ident.clone(), local_id, crate_id, let_statement, @@ -863,6 +865,7 @@ impl NodeInterner { value: None, }); self.global_attributes.insert(id, attributes); + self.globals_by_local_and_ident.insert((local_id, ident), id); id } @@ -1266,6 +1269,14 @@ impl NodeInterner { &self.globals } + pub fn find_global_by_local_and_ident( + &self, + local_id: LocalModuleId, + ident: Ident, + ) -> Option { + self.globals_by_local_and_ident.get(&(local_id, ident)).cloned() + } + /// Returns the type of an item stored in the Interner or Error if it was not found. pub fn id_type(&self, index: impl Into) -> Type { self.id_to_type.get(&index.into()).cloned().unwrap_or(Type::Error) From 32e57995bc8a6e243439a69fd5c8a93634741579 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 14:46:36 +0100 Subject: [PATCH 2/7] Do not replace an existing entry --- compiler/noirc_frontend/src/node_interner.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index eee74dc6bae..8dbb8a93e87 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -854,10 +854,17 @@ impl NodeInterner { let definition_id = self.push_definition(name, mutable, comptime, DefinitionKind::Global(id), location); + // To guarantee equivalence with the former method of looping through all globals + // and returning the first match, do not overwrite an existing entry. + let local_and_ident = (local_id, ident.clone()); + if !self.globals_by_local_and_ident.contains_key(&local_and_ident) { + self.globals_by_local_and_ident.insert(local_and_ident, id); + } + self.globals.push(GlobalInfo { id, definition_id, - ident: ident.clone(), + ident, local_id, crate_id, let_statement, @@ -865,7 +872,6 @@ impl NodeInterner { value: None, }); self.global_attributes.insert(id, attributes); - self.globals_by_local_and_ident.insert((local_id, ident), id); id } From 6e022305c3e3048634ecdfba8db1c6264e9efb7f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 14:52:36 +0100 Subject: [PATCH 3/7] Clippy --- compiler/noirc_frontend/src/node_interner.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 8dbb8a93e87..9e2af13ccd7 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -856,10 +856,7 @@ impl NodeInterner { // To guarantee equivalence with the former method of looping through all globals // and returning the first match, do not overwrite an existing entry. - let local_and_ident = (local_id, ident.clone()); - if !self.globals_by_local_and_ident.contains_key(&local_and_ident) { - self.globals_by_local_and_ident.insert(local_and_ident, id); - } + self.globals_by_local_and_ident.entry((local_id, ident.clone())).or_insert(id); self.globals.push(GlobalInfo { id, From 1d2bd36ab5fb38ebab28b8262dbf13f8020ee461 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 15:03:28 +0100 Subject: [PATCH 4/7] Sanity check that the GlobalId and the definition are the same --- compiler/noirc_frontend/src/elaborator/patterns.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index cb692cacf37..f222876ab53 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -83,6 +83,8 @@ impl<'context> Elaborator<'context> { (_, other) => other, }; let ident = if let Some(global_id) = global_id { + // Sanity check that we don't have conflicting globals. + assert_eq!(definition, DefinitionKind::Global(global_id)); // Globals don't need to be added to scope, they're already in the def_maps let id = self.interner.get_global(global_id).definition_id; let location = Location::new(name.span(), self.file); From ce3fa081e16ef7888f02e269d1a0951a6b410674 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 15:56:31 +0100 Subject: [PATCH 5/7] Sanity check that we found the same as the def --- compiler/noirc_frontend/src/elaborator/patterns.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index f222876ab53..182f9c6ca14 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -392,6 +392,8 @@ impl<'context> Elaborator<'context> { self.interner.find_global_by_local_and_ident(self.local_module, name.clone()); let (ident, resolver_meta) = if let Some(id) = global_id { + // Sanity check that we're referring to the same global. + assert_eq!(definition, DefinitionKind::Global(id)); let global = self.interner.get_global(id); let hir_ident = HirIdent::non_trait_method(global.definition_id, global.location); let ident = hir_ident.clone(); From 5f552d121972bd5e40ac4de12528285bbb3ce11b Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 16:30:52 +0100 Subject: [PATCH 6/7] Remove the reverse index, only look up by global ID --- .../noirc_frontend/src/elaborator/patterns.rs | 43 ++++--------------- compiler/noirc_frontend/src/node_interner.rs | 14 ------ 2 files changed, 9 insertions(+), 48 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 182f9c6ca14..f82244d34fc 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -331,8 +331,8 @@ impl<'context> Elaborator<'context> { warn_if_unused: bool, definition: DefinitionKind, ) -> HirIdent { - if definition.is_global() { - return self.add_global_variable_decl(name, definition); + if let DefinitionKind::Global(global_id) = definition { + return self.add_global_variable_decl(name, global_id); } let location = Location::new(name.span(), self.file); @@ -377,44 +377,19 @@ impl<'context> Elaborator<'context> { } } - pub fn add_global_variable_decl( - &mut self, - name: Ident, - definition: DefinitionKind, - ) -> HirIdent { - let comptime = self.in_comptime_context(); + pub fn add_global_variable_decl(&mut self, name: Ident, global_id: GlobalId) -> HirIdent { let scope = self.scopes.get_mut_scope(); - - // This check is necessary to maintain the same definition ids in the interner. Currently, each function uses a new resolver that has its own ScopeForest and thus global scope. - // We must first check whether an existing definition ID has been inserted as otherwise there will be multiple definitions for the same global statement. - // This leads to an error in evaluation where the wrong definition ID is selected when evaluating a statement using the global. The check below prevents this error. - let global_id = - self.interner.find_global_by_local_and_ident(self.local_module, name.clone()); - - let (ident, resolver_meta) = if let Some(id) = global_id { - // Sanity check that we're referring to the same global. - assert_eq!(definition, DefinitionKind::Global(id)); - let global = self.interner.get_global(id); - let hir_ident = HirIdent::non_trait_method(global.definition_id, global.location); - let ident = hir_ident.clone(); - let resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true }; - (hir_ident, resolver_meta) - } else { - let location = Location::new(name.span(), self.file); - let name = name.0.contents.clone(); - let id = self.interner.push_definition(name, false, comptime, definition, location); - let ident = HirIdent::non_trait_method(id, location); - let resolver_meta = - ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused: true }; - (ident, resolver_meta) - }; + let global = self.interner.get_global(global_id); + let ident = HirIdent::non_trait_method(global.definition_id, global.location); + let resolver_meta = + ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused: true }; let old_global_value = scope.add_key_value(name.0.contents.clone(), resolver_meta); if let Some(old_global_value) = old_global_value { self.push_err(ResolverError::DuplicateDefinition { - name: name.0.contents.clone(), - first_span: old_global_value.ident.location.span, second_span: name.span(), + name: name.0.contents, + first_span: old_global_value.ident.location.span, }); } ident diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 9e2af13ccd7..b80c37c2ce4 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -181,7 +181,6 @@ pub struct NodeInterner { // NOTE: currently only used for checking repeat globals and restricting their scope to a module globals: Vec, global_attributes: HashMap>, - globals_by_local_and_ident: HashMap<(LocalModuleId, Ident), GlobalId>, next_type_variable_id: std::cell::Cell, @@ -661,7 +660,6 @@ impl Default for NodeInterner { next_type_variable_id: std::cell::Cell::new(0), globals: Vec::new(), global_attributes: HashMap::default(), - globals_by_local_and_ident: HashMap::default(), struct_methods: HashMap::default(), primitive_methods: HashMap::default(), type_alias_ref: Vec::new(), @@ -854,10 +852,6 @@ impl NodeInterner { let definition_id = self.push_definition(name, mutable, comptime, DefinitionKind::Global(id), location); - // To guarantee equivalence with the former method of looping through all globals - // and returning the first match, do not overwrite an existing entry. - self.globals_by_local_and_ident.entry((local_id, ident.clone())).or_insert(id); - self.globals.push(GlobalInfo { id, definition_id, @@ -1272,14 +1266,6 @@ impl NodeInterner { &self.globals } - pub fn find_global_by_local_and_ident( - &self, - local_id: LocalModuleId, - ident: Ident, - ) -> Option { - self.globals_by_local_and_ident.get(&(local_id, ident)).cloned() - } - /// Returns the type of an item stored in the Interner or Error if it was not found. pub fn id_type(&self, index: impl Into) -> Type { self.id_to_type.get(&index.into()).cloned().unwrap_or(Type::Error) From d1c31890edceba1d8b7cc10c7a2ae22c9b6dfa40 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 17:03:02 +0100 Subject: [PATCH 7/7] chore(refactor): Remove optional global_id from methods that can get it from the definition (#6283) # Description ## Problem\* Followup to https://github.com/noir-lang/noir/pull/6282 ## Summary\* Some methods in the `Elaborator` received a `global_id: Option` and a `definition: DefinitionKind` parameter. We found that `Elaborator::elaborate_let` actually created `definition` to carry the same global ID as it was given, so `global_id: Some(id)` and `definition: DefinitionKind::Global(id)` always carried the same value. We added an [assertion](https://github.com/noir-lang/noir/pull/6282/commits/1d2bd36ab5fb38ebab28b8262dbf13f8020ee461) that checked that they are never different. The next logical step is to remove the possibility of confusion by only carrying the ID in the definition. ## 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/elaborator/mod.rs | 1 - compiler/noirc_frontend/src/elaborator/patterns.rs | 12 +----------- compiler/noirc_frontend/src/elaborator/statements.rs | 1 - .../src/hir/comptime/interpreter/builtin.rs | 1 - 4 files changed, 1 insertion(+), 14 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index a9173621fc7..74847e27a99 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -760,7 +760,6 @@ impl<'context> Elaborator<'context> { DefinitionKind::Local(None), &mut parameter_idents, true, // warn_if_unused - None, ); parameters.push((pattern, typ.clone(), visibility)); diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index f82244d34fc..8eb1d3fee70 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -37,7 +37,6 @@ impl<'context> Elaborator<'context> { None, &mut Vec::new(), warn_if_unused, - None, ) } @@ -50,7 +49,6 @@ impl<'context> Elaborator<'context> { definition_kind: DefinitionKind, created_ids: &mut Vec, warn_if_unused: bool, - global_id: Option, ) -> HirPattern { self.elaborate_pattern_mut( pattern, @@ -59,7 +57,6 @@ impl<'context> Elaborator<'context> { None, created_ids, warn_if_unused, - global_id, ) } @@ -72,7 +69,6 @@ impl<'context> Elaborator<'context> { mutable: Option, new_definitions: &mut Vec, warn_if_unused: bool, - global_id: Option, ) -> HirPattern { match pattern { Pattern::Identifier(name) => { @@ -82,9 +78,7 @@ impl<'context> Elaborator<'context> { (Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None), (_, other) => other, }; - let ident = if let Some(global_id) = global_id { - // Sanity check that we don't have conflicting globals. - assert_eq!(definition, DefinitionKind::Global(global_id)); + let ident = if let DefinitionKind::Global(global_id) = definition { // Globals don't need to be added to scope, they're already in the def_maps let id = self.interner.get_global(global_id).definition_id; let location = Location::new(name.span(), self.file); @@ -114,7 +108,6 @@ impl<'context> Elaborator<'context> { Some(span), new_definitions, warn_if_unused, - global_id, ); let location = Location::new(span, self.file); HirPattern::Mutable(Box::new(pattern), location) @@ -146,7 +139,6 @@ impl<'context> Elaborator<'context> { mutable, new_definitions, warn_if_unused, - global_id, ) }); let location = Location::new(span, self.file); @@ -170,7 +162,6 @@ impl<'context> Elaborator<'context> { mutable, new_definitions, warn_if_unused, - global_id, ) } } @@ -285,7 +276,6 @@ impl<'context> Elaborator<'context> { mutable, new_definitions, true, // warn_if_unused - None, ); if unseen_fields.contains(&field) { diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 0be71e39587..238160e5aa4 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -110,7 +110,6 @@ impl<'context> Elaborator<'context> { definition, &mut Vec::new(), warn_if_unused, - global_id, ); let attributes = let_stmt.attributes; diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 170cd67e146..3329e9c526a 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -2462,7 +2462,6 @@ fn function_def_set_parameters( DefinitionKind::Local(None), &mut parameter_idents, true, // warn_if_unused - None, ) });