refactor: enhance package handling #180
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors package handling by consolidating package exports and FIR interface data into a single PackageInterface struct, simplifying the build pipeline and reducing redundant data structures.
Changes:
- Introduced
PackageInterfacestruct to encapsulate both exports and FIR interface - Added
build_packageandlink_packageshelper functions to modularize compilation - Simplified
typecheck_packagereturn type and streamlined package processing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn build_package<'a>( | ||
| gensym: &Gensym, | ||
| diagnostics: &mut Diagnostics, | ||
| package: &'a PackageArtifact, | ||
| deps: &[&PackageInterface], | ||
| ) -> (&'a PackageInterface, crate::core::File) { |
There was a problem hiding this comment.
The returned &'a PackageInterface reference is unused at the call site (line 367). The function should return only crate::core::File since the interface is already accessible through the package parameter.
| fn build_package<'a>( | ||
| gensym: &Gensym, | ||
| diagnostics: &mut Diagnostics, | ||
| package: &'a PackageArtifact, | ||
| deps: &[&PackageInterface], | ||
| ) -> (&'a PackageInterface, crate::core::File) { | ||
| let mut env = GlobalTypeEnv::new(); | ||
| for dep in deps { | ||
| dep.exports.apply_to(&mut env); | ||
| } | ||
| package.interface.exports.apply_to(&mut env); | ||
|
|
There was a problem hiding this comment.
The environment building logic here duplicates the pattern in typecheck_packages (lines 131-135 vs lines 273-289). Consider extracting this common pattern into a helper function to reduce code duplication.
| fn build_package<'a>( | |
| gensym: &Gensym, | |
| diagnostics: &mut Diagnostics, | |
| package: &'a PackageArtifact, | |
| deps: &[&PackageInterface], | |
| ) -> (&'a PackageInterface, crate::core::File) { | |
| let mut env = GlobalTypeEnv::new(); | |
| for dep in deps { | |
| dep.exports.apply_to(&mut env); | |
| } | |
| package.interface.exports.apply_to(&mut env); | |
| fn build_env_from_deps_and_package( | |
| package: &PackageArtifact, | |
| deps: &[&PackageInterface], | |
| ) -> GlobalTypeEnv { | |
| let mut env = GlobalTypeEnv::new(); | |
| for dep in deps { | |
| dep.exports.apply_to(&mut env); | |
| } | |
| package.interface.exports.apply_to(&mut env); | |
| env | |
| } | |
| fn build_package<'a>( | |
| gensym: &Gensym, | |
| diagnostics: &mut Diagnostics, | |
| package: &'a PackageArtifact, | |
| deps: &[&PackageInterface], | |
| ) -> (&'a PackageInterface, crate::core::File) { | |
| let env = build_env_from_deps_and_package(package, deps); |
| let mut package_cores = Vec::new(); | ||
| for name in graph.discovery_order.iter() { | ||
| let package = graph | ||
| .packages | ||
| .get(name) | ||
| .ok_or_else(|| compile_error(format!("package {} not found", name)))?; | ||
| let artifact = artifacts | ||
| .get(name) | ||
| .ok_or_else(|| compile_error(format!("missing package artifact for {}", name)))?; | ||
| let package_core = | ||
| compile_match::compile_file(&genv, &gensym, &mut diagnostics, &artifact.tast); | ||
| core_toplevels.extend(package_core.toplevels); | ||
| let mut deps: Vec<_> = package.imports.iter().cloned().collect(); | ||
| deps.sort(); | ||
| let mut dep_interfaces = Vec::new(); | ||
| for dep in deps.iter() { | ||
| let dep_artifact = artifacts | ||
| .get(dep) | ||
| .ok_or_else(|| compile_error(format!("missing package artifact for {}", dep)))?; | ||
| dep_interfaces.push(&dep_artifact.interface); | ||
| } | ||
| let (_interface, core) = | ||
| build_package(&gensym, &mut diagnostics, artifact, &dep_interfaces); | ||
| package_cores.push(core); | ||
| } |
There was a problem hiding this comment.
This loop duplicates the dependency resolution logic from typecheck_packages (lines 274-283). Both locations fetch artifacts, collect dependencies, sort them, and build interface collections. Consider extracting this pattern into a shared helper function to improve maintainability.
No description provided.