fix(modules): exports aliasing a declared function are functions, not var getters - #6962
Conversation
… var getters `export const alias = impl` (where `impl` is a declared function) lands in BOTH `exported_objects` and `exported_functions` — HIR records the alias with the origin's FuncId. The var-vs-function classification only excluded *declaration* names, so the alias was classified as an exported VARIABLE, whose cross-module convention is a zero-arg getter. But origin-name resolution points `perry_fn_<mod>__<alias>` at the #460 forwarding wrapper — the function BODY — so the "getter" call actually INVOKED the function. Two user-visible failures, both covered by the new test: - Reading the binding yielded `impl(<zeroed args>)`'s return value instead of the closure: `typeof NS.alias` was `number`, and calling it threw `TypeError: value is not a function`. - The source module's own namespace populator hit the same path while building its namespace object, so the function RAN during module init. Fix, in `run_pipeline`: - exclude `exported_functions` alias names from `exported_var_names` (the consumer-side binding read), and - resolve alias entries in the namespace-entry builder to the ORIGIN function's wrap symbol — `LocalFunction` for same-module, `ForeignFunction` (under the origin name) for cross-module — instead of falling through to the `ForeignVar` getter. Found compiling the t3 Code server (github.com/pingdotgg/t3code) to native with Effect 4.0.0-beta.78, whose `SchemaParser.ts` is built almost entirely out of this shape (`export const decodeSync = decodeUnknownSync`, `decodeEffect = decodeUnknownEffect`, …). Diagnosed by disassembling the module's `__init_body`: sibling exports lowered to `js_closure_alloc_singleton` while the aliases lowered to a direct `bl perry_fn_…__decodeEffect` with zeroed argument registers.
📝 WalkthroughWalkthroughThe compiler now preserves aliases of declared functions as function bindings during export classification and dynamic namespace construction. A regression test verifies local and named namespace imports remain callable without executing the origin function during binding reads. ChangesNamespace alias function exports
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/run_pipeline.rs (1)
1886-1909: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated alias-lookup logic between local and cross-module branches.
Both branches repeat the same "find name in
exported_functions, then resolve theFuncIdto aFunction" lookup (.find(|(n, _)| n == &fe.source_local || n == &fe.name).and_then(|(_, fid)| ...functions.iter().find(|f| f.id == *fid))). Extracting a small helper (e.g.fn resolve_function_alias<'a>(hir: &'a Module, source_local: &str, name: &str) -> Option<&'a Function>) would keep both call sites in sync if this resolution logic needs a future fix.♻️ Suggested helper
+fn resolve_function_alias<'a>( + hir: &'a perry_hir::Module, + source_local: &str, + name: &str, +) -> Option<&'a perry_hir::Function> { + hir.exported_functions + .iter() + .find(|(n, _)| n == source_local || n == name) + .and_then(|(_, fid)| hir.functions.iter().find(|f| f.id == *fid)) +}Also applies to: 1936-1951
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/run_pipeline.rs` around lines 1886 - 1909, Extract the duplicated exported-function-to-Function lookup into a shared helper, such as resolve_function_alias, accepting the target Module and both source_local and name values. Replace the inline lookup in the shown branch and the corresponding cross-module branch around the other alias-resolution block, preserving the existing matching order and FuncId resolution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry/src/commands/compile/run_pipeline.rs`:
- Around line 1886-1909: Extract the duplicated exported-function-to-Function
lookup into a shared helper, such as resolve_function_alias, accepting the
target Module and both source_local and name values. Replace the inline lookup
in the shown branch and the corresponding cross-module branch around the other
alias-resolution block, preserving the existing matching order and FuncId
resolution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 635adff2-1f48-4991-a89d-21d491bf47a9
📒 Files selected for processing (2)
crates/perry/src/commands/compile/run_pipeline.rscrates/perry/tests/namespace_alias_export_of_function.rs
The bug
export const alias = impl(whereimplis a declared function) lands in bothexported_objectsandexported_functions— HIR records the alias with the origin'sFuncId. The var-vs-function classification inrun_pipelineonly excluded declaration names, so the alias was classified as an exported variable, whose cross-module convention is a zero-arg getter. But origin-name resolution pointsperry_fn_<mod>__<alias>at the #460 forwarding wrapper — the function body — so the "getter" call actually invoked the function.Two user-visible failures:
…and the source module's own namespace populator hits the same path while building its namespace object, so the aliased function runs during module init with zeroed arguments.
The fix
In
run_pipeline:exported_functionsalias names fromexported_var_names(the consumer-side binding read), andLocalFunctionsame-module,ForeignFunction(under the origin name) cross-module — instead of falling through to theForeignVargetter.Tests
crates/perry/tests/namespace_alias_export_of_function.rs— two cases (namespace member, named import). Both fail on main (alias-typeof: number→TypeError: value is not a function) and pass with the fix. Full repro suite for the surrounding campaign also green, no regressions.Provenance
Found compiling the t3 Code server to native with Effect
4.0.0-beta.78, whoseSchemaParser.tsis built almost entirely out of this shape (export const decodeSync = decodeUnknownSync,decodeEffect = decodeUnknownEffect, …). Diagnosed by disassembling that module's__init_body: sibling plain exports lowered tojs_closure_alloc_singleton, while the aliases lowered to a directbl perry_fn_…__decodeEffectwith zeroed argument registers.Summary by CodeRabbit