chore: remove dependency on generational-arena (#4207)

# Description ## Problem\* Resolves #15 (comment) ## Summary\* Replaces `generational-arena`'s `Arena` class with a thin wrapper around `Vec` ## Additional Context The thin wrapper is helpful for: 1. `insert` returning the index 2. `iter` iterating over the `(index, item)` pairs ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: kevaundray <[email protected]> Co-authored-by: Jake Fecher <[email protected]> Co-authored-by: jfecher <[email protected]>
noir-lang · Feb 16, 2024 · 46f2204 · 46f2204
1 parent b283637
commit 46f2204
Show file tree

Hide file tree

Showing 15 changed files with 134 additions and 85 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -69,12 +69,10 @@ noirc_errors = { path = "compiler/noirc_errors" }
 noirc_evaluator = { path = "compiler/noirc_evaluator" }
 noirc_frontend = { path = "compiler/noirc_frontend" }
 noirc_printable_type = { path = "compiler/noirc_printable_type" }
-noir_wasm = { path = "compiler/wasm" }
 
 # Noir tooling workspace dependencies
 nargo = { path = "tooling/nargo" }
 nargo_fmt = { path = "tooling/nargo_fmt" }
-nargo_cli = { path = "tooling/nargo_cli" }
 nargo_toml = { path = "tooling/nargo_toml" }
 noir_lsp = { path = "tooling/lsp" }
 noir_debugger = { path = "tooling/debugger" }
@@ -97,8 +95,6 @@ getrandom = "0.2"
 
 # Debugger
 dap = "0.4.1-alpha1"
-
-cfg-if = "1.0.0"
 clap = { version = "4.3.19", features = ["derive", "env"] }
 codespan = { version = "0.11.1", features = ["serialization"] }
 codespan-lsp = "0.11.1"

diff --git a/aztec_macros/src/lib.rs b/aztec_macros/src/lib.rs
@@ -697,7 +697,7 @@ fn collect_traits(context: &HirContext) -> Vec<TraitId> {
     crates
         .flat_map(|crate_id| context.def_map(&crate_id).map(|def_map| def_map.modules()))
         .flatten()
-        .flat_map(|(_, module)| {
+        .flat_map(|module| {
             module.type_definitions().filter_map(|typ| {
                 if let ModuleDefId::TraitId(struct_id) = typ {
                     Some(struct_id)
@@ -763,11 +763,11 @@ fn transform_event(
         HirExpression::Literal(HirLiteral::Str(signature))
             if signature == SIGNATURE_PLACEHOLDER =>
         {
-            let selector_literal_id = first_arg_id;
+            let selector_literal_id = *first_arg_id;
 
             let structure = interner.get_struct(struct_id);
             let signature = event_signature(&structure.borrow());
-            interner.update_expression(*selector_literal_id, |expr| {
+            interner.update_expression(selector_literal_id, |expr| {
                 *expr = HirExpression::Literal(HirLiteral::Str(signature.clone()));
             });
 
@@ -833,7 +833,7 @@ fn get_serialized_length(
 
     let serialized_trait_impl_kind = traits
         .iter()
-        .filter_map(|&trait_id| {
+        .find_map(|&trait_id| {
             let r#trait = interner.get_trait(trait_id);
             if r#trait.borrow().name.0.contents == "Serialize"
                 && r#trait.borrow().generics.len() == 1
@@ -846,7 +846,6 @@ fn get_serialized_length(
                 None
             }
         })
-        .next()
         .ok_or(AztecMacroError::CouldNotAssignStorageSlots {
             secondary_message: Some("Stored data must implement Serialize trait".to_string()),
         })?;

diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs
@@ -31,7 +31,7 @@ pub struct LocalModuleId(pub Index);
 
 impl LocalModuleId {
     pub fn dummy_id() -> LocalModuleId {
-        LocalModuleId(Index::from_raw_parts(std::usize::MAX, std::u64::MAX))
+        LocalModuleId(Index::dummy())
     }
 }
 

diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs
@@ -1463,7 +1463,7 @@ impl<'a> Resolver<'a> {
                                 // they're used in expressions. We must do this here since the type
                                 // checker does not check definition kinds and otherwise expects
                                 // parameters to already be typed.
-                                if self.interner.id_type(hir_ident.id) == Type::Error {
+                                if self.interner.definition_type(hir_ident.id) == Type::Error {
                                     let typ = Type::polymorphic_integer(self.interner);
                                     self.interner.push_definition_type(hir_ident.id, typ);
                                 }

diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs
@@ -284,8 +284,9 @@ impl<'interner> TypeChecker<'interner> {
                 Type::Tuple(vecmap(&elements, |elem| self.check_expression(elem)))
             }
             HirExpression::Lambda(lambda) => {
-                let captured_vars =
-                    vecmap(lambda.captures, |capture| self.interner.id_type(capture.ident.id));
+                let captured_vars = vecmap(lambda.captures, |capture| {
+                    self.interner.definition_type(capture.ident.id)
+                });
 
                 let env_type: Type =
                     if captured_vars.is_empty() { Type::Unit } else { Type::Tuple(captured_vars) };
@@ -308,7 +309,7 @@ impl<'interner> TypeChecker<'interner> {
             }
         };
 
-        self.interner.push_expr_type(expr_id, typ.clone());
+        self.interner.push_expr_type(*expr_id, typ.clone());
         typ
     }
 
@@ -459,7 +460,7 @@ impl<'interner> TypeChecker<'interner> {
                                 operator: UnaryOp::MutableReference,
                                 rhs: method_call.object,
                             }));
-                        self.interner.push_expr_type(&new_object, new_type);
+                        self.interner.push_expr_type(new_object, new_type);
                         self.interner.push_expr_location(new_object, location.span, location.file);
                         new_object
                     });
@@ -485,7 +486,7 @@ impl<'interner> TypeChecker<'interner> {
                 operator: UnaryOp::Dereference { implicitly_added: true },
                 rhs: object,
             }));
-            self.interner.push_expr_type(&object, element.as_ref().clone());
+            self.interner.push_expr_type(object, element.as_ref().clone());
             self.interner.push_expr_location(object, location.span, location.file);
 
             // Recursively dereference to allow for converting &mut &mut T to T
@@ -682,8 +683,8 @@ impl<'interner> TypeChecker<'interner> {
                 operator: crate::UnaryOp::Dereference { implicitly_added: true },
                 rhs: old_lhs,
             }));
-            this.interner.push_expr_type(&old_lhs, lhs_type);
-            this.interner.push_expr_type(access_lhs, element);
+            this.interner.push_expr_type(old_lhs, lhs_type);
+            this.interner.push_expr_type(*access_lhs, element);
 
             let old_location = this.interner.id_location(old_lhs);
             this.interner.push_expr_location(*access_lhs, span, old_location.file);

diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs
@@ -458,7 +458,7 @@ mod test {
         }
 
         fn local_module_id(&self) -> LocalModuleId {
-            LocalModuleId(arena::Index::from_raw_parts(0, 0))
+            LocalModuleId(arena::Index::unsafe_zeroed())
         }
 
         fn module_id(&self) -> ModuleId {
@@ -509,7 +509,7 @@ mod test {
         let mut def_maps = BTreeMap::new();
         let file = FileId::default();
 
-        let mut modules = arena::Arena::new();
+        let mut modules = arena::Arena::default();
         let location = Location::new(Default::default(), file);
         modules.insert(ModuleData::new(None, location, false));
 

diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs
@@ -192,7 +192,7 @@ impl<'interner> TypeChecker<'interner> {
                         mutable = definition.mutable;
                     }
 
-                    let typ = self.interner.id_type(ident.id).instantiate(self.interner).0;
+                    let typ = self.interner.definition_type(ident.id).instantiate(self.interner).0;
                     typ.follow_bindings()
                 };
 

diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs
@@ -1672,11 +1672,10 @@ fn convert_array_expression_to_slice(
     interner.push_expr_location(call, location.span, location.file);
     interner.push_expr_location(func, location.span, location.file);
 
-    interner.push_expr_type(&call, target_type.clone());
-    interner.push_expr_type(
-        &func,
-        Type::Function(vec![array_type], Box::new(target_type), Box::new(Type::Unit)),
-    );
+    interner.push_expr_type(call, target_type.clone());
+
+    let func_type = Type::Function(vec![array_type], Box::new(target_type), Box::new(Type::Unit));
+    interner.push_expr_type(func, func_type);
 }
 
 impl BinaryTypeOperator {

diff --git a/compiler/noirc_frontend/src/monomorphization/debug.rs b/compiler/noirc_frontend/src/monomorphization/debug.rs
@@ -143,7 +143,7 @@ impl<'interner> Monomorphizer<'interner> {
                     let index_id = self.interner.push_expr(HirExpression::Literal(
                         HirLiteral::Integer(field_index.into(), false),
                     ));
-                    self.interner.push_expr_type(&index_id, crate::Type::FieldElement);
+                    self.interner.push_expr_type(index_id, crate::Type::FieldElement);
                     self.interner.push_expr_location(
                         index_id,
                         call.location.span,
@@ -171,7 +171,7 @@ impl<'interner> Monomorphizer<'interner> {
     fn intern_var_id(&mut self, var_id: DebugVarId, location: &Location) -> ExprId {
         let var_id_literal = HirLiteral::Integer((var_id.0 as u128).into(), false);
         let expr_id = self.interner.push_expr(HirExpression::Literal(var_id_literal));
-        self.interner.push_expr_type(&expr_id, crate::Type::FieldElement);
+        self.interner.push_expr_type(expr_id, crate::Type::FieldElement);
         self.interner.push_expr_location(expr_id, location.span, location.file);
         expr_id
     }

diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs
@@ -696,7 +696,7 @@ impl<'interner> Monomorphizer<'interner> {
         let mutable = definition.mutable;
 
         let definition = self.lookup_local(ident.id)?;
-        let typ = self.convert_type(&self.interner.id_type(ident.id));
+        let typ = self.convert_type(&self.interner.definition_type(ident.id));
 
         Some(ast::Ident { location: Some(ident.location), mutable, definition, name, typ })
     }
@@ -1040,7 +1040,7 @@ impl<'interner> Monomorphizer<'interner> {
     ) {
         match hir_argument {
             HirExpression::Ident(ident) => {
-                let typ = self.interner.id_type(ident.id);
+                let typ = self.interner.definition_type(ident.id);
                 let typ: Type = typ.follow_bindings();
                 let is_fmt_str = match typ {
                     // A format string has many different possible types that need to be handled.

diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs
@@ -75,13 +75,14 @@ pub struct NodeInterner {
 
     // Type checking map
     //
-    // Notice that we use `Index` as the Key and not an ExprId or IdentId
-    // Therefore, If a raw index is passed in, then it is not safe to assume that it will have
-    // a Type, as not all Ids have types associated to them.
-    // Further note, that an ExprId and an IdentId will never have the same underlying Index
-    // Because we use one Arena to store all Definitions/Nodes
+    // This should only be used with indices from the `nodes` arena.
+    // Otherwise the indices used may overwrite other existing indices.
+    // Each type for each index is filled in during type checking.
     id_to_type: HashMap<Index, Type>,
 
+    // Similar to `id_to_type` but maps definitions to their type
+    definition_to_type: HashMap<DefinitionId, Type>,
+
     // Struct map.
     //
     // Each struct definition is possibly shared across multiple type nodes.
@@ -277,12 +278,6 @@ impl DefinitionId {
     }
 }
 
-impl From<DefinitionId> for Index {
-    fn from(id: DefinitionId) -> Self {
-        Index::from_raw_parts(id.0, u64::MAX)
-    }
-}
-
 /// An ID for a global value
 #[derive(Debug, Eq, PartialEq, Hash, Clone, Copy)]
 pub struct GlobalId(usize);
@@ -302,7 +297,7 @@ impl StmtId {
     // This can be anything, as the program will ultimately fail
     // after resolution
     pub fn dummy_id() -> StmtId {
-        StmtId(Index::from_raw_parts(std::usize::MAX, 0))
+        StmtId(Index::dummy())
     }
 }
 
@@ -311,7 +306,7 @@ pub struct ExprId(Index);
 
 impl ExprId {
     pub fn empty_block_id() -> ExprId {
-        ExprId(Index::from_raw_parts(0, 0))
+        ExprId(Index::unsafe_zeroed())
     }
 }
 #[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)]
@@ -322,7 +317,7 @@ impl FuncId {
     // This can be anything, as the program will ultimately fail
     // after resolution
     pub fn dummy_id() -> FuncId {
-        FuncId(Index::from_raw_parts(std::usize::MAX, 0))
+        FuncId(Index::dummy())
     }
 }
 
@@ -396,23 +391,9 @@ macro_rules! into_index {
     };
 }
 
-macro_rules! partialeq {
-    ($id_type:ty) => {
-        impl PartialEq<usize> for &$id_type {
-            fn eq(&self, other: &usize) -> bool {
-                let (index, _) = self.0.into_raw_parts();
-                index == *other
-            }
-        }
-    };
-}
-
 into_index!(ExprId);
 into_index!(StmtId);
 
-partialeq!(ExprId);
-partialeq!(StmtId);
-
 /// A Definition enum specifies anything that we can intern in the NodeInterner
 /// We use one Arena for all types that can be interned as that has better cache locality
 /// This data structure is never accessed directly, so API wise there is no difference between using
@@ -496,6 +477,7 @@ impl Default for NodeInterner {
             id_to_location: HashMap::new(),
             definitions: vec![],
             id_to_type: HashMap::new(),
+            definition_to_type: HashMap::new(),
             structs: HashMap::new(),
             struct_attributes: HashMap::new(),
             type_aliases: Vec::new(),
@@ -545,10 +527,15 @@ impl NodeInterner {
     }
 
     /// Store the type for an interned expression
-    pub fn push_expr_type(&mut self, expr_id: &ExprId, typ: Type) {
+    pub fn push_expr_type(&mut self, expr_id: ExprId, typ: Type) {
         self.id_to_type.insert(expr_id.into(), typ);
     }
 
+    /// Store the type for an interned expression
+    pub fn push_definition_type(&mut self, definition_id: DefinitionId, typ: Type) {
+        self.definition_to_type.insert(definition_id, typ);
+    }
+
     pub fn push_empty_trait(&mut self, type_id: TraitId, unresolved_trait: &UnresolvedTrait) {
         let self_type_typevar_id = self.next_type_variable_id();
 
@@ -660,11 +647,6 @@ impl NodeInterner {
         }
     }
 
-    /// Store the type for an interned Identifier
-    pub fn push_definition_type(&mut self, definition_id: DefinitionId, typ: Type) {
-        self.id_to_type.insert(definition_id.into(), typ);
-    }
-
     /// Store [Location] of [Type] reference
     pub fn push_type_ref_location(&mut self, typ: Type, location: Location) {
         self.type_ref_locations.push((typ, location));
@@ -980,8 +962,13 @@ impl NodeInterner {
         self.id_to_type.get(&index.into()).cloned().unwrap_or(Type::Error)
     }
 
+    /// Returns the type of the definition or `Type::Error` if it was not found.
+    pub fn definition_type(&self, id: DefinitionId) -> Type {
+        self.definition_to_type.get(&id).cloned().unwrap_or(Type::Error)
+    }
+
     pub fn id_type_substitute_trait_as_type(&self, def_id: DefinitionId) -> Type {
-        let typ = self.id_type(def_id);
+        let typ = self.definition_type(def_id);
         if let Type::Function(args, ret, env) = &typ {
             let def = self.definition(def_id);
             if let Type::TraitAsType(..) = ret.as_ref() {

diff --git a/compiler/utils/arena/Cargo.toml b/compiler/utils/arena/Cargo.toml
@@ -4,8 +4,3 @@ version.workspace = true
 authors.workspace = true
 edition.workspace = true
 license.workspace = true
-
-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
-
-[dependencies]
-generational-arena = "0.2.8"