-
Notifications
You must be signed in to change notification settings - Fork 15
Add inline documentation for complex logic in type-checker (#73) #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,22 @@ | ||
| //! Symbol Table | ||
| //! | ||
| //! This module implements a tree-based symbol table for managing scopes and symbols | ||
| //! during type checking. It supports: | ||
| //! | ||
| //! Tree-based scope management for type checking: | ||
| //! - Hierarchical scopes with parent-child relationships | ||
| //! - Type alias, struct, enum, spec, and function symbol registration | ||
| //! - Variable tracking within scopes | ||
| //! - Method resolution on types | ||
| //! - Import registration and resolution | ||
| //! - Type/struct/enum/spec and function registration | ||
| //! - Variable, method, and import tracking | ||
| //! - Visibility checking for access control | ||
| //! | ||
| //! Scopes form a tree structure where each scope can have multiple child scopes. | ||
| //! Symbol lookup walks up the tree from current scope to root until a match is found. | ||
| //! ## Scope Tree | ||
| //! | ||
| //! Symbol lookup walks **up** the parent chain from current scope to root. | ||
| //! Inner scopes can shadow outer scope symbols. Scope navigation: | ||
| //! - `push_scope()` - Create child scope and move down | ||
| //! - `pop_scope()` - Return to parent scope | ||
| //! | ||
| //! ## Default Return Types | ||
| //! | ||
| //! Functions without an explicit return type default to the unit type. This is | ||
| //! represented using `Type::Simple(SimpleTypeKind::Unit)`, which provides a | ||
| //! lightweight value-based representation without heap allocation. | ||
| //! Functions without explicit return type default to unit (`Type::Simple(SimpleTypeKind::Unit)`). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment is less informative than the original one |
||
|
|
||
|
|
||
| use std::cell::RefCell; | ||
| use std::rc::Rc; | ||
|
|
@@ -311,6 +310,7 @@ impl Scope { | |
| if let Some(symbol) = self.lookup_symbol_local(name) { | ||
| return Some(symbol.clone()); | ||
| } | ||
| // Recursively search parent scopes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is necessary |
||
| if let Some(parent) = &self.parent { | ||
| return parent.borrow().lookup_symbol(name); | ||
| } | ||
|
|
@@ -340,6 +340,7 @@ impl Scope { | |
| if let Some((_, ty)) = self.lookup_variable_local(name) { | ||
| return Some(ty); | ||
| } | ||
| // Search parent scopes (enables shadowing) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, shadowing should not be allowed because it leads to potential errors. Can you please add a test to confirm that shadowing takes place, indeed? |
||
| if let Some(parent) = &self.parent { | ||
| return parent.borrow().lookup_variable(name); | ||
| } | ||
|
|
@@ -362,6 +363,7 @@ impl Scope { | |
| { | ||
| return Some(method_info.clone()); | ||
| } | ||
| // Search parent scopes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this comment is necessary |
||
| if let Some(parent) = &self.parent { | ||
| return parent.borrow().lookup_method(type_name, method_name); | ||
| } | ||
|
|
@@ -455,6 +457,11 @@ impl SymbolTable { | |
| self.push_scope_with_name(&name, Visibility::Private) | ||
| } | ||
|
|
||
| /// Create a new child scope and move down. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this analogy of push/pop operations to movement can be misleading, so better not to use it. |
||
| /// | ||
| /// Links the new scope to the current scope as its parent, builds the full path | ||
| /// (e.g., "module::inner"), and updates `current_scope`. Used when entering function | ||
| /// bodies, blocks, or nested modules. | ||
| pub(crate) fn push_scope_with_name(&mut self, name: &str, visibility: Visibility) -> u32 { | ||
| let parent = self.current_scope.clone(); | ||
| let scope_id = self.next_scope_id; | ||
|
|
@@ -483,6 +490,10 @@ impl SymbolTable { | |
| scope_id | ||
| } | ||
|
|
||
| /// Pop the current scope and move back to its parent. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to describe what this function literally does, like: reassigning the |
||
| /// | ||
| /// Counterpart to `push_scope()` / `push_scope_with_name()`. Called when exiting | ||
| /// a scope (function end, block end) to restore the parent scope as `current_scope`. | ||
| pub(crate) fn pop_scope(&mut self) { | ||
| if let Some(current) = &self.current_scope { | ||
| let parent = current.borrow().parent.clone(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,18 @@ | ||
| //! Type Checker Implementation | ||
| //! | ||
| //! This module contains the core type checking logic that infers and validates | ||
| //! types throughout the AST. The type checker operates in multiple phases: | ||
| //! | ||
| //! 1. **process_directives** - Register raw imports from use statements | ||
| //! Multi-phase type checking with forward reference support: | ||
| //! 1. **process_directives** - Register imports from use statements | ||
| //! 2. **register_types** - Collect type/struct/enum/spec definitions | ||
| //! 3. **resolve_imports** - Bind import paths to symbols | ||
| //! 4. **collect_function_and_constant_definitions** - Register functions | ||
| //! 4. **collect_function_and_constant_definitions** - Register function signatures | ||
| //! 5. **infer_variables** - Type-check function bodies | ||
| //! | ||
| //! The type checker continues after encountering errors to collect all issues | ||
| //! before returning. Errors are deduplicated to avoid repeated reports. | ||
| //! Generic type parameters are tracked through phases 4-5. When a generic function is called, | ||
| //! `infer_type_params_from_args()` infers concrete type substitutions from arguments. | ||
| //! Type mismatches (conflicting inference, unresolvable parameters) are reported. | ||
| //! | ||
| //! Errors are collected and deduplicated to report all issues in a single pass. | ||
|
|
||
|
|
||
| use std::rc::Rc; | ||
|
|
||
|
|
@@ -60,14 +62,15 @@ impl TypeChecker { | |
| } | ||
|
|
||
| impl TypeChecker { | ||
| /// Infer types for all definitions in the context. | ||
| /// Infer types for all definitions. | ||
| /// | ||
| /// Executes the 5-phase algorithm in order. Phase ordering is critical: types must be | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the original comment is clearer. The |
||
| /// registered before functions can reference them, and imports must be resolved before | ||
| /// they're used. Continues on errors to collect all issues. | ||
| /// | ||
| /// Phase ordering: | ||
| /// 1. `process_directives()` - Register raw imports in scopes | ||
| /// 2. `register_types()` - Collect type definitions into symbol table | ||
| /// 3. `resolve_imports()` - Bind import paths to symbols | ||
| /// 4. `collect_function_and_constant_definitions()` - Register functions | ||
| /// 5. Infer variable types in function bodies | ||
| /// # Errors | ||
| /// | ||
| /// Returns error with all accumulated type errors if any occurred. | ||
| pub fn infer_types(&mut self, ctx: &mut TypedContext) -> anyhow::Result<SymbolTable> { | ||
| self.process_directives(ctx); | ||
| self.register_types(ctx); | ||
|
|
@@ -475,14 +478,21 @@ impl TypeChecker { | |
| } | ||
|
|
||
| #[allow(clippy::needless_pass_by_value)] | ||
| /// Infer types for function parameters and body. | ||
| /// | ||
| /// Creates a new scope for the function, collects type parameter names, registers | ||
| /// parameters as variables, and type-checks all statements. Generic type parameters | ||
| /// (e.g., `T` in `fn foo<T>(x: T)`) are preserved via `TypeInfo::new_with_type_params()` | ||
| /// for later substitution when the function is called. | ||
| fn infer_variables( | ||
| &mut self, | ||
| function_definition: Rc<FunctionDefinition>, | ||
| ctx: &mut TypedContext, | ||
| ) { | ||
| self.symbol_table.push_scope(); | ||
|
|
||
| // Collect type parameter names for proper TypeInfo construction | ||
| // Collect type parameter names for proper TypeInfo construction. | ||
| // These names identify which type references are generic variables vs. concrete types. | ||
| let type_param_names: Vec<String> = function_definition | ||
| .type_parameters | ||
| .as_ref() | ||
|
|
@@ -493,6 +503,9 @@ impl TypeChecker { | |
| for argument in arguments { | ||
| match argument { | ||
| ArgumentType::Argument(arg) => { | ||
| // Create TypeInfo with awareness of generic type parameters. | ||
| // Non-generic arguments get concrete types, while references to | ||
| // type parameters (e.g., 'T' in 'fn foo<T>(x: T)') become Generic kinds. | ||
| let arg_type = TypeInfo::new_with_type_params(&arg.ty, &type_param_names); | ||
| if let Err(err) = self | ||
| .symbol_table | ||
|
|
@@ -517,7 +530,9 @@ impl TypeChecker { | |
| } | ||
| } | ||
|
|
||
| // Build return type with type parameter awareness | ||
| // Build return type with type parameter awareness. | ||
| // The return type is needed for statement type checking to validate return statements. | ||
| // Generic type parameters in the return type will be resolved when the function is called. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what it means. Can you please give an example? |
||
| let return_type = function_definition | ||
| .returns | ||
| .as_ref() | ||
|
|
@@ -2011,12 +2026,12 @@ impl TypeChecker { | |
| } | ||
| } | ||
|
|
||
| /// Attempt to infer type parameters from argument types. | ||
| /// | ||
| /// For each parameter that is a type variable (Generic), try to find a | ||
| /// concrete type from the corresponding argument. | ||
| /// Infer concrete type substitutions for generic parameters from arguments. | ||
| /// | ||
| /// Returns a substitution map if inference succeeds, empty map otherwise. | ||
| /// For each generic parameter in the signature, examines corresponding arguments to infer | ||
| /// concrete types. Detects conflicting inference (e.g., `fn id<T>(a: T, b: T)` called | ||
| /// with different types for a and b) and missing parameters (T appears in signature | ||
| /// but no argument provides it). | ||
| #[allow(clippy::type_complexity)] | ||
| fn infer_type_params_from_args( | ||
| &mut self, | ||
|
|
@@ -2032,19 +2047,17 @@ impl TypeChecker { | |
| None => return substitutions, | ||
| }; | ||
|
|
||
| // For each parameter, check if it contains a type variable | ||
| // Infer type from each argument if its parameter is generic | ||
| for (i, param_type) in signature.param_types.iter().enumerate() { | ||
| if i >= args.len() { | ||
| break; | ||
| } | ||
|
|
||
| // If the parameter type is a type variable, infer from argument | ||
| if let TypeInfoKind::Generic(type_param_name) = ¶m_type.kind { | ||
| // Infer the argument type | ||
| let arg_type = self.infer_expression(&args[i].1.borrow(), ctx); | ||
|
|
||
| if let Some(arg_type) = arg_type { | ||
| // Check for conflicting inference | ||
| // Check for conflicting inference across arguments | ||
| if let Some(existing) = substitutions.get(type_param_name) { | ||
| if *existing != arg_type { | ||
| self.errors.push(TypeCheckError::ConflictingTypeInference { | ||
|
|
@@ -2061,7 +2074,7 @@ impl TypeChecker { | |
| } | ||
| } | ||
|
|
||
| // Check if we found substitutions for all type parameters | ||
| // Verify all type parameters were inferred | ||
| for type_param in &signature.type_params { | ||
| if !substitutions.contains_key(type_param) { | ||
| self.errors.push(TypeCheckError::CannotInferTypeParameter { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did method resolution types and import registration comments removed?