tweaks

Author: simontran7

docs/ARCHITECTURE.md

@@ -331,21 +331,13 @@ However, we don't emit error nodes for missing delimiters, but rather, recover b
 
 ## Stage 3: Semantic Analysis and AST Lowering
 
-**Semantic Analysis** involves walking the AST (a post-order traversal) and constructing a **high-level intermediate representation (HIR)**. Specifically, to create the nodes of an HIR, it involves three parts:
-- Name Resolution
-- Type Checking
-- Desugaring
+**Semantic Analysis** involves traversing the AST starting at the root node, and constructing a **high-level intermediate representation (HIR)**. Specifically, semantic analysis involves three phases:
+- **Collection**: register item signatures because crawfish supports top-level and block-level item hoisting
+- **Name Resolution**: resolve identifiers at use-sites to their bindings
+- **Type Checking**: verify type correctness of expressions and statements
+- **Desugaring**: As the HIR is constructed, certain AST constructs are desugared along the way.
 
-However, that two-pass design breaks when resolution depends on types ([source](https://www.reddit.com/r/ProgrammingLanguages/comments/w0biir/comment/igdt1ce/)). For instance:
-```
-// Which `foo`? Depends on type of `x`
-x.foo()
-
-// Which `+`? Depends on types of operands (if you have overloading)
-a + b
-```
-
-### Name Resolution
+### Symbol Table
 
 An identifier's **scope** is the part of a program where it's accessible. An identifier may refer to different values in different parts of the program. Crawfish particularly has **static scope**, which means the visibility and accessibility of an identifier are determined by its physical location within the source code, at compile-time.
 
@@ -366,8 +358,6 @@ func outer(x: i32) -> i32 {
 
 This means that scope frames are not always "transparent" (i.e., the semantic analyzer can see outer names). As such, each scope frame has an associated `ScopeKind` to be used as a flag that tells lookup when to start filtering out locals. Most scopes are `ScopeKind::Normal`, which behave like ordinary lexical scopes. However, when the semantic analyzer enters a nested function item, it pushes a `ScopeKind::Item` scope, which acts as an item boundary. During lookup, once resolution crosses an Item scope, outer local bindings (`BindingId::Local`) are no longer visible, while item bindings (`BindingId::Item`) remain visible.
 
-Additionally, since crawfish supports top-level and block-level item hoisting, a simple single-pass AST walk is not sufficient. Inspired by rustc, the compiler uses a two-step approach per scope. First, it walks the immediate contents of a module or block and records all item names into the scope's symbol table. Then, it walks statements and expressions in that scope and resolves each identifier by looking it up in the symbol tables that were already populated ([source](https://rustc-dev-guide.rust-lang.org/name-resolution.html#overall-strategy)).
-
 > [!NOTE]
 > Since most code don't nest very deeply, we _may_ further optimize the symbol table (specifically, avoiding allocation churn) by pre-allocating around 4 to 8 empty hashmaps and reusing cleared hashmaps instead of allocating and dropping them ([source](https://www.reddit.com/r/Compilers/comments/1dy9722/comment/lc833ho/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button))
 
@@ -378,16 +368,27 @@ Additionally, since crawfish supports top-level and block-level item hoisting, a
 Our high-level intermediate representation is close to our AST, and should be able to support high-level optimizations such as inlining and constant folding ([source](https://www.cs.cornell.edu/courses/cs4120/2026sp/notes.html?id=ir)).
 
 There are two common approaches for high-level IRs ([source](https://www.reddit.com/r/ProgrammingLanguages/comments/1cj0oj2/comment/l2fqafw/)):
--
--
+- ...
+- ...
 
-Unlike the AST, the HIR doesn't follow a SoA design because it's [less ergonomic for semantic analysis](https://www.reddit.com/r/rust/comments/160kqz9/comment/jxq36ug/).
+Unlike the AST, the HIR doesn't follow a SoA design because it's less ergonomic for semantic analysis due to the several passes involved ([source](https://www.reddit.com/r/rust/comments/160kqz9/comment/jxq36ug/)).
 
 ### Semantic Analyzer
 
+The collecting phase which allows forward references involves two parts per scope: first scan for item definitions to populate the symbol table, then walk statements sequentially ([source](https://rustc-dev-guide.rust-lang.org/name-resolution.html#overall-strategy), [source 2, page 23](https://web.stanford.edu/class/cs143/lectures/lecture09.pdf)).
+
+Name resolution and type checking must be interleaved when resolution depends on types ([source](https://www.reddit.com/r/ProgrammingLanguages/comments/w0biir/comment/igdt1ce/)). For instance:
+```
+// Which `foo`? Depends on type of `x`
+x.foo()
+
+// Which `+`? Depends on types of operands (if you have overloading)
+a + b
+```
+
 ## Stage 4: HIR Lowering
 
-https://www.reddit.com/r/ProgrammingLanguages/comments/1boul8y/comment/kwtxulc/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button
+Rather than going from HIR directly to LLVM, it makes sense to [source](https://www.reddit.com/r/ProgrammingLanguages/comments/1boul8y/comment/kwtxulc/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button).
 
 ### Mid-level Intermediateion Representation (MIR)
 

src/front_end/semantic_analysis/module_table.rs

@@ -3,8 +3,11 @@ use crate::diagnostics::diagnostic_store::Diagnostic;
 use crate::diagnostics::semantic_diagnostics::SemanticDiagnostic;
 use crate::front_end::semantic_analysis::hir::{BindingId, Hir};
 use crate::front_end::semantic_analysis::symbol_table::{DefineError, ScopeKind, SymbolTable};
-use crate::front_end::semantic_analysis::types::{Ty, TypeInterner};
-use crate::front_end::syntactic_analysis::ast::{Ast, FunctionDefinitionId, ItemKind};
+use crate::front_end::semantic_analysis::types::TypeInterner;
+use crate::front_end::syntactic_analysis::ast::{
+    Ast, BlockExpressionId, ConstantDefinitionId, ExpressionId, FunctionDefinitionId, ItemId,
+    ItemKind, StatementId, StatementKind,
+};
 
 pub struct SemanticAnalyzer<'ast> {
     ast: &'ast Ast,
@@ -28,103 +31,127 @@ impl<'ast> SemanticAnalyzer<'ast> {
     }
 
     pub fn analyze(mut self) -> (Hir, Vec<Diagnostic>) {
-        self.collect_source_file_items();
         self.analyze_source_file_items();
         (self.hir, self.diagnostics)
     }
 
-    fn collect_source_file_items(&mut self) {
+    fn analyze_source_file_items(&mut self) {
         self.symbols.enter_scope(ScopeKind::Item);
-        let unknown_type = self.types.intern(Ty::Unknown);
-        for &item in &self.ast.source_file_items {
-            match item.kind() {
-                ItemKind::FunctionDefinition => {
-                    let node = &self.ast.function_definitions[item.index().into()];
-                    let Some(name) = self.ast.resolve_identifier(node.name) else {
-                        continue;
-                    };
-                    let item_binding = self.hir.create_item_binding(name, unknown_type, node.span);
-                    if let Err(DefineError::AlreadyDefined { prev_binding }) = self
-                        .symbols
-                        .add_binding(name, BindingId::Item(item_binding))
-                    {
-                        self.diagnostics.push(Diagnostic::Semantic(
-                            SemanticDiagnostic::DuplicateDefinition {
-                                name: self.string_interner.resolve(name).unwrap().to_string(),
-                                span: node.span,
-                                previous_span: self.hir.get_item_binding_info(prev_binding).span,
-                            },
-                        ));
-                    }
-                }
-                ItemKind::ConstantDefinition => {
-                    let node = &self.ast.constant_definitions[item.index().into()];
-                    let Some(name) = self.ast.resolve_identifier(node.name) else {
-                        continue;
-                    };
-                    let item_binding = self.hir.create_item_binding(name, unknown_type, node.span);
-                    if let Err(DefineError::AlreadyDefined { prev_binding }) = self
-                        .symbols
-                        .add_binding(name, BindingId::Item(item_binding))
-                    {
-                        self.diagnostics.push(Diagnostic::Semantic(
-                            SemanticDiagnostic::DuplicateDefinition {
-                                name: self.string_interner.resolve(name).unwrap().to_string(),
-                                span: node.span,
-                                previous_span: self.hir.get_item_binding_info(prev_binding).span,
-                            },
-                        ));
-                    }
-                }
-                ItemKind::Error => continue,
-            }
+
+        // pre-pass for item hoisting
+        let start = self.ast.source_file.items.start as usize;
+        let len = self.ast.source_file.items.len as usize;
+        for &item_id in &self.ast.source_file_items[start..start + len] {
+            self.collect_item(item_id);
         }
-    }
 
-    fn analyze_source_file_items(&mut self) {
-        for &item in &self.ast.source_file_items {
-            match item.kind() {
+        // analyze
+        let start = self.ast.source_file.items.start as usize;
+        let len = self.ast.source_file.items.len as usize;
+        for &item_id in &self.ast.source_file_items[start..start + len] {
+            match item_id.kind() {
                 ItemKind::FunctionDefinition => {
-                    self.analyze_function_definition(item.index().into());
+                    self.analyze_function_definition(item_id.index().into());
                 }
                 ItemKind::ConstantDefinition => {
-                    self.analyze_constant_definition(item.index().into());
+                    self.analyze_constant_definition(item_id.index().into());
                 }
                 ItemKind::Error => continue,
             }
         }
+
         self.symbols.exit_scope();
     }
 
-    fn analyze_function_definition(&mut self, node: FunctionDefinitionId) {
+    fn analyze_function_definition(&mut self, id: FunctionDefinitionId) {
         todo!()
     }
 
-    fn analyze_constant_definition(&mut self, node: FunctionDefinitionId) {
+    fn analyze_constant_definition(&mut self, id: ConstantDefinitionId) {
         todo!()
     }
-}
 
-//     fn lower_block(&mut self, block: &ast::BlockExpressionId) -> hir::NodeId {
-//         self.symbols.enter_scope(ScopeKind::Item);
-//         for statement in self.ast.block_statements {
-//             if let StatementKind::FunctionDefinition(...) | StatementKind::ConstDefinition(...) = stmt {
-//                 let item_id = self.hir.push_node(...);
-//                 self.symbols.add_binding(name, BindingId::Item(item_id))?;
-//             }
-//         }
+    fn analyze_block(&mut self, id: BlockExpressionId) {
+        let block = &self.ast.block_expressions[id];
+        let start = block.statements.start as usize;
+        let len = block.statements.len as usize;
 
-//     self.symbol_table.enter_scope(ScopeKind::Normal);
-//     for stmt in block.statements {
-//         self.lower_statement(stmt);
-//     }
-//     let tail = block.tail.map(|e| self.lower_expression(e));
+        // pre-pass for item hoisting
+        self.symbols.enter_scope(ScopeKind::Item);
+        for &statement_id in &self.ast.block_statements[start..start + len] {
+            if let StatementKind::ItemStatement = statement_id.kind() {
+                let node = &self.ast.item_statements[statement_id.index().into()];
+                self.collect_item(node.item);
+            }
+        }
 
-//     // 3. exit both scopes
+        // main pass: normal scope, walk statements sequentially
+        self.symbols.enter_scope(ScopeKind::Normal);
+        for &statement_id in &self.ast.block_statements[start..start + len] {
+            self.analyze_statement(statement_id);
+        }
+        let tail = block.tail.map(|expr_id| self.analyze_expression(expr_id));
+        self.symbols.exit_scope();
 
-//     // emit the block node
-//     }
-// }
+        self.symbols.exit_scope();
+    }
+
+    fn analyze_statement(&mut self, id: StatementId) {
+        todo!()
+    }
+
+    fn analyze_expression(&mut self, id: ExpressionId) {
+        todo!()
+    }
+
+    fn collect_item(&mut self, id: ItemId) {
+        match id.kind() {
+            ItemKind::FunctionDefinition => {
+                let node = &self.ast.function_definitions[id.index().into()];
+                let Some(name) = self.ast.resolve_identifier(node.name) else {
+                    return;
+                };
+                let item_binding_id =
+                    self.hir
+                        .create_item_binding(name, self.types.unknown_id, node.span);
+                if let Err(DefineError::AlreadyDefined { prev_binding_id }) = self
+                    .symbols
+                    .add_binding(name, BindingId::Item(item_binding_id))
+                {
+                    self.diagnostics.push(Diagnostic::Semantic(
+                        SemanticDiagnostic::DuplicateDefinition {
+                            name: self.string_interner.resolve(name).unwrap().to_string(),
+                            span: node.span,
+                            previous_span: self.hir.get_item_binding_info(prev_binding_id).span,
+                        },
+                    ));
+                }
+            }
+            ItemKind::ConstantDefinition => {
+                let node = &self.ast.constant_definitions[id.index().into()];
+                let Some(name) = self.ast.resolve_identifier(node.name) else {
+                    return;
+                };
+                let item_binding_id =
+                    self.hir
+                        .create_item_binding(name, self.types.unknown_id, node.span);
+                if let Err(DefineError::AlreadyDefined { prev_binding_id }) = self
+                    .symbols
+                    .add_binding(name, BindingId::Item(item_binding_id))
+                {
+                    self.diagnostics.push(Diagnostic::Semantic(
+                        SemanticDiagnostic::DuplicateDefinition {
+                            name: self.string_interner.resolve(name).unwrap().to_string(),
+                            span: node.span,
+                            previous_span: self.hir.get_item_binding_info(prev_binding_id).span,
+                        },
+                    ));
+                }
+            }
+            ItemKind::Error => return,
+        }
+    }
+}
 
 //     fn analyze_let(&mut self, node: &LetStatementNode) -> StatementId {
 //         let value = self.lower_expression(node.value);
@@ -151,24 +178,6 @@ impl<'ast> SemanticAnalyzer<'ast> {
 //     }
 // }
 
-//     fn analyze_top_level_items(&mut self) {
-//         for &item_id in &self.ast.top_level_items {
-//             if item_id.is_error() {
-//                 continue;
-//             }
-
-//             match item_id.kind() {
-//                 TopLevelItemKind::ConstDefinition => {
-//                     self.analyze_const_definition(item_id);
-//                 }
-//                 TopLevelItemKind::FunctionDefinition => {
-//                     // TODO: Implement later
-//                 }
-//                 TopLevelItemKind::Error => {}
-//             }
-//         }
-//     }
-
 //     fn analyze_const_definition(
 //         &mut self,
 //         item_id: crate::front_end::syntactic_analysis::ast::TopLevelItemId,

src/front_end/semantic_analysis/semantic_analyzer.rs

@@ -2,9 +2,8 @@ use crate::common::string_interner::Symbol;
 use crate::front_end::semantic_analysis::hir::BindingId;
 use std::collections::HashMap;
 
-#[derive(Debug)]
-pub enum DefineError {
-    AlreadyDefined { prev_binding: BindingId },
+pub struct SymbolTable {
+    scopes: Vec<Scope>,
 }
 
 #[derive(Clone, Debug)]
@@ -20,16 +19,14 @@ pub enum ScopeKind {
     Item,
 }
 
-#[derive(Default)]
-pub struct SymbolTable {
-    scopes: Vec<Scope>,
+#[derive(Debug)]
+pub enum DefineError {
+    AlreadyDefined { prev_binding_id: BindingId },
 }
 
 impl SymbolTable {
     pub fn new() -> Self {
-        let mut st = Self::default();
-        st.enter_scope(ScopeKind::Normal);
-        st
+        SymbolTable { scopes: Vec::new() }
     }
 
     pub fn enter_scope(&mut self, kind: ScopeKind) {
@@ -43,30 +40,29 @@ impl SymbolTable {
         self.scopes.pop().expect("pop on empty scope stack");
     }
 
-    pub fn add_binding(&mut self, name: Symbol, binding: BindingId) -> Result<(), DefineError> {
+    pub fn add_binding(&mut self, name: Symbol, binding_id: BindingId) -> Result<(), DefineError> {
         let scope = self.scopes.last_mut().unwrap();
-        if let Some(&prev) = scope.bindings.get(&name) {
-            return Err(DefineError::AlreadyDefined { prev_binding: prev });
+        if let Some(&prev_binding_id) = scope.bindings.get(&name) {
+            return Err(DefineError::AlreadyDefined { prev_binding_id });
         }
-        scope.bindings.insert(name, binding);
+        scope.bindings.insert(name, binding_id);
         Ok(())
     }
 
     pub fn find_binding(&self, name: Symbol) -> Option<BindingId> {
         let mut block_outer_locals = false;
         for scope in self.scopes.iter().rev() {
-            if let Some(&binding) = scope.bindings.get(&name) {
-                match binding {
-                    BindingId::Item(_) => return Some(binding),
-                    BindingId::Local(_) if !block_outer_locals => return Some(binding),
+            if let Some(&binding_id) = scope.bindings.get(&name) {
+                match binding_id {
+                    BindingId::Item(_) => return Some(binding_id),
+                    BindingId::Local(_) if !block_outer_locals => return Some(binding_id),
                     BindingId::Local(_) => {} // crossed item boundary: block outer locals
                 }
             }
             if scope.kind == ScopeKind::Item {
                 block_outer_locals = true;
             }
         }
-
         None
     }
 }