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/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

@@ -1515,7 +1515,7 @@
                 Type::Tuple(fields)
             }
             Type::Forall(typevars, typ) => {
                 // Trying to substitute_helper a variable de, substitute_bound_typevarsfined within a nested Forall
                 // is usually impossible and indicative of an error in the type checker somewhere.
                 for var in typevars {
                     assert!(!type_bindings.contains_key(&var.id()));
@@ -1672,11 +1672,10 @@
     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 @@
                     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,14 +171,14 @@
     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
     }
 }
 
 fn element_type_at_index(ptype: &PrintableType, i: usize) -> &PrintableType {
     match ptype {
         PrintableType::Array { length: _length, typ } => typ.as_ref(),
         PrintableType::Tuple { types } => &types[i],
         PrintableType::Struct { name: _name, fields } => &fields[i].1,

compiler/noirc_frontend/src/monomorphization/mod.rs

@@ -696,7 +696,7 @@
         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 })
     }
@@ -967,7 +967,7 @@
                     // The second argument is expected to always be an ident
                     self.append_printable_type_info(&hir_arguments[1], &mut arguments);
                 } else if name.as_str() == "assert_message" {
                     // The first argument to the `assert_message` oracle is the expression passed as a mesage to an `assert` or `assert_eq` statement
                     self.append_printable_type_info(&hir_arguments[0], &mut arguments);
                 }
             }
@@ -1038,7 +1038,7 @@
     ) {
         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

@@ -6,9 +6,9 @@
 use fm::FileId;
 use iter_extended::vecmap;
 use noirc_errors::{Location, Span, Spanned};
 use petgraph::algo::tarjan_scc;
 use petgraph::prelude::DiGraph;
 use petgraph::prelude::NodeIndex as PetGraphIndex;
 
 use crate::ast::Ident;
 use crate::graph::CrateId;
@@ -60,7 +60,7 @@
     function_modules: HashMap<FuncId, ModuleId>,
 
     /// This graph tracks dependencies between different global definitions.
     /// This is used to ensure the absense of dependency cycles for globals and types.
     dependency_graph: DiGraph<DependencyId, ()>,
 
     /// To keep track of where each DependencyId is in `dependency_graph`, we need
@@ -75,13 +75,14 @@
 
     // 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 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 @@
     // 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 @@
 
 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 @@
     // 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! 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
@@ -491,11 +472,12 @@
             function_modifiers: HashMap::new(),
             function_modules: HashMap::new(),
             func_id_to_trait: HashMap::new(),
             dependency_graph: petgraph::graph::DiGraph::new(),
             dependency_graph_indices: HashMap::new(),
             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 @@
     }
 
     /// 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 @@
         }
     }
 
-    /// 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 @@
         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)]