From 336bbb7a0a4f659ea9c9a590ead48bdff58938db Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 1 Sep 2023 09:20:53 -0500 Subject: [PATCH] Add fix for #2502 --- .../src/proof_system.rs | 10 ++-- .../type_aliases/src/main.nr | 7 +++ .../src/hir/def_collector/dc_crate.rs | 14 ++--- .../src/hir/def_collector/dc_mod.rs | 18 +++--- crates/noirc_frontend/src/hir/mod.rs | 2 +- .../src/hir/resolution/import.rs | 2 +- crates/noirc_frontend/src/node_interner.rs | 55 ++++++++++++------- 7 files changed, 63 insertions(+), 45 deletions(-) diff --git a/crates/acvm_backend_barretenberg/src/proof_system.rs b/crates/acvm_backend_barretenberg/src/proof_system.rs index 33ec8457a43..8a24559c867 100644 --- a/crates/acvm_backend_barretenberg/src/proof_system.rs +++ b/crates/acvm_backend_barretenberg/src/proof_system.rs @@ -76,9 +76,8 @@ impl ProofSystemCompiler for Barretenberg { let temp_dir_path_str = temp_directory.to_str().unwrap(); // Create a temporary file for the witness - let serialized_witnesses: Vec = witness_values - .try_into() - .expect("could not serialize witness map"); + let serialized_witnesses: Vec = + witness_values.try_into().expect("could not serialize witness map"); let witness_path = temp_directory.join("witness").with_extension("tr"); write_to_file(&serialized_witnesses, &witness_path); @@ -232,9 +231,8 @@ fn prepend_public_inputs(proof: Vec, public_inputs: Vec) -> Ve return proof; } - let public_inputs_bytes = public_inputs - .into_iter() - .flat_map(|assignment| assignment.to_be_bytes()); + let public_inputs_bytes = + public_inputs.into_iter().flat_map(|assignment| assignment.to_be_bytes()); public_inputs_bytes.chain(proof.into_iter()).collect() } diff --git a/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr b/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr index 85a40c739e2..7197c23f664 100644 --- a/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/type_aliases/src/main.nr @@ -26,4 +26,11 @@ fn main(x : [Field; 2]) { foo: 10 }; assert(s.foo == 10); + + let _regression2502: Regression2502Alias = Regression2502 {}; } + +// An ICE was occurring if a type alias referred to a struct before it was initialized +// during name resolution. The fix was to initialize structs during def collection instead. +type Regression2502Alias = Regression2502; +struct Regression2502 {} diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index 03c0aad7c0f..bc0d8d6cfc0 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -264,10 +264,10 @@ fn collect_impls( if let Some(struct_type) = get_struct_type(&typ) { let struct_type = struct_type.borrow(); - let type_module = struct_type.id.0.local_id; + let type_module = struct_type.id.local_module_id(); // `impl`s are only allowed on types defined within the current crate - if struct_type.id.0.krate != crate_id { + if struct_type.id.krate() != crate_id { let span = *span; let type_name = struct_type.name.to_string(); let error = DefCollectorErrorKind::ForeignImpl { span, type_name }; @@ -379,14 +379,8 @@ fn resolve_structs( crate_id: CrateId, errors: &mut Vec, ) { - // We must first go through the struct list once to ensure all IDs are pushed to - // the def_interner map. This lets structs refer to each other regardless of declaration order - // without resolve_struct_fields non-deterministically unwrapping a value - // that isn't in the HashMap. - for (type_id, typ) in &structs { - context.def_interner.push_empty_struct(*type_id, typ); - } - + // Resolve each field in each struct. + // Each struct should already be present in the NodeInterner after def collection. for (type_id, typ) in structs { let (generics, fields) = resolve_struct_fields(context, crate_id, typ, errors); context.def_interner.update_struct(type_id, |struct_def| { diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs index f72b72f54df..809623623ee 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -4,7 +4,7 @@ use noirc_errors::{FileDiagnostic, Location}; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, - node_interner::{StructId, TraitId}, + node_interner::TraitId, parser::SubModule, FunctionDefinition, FunctionReturnType, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTypeAlias, ParsedModule, TraitImpl, TraitImplItem, TraitItem, TypeImpl, @@ -60,7 +60,7 @@ pub fn collect_defs( collector.collect_traits(ast.traits, crate_id, errors); - collector.collect_structs(ast.types, crate_id, errors); + collector.collect_structs(context, ast.types, crate_id, errors); collector.collect_type_aliases(context, ast.type_aliases, errors); @@ -377,6 +377,7 @@ impl<'a> ModCollector<'a> { /// Returns a vector of errors if any structs were already defined. fn collect_structs( &mut self, + context: &mut Context, types: Vec, krate: CrateId, errors: &mut Vec, @@ -384,9 +385,15 @@ impl<'a> ModCollector<'a> { for struct_definition in types { let name = struct_definition.name.clone(); + let unresolved = UnresolvedStruct { + file_id: self.file_id, + module_id: self.module_id, + struct_def: struct_definition, + }; + // Create the corresponding module for the struct namespace let id = match self.push_child_module(&name, self.file_id, false, false, errors) { - Some(local_id) => StructId(ModuleId { krate, local_id }), + Some(local_id) => context.def_interner.new_struct(&unresolved, krate, local_id), None => continue, }; @@ -404,11 +411,6 @@ impl<'a> ModCollector<'a> { } // And store the TypeId -> StructType mapping somewhere it is reachable - let unresolved = UnresolvedStruct { - file_id: self.file_id, - module_id: self.module_id, - struct_def: struct_definition, - }; self.def_collector.collected_types.insert(id, unresolved); } } diff --git a/crates/noirc_frontend/src/hir/mod.rs b/crates/noirc_frontend/src/hir/mod.rs index af7494beabf..5868872fa1b 100644 --- a/crates/noirc_frontend/src/hir/mod.rs +++ b/crates/noirc_frontend/src/hir/mod.rs @@ -98,7 +98,7 @@ impl Context { /// For example, if you project contains a `main.nr` and `foo.nr` and you provide the `main_crate_id` and the /// `bar_struct_id` where the `Bar` struct is inside `foo.nr`, this function would return `foo::Bar` as a [String]. pub fn fully_qualified_struct_path(&self, crate_id: &CrateId, id: StructId) -> String { - let module_id = id.0; + let module_id = id.module_id(); let child_id = module_id.local_id.0; let def_map = self.def_map(&module_id.krate).expect("The local crate should be analyzed already"); diff --git a/crates/noirc_frontend/src/hir/resolution/import.rs b/crates/noirc_frontend/src/hir/resolution/import.rs index 68796cf26bd..8949c766881 100644 --- a/crates/noirc_frontend/src/hir/resolution/import.rs +++ b/crates/noirc_frontend/src/hir/resolution/import.rs @@ -151,7 +151,7 @@ fn resolve_name_in_module( ModuleDefId::ModuleId(id) => id, ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), // TODO: If impls are ever implemented, types can be used in a path - ModuleDefId::TypeId(id) => id.0, + ModuleDefId::TypeId(id) => id.module_id(), ModuleDefId::TypeAliasId(_) => panic!("type aliases cannot be used in type namespace"), ModuleDefId::TraitId(id) => id.0, ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index ae53a71cba6..d72b8d9d949 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -140,7 +140,7 @@ impl FuncId { } #[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)] -pub struct StructId(pub ModuleId); +pub struct StructId(ModuleId); impl StructId { //dummy id for error reporting @@ -149,6 +149,18 @@ impl StructId { pub fn dummy_id() -> StructId { StructId(ModuleId { krate: CrateId::dummy_id(), local_id: LocalModuleId::dummy_id() }) } + + pub fn module_id(self) -> ModuleId { + self.0 + } + + pub fn krate(self) -> CrateId { + self.0.krate + } + + pub fn local_module_id(self) -> LocalModuleId { + self.0.local_id + } } #[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)] @@ -347,24 +359,29 @@ impl NodeInterner { ); } - pub fn push_empty_struct(&mut self, type_id: StructId, typ: &UnresolvedStruct) { - self.structs.insert( - type_id, - Shared::new(StructType::new( - type_id, - typ.struct_def.name.clone(), - typ.struct_def.span, - Vec::new(), - vecmap(&typ.struct_def.generics, |_| { - // Temporary type variable ids before the struct is resolved to its actual ids. - // This lets us record how many arguments the type expects so that other types - // can refer to it with generic arguments before the generic parameters themselves - // are resolved. - let id = TypeVariableId(0); - (id, Shared::new(TypeBinding::Unbound(id))) - }), - )), - ); + pub fn new_struct( + &mut self, + typ: &UnresolvedStruct, + krate: CrateId, + local_id: LocalModuleId, + ) -> StructId { + let struct_id = StructId(ModuleId { krate, local_id }); + let name = typ.struct_def.name.clone(); + + // Fields will be filled in later + let no_fields = Vec::new(); + let generics = vecmap(&typ.struct_def.generics, |_| { + // Temporary type variable ids before the struct is resolved to its actual ids. + // This lets us record how many arguments the type expects so that other types + // can refer to it with generic arguments before the generic parameters themselves + // are resolved. + let id = TypeVariableId(0); + (id, Shared::new(TypeBinding::Unbound(id))) + }); + + let new_struct = StructType::new(struct_id, name, typ.struct_def.span, no_fields, generics); + self.structs.insert(struct_id, Shared::new(new_struct)); + struct_id } pub fn push_type_alias(&mut self, typ: &UnresolvedTypeAlias) -> TypeAliasId {