Typechecker phase 3

[navicore]

No description provided.

[claude]

Code Review: Typechecker Phase 3

Overview

This is a large-scale structural refactoring (7031 additions / 6875 deletions) that splits monolithic files in crates/compiler/src/ into organized submodules. No functional logic is changed — the PR is purely about code organization and maintainability.

Files affected across 11 modules:

Before	After
ast.rs (1107 lines)	ast/{program.rs, types.rs, tests.rs}
call_graph.rs	call_graph/tests.rs extracted
error_flag_lint.rs (800 lines)	error_flag_lint/{analyzer.rs, state.rs, tests.rs}
ffi.rs (670 lines)	ffi/{bindings.rs, manifest.rs, tests.rs}
lint.rs (849 lines)	lint/{linter.rs, types.rs, tests.rs}
main.rs	main/{build.rs, lint.rs, test.rs, venv.rs}
resolver.rs	resolver/{helpers.rs, tests.rs}
resource_lint.rs (1869 lines)	resource_lint/{program.rs, state.rs, word.rs, tests.rs}
test_runner.rs, types.rs, unification.rs	*/tests.rs extracted

---

What works well

• Separation of concerns is clean. resource_lint splitting into program.rs / state.rs / word.rs is a particularly good decomposition — those were previously three distinct responsibilities tangled in one file.
• error_flag_lint split into analyzer.rs + state.rs is idiomatic and makes the state machine much easier to reason about in isolation.
• Test extraction is consistent — every module now has a dedicated tests.rs, which makes it easy to find tests and reduces line count in the implementation files.
• Re-exports are correct. The pub use declarations in parent modules (e.g., pub use helpers::{check_collisions, check_union_collisions, find_stdlib};) preserve the existing public API surface without breakage.

---

Observations and suggestions

#[path] attributes on lib.rs and main.rs

// lib.rs
#[cfg(test)]
#[path = "lib/tests.rs"]
mod tests;

// main.rs
#[path = "main/build.rs"]
mod build;

This is functionally correct — since lib.rs and main.rs are crate roots (not mod.rs files), Rust won't automatically look for submodules in lib/ or main/ directories. Using #[path] is the right solution here.

That said, it's worth leaving a brief comment explaining why the #[path] attribute is needed, since developers unfamiliar with this pattern may be confused:

// lib.rs — crate roots can't use automatic path resolution for subdirectories
#[path = "lib/tests.rs"]
mod tests;

Glob re-export in ast.rs

pub use types::*;

This is a wide export. If types.rs grows, new types will silently become part of ast's public API. Consider explicit re-exports (pub use types::{SourceLocation, Type, ...}) to make the API surface intentional and searchable. This is a low-priority style preference — the current approach works fine.

No new tests added

This is expected for a pure refactoring, but it's worth calling out: the submodule boundaries create natural seams where integration tests between modules would add value in future PRs (e.g., testing that resolver → lint → resource_lint pipeline produces expected output for a known input).

resource_lint/state.rs — clone-heavy branch analysis

let mut then_state = state.clone();
let mut else_state = state.clone();

This is pre-existing code, not introduced here. But now that StackState lives in its own file, it's more visible — worth considering whether StackState could implement a cheaper fork()/merge() API in a future pass rather than full cloning.

---

Correctness assessment

• All pub use re-exports correctly expose the symbols previously accessible from the monolithic files.
• use super::* in test files is appropriate and matches the previous inline test pattern.
• The use super::state::{...} imports in resource_lint/program.rs and resource_lint/word.rs are explicit, which is better than glob-importing across module boundaries.
• No unsafe code added.
• No changes to public API signatures.

---

Summary

This is a clean, well-executed refactoring. The modular structure makes the codebase significantly easier to navigate and will make future typechecker phases easier to scope and review. No correctness issues found.

Suggested follow-ups (not blocking):

1. Add comments explaining the #[path] workaround on lib.rs / main.rs
2. Consider explicit re-exports in place of pub use types::* in ast.rs
3. Future: integration tests at module boundaries

Risk: Low. Ready to merge pending CI green.