Add inline documentation for complex logic in type-checker (#73) #100
Conversation
|
hey @ALIPHATICHYD please confirm that you've read this contribution guide and followed the steps on what to mention in your PR description. |
Yes I confirm that I've read the guideline |
| //! 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)`). |
There was a problem hiding this comment.
I think this comment is less informative than the original one
| //! - Method resolution on types | ||
| //! - Import registration and resolution | ||
| //! - Type/struct/enum/spec and function registration | ||
| //! - Variable, method, and import tracking |
There was a problem hiding this comment.
Why did method resolution types and import registration comments removed?
| if let Some(symbol) = self.lookup_symbol_local(name) { | ||
| return Some(symbol.clone()); | ||
| } | ||
| // Recursively search parent scopes |
There was a problem hiding this comment.
I don't think it is necessary
| if let Some((_, ty)) = self.lookup_variable_local(name) { | ||
| return Some(ty); | ||
| } | ||
| // Search parent scopes (enables shadowing) |
There was a problem hiding this comment.
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?
| { | ||
| return Some(method_info.clone()); | ||
| } | ||
| // Search parent scopes |
There was a problem hiding this comment.
I don't think this comment is necessary
| scope_id | ||
| } | ||
|
|
||
| /// Pop the current scope and move back to its parent. |
There was a problem hiding this comment.
I think it is better to describe what this function literally does, like: reassigning the current_scope to the parent if it exists
| } | ||
| } | ||
|
|
||
| pub(crate) fn register_type(&mut self, name: &str, ty: Option<&Type>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Do you think register_* functions are clear enough so not necessary to add docstrings for them?
| /// 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 |
There was a problem hiding this comment.
I think the original comment is clearer. The # Errors section can mislead in this form because such a format of comments is about what errors a function can throw.
| // 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. |
There was a problem hiding this comment.
I don't understand what it means. Can you please give an example?
| } | ||
|
|
||
| #[allow(clippy::needless_pass_by_value)] | ||
| fn infer_method_variables( |
There was a problem hiding this comment.
Do you think infer_* functions are simple enough so that comments are not required?
3 participants
Closes #73
Description
Added comprehensive documentation to type_checker.rs and symbol_table.rs to improve code readability and maintainability. The documentation explains the purpose, key functions, and usage patterns for both modules.
Specific Changes
Type
Testing
No tests added (documentation-only change)
AI-Generated Code
No AI-generated code in this PR.
Files Modified