Skip to content

fix(codegen,runtime): #5437 — require-derived captured local drops to undefined (no decl-site snapshot) - #5568

Merged
proggeramlug merged 2 commits into
mainfrom
fix/w6-require-derived-capture-5437
Jun 23, 2026
Merged

fix(codegen,runtime): #5437 — require-derived captured local drops to undefined (no decl-site snapshot)#5568
proggeramlug merged 2 commits into
mainfrom
fix/w6-require-derived-capture-5437

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

What

Follow-up to the #5437 W6 CaptureFill fix (v0.5.1202). Fixes the next Next.js wall (trace.getSpan on undefined) and a real over-aggressiveness in that fix.

Root

The W6 fix made inline_constructor_param_values_with_class (lower_call/new.rs) ALWAYS read a cap param from the decl-site snapshot js_class_capture_value(cid, slot) when the class is in ctx.class_ids. But a snapshot only exists for classes reaching a RegisterClassCaptures decl-site. An inline anonymous class capturing a require()-derived local (const h = require(s).x) gets a cid with no registered snapshotjs_class_capture_value returns TAG_UNDEFINED → captured value reads undefined in methods. Decisive probe: CAPVAL cid=5 idx=0 snapshot_exists=false result_bits=0x7ffc...0001. In the bundle this was the OTel tracer capturing a require("@opentelemetry/api")-derived trace.

Minimal repro (w6-repro/otel/)

const h = require("./plainmod.js").trace;            // module.exports = { trace: { getSpan(){…} } }
const impl = new class { m() { return typeof h; } };
console.log(impl.m());   // perry: "undefined"   node: "object"

FAILS = class-capture of a require()-derived local; PASSES = capturing the whole require result / a plain-obj property / capture by an arrow closure.

Fix (codegen + runtime, 3 files +test)

New FFI js_class_capture_value_or(cid, slot, fallback) (object/class_constructors.rs, + #[used] keepalive, declared in runtime_decls/strings.rs): returns the snapshot slot when registered (W6: authoritative), else fallback. CaptureFill arm passes the new-site appended cap arg as fallback. Keeps W6 (snapshot wins when present); restores the appended value for the snapshot-less inline-class case.

Validation (rigorous 4-point gate)

Refs #5437.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where values captured from require() calls within class methods could incorrectly resolve to undefined.
  • Tests

    • Added regression test for captured requirement-derived values in class methods.

Ralph Küpper added 2 commits June 23, 2026 07:37
… drop to undefined when no decl-site snapshot exists

The W6 fix made a bare/member `new C(captures...)` fill the synthesized
__perry_cap_* params from the class's decl-site capture snapshot
(js_class_capture_value), discarding the new-site appended cap arg —
the snapshot being authoritative because the bundle's multi-level
capture chain can materialize a mis-boxed value into the appended arg.

But the snapshot only exists for classes that reach the
RegisterClassCaptures decl-site. An inline anonymous class
(`new class { m(){ return capturedLocal } }`) capturing a local whose
initializer derives from a require() result (`const h = require(s).x` /
`const {x:h} = require(s)`) has NO registered snapshot, so the snapshot
read returned undefined and dropped the (correct) appended cap value.

This was the next Next.js wall after W6: the render threw
`TypeError: Cannot read properties of undefined (reading 'getSpan')`
from the OTel-backed tracer's getActiveScopeSpan() — `trace` is
destructured from the require'd @opentelemetry/api module and captured
by the tracer class, so it read undefined inside the method.

Fix: js_class_capture_value_or(cid, slot, fallback) returns the snapshot
slot when a snapshot is registered for cid, else the appended cap arg
(fallback). Keeps W6 (snapshot wins when present) while restoring the
appended value for the snapshot-less case. Codegen passes the appended
cap arg as the fallback at the bare/member new site.

Regression test issue_5437_require_derived_capture covers
`require().prop` and destructured `{prop}=require()` captured by class
methods (member, function-scoped, and whole-require / plain-object
controls).
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 54714b05-66df-4a5e-9dc5-26957f473f8c

📥 Commits

Reviewing files that changed from the base of the PR and between bf06f90 and f4cecb7.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • CHANGELOG.md
  • CLAUDE.md
  • Cargo.toml
  • crates/perry-codegen/src/lower_call/new.rs
  • crates/perry-codegen/src/runtime_decls/strings.rs
  • crates/perry-runtime/src/object/class_constructors.rs
  • crates/perry/tests/issue_5437_require_derived_capture.rs

📝 Walkthrough

Walkthrough

Fixes a bug where js_class_capture_value returned TAG_UNDEFINED for captured locals derived from require() when an inline anonymous class lacked a decl-site snapshot. Introduces a new FFI js_class_capture_value_or(cid, slot, fallback) in the runtime, updates the CaptureFill codegen lowering to pass the appended cap arg as the fallback, and adds a regression test for issue #5437. Bumps version to 0.5.1204.

Changes

require-derived inline-class capture fix

Layer / File(s) Summary
New js_class_capture_value_or FFI implementation
crates/perry-runtime/src/object/class_constructors.rs
Adds the js_class_capture_value_or FFI function: returns snapshot slot values when a decl-site snapshot exists for class_id, returns TAG_UNDEFINED for out-of-bounds indices within an existing snapshot, and returns the caller-provided fallback when no snapshot is registered. Adds the KEEP_JS_CLASS_CAPTURE_VALUE_OR keepalive anchor.
CaptureFill codegen lowering update
crates/perry-codegen/src/runtime_decls/strings.rs, crates/perry-codegen/src/lower_call/new.rs
Declares js_class_capture_value_or in Phase B runtime string declarations and updates inline_constructor_param_values_with_class to call it with the appended cap arg as a fallback, replacing the prior js_class_capture_value call that discarded the cap arg.
Regression test and version bumps
crates/perry/tests/issue_5437_require_derived_capture.rs, CHANGELOG.md, CLAUDE.md, Cargo.toml
Adds a regression test covering require/destructure/function capture variants, compiling and executing test scripts via the perry CLI and asserting SPAN_OK/PLAIN_OK stdout. Bumps workspace version to 0.5.1204 and adds a changelog entry.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PerryTS/perry#5561: Directly precedes this fix — introduced the initial CaptureFill-based constructor capture wiring in lower_call/new.rs that this PR updates to use js_class_capture_value_or with a fallback.

Poem

🐇 A capture was lost in the snapshot-less night,
require()'d locals turned undefined in fright.
But now with a fallback appended with care,
js_class_capture_value_or fills the snare.
The OTel trace shines—the rabbit hops right! 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing a codegen/runtime bug where require-derived captured locals became undefined when an inline class lacks a decl-site snapshot, directly addressing issue #5437.
Description check ✅ Passed The description comprehensively covers all required sections: what the fix addresses, root cause analysis, minimal reproduction, the solution with technical details, and validation approach. It follows the template structure and provides clear, specific information.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/w6-require-derived-capture-5437

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant