feat(error): source location on dynamic call-dispatch TypeErrors (#5247)#5250
Merged
Conversation
First bounded increment of #5247: give the dynamic call-dispatch throw path ("X is not a function") a real source `file:line` in the thrown TypeError's `.stack`, gated behind `--debug-symbols` so the default (release) build is byte-identical and incurs no per-call overhead. Pipeline (byte_offset -> line table -> stack): 1. HIR: `Expr::Call` gains a `byte_offset: u32` (`call.span.lo.0`), set at AST->HIR lowering (mirrors #5230's DynamicImport.byte_offset). 0 sentinel for synthesized/transform/intrinsic calls. Excluded from the stable hash (diagnostic-only) so source-offset shifts don't bust the object cache. 2. Codegen: `StringPool` carries an optional per-module debug-location context (file path + source) installed only under --debug-symbols, and resolves a byte offset -> 1-based line via `call_location_for`. The `Expr::Call` dispatcher records the pending offset; the dynamic method-dispatch emission sites (typed-feedback + universal `js_native_call_method`) emit `js_set_call_location(file,line)` immediately before the throwing dispatch (after args lower, so a nested-call arg's location no longer shadows the outer call's). 3. Runtime: a thread-local CURRENT_CALL_LOCATION (set by the new `js_set_call_location` FFI) is read by `make_stack` to render `at <file>:<line>`; falls back to `at <anonymous>` when unset. Flag: reuses --debug-symbols (CompileArgs.debug_symbols -> CompileOptions.debug_locations + module_source). Added to the object-cache key so toggling it invalidates cached .o. Tests: byte->line unit tests (boundaries, first/last line, zero-offset, no-context, UTF-8 safety); integration issue_5247 (`f.test()` -> with the flag `.stack` contains `at main.ts:4`; without it stays `<anonymous>`); cache-key toggle test. Deferred (noted for follow-up): multi-frame stacks; the other throw sites (`new X` not-a-constructor, string-typed unknown-method catch-all, proxy/reflect, collection-iter); clearing a stale location for an offset-0 synthesized call that throws. NOTE (maintainer): no version/CHANGELOG/Cargo.lock bump per contributor guidance — please fold in at merge.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds ChangesSource location for dynamic-dispatch TypeErrors
Sequence Diagram(s)sequenceDiagram
participant CLI as perry CLI
participant Compile as compile.rs
participant StringPool
participant lower_expr as lower_expr (Expr::Call)
participant dispatch as property_get / console_promise
participant emit as emit_call_location_at
participant Runtime as js_set_call_location
participant make_stack
CLI->>Compile: --debug-symbols flag
Compile->>StringPool: set_debug_location_ctx(file, source)
lower_expr->>StringPool: set_pending_call_offset(byte_offset)
dispatch->>StringPool: pending_call_offset() → call_byte_offset
dispatch->>emit: emit_call_location_at(ctx, call_byte_offset)
emit->>StringPool: call_location_for(offset) → (file, line)
emit->>Runtime: js_set_call_location(file, line)
Note over Runtime: TypeError thrown by dynamic dispatch
make_stack->>Runtime: current_stack_frame()
Runtime-->>make_stack: "at main.ts:4"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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/calls.rs`:
- Around line 63-69: The early return in the else block (when call_location_for
returns None) skips the runtime clear path, allowing stale location state from
prior dispatches to persist. Before returning when
call_location_for(byte_offset) fails to resolve a location, invoke the runtime
clear operation (the same cleanup that executes in the normal path) to reset the
call location state to its default anonymous value, ensuring offsetless or
synthesized calls do not inherit stale file:line information from previous
operations.
In `@crates/perry-runtime/src/error.rs`:
- Around line 109-116: The thread-local storage location is being set via the
function containing the diff but is never cleared after being consumed, causing
stale file:line data to leak to subsequent unrelated errors. After the call
location is read and used for stack rendering (which occurs at the location
referenced as "Also applies to: 126-131"), clear the thread-local storage by
setting it back to None to prevent stale frame data from persisting across
multiple error contexts.
In `@crates/perry/src/commands/compile/object_cache.rs`:
- Around line 168-173: The cache key generation for debug locations (the dbgloc
field) only considers whether the debug_locations flag is enabled, but doesn't
include information about the actual source text content. This allows stale
cached objects to be reused when the source code changes but the HIR remains
identical, causing incorrect file:line information to be reported. Include a
fingerprint of the module_source in the cache key when opts.debug_locations is
true, so that source-text edits properly invalidate the cached object and
prevent serving stale results with incorrect location data.
🪄 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: c840a7c9-da63-4c8f-b377-5d80541ab6cc
📒 Files selected for processing (70)
crates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/collectors/pointer_locals.rscrates/perry-codegen/src/expr/calls.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-codegen/src/strings.rscrates/perry-codegen/tests/class_keys_gc_root.rscrates/perry-codegen/tests/constructor_recursion.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/assignment_stmt.rscrates/perry-hir/src/dynamic_import/tests.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/jsx.rscrates/perry-hir/src/lower/array_fold.rscrates/perry-hir/src/lower/const_fold_fn.rscrates/perry-hir/src/lower/decorators.rscrates/perry-hir/src/lower/eval_super_scan.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/expr_call/crypto.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/intrinsics.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/expr_call/module_class_static.rscrates/perry-hir/src/lower/expr_call/module_static.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/expr_call/post_args_dispatch.rscrates/perry-hir/src/lower/expr_call/url_date_instance.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-hir/src/lower/expr_misc.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/expr_object.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/stmt_loops.rscrates/perry-hir/src/lower/widget_decl.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/body_stmt/detect.rscrates/perry-hir/src/lower_decl/body_stmt/for_await.rscrates/perry-hir/src/lower_decl/class_computed.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/helpers.rscrates/perry-hir/src/monomorph/driver.rscrates/perry-hir/src/monomorph/substitute_expr.rscrates/perry-hir/src/monomorph/tests.rscrates/perry-hir/src/monomorph/update_call_sites.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-runtime/src/error.rscrates/perry-transform/src/deforest/call_sites.rscrates/perry-transform/src/deforest/tests.rscrates/perry-transform/src/generator/helpers.rscrates/perry-transform/src/generator/linearize.rscrates/perry-transform/src/generator/lower.rscrates/perry-transform/src/state_desugar.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/tests/issue_5247_runtime_error_source_location.rs
Comment on lines
+63
to
+69
| let Some((file, line)) = ctx | ||
| .strings | ||
| .call_location_for(byte_offset) | ||
| .map(|(f, l)| (f.to_string(), l)) | ||
| else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Clear stale call-location state when no location is resolvable.
On Line 67, returning early skips the runtime clear path. If a prior dispatch set a location, later offsetless/synthesized calls can inherit that stale <file>:<line> instead of falling back to <anonymous>.
Suggested fix
pub(crate) fn emit_call_location_at(ctx: &mut FnCtx<'_>, byte_offset: u32) {
let Some((file, line)) = ctx
.strings
.call_location_for(byte_offset)
.map(|(f, l)| (f.to_string(), l))
else {
+ // Clear any previously latched call-site location.
+ ctx.block().call_void(
+ "js_set_call_location",
+ &[(PTR, "null"), (I64, "0"), (I32, "0")],
+ );
return;
};🤖 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/calls.rs` around lines 63 - 69, The early
return in the else block (when call_location_for returns None) skips the runtime
clear path, allowing stale location state from prior dispatches to persist.
Before returning when call_location_for(byte_offset) fails to resolve a
location, invoke the runtime clear operation (the same cleanup that executes in
the normal path) to reset the call location state to its default anonymous
value, ensuring offsetless or synthesized calls do not inherit stale file:line
information from previous operations.
Comment on lines
+109
to
+116
| if line == 0 || file_ptr.is_null() || file_len == 0 { | ||
| CURRENT_CALL_LOCATION.with(|c| *c.borrow_mut() = None); | ||
| return; | ||
| } | ||
| let bytes = std::slice::from_raw_parts(file_ptr, file_len); | ||
| let file = String::from_utf8_lossy(bytes).into_owned(); | ||
| CURRENT_CALL_LOCATION.with(|c| *c.borrow_mut() = Some((file, line))); | ||
| } |
There was a problem hiding this comment.
Consume call location after first stack render to avoid stale frame leakage
At Lines 126-131, the TLS location is read but never cleared. After one instrumented call site sets it, later unrelated errors can inherit stale file:line data.
Suggested fix
fn current_stack_frame() -> String {
- CURRENT_CALL_LOCATION.with(|c| match &*c.borrow() {
- Some((file, line)) => format!(" at {}:{}", file, line),
- None => " at <anonymous>".to_string(),
- })
+ CURRENT_CALL_LOCATION.with(|c| {
+ let mut slot = c.borrow_mut();
+ match slot.take() {
+ Some((file, line)) => format!(" at {}:{}", file, line),
+ None => " at <anonymous>".to_string(),
+ }
+ })
}Also applies to: 126-131
🤖 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/error.rs` around lines 109 - 116, The thread-local
storage location is being set via the function containing the diff but is never
cleared after being consumed, causing stale file:line data to leak to subsequent
unrelated errors. After the call location is read and used for stack rendering
(which occurs at the location referenced as "Also applies to: 126-131"), clear
the thread-local storage by setting it back to None to prevent stale frame data
from persisting across multiple error contexts.
Comment on lines
+168
to
173
| // #5247: `--debug-symbols` flips per-call `js_set_call_location` emission, | ||
| // which changes the emitted IR (and `.o` bytes). Without this in the key, | ||
| // toggling the flag would serve the previously-cached object and the | ||
| // source locations would silently not appear. | ||
| h.field("dbgloc", if opts.debug_locations { "1" } else { "0" }); | ||
| h.field( |
There was a problem hiding this comment.
Include source-text fingerprint in cache key when debug locations are enabled
At Line 172, dbgloc invalidates only on flag toggles, but debug-location output also depends on module_source line layout. Source-only edits that keep HIR identical can currently reuse stale cached objects and report incorrect file:line.
Suggested fix
h.field("dbgloc", if opts.debug_locations { "1" } else { "0" });
+ if opts.debug_locations {
+ // Debug-location emission depends on source layout (byte->line mapping).
+ let src_hash = opts
+ .module_source
+ .as_ref()
+ .map(|s| format!("{:016x}", djb2_hash(s.as_bytes())))
+ .unwrap_or_else(|| "none".to_string());
+ h.field("dbgloc_src", &src_hash);
+ }🤖 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 cache key generation for debug locations (the dbgloc field) only considers
whether the debug_locations flag is enabled, but doesn't include information
about the actual source text content. This allows stale cached objects to be
reused when the source code changes but the HIR remains identical, causing
incorrect file:line information to be reported. Include a fingerprint of the
module_source in the cache key when opts.debug_locations is true, so that
source-text edits properly invalidate the cached object and prevent serving
stale results with incorrect location data.
Formats four files flagged by the cargo fmt --check lint gate: - perry-codegen/src/strings.rs (this PR's change) - perry-stdlib/src/readline.rs, perry/.../collect_modules.rs, perry/.../collect_modules/wasm_asset.rs (inherited from main — merged during the CI window where the Tests workflow was in startup_failure and lint never ran).
proggeramlug
added a commit
that referenced
this pull request
Jun 16, 2026
…set after #5250) (#5266) * test(hir): add byte_offset to Expr::Call inits in cheerio_call_rewrite (fix main test compile after #5250) #5250 added Expr::Call.byte_offset but missed this integration test → cargo test -p perry-hir failed to compile on main. Add byte_offset: 0 to the 3 Expr::Call constructions. * test(hir): rustfmt — indent byte_offset in loop-body Expr::Call (fix lint) --------- Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
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.
#5247 — source location on dynamic call-dispatch TypeErrors (first increment)
Perry runtime errors carried no source location:
make_stackhardcodedat <anonymous>, so a native-compiled npm package's runtimeTypeError("X is not a function") couldn't be localized.
This is the bounded first increment: the dynamic call-dispatch throw path
("X is not a function") now carries a real
file:linein the thrownTypeError's
.stack, gated behind--debug-symbols. Default (release) buildsare byte-identical with no per-call overhead.
Pipeline (byte_offset → line table → stack)
Expr::Callgainsbyte_offset: u32set fromcall.span.lo.0atAST→HIR lowering (follows AOT: defer non-resolvable dynamic import() to a runtime error (shared notice + strict), like #5206 #5230's
DynamicImport.byte_offsetprecedent).0for synthesized/transform/intrinsic calls. Excluded from the stable hash
(diagnostic-only) so source-offset shifts don't bust the object cache.
StringPoolcarries an optional per-module debug-locationcontext (file path + source text) installed only under
--debug-symbols, andresolves
byte_offset → 1-based lineviacall_location_for. TheExpr::Calldispatcher records the pending offset; the dynamic method-dispatch emission
sites (
js_typed_feedback_native_call_method+ universaljs_native_call_method) emitjs_set_call_location(file, line)immediatelybefore the throwing dispatch — after the call's args are lowered, so a
nested-call argument's location no longer shadows the outer call's.
CURRENT_CALL_LOCATION(set by the newjs_set_call_locationFFI) is read bymake_stack→at <file>:<line>;falls back to
at <anonymous>when unset.Which throws carry location
(number/string/...).method is not a functionand barevalue is not a functionvia the dynamic method dispatch (js_native_call_methodand thetyped-feedback wrapper) — the highest-value path (npm-package method calls on
any/primitive receivers).Flag
Reuses
--debug-symbols(CompileArgs.debug_symbols→CompileOptions.debug_locations+module_source). Default OFF. Added to theobject-cache key (
dbgloc) so toggling invalidates cached.o.Tests
StringPool::call_location_for): line boundaries, first/last line,zero-offset sentinel, no-context (default build), UTF-8 safety.
issue_5247_runtime_error_source_location):const f: any = 5; f.test();→ WITH--debug-symbolsthe.stackcontainsat main.ts:4and no
<anonymous>; WITHOUT the flag it staysat <anonymous>with nofile:line.debug_locationschanges the object-cache key.Test evidence (local, macOS): byte→line unit tests 6/6; codegen lib 103/103;
perry-hir lib 148/148; perry-transform lib 34/34; perry-runtime
error::4/4;integration 2/2; cache-key toggle 1/1 — all green.
f.test()resultajv localization (payoff check)
new Ajv().compile({...})under--debug-symbols:The
(number).testsentinel bug now localizes to the exact compiled-packagefile:line (reported only — not fixed here).
Deferred (follow-up)
new Xnot-a-constructor, the string-typed unknown-methodcatch-all (
lower_string_method.rs), proxy/reflect, collection-iter.throws (currently the previous located frame persists for such a throw).
Version / CHANGELOG note (maintainer)
No
[workspace.package] version,CLAUDE.mdCurrent Version,CHANGELOG.md, orCargo.lockedits — per contributor guidance, please fold the version bump +changelog entry in at merge.
Summary by CodeRabbit
Release Notes
file:line) when compiled with the--debug-symbolsflag, providing better error diagnostics instead of generic location markers.