Repository navigation
Conversation
A SyntaxError raised by JavaScriptCore's own parser (strict-mode 'with', 'delete x', invalid regex literals, etc.) during module load has no JS stack frames: the error is created before any JS runs. addErrorInfo() records line and sourceURL on the ErrorInstance's C++ fields, but materializeErrorInfoIfNeeded() only publishes them as .line/.sourceURL JS properties when a stack trace exists, so the error printer found nothing and emitted a bare 'SyntaxError: ...' with no file, line, or code frame. fromErrorInstance() now reads err->sourceURL()/err->line() from the C++ fields directly and synthesizes a frame when no stack is available. JSC never records the parser-error column (column is always 0), and a source-map lookup at column 0 resolves to bun's own start-of-line mapping for the previous source line when the offending statement is indented. Both the new path and the existing <parse> frame in formatStackTrace() now query past end-of-line so the lookup lands on the last mapping of the generated line, which reliably resolves to the correct source line. Fixes #5192
|
Status: reproduced, fix verified locally, review threads resolved.
CI (build 86698):
None of these touch the error-printer surface this PR changes. Ready for a maintainer. PR: #36634 |
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 2:56 AM PT - Aug 1st, 2026
❌ @robobun, your commit 02618d2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36634That installs a local version of the PR into your bun-36634 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found. Traced the INT32_MAX column trick through Mapping::find_index_from_generated (src/sourcemap/Mapping.rs:156) — it's a lower-bound binary search, so "past end-of-line" resolves to the last mapping on the generated line as intended, and remap_stack_frame_positions calls resolve_source_mapping with NoSourceContents so the sentinel column never reaches the ParseUrlResultHint::All hint. Deferring to a human because this is a cross-language change to shared error-formatting paths (fromErrorInstance, formatStackTrace) that every printed error flows through, and the past-EOL remap approach is worth a maintainer's eyes.
Extended reasoning...
Overview
The PR touches three C++ files in src/jsc/bindings/ and adds a regression test. It fixes #5192: JSC-parser SyntaxErrors (strict-mode with, delete x, etc.) printed with no file/line because the error has no JS stack and materializeErrorInfoIfNeeded() never published .line/.sourceURL to the JS side. The fix reads err->sourceURL()/err->line() from the C++ fields directly in fromErrorInstance() and synthesizes a single frame; a new shared helper Bun__remapParseErrorFrame handles the source-map lookup for both this new path and the existing <parse> frame in formatStackTrace(). Because JSC's addErrorInfo() records column 0 for parser errors, the helper queries at column INT32_MAX so the lookup lands on the last mapping of the generated line, then reports column 1.
Security risks
None. This is diagnostic-output formatting only; no untrusted-input parsing, allocation sizing, or privilege boundaries are affected.
Level of scrutiny
Moderate-to-high. The change is small in line count but sits in shared error-formatting code that every printed error flows through (fromErrorInstance, formatStackTrace), spans the C++/Rust FFI boundary, and relies on a specific implementation detail of the source-map lookup (that find_index_from_generated is a "last mapping ≤ target" binary search returning None only when no mapping exists on the target line). I traced that through src/sourcemap/Mapping.rs:156-185 and it holds; remap_stack_frame_positions in VirtualMachine.rs calls resolve_source_mapping with NoSourceContents, so the INT32_MAX column never reaches the ParseUrlResultHint::All { column, .. } hint path. The is_invalid() gate in the Rust batch loop requires all three position fields to be −1, and the helper sets line/column to non-negative values, so remapping proceeds.
Other factors
Test coverage is thorough (7 concurrent subprocess tests covering entry vs. static import vs. dynamic import vs. require(), .mjs/.ts/.cjs, and the indented-statement off-by-one that motivated the INT32_MAX trick). The comment-cop bot flags on long comments were addressed in follow-up commits (53e3fe6c). My earlier nit about test-file placement was answered — the author confirmed #5192 did work in bun 1.0.1 and later regressed, so test/regression/issue/ is correct. I'm deferring rather than approving because the past-EOL remap heuristic and the new early-return branch in fromErrorInstance (which now short-circuits the JS-property fallback whenever the C++ sourceURL() is non-empty) are the kind of behavior change a maintainer should confirm doesn't affect adjacent error shapes.
There was a problem hiding this comment.
No bugs found, but this touches JSC error-printer C++ with a source-map heuristic that warrants a human look — specifically the INT32_MAX column trick in Bun__remapParseErrorFrame, which relies on the Rust remapper clamping to the last mapping on the generated line, and which also changes the existing <parse>-frame path in formatStackTrace() (previously looked up column 0).
What was reviewed:
fromErrorInstancenew branch:frames_ptr[0]write matches the sibling JS-property fallback (samederef()/frames_len=1pattern, noframes_capcheck needed).err->sourceURL()/line()/column()are plain C++ field reads, no JS re-entry or exception scope needed.- Test file placement (my earlier nit) was answered — #5192 is a genuine regression per the 1.0.1 output; thread resolved.
- Tests cover entry/import/require/dynamic-import and the indented off-by-one;
test.concurrent, hermetic, drain both pipes.
Extended reasoning...
Overview
The PR fixes #5192: JSC-parser SyntaxErrors (strict-mode with, delete x, etc.) printed with no file/line because the ErrorInstance has no JS stack and materializeErrorInfoIfNeeded() never published .line/.sourceURL as JS properties. The fix reads err->sourceURL()/err->line() directly from the C++ fields in fromErrorInstance() and synthesizes a single frame. It also extracts a shared Bun__remapParseErrorFrame helper in headers-handwritten.h and rewires the existing <parse>-frame path in FormatStackTraceForJS.cpp to use it.
Security risks
None identified. The change is confined to error-message formatting; no untrusted-input parsing, no allocation driven by external sizes, no auth/crypto.
Level of scrutiny
Medium. This is C++ JSC-bindings code on the error-reporting path — user-visible and historically fiddly — but the blast radius is diagnostic output, not runtime semantics. Two things push it above auto-approve:
- The
INT32_MAXcolumn heuristic.Bun__remapParseErrorFramesetscolumn_zero_based = INT32_MAXwhen JSC's parser column is 0, so the source-map lookup lands on the last mapping of the generated line rather than the previous line's tail. This depends on howVirtualMachine::remap_stack_frame_positions/ the underlying source-map lookup handles a column past end-of-line. The PR asserts it "reliably resolves to the correct source line" and the two exact-line tests (mod.cjs:2,module.mjs:3) back it, but a reviewer familiar with the source-map implementation should confirm this holds generally (e.g. multi-mapping lines, transpiled TS where the printer emits several source-line mappings on one generated line, or files with no source map at all). - It modifies an existing code path. The
<parse>frame informatStackTrace()previously remapped at column 0; it now goes through the same INT32_MAX helper. That's a behavioral change to an already-shipping path (used byrequire()of a bad CJS module, per the tests), not purely additive.
Other factors
- Tests are solid: 7 concurrent subprocess tests covering entry vs. imported
.mjs/.ts,delete x, dynamicimport()re-thrown viaconsole.error,require()of CJS, and the indented-statement off-by-one. Two assert exact line numbers. Verified fail-on-main / pass-on-PR under both ASAN debug and release. - The comment-cop bot flags were addressed (commits "trim comment blocks to single lines" and "extract Bun__remapParseErrorFrame helper; trim comments").
- My earlier nit about
test/regression/issue/placement was answered: the author showed bun 1.0.1 did print a location, so it is a true regression. - The one CI failure (
test/regression/issue/36577.test.tson Windows x64) is unrelated to this diff. - The new
frames_ptr[0]write follows the exact pattern of the pre-existing JS-property fallback immediately below it (samesource_url.deref(), sameframes_len = 1), so no new memory-ownership concern relative to what's already there.
This branch has not been deployed
2 participants
Fixes #5192.
Problem
A
SyntaxErrorraised by JavaScriptCore's own parser (strict-modewith,delete xon an unqualified identifier, invalid regex literals, etc.) during module load printed only the bare message with no file, line, or code frame:Bun's own parser errors already showed location; this is specific to errors JSC catches after Bun's transpiler accepted the source.
Cause
These errors are created before any JS runs, so the
ErrorInstancehas no stack.addErrorInfo()records the line and sourceURL on the instance's C++ fields, butmaterializeErrorInfoIfNeeded()only publishes them as.line/.sourceURLJS properties when a stack trace exists.fromErrorInstance()read those JS properties, found nothing, and produced zero frames, so the code-frame printer never ran.Fix
fromErrorInstance()now readserr->sourceURL()/err->line()from the C++ fields directly and synthesizes a frame when no stack is available.JSC never records the parser-error column (it is always 0). A source-map lookup at column 0 resolves to the printer's start-of-line mapping, which points at the tail of the previous source line when the offending statement is indented, so the reported line was off by one. Both the new path and the existing
<parse>frame informatStackTrace()now query past end-of-line so the lookup lands on the last mapping of the generated line, which reliably resolves to the correct source line; the column is reported as 1 since the true column is unknown.After
Verification
Covers: direct entry,
importof.mjs/.ts,await import()re-thrown viaconsole.error,require()of CJS, and the indented-statement off-by-one.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file