fix(debug-symbols): destructure-of-undefined location coverage + CJS-wrap coordinate mapping#5321
Merged
Merged
Conversation
added 2 commits
June 17, 2026 12:42
…-wrap coordinate mapping Gap 1 (coverage): emit js_set_call_location at the object-destructuring RequireObjectCoercible site so a destructure-of-undefined throw renders at the destructure line instead of the stale last-tracked call. The obj-pattern byte offset rides as a 2nd literal arg to the requireObjectCoercible runtime call; codegen acts on it only under --debug-symbols (default build byte-identical). Gap 2 (CJS-wrap skew): cjs-wrapped modules parse WRAPPED text, so HIR byte_offsets are in wrapped coords but resolved against the original file. Record the wrapper prefix line count + wrapped source per module (only under --debug-symbols), pass wrapped source as module_source and debug_source_line_offset to codegen, and subtract the prefix in call_location_for to recover original-source lines.
…wrap offset - strings.rs unit tests: call_location_for subtracts the wrapper prefix line count (CJS-wrap skew correction) and maps preamble offsets to None. - cjs_wrap unit test: wrap_commonjs_with_body_offset locates the original body so a wrapped body line maps back to its original-source line. - codegen IR-assertion test: under --debug-symbols the destructure path emits js_set_call_location before js_require_object_coercible; default build emits no such CALL (byte-identical).
📝 WalkthroughWalkthroughFixes issue ChangesCJS-wrap debug source line offset fix
Sequence Diagram(s)sequenceDiagram
participant CLI as compile.rs
participant Collect as collect_modules.rs
participant Wrap as wrap_commonjs_with_body_offset
participant Ctx as CompilationContext
participant Codegen as compile_module
participant Pool as StringPool
participant Native as lower_native_method_call
CLI->>Ctx: ctx.debug_symbols = args.debug_symbols
CLI->>Collect: collect_modules(ctx)
Collect->>Wrap: wrap_commonjs_with_body_offset(source)
Wrap-->>Collect: (wrapped_source, body_offset)
Collect->>Collect: compute prefix_line_count (with createRequire delta)
Collect->>Ctx: cjs_wrap_debug_sources[path] = CjsWrapDebugSource { wrapped_source, prefix_line_count }
CLI->>Codegen: compile_module(opts { module_source, debug_source_line_offset })
Codegen->>Pool: set_debug_source_line_offset(opts.debug_source_line_offset)
Note over Native,Pool: During IR lowering
Native->>Pool: call_location_for(span.lo.0)
Pool-->>Native: Some(original_line) or None
Native->>Native: emit js_set_call_location(file, line)
Native->>Native: emit js_require_object_coercible(value)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/compile/object_cache.rs (1)
168-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
debug_source_line_offsetin object-cache key derivation.Line 172 includes
debug_locations, butopts.debug_source_line_offsetis not hashed. Under debug symbols, this offset changesjs_set_call_locationline literals, so cache reuse can return stale objects with wrong file:line attribution.Suggested fix
h.field("dbgloc", if opts.debug_locations { "1" } else { "0" }); + h.field( + "debug_source_line_offset", + &opts.debug_source_line_offset.to_string(), + );🤖 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/object_cache.rs` around lines 168 - 173, The object-cache key derivation is missing the debug_source_line_offset field which affects js_set_call_location line literals. In the section where h.field("dbgloc", ...) is called to hash debug_locations, add an additional h.field() call to hash opts.debug_source_line_offset using a descriptive key name. This ensures that toggling the debug source line offset flag will generate a different cache key and prevent serving stale objects with incorrect file:line attribution.
🧹 Nitpick comments (3)
crates/perry/src/commands/compile/types.rs (1)
842-851: ⚡ Quick winAlign source-text docs with the actual stored payload.
The comment on
cjs_wrap_debug_sourcesdescribes storing ORIGINAL/pre-wrap text, butCjsWrapDebugSourceclearly stores WRAPPED text. This mismatch is easy to cargo-cult into incorrect future usage.♻️ Suggested doc fix
- /// `#5247` (CJS-wrap coordinate skew): for each CommonJS module rewritten by - /// `cjs_wrap::wrap_commonjs_for_target`, the ORIGINAL (pre-wrap) source - /// text plus the number of newline characters the injected wrapper prefix + /// `#5247` (CJS-wrap coordinate skew): for each CommonJS module rewritten by + /// `cjs_wrap::wrap_commonjs_for_target`, the WRAPPED source text plus the + /// number of newline characters the injected wrapper prefix /// prepended before the original module body. Under `--debug-symbols`, /// codegen resolves a node's `byte_offset` (which is in WRAPPED /// coordinates) to a line by deducting this prefix line count and looking - /// up the original source — so a throw renders `at <module>:<original-line>` + /// up the wrapped source — so a throw renders `at <module>:<original-line>` /// rather than a line shifted by the preamble.Also applies to: 859-866
🤖 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/types.rs` around lines 842 - 851, The documentation comment for the cjs_wrap_debug_sources field incorrectly states that it stores ORIGINAL (pre-wrap) source text, but the actual CjsWrapDebugSource structure stores WRAPPED text. Correct the doc comment to accurately reflect that the stored source is in WRAPPED coordinates, not original coordinates. This includes updating the description across all related documentation (including the section that applies to lines 859-866) to ensure consistency and prevent future misuse of this field.crates/perry/src/commands/compile/cjs_wrap/wrap.rs (1)
16-22: ⚡ Quick winAvoid unconditional body-offset search in the non-debug wrapper path.
wrap_commonjs_for_targetnow always pays forwrapped.find(...)even when the offset is discarded. Since this path is hot, keep the fast string-only route and compute the offset only for callers that explicitly need it.♻️ Refactor sketch
pub(in crate::commands::compile) fn wrap_commonjs_for_target( source: &str, source_path: &Path, target: Option<&str>, ) -> String { - wrap_commonjs_with_body_offset(source, source_path, target).0 + wrap_commonjs_inner(source, source_path, target, false).0 } pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( source: &str, source_path: &Path, target: Option<&str>, ) -> (String, Option<usize>) { + wrap_commonjs_inner(source, source_path, target, true) +} + +fn wrap_commonjs_inner( + source: &str, + source_path: &Path, + target: Option<&str>, + need_body_offset: bool, +) -> (String, Option<usize>) { // ... existing wrap construction ... - let body_offset = if body_for_iife.is_empty() { - None - } else { - wrapped.find(body_for_iife.as_str()) - }; + let body_offset = if need_body_offset && !body_for_iife.is_empty() { + wrapped.find(body_for_iife.as_str()) + } else { + None + }; (wrapped, body_offset) }Also applies to: 744-753
🤖 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/cjs_wrap/wrap.rs` around lines 16 - 22, The wrap_commonjs_for_target function currently calls wrap_commonjs_with_body_offset which performs an unnecessary body-offset search via find(...) even though the offset result is discarded. To optimize this hot path, refactor wrap_commonjs_for_target to use a simpler wrapping implementation that skips the offset computation entirely, and only have callers that explicitly need the offset call wrap_commonjs_with_body_offset instead. This ensures the fast string-only route avoids unnecessary processing.crates/perry/src/commands/compile/collect_modules.rs (1)
435-435: 💤 Low valueOptional micro-optimization: Move line counting inside the conditional.
Line counting executes unconditionally but the result is only used when
was_cjs_wrapped && ctx.debug_symbols(line 437). Moving this line inside that conditional would avoid the scan for non-wrapped modules and non-debug builds.Impact is negligible (one fast byte scan per module in production), and the current placement keeps the before/after counts visually adjacent to the transform. Only worth changing if you value the minor production build efficiency over the current clarity.
♻️ Optional refactor
-let lines_before_transform = source.bytes().filter(|&b| b == b'\n').count(); let source = transform_create_require_literal_requires(&source, &ctx.compile_packages); if was_cjs_wrapped && ctx.debug_symbols { if let Some(prefix_lines) = cjs_wrap_body_prefix_lines { + let lines_before_transform = source.bytes().filter(|&b| b == b'\n').count(); let lines_after_transform = source.bytes().filter(|&b| b == b'\n').count();Wait, that's wrong - we need to count BEFORE the transform. Let me fix:
-let lines_before_transform = source.bytes().filter(|&b| b == b'\n').count(); let source = transform_create_require_literal_requires(&source, &ctx.compile_packages); if was_cjs_wrapped && ctx.debug_symbols { if let Some(prefix_lines) = cjs_wrap_body_prefix_lines { + // Note: would need to count lines before the transform above, + // so this optimization requires restructuring to avoid double-scanning let lines_after_transform = source.bytes().filter(|&b| b == b'\n').count();Actually, this is trickier than it looks because we need the count BEFORE the transform but the conditional check is AFTER. We'd need to either:
- Store the original source temporarily, or
- Count before transform only when the condition will be true
This makes the refactor more complex than initially assessed. Let me revise the suggestion.
[actions]
Actually, skip this comment. The refactor is more complex than it appears (would require restructuring), and the benefit is truly minimal. This doesn't meet the threshold for even an optional suggestion in a Chill review.
[scratchpad_end] -->🤖 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/collect_modules.rs` at line 435, No changes are required for this code. The reviewer has determined that while moving the line counting operation inside the conditional (before the lines_before_transform variable in the section near the was_cjs_wrapped check) could theoretically optimize performance, the refactor is too complex to implement cleanly since the count must occur before the transform but the conditional check happens after. The minimal performance benefit does not justify the added code complexity, so this suggestion should be disregarded.
🤖 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.
Outside diff comments:
In `@crates/perry/src/commands/compile/object_cache.rs`:
- Around line 168-173: The object-cache key derivation is missing the
debug_source_line_offset field which affects js_set_call_location line literals.
In the section where h.field("dbgloc", ...) is called to hash debug_locations,
add an additional h.field() call to hash opts.debug_source_line_offset using a
descriptive key name. This ensures that toggling the debug source line offset
flag will generate a different cache key and prevent serving stale objects with
incorrect file:line attribution.
---
Nitpick comments:
In `@crates/perry/src/commands/compile/cjs_wrap/wrap.rs`:
- Around line 16-22: The wrap_commonjs_for_target function currently calls
wrap_commonjs_with_body_offset which performs an unnecessary body-offset search
via find(...) even though the offset result is discarded. To optimize this hot
path, refactor wrap_commonjs_for_target to use a simpler wrapping implementation
that skips the offset computation entirely, and only have callers that
explicitly need the offset call wrap_commonjs_with_body_offset instead. This
ensures the fast string-only route avoids unnecessary processing.
In `@crates/perry/src/commands/compile/collect_modules.rs`:
- Line 435: No changes are required for this code. The reviewer has determined
that while moving the line counting operation inside the conditional (before the
lines_before_transform variable in the section near the was_cjs_wrapped check)
could theoretically optimize performance, the refactor is too complex to
implement cleanly since the count must occur before the transform but the
conditional check happens after. The minimal performance benefit does not
justify the added code complexity, so this suggestion should be disregarded.
In `@crates/perry/src/commands/compile/types.rs`:
- Around line 842-851: The documentation comment for the cjs_wrap_debug_sources
field incorrectly states that it stores ORIGINAL (pre-wrap) source text, but the
actual CjsWrapDebugSource structure stores WRAPPED text. Correct the doc comment
to accurately reflect that the stored source is in WRAPPED coordinates, not
original coordinates. This includes updating the description across all related
documentation (including the section that applies to lines 859-866) to ensure
consistency and prevent future misuse of this field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2ec450f-eca6-4fb3-9468-684e7c6b6ddb
📒 Files selected for processing (24)
crates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/lower_call/native/mod.rscrates/perry-codegen/src/strings.rscrates/perry-codegen/tests/argless_builtin_extra_args.rscrates/perry-codegen/tests/class_keys_gc_root.rscrates/perry-codegen/tests/constructor_recursion.rscrates/perry-codegen/tests/destructure_call_location.rscrates/perry-codegen/tests/large_object_barriers.rscrates/perry-codegen/tests/macos_bundle_chdir_gate.rscrates/perry-codegen/tests/native_proof_buffer_views.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/shadow_slot_hygiene.rscrates/perry-codegen/tests/static_symbol_hygiene.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-codegen/tests/typed_shape_descriptor.rscrates/perry-codegen/tests/typed_shape_descriptors.rscrates/perry-hir/src/destructuring/pattern_binding.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/cjs_wrap/wrap.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/types.rs
This was referenced Jun 18, 2026
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.
Summary
Improves
--debug-symbolssource-location diagnostics for two runtime-throw classes that were previously misreported in native-npm package compiles. Both fixes are gated behind--debug-symbols; the default build is unchanged (byte-identical, zero added emission/cloning).Gap 1 — coverage: destructure-of-undefined (pino)
js_set_call_locationwas emitted only at dynamic call/new dispatch + a couple of property/console paths, never for object destructuring. A throw likeconst {a} = undefined(Cannot convert undefined or null to object) leftCURRENT_CALL_LOCATIONstale, so the renderedat <file>:<line>pointed at the last tracked call — in a different file.Fix: the HIR object-pattern lowering (
pattern_binding.rs) now carries the pattern's source byte offset as a second literal arg to therequireObjectCoercibleruntime call. Codegen (lower_call/native/mod.rs) emitsjs_set_call_locationimmediately beforejs_require_object_coercible— but only under--debug-symbols(default codegen readsargs.first()only, so the extra arg is inert).at safe-stable-stringify/index.js:272(stale, wrong file)at node_modules/pino/pino.js:23(theconst { …syms… } = symbolsdestructure — matches Node's intuition: Node points at pino.js:23 too)Not done (deferred): matching Node's richer message
Cannot destructure property 'a' of 'x' as it is undefined. Doing so cheaply isn't possible —js_require_object_coercibleis shared across empty patterns and assignment-destructuring and has no property/source-name context; it would need a new per-destructure runtime entry point threading the property name + source identifier text. Left as-is (generic ToObject message); the location is now correct.Gap 2 — CJS-wrap coordinate skew (ajv)
A CommonJS module is parsed as WRAPPED text (
wrap_commonjs_for_targetinjects an IIFE +module/exports/requirepreamble), so HIRbyte_offsets are in wrapped coordinates — butmodule_sourcewas read fresh from disk (the ORIGINAL file), shifting every resolved line by the preamble length (ajv also rendered<anonymous>).Fix: under
--debug-symbols,collect_modulesrecords per CJS-wrapped module (ctx.cjs_wrap_debug_sources): the wrapped source + the wrapper-prefix line count (newlines before the original body, located via the newwrap_commonjs_with_body_offset; blanking/hoisting are newline-preserving so body line offsets are stable, and thecreate_requireprepend is accounted for by a line delta). Codegen resolves offsets against the WRAPPED source then subtracts the prefix line count (StringPool::call_location_for+debug_source_line_offsetonCompileOptions), recovering original-source lines. Offsets inside the preamble resolve to no location.undefined is not a constructorat<anonymous>at node_modules/ajv/dist/compile/codegen/scope.js:17(the realnew code_1.Name(...)site)Minimal CJS repro: a
node_modules/xmodule throwing on line 5 → perry reportsindex.js:5(matches Node); the wrapped line was 106 (skew ~101 lines).Default path unchanged
--debug-symbols; the map is only populated then.no_call_location_without_debug_symbolsasserts nojs_set_call_locationCALL is emitted by default.PERRY_BIN=… python3 run.py, no--debug-symbols): 53/59 — no regressions (the 6 failures are pre-existing unrelated gaps: ajv/pino/bluebird/execa/semver/winston).Tests added
strings.rsunit tests: prefix-line subtraction + preamble→None.cjs_wrapunit test:wrap_commonjs_with_body_offsetmaps a wrapped body line back to its original-source line.destructure_call_location.rscodegen IR-assertion test: location call precedes the coercibility check under--debug-symbols, none by default.Version / changelog
Per contributor convention, this PR does NOT bump
Cargo.tomlversion,CLAUDE.mdCurrent Version,CHANGELOG.md, orCargo.lock. Maintainer to fold in metadata at merge.Summary by CodeRabbit
Release Notes