runtime: createRequire node:-prefix normalization + diagnostics_channel builtin (#6644) - #6647
Merged
proggeramlug merged 2 commits intoJul 19, 2026
Conversation
added 2 commits
July 18, 2026 22:47
…lass expressions (PerryTS#6604) A class EXPRESSION capturing an enclosing-function var assigned AFTER the class in source order — semver's shape in every bundled class file: var Comparator = class _Comparator { constructor(comp, options) { options = parseOptions(options); ... } }; module.exports = Comparator; var parseOptions = require_parse_options(); // AFTER the class replayed a stale declaration-time capture snapshot on DYNAMIC construction of the escaped class value: the end-of-body refresh machinery (PerryTS#6037/PerryTS#6052) scanned only `ast::Decl::Class` DECLARATION statements in both body twins (`lower_fn_body_block_stmt` and `lower_fn_expr`), so `var C = class ...` never got refresh statements. The captured var stayed `undefined` in the name-keyed CLASS_CAPTURE_VALUES snapshot forever, the per-evaluation `__perry_ctor_caps` array had snapshotted the same pre-assignment `undefined` (TDZ-suppressed, PerryTS#6523), and the ctor prologue's param-or-snapshot rebind (PerryTS#5437) found `undefined` on both sides → "TypeError: value is not a function" at pi-native init (wall PerryTS#2 of the pi bring-up, PerryTS#6564, directly behind PerryTS#6593). Fix: `lower_class_expr` records every capturing class expression lowered in a function body — `(resolved_registration_name, captured_ids)`, sidestepping the expression-ident/binding-name and rename subtleties by recording the name AFTER dedup/rename resolution — in a new body-scoped `ctx.body_class_expr_captures` list. Both body twins mark the list length at entry and drain their own suffix into the existing re_regs/re_reg_capsets machinery, so class expressions get the same refresh-after-assignment, refresh-before-return, and end-of-body re-registration as declarations. The runtime construct replay then self-heals through the existing ctor-prologue param-or-snapshot rebind: the stale per-eval `undefined` cap param falls back to the now-live name-keyed snapshot. No codegen/runtime changes. Scoping: entries' LocalIds are only meaningful in their own function's numbering, so every body path drains/truncates back to its entry mark: the twins drain; arrows truncate on exit (mark at scope entry, covering expression bodies); the block twin truncates on its error path; and `get_param_default` self-truncates so a class expression in a default-param value — lowered BEFORE the callee body's twin takes its mark at fn-decl / ctor / method param sites — can never be drained by the wrong (enclosing) body. No `append_new_args_stmt` pass runs for expressions — their construct sites are either static (live locals appended at the site) or dynamic (replayed through the snapshot). Known limitation (noted in the PR): a MULTI-evaluation factory whose class captures a var assigned after the class still reads the LAST evaluation's refreshed snapshot for slots that were undefined at its own evaluation — same last-writer-wins semantics the name-keyed store always had (PerryTS#5437/PerryTS#685); pre-fix those slots were `undefined`, so this strictly improves the broken case. Per-evaluation isolation of the normal assigned-before-class shape is unchanged (per-eval `__perry_ctor_caps` values win in the param-first rebind). Tests: tests/test_class_expr_capture_refresh_6604.sh (var/let/const named + anonymous class expressions, argument-position, the esbuild __commonJS semver comparator.js layout, captured-var reassignment, multi-eval isolation) and node-suite parity fixture test-parity/node-suite/object/class-expr-capture-refresh.ts — all node-identical; failing (TypeError) before this fix. Fixes PerryTS#6604 Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
…el builtin (PerryTS#6644) require('node:diagnostics_channel') through createRequire (the esbuild banner shim every ESM bundle of CJS deps produces) threw ERR_PERRY_UNSUPPORTED_CREATE_REQUIRE. Two gaps: - diagnostics_channel was missing from the createRequire allowlist and from process.getBuiltinModule's, even though a full implementation (real pub/sub channel registry, tracingChannel, bindStore) already lives in node_submodules/diagnostics.rs. Both resolvers now allowlist it and route to js_node_submodule_namespace like timers/promises. - The require closure resolves builtins from a RUNTIME string, so codegen could not emit per-module dispatch installs; a dynamically required module's namespace came back method-dead unless something else had armed the registries. New js_module_create_require_devirt arms the nm + submod install-all hooks (mirrors js_process_get_builtin_module_devirt); the NativeModSig row targets it, so the all-buckets reference is only linked into programs that actually call createRequire. The node: prefix strip itself already existed in both resolvers (supported_require_builtin / getBuiltinModule); module.isBuiltin also handles both spellings — no gap there. Fixes PerryTS#6644 Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
📝 WalkthroughWalkthroughThe PR adds ChangescreateRequire built-in resolution
Class-expression capture refresh
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant js_module_create_require_devirt
participant NativeHooks
participant NodeSubmoduleHooks
participant js_module_create_require
Caller->>js_module_create_require_devirt: create require(filename_or_url)
js_module_create_require_devirt->>NativeHooks: enable install-all hooks
js_module_create_require_devirt->>NodeSubmoduleHooks: enable install-all hooks
js_module_create_require_devirt->>js_module_create_require: construct require closure
js_module_create_require-->>Caller: return require closure
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
crates/perry-runtime/src/module_require.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated built-in module resolution logic.
The special-case routing for
diagnostics_channel(and similarly fortimers/promises) tonode_submodules::js_node_submodule_namespaceis duplicated in bothmodule_require.rsandnode_module.rs. Consider extracting this resolution logic into a shared helper function (e.g., innode_submodules/mod.rsorobject/native_module_registry.rs) to keep the routing DRY and ensure any future submodule additions are handled consistently.
crates/perry-runtime/src/module_require.rs#L132-144: Replace this block with a call to the shared helper.crates/perry-runtime/src/process/node_module.rs#L859-869: Replace this block with a call to the same shared helper.🤖 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/module_require.rs` at line 1, Extract the duplicated built-in submodule routing for diagnostics_channel and timers/promises into a shared helper near the existing node_submodules or native module registry logic. Update the require resolution flow in module_require.rs and the corresponding node module resolution flow in node_module.rs to call this helper, preserving current fallback behavior and ensuring future submodule additions use the same routing.
🤖 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-hir/src/lower_patterns.rs`:
- Around line 1457-1469: Update the default-expression handling around
lower_expr in the parameter-lowering path to retain body_class_expr_captures
entries for capturing class expressions instead of truncating them. Scope those
entries to the callee body’s capture-refresh machinery so they are not consumed
by the enclosing body while still observing later parameter/body assignments;
add regression coverage for a default class capturing a reassigned binding.
In `@crates/perry-hir/src/lower/lower_expr/arm_class.rs`:
- Around line 189-205: Update the class-expression capture refresh flow around
body_class_expr_captures and lookup_class_captures so refresh state is stored
per fresh class evaluation rather than in a single name-keyed global snapshot.
Ensure captures from later evaluations such as B cannot populate stale slots for
A, while preserving assigned-after-class refresh behavior. Add a regression
covering multiple assigned-after-class factory evaluations and construction of
the earlier class.
---
Nitpick comments:
In `@crates/perry-runtime/src/module_require.rs`:
- Line 1: Extract the duplicated built-in submodule routing for
diagnostics_channel and timers/promises into a shared helper near the existing
node_submodules or native module registry logic. Update the require resolution
flow in module_require.rs and the corresponding node module resolution flow in
node_module.rs to call this helper, preserving current fallback behavior and
ensuring future submodule additions use the same routing.
🪄 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: 5c34f5c4-35bb-405f-a1ff-0bc60d32ad66
📒 Files selected for processing (14)
crates/perry-codegen/src/lower_call/native_table/node_core/module_sea_tls_test.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/lower_expr/arm_class.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_patterns.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/node_module.rscrates/perry/tests/createrequire_builtin_modules.rstest-parity/node-suite/object/class-expr-capture-refresh.tstests/test_class_expr_capture_refresh_6604.sh
Comment on lines
+1457
to
+1469
| // #6604: a capturing class EXPRESSION used as a default value | ||
| // (`function f(C = class { … }) {}`) must NOT register with the | ||
| // enclosing body's end-of-body capture-refresh machinery: param | ||
| // defaults are lowered BEFORE the callee's own body twin takes | ||
| // its list mark (fn-decl / ctor / method param sites), so the | ||
| // entry would be drained by the WRONG (enclosing) body and its | ||
| // ids interpreted in the wrong function's local numbering. | ||
| // Truncate whatever this default expression recorded — the | ||
| // default is re-evaluated at every call anyway, so its | ||
| // evaluation-time snapshot is per-call fresh. | ||
| let mark = ctx.body_class_expr_captures.len(); | ||
| let default_expr = lower_expr(ctx, &assign.right)?; | ||
| ctx.body_class_expr_captures.truncate(mark); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not discard default class expressions that capture body-mutated bindings.
Per-call evaluation is fresh, but the class must still observe later body assignments:
function f(x, C = class { get() { return x; } }) {
x = 2;
return C;
}Truncating here excludes C from assignment refresh, leaving its snapshot at 1. Scope these entries to the callee body instead of dropping them, and add regression coverage.
🤖 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-hir/src/lower_patterns.rs` around lines 1457 - 1469, Update the
default-expression handling around lower_expr in the parameter-lowering path to
retain body_class_expr_captures entries for capturing class expressions instead
of truncating them. Scope those entries to the callee body’s capture-refresh
machinery so they are not consumed by the enclosing body while still observing
later parameter/body assignments; add regression coverage for a default class
capturing a reassigned binding.
Comment on lines
+189
to
+205
| // #6604: register this capturing class EXPRESSION with the enclosing | ||
| // body's end-of-body capture-refresh machinery (#6037/#6052), which | ||
| // previously scanned class DECLARATION statements only. Without the | ||
| // refresh, a captured var assigned AFTER the class expression (semver's | ||
| // `var Comparator = class _Comparator { … }; …; var parseOptions = | ||
| // require_parse_options()`) stays `undefined` in the decl-site snapshot, | ||
| // and dynamic construction of the escaped class value replays that stale | ||
| // snapshot. Recording the RESOLVED registration name here (post | ||
| // rename/dedup) sidesteps re-deriving it from the AST at body end. Module | ||
| // top is skipped — module-level ids are stripped from capture lists by | ||
| // `filter_module_level_captures`, so there is nothing to refresh. | ||
| if !at_module_top && !captured_args.is_empty() { | ||
| if let Some(ids) = ctx.lookup_class_captures(&synthetic_name) { | ||
| ctx.body_class_expr_captures | ||
| .push((synthetic_name.clone(), ids.to_vec())); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve per-evaluation captures for assigned-after-class factories.
This registers ClassExprFresh captures in one global, name-keyed snapshot. With A = make("a"); B = make("b"); new A(), stale slots in A can be backfilled from B’s later snapshot.
Keep refresh state on the fresh class object, or otherwise key it per evaluation. Add this assigned-after-class multi-evaluation regression alongside the existing assigned-before-class case.
🤖 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-hir/src/lower/lower_expr/arm_class.rs` around lines 189 - 205,
Update the class-expression capture refresh flow around body_class_expr_captures
and lookup_class_captures so refresh state is stored per fresh class evaluation
rather than in a single name-keyed global snapshot. Ensure captures from later
evaluations such as B cannot populate stale slots for A, while preserving
assigned-after-class refresh behavior. Add a regression covering multiple
assigned-after-class factory evaluations and construction of the earlier class.
This was referenced Jul 18, 2026
Contributor
Author
|
CodeRabbit's two findings here (param-default class expressions discarded; name-keyed snapshot cross-contamination in multi-eval factories) both target the #6604 refresh machinery that merged via #6633 — this PR's diff only carries it as a stacked base. Consolidated, with the third sibling finding from #6650/#6653, into follow-up issue #6654 for fixture-driven verification. |
13 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.
Root cause
require('node:diagnostics_channel')throughcreateRequire(import.meta.url)— the esbuild banner shim every ESM bundle of CJS deps produces; in the pi bundle it's lru-cache's node build — threw:Two gaps, one non-gap:
node:prefix strip already existed insupported_require_builtin(and inprocess.getBuiltinModule;module.isBuiltinhandles both spellings too). The real hole:diagnostics_channelwas absent from both the createRequire and getBuiltinModule allowlists — even though a full implementation (real pub/sub channel registry,tracingChannel,bindStore, cross-thread publishes) already lives innode_submodules/diagnostics.rsand serves staticimports. Both resolvers now allowlist it and route tojs_node_submodule_namespace("diagnostics_channel"), exactly like the existingtimers/promisesspecial case.diagnostics_channelwould have been an unresolved stub even once allowlisted). Newjs_module_create_require_devirtarms the nm + submod install-all hooks and delegates — mirroringjs_process_get_builtin_module_devirt. TheNativeModSigrow formodule.createRequiretargets the devirt symbol, so the all-buckets reference is linked only into programs whose source actually callscreateRequire; the plainjs_module_create_require(reachable via the always-pinned ambient-require keepalives and themoduledispatch bucket) stays free of it, preserving per-module dead-stripping.What shipped
The real implementation, not a stub:
channel(name)returns name-keyed singleton Channel objects with workingpublish→ subscriber delivery,subscribe/unsubscribe, livehasSubscribers, andtracingChannel()— all backed by the existingnode_submodules/diagnostics.rsregistry, now reachable fromcreateRequire(bothdiagnostics_channelandnode:diagnostics_channelspellings) andprocess.getBuiltinModule.Known follow-up (pre-existing, unchanged): the ambient computed
require(expr)fallthrough (js_module_ambient_require_apply) does not arm the hooks itself (its keepalive anchors are pinned into every program, so referencing install-all there would re-pin all buckets globally); it resolves fine once anything armed them. Also pre-existing: perry over-accepts a few bare spellings ofnode:-only builtins (e.g.sqlite).Tests
createrequire_resolves_diagnostics_channel_and_node_prefixed_builtinsincrates/perry/tests/createrequire_builtin_modules.rs: both spellings, module shape, real pub/sub across handles from each spelling (subscribe → hasSubscribers flips → publish delivers on both handles → unsubscribe returns true → flips back), tracingChannel shape,require('node:path')through the same require, and thegetBuiltinModulesibling path. Compiled outputs verified byte-identical to node for the issue's minimal fixture and the full fixture during development.cargo test -p perry --test createrequire_builtin_modules: 2 passed.cargo test -p perry-runtime --lib -- --test-threads=1: 1422 passed, 0 failed.cargo test -p perry-codegen --test native_proof_regressions(242 passed) +--lib(205 passed). (--test perry_builtin_name_collisionfails to compile on the base branch already — pre-existing, unrelated.)cargo fmt --all -- --check: no new diffs vs. base (one pre-existing diff inissue_6559_dyn_function_interpreter.rsuntouched).Pi bring-up wall #3 (tracker #6564): the recompiled pi bundle gets past the #6644 throw (validation result in the issue/tracker).
Stacks on #6633 — branched from
fix/6604-class-expr-capture-refresh; merge after it.Fixes #6644
https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
Summary by CodeRabbit
New Features
createRequirenow supports thediagnostics_channelbuilt-in module in both bare andnode:-prefixed forms.process.getBuiltinModulecan resolvenode:diagnostics_channel.Bug Fixes