chore: Document name resolution (#925)

* Document name resolution

* Reword note on CompTime::Maybe

---------

Co-authored-by: TomAFrench <[email protected]>

crates/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -21,7 +21,9 @@ struct ModCollector<'a> {
     pub(crate) module_id: LocalModuleId,
 }
 
-/// Walk a module and collect it's definitions
+/// Walk a module and collect its definitions.
+///
+/// This performs the entirety of the definition collection phase of the name resolution pass.
 pub fn collect_defs(
     def_collector: &mut DefCollector,
     ast: ParsedModule,

crates/noirc_frontend/src/hir/def_collector/mod.rs

@@ -1,3 +1,22 @@
+//! This set of modules implements the name resolution pass which converts the AST into
+//! the HIR. In comparison to the AST, the HIR is interned in the NodeInterner and attaches
+//! DefinitionIds to each Variable AST node to link them up to their definition. In doing so,
+//! this pass validates scoping requirements and is responsible for issuing name/definition
+//! related errors including missing definitions and duplicate definitions. One aspect of
+//! handling definition links is handling and tracking imports. Another result of this pass
+//! is that all imports will be removed from the program, and the AST of each file will
+//! be combined into a larger Hir stored in the NodeInterner and linked together via DefinitionIds.
+//!
+//! The pass is comprised of two parts. The first part - definition collection - is implemented
+//! in dc_mod::collect_defs and is responsible for collecting all the public symbols used by
+//! the program so that we can resolve them later without worrying about cyclic references or
+//! variables that aren't defined yet.
+//!
+//! The second part of the pass is the actual name resolution. Name resolution is handled by
+//! super::resolution::Resolvers which traverse the entirety of a definition, ensure all names
+//! are defined and linked, and convert the definition into Hir.
+//!
+//! These passes are performed sequentially (along with type checking afterward) in dc_crate.
 pub mod dc_crate;
 pub mod dc_mod;
 mod errors;

crates/noirc_frontend/src/hir/def_map/mod.rs

@@ -17,6 +17,7 @@ mod module_data;
 pub use module_data::*;
 mod namespace;
 pub use namespace::*;
+
 // XXX: Ultimately, we want to constrain an index to be of a certain type just like in RA
 /// Lets first check if this is offered by any external crate
 /// XXX: RA has made this a crate on crates.io
@@ -35,6 +36,9 @@ pub struct ModuleId {
     pub local_id: LocalModuleId,
 }
 
+/// Map of all modules and scopes defined within a crate.
+///
+/// The definitions of the crate are accessible indirectly via the scopes of each module.
 #[derive(Debug)]
 pub struct CrateDefMap {
     pub(crate) root: LocalModuleId,

crates/noirc_frontend/src/hir/def_map/module_data.rs

@@ -6,6 +6,8 @@ use crate::Ident;
 
 use super::{ItemScope, LocalModuleId};
 
+/// Contains the actual contents of a module: its parent (if one exists),
+/// children, and scope with all definitions defined within the scope.
 #[derive(Default, Debug, PartialEq, Eq)]
 pub struct ModuleData {
     pub parent: Option<LocalModuleId>,

crates/noirc_frontend/src/hir/def_map/module_def.rs

@@ -2,6 +2,7 @@ use crate::node_interner::{FuncId, StmtId, StructId};
 
 use super::ModuleId;
 
+/// A generic ID that references either a module, function, type, or global
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum ModuleDefId {
     ModuleId(ModuleId),

crates/noirc_frontend/src/hir/resolution/mod.rs

@@ -1,3 +1,10 @@
+//! This set of modules implements the second half of the name resolution pass.
+//! After all definitions are collected by def_collector, resovler::Resolvers are
+//! created to resolve all names within a definition. In this context 'resolving'
+//! a name means validating that it has a valid definition, and that it was not
+//! redefined multiple times in the same scope. Once this is validated, it is linked
+//! to that definition via a matching DefinitionId. All references to the same definition
+//! will have the same DefinitionId.
 pub mod errors;
 pub mod import;
 pub mod path_resolver;

crates/noirc_frontend/src/hir/resolution/resolver.rs

@@ -11,12 +11,6 @@
 // XXX: Change mentions of intern to resolve. In regards to the above comment
 //
 // XXX: Resolver does not check for unused functions
-#[derive(Debug, PartialEq, Eq)]
-struct ResolverMeta {
-    num_times_used: usize,
-    ident: HirIdent,
-}
-
 use crate::hir_def::expr::{
     HirBinaryOp, HirBlockExpression, HirCallExpression, HirCastExpression,
     HirConstructorExpression, HirExpression, HirForExpression, HirIdent, HirIfExpression,
@@ -63,6 +57,12 @@ type Scope = GenericScope<String, ResolverMeta>;
 type ScopeTree = GenericScopeTree<String, ResolverMeta>;
 type ScopeForest = GenericScopeForest<String, ResolverMeta>;
 
+/// The primary jobs of the Resolver are to validate that every variable found refers to exactly 1
+/// definition in scope, and to convert the AST into the HIR.
+///
+/// A Resolver is a short-lived struct created to resolve a top-level definition.
+/// One of these is created for each function definition and struct definition.
+/// This isn't strictly necessary to its function, it could be refactored out in the future.
 pub struct Resolver<'a> {
     scopes: ScopeForest,
     path_resolver: &'a dyn PathResolver,
@@ -88,6 +88,13 @@ pub struct Resolver<'a> {
     lambda_index: usize,
 }
 
+/// ResolverMetas are tagged onto each definition to track how many times they are used
+#[derive(Debug, PartialEq, Eq)]
+struct ResolverMeta {
+    num_times_used: usize,
+    ident: HirIdent,
+}
+
 impl<'a> Resolver<'a> {
     pub fn new(
         interner: &'a mut NodeInterner,

crates/noirc_frontend/src/hir_def/expr.rs

@@ -8,6 +8,11 @@ use crate::{BinaryOp, BinaryOpKind, Ident, Shared, UnaryOp};
 use super::stmt::HirPattern;
 use super::types::{StructType, Type};
 
+/// A HirExpression is the result of an Expression in the AST undergoing
+/// name resolution. It is almost identical to the Expression AST node, but
+/// references other HIR nodes indirectly via IDs rather than directly via
+/// boxing. Variables in HirExpressions are tagged with their DefinitionId
+/// from the definition that refers to them so there is no ambiguity with names.
 #[derive(Debug, Clone)]
 pub enum HirExpression {
     Ident(HirIdent),
@@ -35,6 +40,7 @@ impl HirExpression {
     }
 }
 
+/// Corresponds to a variable in the source code
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub struct HirIdent {
     pub location: Location,
@@ -89,6 +95,8 @@ pub struct HirInfixExpression {
     pub rhs: ExprId,
 }
 
+/// This is always a struct field access `mystruct.field`
+/// and never a method call. The later is represented by HirMethodCallExpression.
 #[derive(Debug, Clone)]
 pub struct HirMemberAccess {
     pub lhs: ExprId,
@@ -104,6 +112,7 @@ pub struct HirIfExpression {
     pub alternative: Option<ExprId>,
 }
 
+// `lhs as type` in the source code
 #[derive(Debug, Clone)]
 pub struct HirCastExpression {
     pub lhs: ExprId,
@@ -160,6 +169,7 @@ pub struct HirConstructorExpression {
     pub fields: Vec<(Ident, ExprId)>,
 }
 
+/// Indexing, as in `array[index]`
 #[derive(Debug, Clone)]
 pub struct HirIndexExpression {
     pub collection: ExprId,

crates/noirc_frontend/src/hir_def/function.rs

@@ -115,14 +115,22 @@ impl From<Vec<Param>> for Parameters {
     }
 }
 
+/// A FuncMeta contains the signature of the function and any associated meta data like
+/// the function's Location, FunctionKind, and attributes. If the function's body is
+/// needed, it can be retrieved separately via `NodeInterner::function(&self, &FuncId)`.
 #[derive(Debug, Clone)]
 pub struct FuncMeta {
     pub name: HirIdent,
 
     pub kind: FunctionKind,
 
+    /// A function's attributes are the `#[...]` items above the function
+    /// definition, if any. Currently, this is limited to a maximum of only one
+    /// Attribute per function.
     pub attributes: Option<Attribute>,
+
     pub parameters: Parameters,
+
     pub return_visibility: AbiVisibility,
 
     /// The type of this function. Either a Type::Function

crates/noirc_frontend/src/hir_def/stmt.rs

@@ -4,6 +4,20 @@ use crate::{Ident, Type};
 use fm::FileId;
 use noirc_errors::Span;
 
+/// A HirStatement is the result of performing name resolution on
+/// the Statement AST node. Unlike the AST node, any nested nodes
+/// are referred to indirectly via ExprId or StmtId, which can be
+/// used to retrieve the relevant node via the NodeInterner.
+#[derive(Debug, Clone)]
+pub enum HirStatement {
+    Let(HirLetStatement),
+    Constrain(HirConstrainStatement),
+    Assign(HirAssignStatement),
+    Expression(ExprId),
+    Semi(ExprId),
+    Error,
+}
+
 #[derive(Debug, Clone)]
 pub struct HirLetStatement {
     pub pattern: HirPattern,
@@ -20,32 +34,20 @@ impl HirLetStatement {
     }
 }
 
+/// Corresponds to `lvalue = expression;` in the source code
 #[derive(Debug, Clone)]
 pub struct HirAssignStatement {
     pub lvalue: HirLValue,
     pub expression: ExprId,
 }
 
+/// Corresponds to `constrain expr;` in the source code.
+/// This node also contains the FileId of the file the constrain
+/// originates from. This is used later in the SSA pass to issue
+/// an error if a constrain is found to be always false.
 #[derive(Debug, Clone)]
 pub struct HirConstrainStatement(pub ExprId, pub FileId);
 
-#[derive(Debug, Clone)]
-pub struct BinaryStatement {
-    pub lhs: ExprId,
-    pub r#type: Type,
-    pub expression: ExprId,
-}
-
-#[derive(Debug, Clone)]
-pub enum HirStatement {
-    Let(HirLetStatement),
-    Constrain(HirConstrainStatement),
-    Assign(HirAssignStatement),
-    Expression(ExprId),
-    Semi(ExprId),
-    Error,
-}
-
 #[derive(Debug, Clone)]
 pub enum HirPattern {
     Identifier(HirIdent),
@@ -79,7 +81,8 @@ impl HirPattern {
     }
 }
 
-/// Represents an Ast form that can be assigned to
+/// Represents an Ast form that can be assigned to. These
+/// can be found on the left hand side of an assignment `=`.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum HirLValue {
     Ident(HirIdent, Type),