feat(error): source location on construct + ReferenceError throws (#5253)#5274
Open
proggeramlug wants to merge 1 commit into
Open
feat(error): source location on construct + ReferenceError throws (#5253)#5274proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
) Follow-up to #5247/#5250. Extends the `--debug-symbols` runtime source-location pipeline (which #5250 gave the dynamic call-dispatch "X is not a function" throw) to two more throw classes: 1. `new X()` "X is not a constructor" — adds `byte_offset` to `Expr::New`, `Expr::NewDynamic`, and `Expr::NewDynamicSpread`, set from the AST `new_expr.span.lo.0` at lowering. Codegen calls `emit_call_location_at` before the `js_new_function_construct{,_apply}` / `js_throw_not_a_constructor` dispatch. `const X: any = undefined; new X()` lowers to `NewDynamic` (callee `LocalGet`), so the offset lands on that variant — localizing ajv's `undefined is not a constructor`. 2. `ReferenceError: X is not defined` — the bare unresolved-identifier read already lowers to a `Call` into `js_global_get_or_throw_unresolved`; that Call now carries `ident.span.lo.0` as its `byte_offset`, and `lower_call` emits `js_set_call_location` before the helper call. Localizes winston's `module is not defined`. The runtime needs no change: both throw paths already route through `alloc_error` → `make_stack`, which reads `CURRENT_CALL_LOCATION`. Default behavior is unchanged (no debug-location ctx installed → no-op; offset 0 → `<anonymous>`). `byte_offset` is excluded from the stable hash (like `Call.byte_offset`) so whitespace edits don't bust the object cache. All other New construction sites (transforms, intrinsics, synthesized news) pass `byte_offset: 0`. Extracted `module_constructor_name` / `global_member_constructor_name` from `expr_new.rs` into a new `expr_new_builtins.rs` to keep the file under the 2000-line cap after the field additions. Tests: new `issue_5253_construct_reference_source_location.rs` (4 cases: not-a-constructor + ReferenceError, each WITH/WITHOUT `--debug-symbols`). #5247/#5250 tests stay green. No version/CHANGELOG/Cargo.lock edits (maintainer folds in at merge).
📝 WalkthroughWalkthroughAdds a ChangesIssue
Sequence Diagram(s)sequenceDiagram
participant AST as AST (new expr)
participant lower_new as lower_new
participant HIR as Expr::New / NewDynamic / NewDynamicSpread
participant codegen as new_dynamic codegen
participant runtime as JS runtime throw helper
AST->>lower_new: new_expr.span.lo.0 → new_byte_offset
lower_new->>HIR: construct with byte_offset = new_byte_offset
HIR->>codegen: read byte_offset → new_byte_offset
codegen->>codegen: emit_call_location_at(ctx, new_byte_offset)
codegen->>runtime: js_throw_not_a_constructor / js_new_function_construct
runtime-->>AST: TypeError at file:line (with --debug-symbols)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-hir/src/analysis.rs (1)
1409-1414:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
NewDynamicSpreadin these ad-hoc traversals too.These updates make
NewDynamicaccept the new field, butNewDynamicSpreadstill falls through to_ => {}. That skips assignments andthisrewrites insidenew <callee>(...args)spread constructor expressions.Proposed fix
Expr::NewDynamic { callee, args, .. } => { collect_assigned_locals_expr(callee, assigned); for arg in args { collect_assigned_locals_expr(arg, assigned); } } + Expr::NewDynamicSpread { callee, args, .. } => { + collect_assigned_locals_expr(callee, assigned); + for arg in args { + match arg { + CallArg::Expr(e) | CallArg::Spread(e) => { + collect_assigned_locals_expr(e, assigned) + } + } + } + }Expr::NewDynamic { callee, args, .. } => { replace_this_in_expr(callee, this_id); for a in args { replace_this_in_expr(a, this_id); } } + Expr::NewDynamicSpread { callee, args, .. } => { + replace_this_in_expr(callee, this_id); + for a in args { + match a { + CallArg::Expr(e) | CallArg::Spread(e) => replace_this_in_expr(e, this_id), + } + } + }Also applies to: 2056-2061
🤖 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/analysis.rs` around lines 1409 - 1414, The `collect_assigned_locals_expr` function currently handles `Expr::NewDynamic` by collecting assigned locals from the callee and arguments, but `Expr::NewDynamicSpread` falls through to the default case which does nothing, causing assignments and this rewrites inside spread constructor expressions to be skipped. Add a handler for `NewDynamicSpread` that mirrors the `NewDynamic` handler by collecting assigned locals from both the callee and args in the same way, and apply this same pattern to all other locations in the file where similar ad-hoc traversals exist that handle `NewDynamic` but not `NewDynamicSpread`.crates/perry-hir/src/js_transform/imports.rs (1)
708-729:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
Expr::NewDynamicSpreadalongsideExpr::NewDynamic. The HIR now distinguishes spread constructors, but these recursive passes still only matchNewDynamic, so children undernew Foo(...args)fall through the wildcard and get skipped.
crates/perry-hir/src/js_transform/imports.rs#L708-L729: add aNewDynamicSpreadarm here so import rewriting still recurses through spread constructor args.crates/perry-hir/src/lower/closure_analysis.rs#L249-L254: add the spread arm inwiden_mutable_captures_expr.crates/perry-hir/src/lower/closure_analysis.rs#L557-L562: add the spread arm incollect_closure_assigned_expr.crates/perry-hir/src/lower/closure_analysis.rs#L1284-L1289: add the spread arm incollect_closure_assigned_in_body_expr.crates/perry-hir/src/js_transform/cross_module_natives.rs#L908-L913: add the spread arm here, and mirror it in the sibling scan helper above.🤖 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/js_transform/imports.rs` around lines 708 - 729, The transform functions are not handling the NewDynamicSpread variant of constructor expressions, causing arguments in spread constructor calls like new Foo(...args) to be skipped during traversal. Add NewDynamicSpread match arms alongside NewDynamic in five locations: (1) in crates/perry-hir/src/js_transform/imports.rs lines 708-729 within the match statement that transforms constructors for import rewriting, (2) in crates/perry-hir/src/lower/closure_analysis.rs lines 249-254 in the widen_mutable_captures_expr function, (3) in crates/perry-hir/src/lower/closure_analysis.rs lines 557-562 in the collect_closure_assigned_expr function, (4) in crates/perry-hir/src/lower/closure_analysis.rs lines 1284-1289 in the collect_closure_assigned_in_body_expr function, and (5) in crates/perry-hir/src/js_transform/cross_module_natives.rs lines 908-913 where you should add the spread arm and mirror it in the sibling scan helper. Each NewDynamicSpread arm should recursively process the callee and args in the same way as NewDynamic to ensure spread constructors are properly analyzed.
🧹 Nitpick comments (1)
crates/perry/tests/issue_5253_construct_reference_source_location.rs (1)
135-145: ⚡ Quick winTighten default-build assertions to require
<anonymous>fallback.Line 142 and Line 185 only check that
at main.ts:is absent. To match the stated regression contract, assert<anonymous>is present as well.Suggested patch
#[test] fn default_build_keeps_anonymous_frame_for_not_a_constructor() { @@ assert!( !stdout.contains("at main.ts:"), "default build must NOT emit a source location; got:\n{stdout}" ); + assert!( + stdout.contains("<anonymous>"), + "default build should keep an anonymous frame; got:\n{stdout}" + ); } @@ #[test] fn default_build_keeps_anonymous_frame_for_reference_error() { @@ assert!( !stdout.contains("at main.ts:"), "default build must NOT emit a source location; got:\n{stdout}" ); + assert!( + stdout.contains("<anonymous>"), + "default build should keep an anonymous frame; got:\n{stdout}" + ); }Also applies to: 178-188
🤖 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/tests/issue_5253_construct_reference_source_location.rs` around lines 135 - 145, The assertions in default_build_keeps_anonymous_frame_for_not_a_constructor are incomplete. Currently they only verify that at main.ts is absent, but to properly validate the regression contract, they must also assert that the <anonymous> fallback is present in the output. Add a second assertion using stdout.contains to verify that <anonymous> appears in stdout. Apply the same tightened assertion pattern to the other test function mentioned in the comment (the sibling location) to ensure both tests explicitly validate the presence of the <anonymous> fallback rather than just the absence of source locations.
🤖 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/tests/issue_5253_construct_reference_source_location.rs`:
- Around line 98-100: The test at lines 98-100 runs the compiled binary and
captures stdout but does not verify that the command executed successfully
before processing the output. Add a check for `run.status.success()` after the
Command output is obtained and before converting the stdout to a String. This
ensures that if the binary exits with a non-zero status code, the test fails
immediately rather than attempting to validate potentially incomplete or
corrupted output, which could mask regressions.
---
Outside diff comments:
In `@crates/perry-hir/src/analysis.rs`:
- Around line 1409-1414: The `collect_assigned_locals_expr` function currently
handles `Expr::NewDynamic` by collecting assigned locals from the callee and
arguments, but `Expr::NewDynamicSpread` falls through to the default case which
does nothing, causing assignments and this rewrites inside spread constructor
expressions to be skipped. Add a handler for `NewDynamicSpread` that mirrors the
`NewDynamic` handler by collecting assigned locals from both the callee and args
in the same way, and apply this same pattern to all other locations in the file
where similar ad-hoc traversals exist that handle `NewDynamic` but not
`NewDynamicSpread`.
In `@crates/perry-hir/src/js_transform/imports.rs`:
- Around line 708-729: The transform functions are not handling the
NewDynamicSpread variant of constructor expressions, causing arguments in spread
constructor calls like new Foo(...args) to be skipped during traversal. Add
NewDynamicSpread match arms alongside NewDynamic in five locations: (1) in
crates/perry-hir/src/js_transform/imports.rs lines 708-729 within the match
statement that transforms constructors for import rewriting, (2) in
crates/perry-hir/src/lower/closure_analysis.rs lines 249-254 in the
widen_mutable_captures_expr function, (3) in
crates/perry-hir/src/lower/closure_analysis.rs lines 557-562 in the
collect_closure_assigned_expr function, (4) in
crates/perry-hir/src/lower/closure_analysis.rs lines 1284-1289 in the
collect_closure_assigned_in_body_expr function, and (5) in
crates/perry-hir/src/js_transform/cross_module_natives.rs lines 908-913 where
you should add the spread arm and mirror it in the sibling scan helper. Each
NewDynamicSpread arm should recursively process the callee and args in the same
way as NewDynamic to ensure spread constructors are properly analyzed.
---
Nitpick comments:
In `@crates/perry/tests/issue_5253_construct_reference_source_location.rs`:
- Around line 135-145: The assertions in
default_build_keeps_anonymous_frame_for_not_a_constructor are incomplete.
Currently they only verify that at main.ts is absent, but to properly validate
the regression contract, they must also assert that the <anonymous> fallback is
present in the output. Add a second assertion using stdout.contains to verify
that <anonymous> appears in stdout. Apply the same tightened assertion pattern
to the other test function mentioned in the comment (the sibling location) to
ensure both tests explicitly validate the presence of the <anonymous> fallback
rather than just the absence of source locations.
🪄 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: 32291315-c35b-4582-bbaf-f455e46b0b0e
📒 Files selected for processing (29)
crates/perry-codegen-js/src/emit/exprs.rscrates/perry-codegen-wasm/src/emit/expr/classes.rscrates/perry-codegen-wasm/src/emit/string_collection.rscrates/perry-codegen/src/collectors/escape_check.rscrates/perry-codegen/src/collectors/escape_news.rscrates/perry-codegen/src/collectors/refs.rscrates/perry-codegen/src/collectors/this_as_value.rscrates/perry-codegen/src/expr/new_dynamic.rscrates/perry-codegen/src/lower_call/mod.rscrates/perry-hir/src/analysis.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/js_transform/cross_module_natives.rscrates/perry-hir/src/js_transform/imports.rscrates/perry-hir/src/lower/closure_analysis.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/expr_new_builtins.rscrates/perry-hir/src/lower/expr_object.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/mod.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/monomorph/driver.rscrates/perry-hir/src/monomorph/substitute_expr.rscrates/perry-hir/src/monomorph/update_call_sites.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry/tests/issue_5253_construct_reference_source_location.rs
Comment on lines
+98
to
+100
| let bin = root.join("main_bin"); | ||
| let run = Command::new(&bin).output().expect("run compiled binary"); | ||
| String::from_utf8_lossy(&run.stdout).into_owned() |
There was a problem hiding this comment.
Assert fixture process success before validating stdout.
At Line 99, the test runs main_bin but never checks run.status.success(). A non-zero exit can still produce partial stdout and hide regressions.
Suggested patch
fn run_fixture(fixture: &str, extra_args: &[&str]) -> String {
@@
let bin = root.join("main_bin");
let run = Command::new(&bin).output().expect("run compiled binary");
+ assert!(
+ run.status.success(),
+ "compiled binary must exit successfully (args {extra_args:?}); stderr:\n{}",
+ String::from_utf8_lossy(&run.stderr)
+ );
String::from_utf8_lossy(&run.stdout).into_owned()
}🤖 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/tests/issue_5253_construct_reference_source_location.rs` around
lines 98 - 100, The test at lines 98-100 runs the compiled binary and captures
stdout but does not verify that the command executed successfully before
processing the output. Add a check for `run.status.success()` after the Command
output is obtained and before converting the stdout to a String. This ensures
that if the binary exits with a non-zero status code, the test fails immediately
rather than attempting to validate potentially incomplete or corrupted output,
which could mask regressions.
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.
Follow-up to #5247/#5250. Extends the
--debug-symbolsruntime source-location pipeline — which #5250 gave the dynamic call-dispatch "X is not a function" throw anat <file>:<line>frame — to two more throw classes. Default behavior is unchanged; only--debug-symbolsbuilds gain the location.Throw classes now covered
new X()→ "X is not a constructor" — localizes ajv'sundefined is not a constructor.ReferenceError: X is not defined/module is not defined— localizes winston'smodule is not defined.What changed
New/NewDynamic/NewDynamicSpreadbyte_offsetbyte_offset: u32toExpr::New,Expr::NewDynamic,Expr::NewDynamicSpread(crates/perry-hir/src/ir/expr.rs), set fromnew_expr.span.lo.0inexpr_new.rs::lower_new. Key nuance:const X: any = undefined; new X()lowers toExpr::NewDynamic(calleeLocalGet), so the offset must live on that variant — which is exactly the ajv shape.stable_hash/expr.rs), mirroringCall.byte_offset(Diagnostics: runtime errors have no source location (at <anonymous>) — add file:line to throws #5247), so whitespace edits don't bust the object cache.byte_offset: 0. Walkers/collectors/JS+WASM+ArkTS backends updated to ignore the new field.expr/new_dynamic.rs) callsemit_call_location_atright before eachjs_new_function_construct{,_apply}/js_throw_not_a_constructordispatch (theNew,NewDynamicprimitive-literal + routed + fallback,NewDynamicSpread, and ternary-synthesized arms).ReferenceError path
Call { callee: ExternFuncRef("js_global_get_or_throw_unresolved"), args: [name] }. That Call now carriesident.span.lo.0asbyte_offset(lower_expr.rs), andlower_call(lower_call/mod.rs) emitsjs_set_call_locationfrom the pending offset before that specific extern call.Runtime
alloc_error→make_stack, which readsCURRENT_CALL_LOCATION. They light up once codegen sets the location.Line-cap housekeeping
expr_new.rspast the 2000-line cap; extracted the leaf helpersmodule_constructor_name/global_member_constructor_nameinto a newexpr_new_builtins.rs(pure mechanical move).expr_new.rsis now 1968 lines.Test evidence
New
crates/perry/tests/issue_5253_construct_reference_source_location.rs(4 cases): not-a-constructor and ReferenceError, each WITH and WITHOUT--debug-symbols. All pass; #5247/#5250 tests stay green.Manual
.tsfixtures (WITH--debug-symbols):ajv / winston localization (payoff)
Compiled from
secret-tests/corpuswithcompilePackages:["*"]+--debug-symbols:ReferenceError: module is not definedatnode_modules/@colors/colors/lib/custom/trap.js:1— localized.TypeError: undefined is not a constructorat<anonymous>.Deferred / known limitation
ajv's not-a-constructor throw is inside a CJS-wrapped module. The byte offset on the
newnode is in wrapped-source coordinates (verified via--print-hir:NewDynamic { byte_offset: 4849 }) whilemodule_source(consulted bycall_location_for) holds the original unwrapped file, so the offset is out of range →<anonymous>. This is a pre-existing limitation shared with #5250 — the call-dispatch path shows<anonymous>for.js/CJS modules too (confirmed). Mapping CJS-wrap coordinates back to original source is a separate follow-up; this PR's instrumentation is correct and complete and works for.ts/ESM modules (winston'smodulethrow happens to be in an un-wrapped module, hence it localizes).Version / CHANGELOG
Per the contributor guidance, this PR does not touch
[workspace.package] version, theCurrent Versionline,CHANGELOG.md, orCargo.lock— maintainer folds those in at merge.Summary by CodeRabbit
Release Notes
--debug-symbolsis enabled, eliminating anonymous stack frames for better debugging experience.