fix: DefinitionIds should not be stored in id_to_type

aztec_macros/src/lib.rs

@@ -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()),
         })?;

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(std::usize::MAX))
+        LocalModuleId(Index::dummy())
     }
 }
 

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);
                                 }

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);

compiler/noirc_frontend/src/hir/type_check/mod.rs

@@ -458,7 +458,7 @@ mod test {
         }
 
         fn local_module_id(&self) -> LocalModuleId {
-            LocalModuleId(arena::Index(0, generational_arena::Index::from_raw_parts(0, 0)))
+            LocalModuleId(arena::Index::unsafe_zeroed())
         }
 
         fn module_id(&self) -> ModuleId {

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()
                 };
 

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 {

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
     }

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 })
     }
@@ -1038,7 +1038,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.

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(id.0)
-    }
-}
-
 /// 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(std::usize::MAX))
+        StmtId(Index::dummy())
     }
 }
 
@@ -311,7 +306,7 @@ pub struct ExprId(Index);
 
 impl ExprId {
     pub fn empty_block_id() -> ExprId {
-        ExprId(Index(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(std::usize::MAX))
+        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 .0;
-                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() {

compiler/utils/arena/src/lib.rs

@@ -4,7 +4,26 @@
 #![warn(clippy::semicolon_if_nothing_returned)]
 
 #[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd, Hash)]
-pub struct Index(pub usize);
+pub struct Index(usize);
+
+impl Index {
+    #[cfg(test)]
+    pub fn test_new(index: usize) -> Index {
+        Self(index)
+    }
+
+    /// Return a dummy index (max value internally).
+    /// This should be avoided over `Option<Index>` if possible.
+    pub fn dummy() -> Self {
+        Self(usize::MAX)
+    }
+
+    /// Return the zeroed index. This is unsafe since we don't know
+    /// if this is a valid index for any particular map yet.
+    pub fn unsafe_zeroed() -> Self {
+        Self(0)
+    }
+}
 
 // #[derive(Debug, Eq, PartialEq, Hash, Clone)]
 #[derive(Debug, Clone)]