fix(runtime): closure-nested dynamic import() invisible to the resolver rejected with literal undefined — pi one-shot lost-rejection wall (#6660) - #6665
Merged
proggeramlug merged 1 commit intoJul 19, 2026
Conversation
…nresolved import() rejects with Error, not undefined (PerryTS#6660) Two root causes behind pi wall PerryTS#8 (one-shot -p dying with a reasonless 'Uncaught (in promise) undefined' before its first output): 1. Visitor asymmetry (perry-hir dynamic_import/visitors.rs): the read-only for_each_dynamic_import never descended into Expr::Closure bodies while its _mut sibling did. The driver aligns per-site resolution outcomes 1:1 by traversal order (collect via ref visitor, fill via mut visitor), so a closure-nested import() — pi-ai's `importNodeModule = (specifier) => import(rewrite(specifier))` — was invisible to the resolver but still visited by the fill pass: the site kept empty `paths` with no `deferred_error` and codegen lowered it to the defensive `js_promise_rejected(undefined)` arm. The ref visitor now descends into closure bodies exactly like the mut one (the worker-new visitor pair already did). 2. Reasonless fallthroughs (perry-codegen dyn_extern_i18n.rs): the empty-paths / unmapped-target / no-match dynamic-import arms rejected with literal `undefined`. They now route through a runtime fallback — js_module_dynamic_import_fallback, the import() analog of the PerryTS#5389 ambient-require fallthrough: node builtins resolve by string to the same namespace require(spec)/getBuiltinModule(spec) produce (install-all hooks armed, PerryTS#6644 pattern), anything else rejects with a descriptive Error carrying code ERR_MODULE_NOT_FOUND. The PerryTS#5230 deferred arm routes through js_module_dynamic_import_deferred: builtins resolve at runtime, unknown modules keep the site-specific file:line deferral message. Also: PERRY_REJECTION_DIAG=1 (kept, documented like PERRY_BIGINT_MIX_DIAG) dumps the raw rejection reason (tag class, bits, preview) plus a native backtrace at unhandled-rejection track and report time — the instrumentation that located the rejecting call site in a compiled bundle. e2e coverage in crates/perry/tests/issue_6660_closure_dynamic_import.rs: the wild shape (closure import + async try/catch fileExists), the bare passthrough closure for fs/promises + os, unknown-module rejection contract (Error, defined reason, message), and a closure/top-level outcome-alignment guard. All byte-identical to node v26 where node semantics are reproducible. Fixes PerryTS#6660 Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
📝 WalkthroughWalkthroughDynamic import traversal now includes closure bodies, while unresolved and deferred imports use runtime fallback entry points that resolve builtins or reject with descriptive errors. Optional unhandled-rejection diagnostics report promise details and backtraces. End-to-end regression tests cover these paths. ChangesDynamic import resolution
Unhandled rejection diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DynamicImportVisitor
participant DynamicImportCodegen
participant ModuleRequireRuntime
participant PromiseRuntime
DynamicImportVisitor->>DynamicImportCodegen: Visit import expression and closure body
DynamicImportCodegen->>ModuleRequireRuntime: Call fallback or deferred entry point
ModuleRequireRuntime->>PromiseRuntime: Resolve builtin or reject unknown module
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@crates/perry-codegen/src/expr/dyn_extern_i18n.rs`:
- Around line 654-663: Update the single-target fallback around
js_module_dynamic_import_fallback to pass the specifier returned by
js_module_dynamic_import_apply_hooks rather than the original path_val. Preserve
the existing runtime fallback behavior while ensuring loader rewrites are
reflected in the module name and match the multi-target path.
In `@crates/perry-runtime/src/promise/rejection.rs`:
- Around line 177-186: The Error-object branch in the rejection diagnostic
formatter currently emits the entire stack string without a size limit. In the
pointer handling logic, apply the same 120-byte, character-boundary truncation
used by the string-preview branch to the value returned by
string_header_to_string, while preserving the existing error pointer and
formatting behavior.
- Around line 121-131: Move the existing diagnostic behavior doc comment from
rejection_diag_enabled to the rejection_diag function it describes. Keep
rejection_diag_enabled documented only as the PERRY_REJECTION_DIAG
environment-variable presence check, without changing either function’s
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9af2590-354c-4563-b216-1c8cc7fe3d9e
📒 Files selected for processing (6)
crates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/dynamic_import/visitors.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/promise/rejection.rscrates/perry/tests/issue_6660_closure_dynamic_import.rs
Comment on lines
+654
to
+663
| // Driver didn't resolve this path to a target module — | ||
| // route through the runtime fallback (#6660: builtin | ||
| // specifiers resolve like Node, everything else rejects | ||
| // with `ERR_MODULE_NOT_FOUND` instead of the old | ||
| // literal-`undefined` rejection). | ||
| return Ok(blk.call( | ||
| DOUBLE, | ||
| "js_module_dynamic_import_fallback", | ||
| &[(DOUBLE, &path_val)], | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/perry-codegen/src/expr/dyn_extern_i18n.rs"
printf '\n== line count ==\n'
wc -l "$file"
printf '\n== occurrences ==\n'
rg -n "js_module_dynamic_import_(apply_hooks|fallback)|lower_expr\\(ctx, arg\\)|path_val" "$file"
printf '\n== context around 560-700 ==\n'
sed -n '560,700p' "$file"
printf '\n== context around 760-820 ==\n'
sed -n '760,820p' "$file"Repository: PerryTS/perry
Length of output: 12046
Use the hook result in the single-target fallback js_module_dynamic_import_apply_hooks drops its return value here, so js_module_dynamic_import_fallback still sees the pre-hook specifier. That diverges from the multi-target path and can report the wrong module name when a loader rewrites the import.
🤖 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-codegen/src/expr/dyn_extern_i18n.rs` around lines 654 - 663,
Update the single-target fallback around js_module_dynamic_import_fallback to
pass the specifier returned by js_module_dynamic_import_apply_hooks rather than
the original path_val. Preserve the existing runtime fallback behavior while
ensuring loader rewrites are reflected in the module name and match the
multi-target path.
Comment on lines
+121
to
+131
| /// `PERRY_REJECTION_DIAG=1`: dump the raw rejection reason (tag class, bits, | ||
| /// value preview) plus a native backtrace when a rejection is first tracked as | ||
| /// unhandled — the backtrace there is the rejecting call site — and again when | ||
| /// it is reported at a checkpoint. Diagnostic aid for compiled bundles, where | ||
| /// an `Uncaught (in promise) undefined` line gives no way to tell whether the | ||
| /// reason was genuinely `undefined` or was lost en route (used to root-cause | ||
| /// #6660; same opt-in pattern as `PERRY_BIGINT_MIX_DIAG`). Only consulted on | ||
| /// the already-cold unhandled-rejection paths. | ||
| fn rejection_diag_enabled() -> bool { | ||
| std::env::var_os("PERRY_REJECTION_DIAG").is_some() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment describes rejection_diag, but sits over rejection_diag_enabled.
The comment at lines 121-128 documents the dump-and-backtrace behavior (stage tagging, tag/bits/preview, backtrace capture), which is actually implemented in rejection_diag (133-143). rejection_diag_enabled (129-131) is just the env-var presence check. Moving the doc down avoids misleading future readers into thinking the enable-check does the dumping.
📝 Suggested fix
-/// `PERRY_REJECTION_DIAG=1`: dump the raw rejection reason (tag class, bits,
-/// value preview) plus a native backtrace when a rejection is first tracked as
-/// unhandled — the backtrace there is the rejecting call site — and again when
-/// it is reported at a checkpoint. Diagnostic aid for compiled bundles, where
-/// an `Uncaught (in promise) undefined` line gives no way to tell whether the
-/// reason was genuinely `undefined` or was lost en route (used to root-cause
-/// `#6660`; same opt-in pattern as `PERRY_BIGINT_MIX_DIAG`). Only consulted on
-/// the already-cold unhandled-rejection paths.
fn rejection_diag_enabled() -> bool {
std::env::var_os("PERRY_REJECTION_DIAG").is_some()
}
+/// `PERRY_REJECTION_DIAG=1`: dump the raw rejection reason (tag class, bits,
+/// value preview) plus a native backtrace when a rejection is first tracked as
+/// unhandled — the backtrace there is the rejecting call site — and again when
+/// it is reported at a checkpoint. Diagnostic aid for compiled bundles, where
+/// an `Uncaught (in promise) undefined` line gives no way to tell whether the
+/// reason was genuinely `undefined` or was lost en route (used to root-cause
+/// `#6660`; same opt-in pattern as `PERRY_BIGINT_MIX_DIAG`). Only consulted on
+/// the already-cold unhandled-rejection paths.
#[cold]
fn rejection_diag(stage: &str, promise: *mut Promise) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// `PERRY_REJECTION_DIAG=1`: dump the raw rejection reason (tag class, bits, | |
| /// value preview) plus a native backtrace when a rejection is first tracked as | |
| /// unhandled — the backtrace there is the rejecting call site — and again when | |
| /// it is reported at a checkpoint. Diagnostic aid for compiled bundles, where | |
| /// an `Uncaught (in promise) undefined` line gives no way to tell whether the | |
| /// reason was genuinely `undefined` or was lost en route (used to root-cause | |
| /// #6660; same opt-in pattern as `PERRY_BIGINT_MIX_DIAG`). Only consulted on | |
| /// the already-cold unhandled-rejection paths. | |
| fn rejection_diag_enabled() -> bool { | |
| std::env::var_os("PERRY_REJECTION_DIAG").is_some() | |
| } | |
| fn rejection_diag_enabled() -> bool { | |
| std::env::var_os("PERRY_REJECTION_DIAG").is_some() | |
| } | |
| /// `PERRY_REJECTION_DIAG=1`: dump the raw rejection reason (tag class, bits, | |
| /// value preview) plus a native backtrace when a rejection is first tracked as | |
| /// unhandled — the backtrace there is the rejecting call site — and again when | |
| /// it is reported at a checkpoint. Diagnostic aid for compiled bundles, where | |
| /// an `Uncaught (in promise) undefined` line gives no way to tell whether the | |
| /// reason was genuinely `undefined` or was lost en route (used to root-cause | |
| /// `#6660`; same opt-in pattern as `PERRY_BIGINT_MIX_DIAG`). Only consulted on | |
| /// the already-cold unhandled-rejection paths. | |
| #[cold] | |
| fn rejection_diag(stage: &str, promise: *mut Promise) { |
🤖 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-runtime/src/promise/rejection.rs` around lines 121 - 131, Move
the existing diagnostic behavior doc comment from rejection_diag_enabled to the
rejection_diag function it describes. Keep rejection_diag_enabled documented
only as the PERRY_REJECTION_DIAG environment-variable presence check, without
changing either function’s behavior.
Comment on lines
+177
to
+186
| if jv.is_pointer() { | ||
| let ptr = jv.as_pointer::<u8>() as usize; | ||
| if crate::value::addr_class::is_plausible_heap_addr(ptr) | ||
| && unsafe { *(ptr as *const u32) } == crate::error::OBJECT_TYPE_ERROR | ||
| { | ||
| let eh = ptr as *const crate::error::ErrorHeader; | ||
| let stack = unsafe { crate::exception::string_header_to_string((*eh).stack) }; | ||
| return format!("error(0x{ptr:x}) stack={stack:?}"); | ||
| } | ||
| return format!("pointer(0x{ptr:x})"); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Error .stack is printed unbounded, unlike the truncated string preview.
The string-reason branch (Lines 164-176) caps output at 120 bytes at a char boundary, but the Error-object branch prints (*eh).stack in full via format!("error(0x{ptr:x}) stack={stack:?}"). A deep or looping stack trace on the rejected Error would dump unbounded text to stderr once PERRY_REJECTION_DIAG=1 is set (and it fires twice per rejection: track + report), whereas the string case is deliberately bounded. Apply the same truncation used for the string preview here for consistency.
🤖 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-runtime/src/promise/rejection.rs` around lines 177 - 186, The
Error-object branch in the rejection diagnostic formatter currently emits the
entire stack string without a size limit. In the pointer handling logic, apply
the same 120-byte, character-boundary truncation used by the string-preview
branch to the value returned by string_header_to_string, while preserving the
existing error pointer and formatting behavior.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wall #8 of the one-shot bring-up (tracker #6564): the compiled binary's
-pflow died with a reasonlessUncaught (in promise) undefined(stdout empty) where node completes a full API 401 round-trip.Root cause
Diagnosis (breakpoint on
track_unhandled_rejection+ the newPERRY_REJECTION_DIAG=1dump) traced the rejection to an asyncfileExistshelper awaiting a dynamicimport()inside a closure:Under the compiled binary the
import()compiled to a barejs_promise_rejected(undefined)(disassembly-confirmed: no hooks call, no error construction) — codegen's "unreachable" defensive arm for aDynamicImportwith emptypathsand nodeferred_error.Two real bugs stacked:
Visitor asymmetry (
perry-hir/src/dynamic_import/visitors.rs): the read-onlyfor_each_dynamic_importdid not descend intoExpr::Closurebodies, while its_mutsibling did. The driver aligns per-site resolution outcomes 1:1 by traversal order (collect via ref visitor → fill via mut visitor), so every closure-nestedimport()was invisible to the resolver but still visited by the fill pass: the outcome stream ran dry / mis-assigned, the site kept emptypathswith nodeferred_error, and codegen emitted the reject-undefined arm. (Thefor_each_worker_newpair already descends on both sides — this was the one odd visitor out.)Reasonless fallthroughs (
perry-codegen/src/expr/dyn_extern_i18n.rs): the empty-paths / unmapped-target / no-match arms rejected with literalundefined— an unreportable, uncatchable-looking failure.Fix
js_module_dynamic_import_fallback— theimport()analog of the compilePackages: ambient/computed require(expr) support (createRequire-backed) — two-tier plan #5389 Tier-2 ambient-require fallthrough: a specifier naming a node builtin resolves by string to the same namespacerequire(spec)/process.getBuiltinModule(spec)produce (install-all hooks armed, runtime: createRequire rejects node:-prefixed builtins / missing diagnostics_channel — pi wall #3 (require('node:diagnostics_channel') via bundle shim) #6644 pattern), anything else rejects with a descriptiveErrorcarryingcode: 'ERR_MODULE_NOT_FOUND'(node's dynamic-import failure family) — never literalundefined.js_module_dynamic_import_deferred: a runtime-computed specifier that names a builtin now resolves like node; a genuinely unknown module keeps the site-specificfile:linedeferral message.PERRY_REJECTION_DIAG=1dumps the raw rejection reason (tag class, bits, value preview, Error stack when present) plus a native backtrace at unhandled-rejection track time (= the rejecting call site) and report time. Documented likePERRY_BIGINT_MIX_DIAG.Tests
crates/perry/tests/issue_6660_closure_dynamic_import.rs(all byte-identical to node v26 where node semantics are reproducible):fileExists) →true/falseconst imp = (s) => import(s)) resolving a submodule-spec builtin (node:fs/promises) and a native-module builtin (node:os)instanceof Error, defined reason, non-empty message (message text pins behavior, not bytes — node's names the importing file, an AOT binary reports the deferral site)Suites:
cargo test -p perry-runtime --lib -- --test-threads=1→ 1426 passed / 0 failed.issue_5230_deferred_dynamic_import,issue_5207_registry_object_dynamic_import,createrequire_builtin_modules, and the new test file all green.cargo fmtclean on touched files. (Pre-existing, unrelated on the base commit:c262_parity::logical_property_assignment_short_circuits_the_store_4586— verified failing at the parent commit in a pristine worktree.)End-to-end (GATE 2a)
Full recompile of the 13 MB one-shot target: the previously-dying repro now completes the full live API round-trip with stdout+stderr byte-identical to node v26 after masking the per-request
request_id— model-not-in-catalog warning, deprecation notice, and the401 authentication_errorJSON:The compile notice now correctly lists the closure-nested
import()sites as runtime-deferred (they were silently mis-lowered before).New wall exposed behind this one (documented, out of scope here): exit code. node exits
1(the one-shot flow setsprocess.exitCode = 1on stream error and returns), the compiled binary exits0. Isolated to two 2-line fixtures:process.exitCode = 1at top level (node rc=1, perry rc=0). The runtime HAS thePROCESS_EXIT_CODEcell (#1350) andprocess.exit()honors it, but the natural-exit epilogue (event-loop drain → main return) never consults it. Distinct root cause, next wall in the ladder.Fixes #6660
https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
Summary by CodeRabbit
Bug Fixes
import()handling for unresolved and runtime-computed module specifiers.Diagnostics
PERRY_REJECTION_DIAG, including reason previews and backtraces.