Repository navigation
Conversation
The excerpt of a long source line is cut to about 120 bytes around the error, but the caret under it was padded to the error's column in the whole line. A one-line bun.lock with N bad integrity hashes printed N caret lines of up to the line's width, so stderr grew quadratically (22 MB for 1000 entries) and the caret never pointed at the excerpt. Location records how many columns the window drops on the left, and the printer subtracts that from the column and bounds the indent by the excerpt length. An error in the last 80 bytes of a long line now gets the window too, instead of printing the whole line.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 1 minute for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 8:18 PM PT - Sep 19th, 2026
❌ @robobun, your commit 7be9837 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 41658That installs a local version of the PR into your bun-41658 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…e start after U+2028 Drop the widened window gate: consumers that only know the column (the bake overlay, BuildMessage.position) still get the full line for an error in its last 80 bytes. Count the kept prefix with the same scanner that produced the column. Start the excerpt at the line's first byte so a line after a U+2028/U+2029 terminator does not begin with a stray continuation byte. Port the left-trim, non-ASCII, astral, error+note, package.json and U+2028 caret tests from #37848. Drain stdout in the lockfile test and assert the output size is linear.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Both earlier concerns are addressed in the latest push: the || full_line.len() > 120 widening of the window gate is reverted (with a comment naming the bake overlay and BuildMessage.position as consumers that still index the excerpt by column - 1), and the bun-lock test now drains stdout alongside stderr/exited. The test's stderr-length bound was correspondingly relaxed to account for the last-80-bytes entries still printing the full line — checked that this stays linear in the entry count.
Extended reasoning...
The follow-up commit reverts the tail-of-line window widening that the previous review flagged as regressing other Location consumers, and instead documents the constraint in a comment; it also fixes the undrained stdout pipe. The kept-bytes column count now reuses ErrorPositionState::advance (the same scanner that produced column_count), and the line_start - 1 back-up plus the trim_left(full_line, b"\n\r") are both removed together, which is the U+2028 continuation-byte fix covered by the new test. The remaining inline finding is a Windows CRLF nit in the new bun-lock test.
There was a problem hiding this comment.
LGTM — all three points from earlier rounds are addressed: the || full_line.len() > 120 widening is gone (so the bake overlay / BuildMessage.position consumers keep seeing the full line for tail-of-line errors), the bun-lock test now drains stdout concurrently, and both new tests split stderr on /\r?\n/.
Checked that line_text_start_column is threaded through every Location constructor/clone site (Clone, Default, dupe, init_or_zero, both init_or_null_impl branches); the line_start - 1 removal and the dropped trim_left on full_line are paired and covered by the U+2028 test; the line_column_tracker_tests unit tests compare tracker vs. full-scan through the same to_error_position, so they stay consistent. bunEnv already sets NO_COLOR: "1", so dropping the explicit override in the refactored helper is fine.
Extended reasoning...
Overview
This PR fixes caret misalignment in Bun's diagnostic printer when a long source line is windowed to ~120 bytes. It adds line_text_start_column: usize to ast::Location to record how many UTF-16 column units were dropped from the left of the excerpt, and Data::write_format subtracts that offset (clamped to the excerpt length) when indenting the ^. ErrorPositionState::to_error_position no longer subtracts 1 from line_start, and the corresponding trim_left(full_line, b"\n\r") on the stored line_text is removed — this also fixes a stray UTF-8 continuation byte after U+2028/U+2029. Two test files are extended: parse-error-column.test.ts gains a printedExcerpts helper and seven cases (ASCII/2-byte/astral prefixes, error+note, bun install on long package.json, U+2028), and bun-lock.test.ts gains a 300-warning single-line lockfile test asserting caret placement and linear stderr growth.
Security risks
None. This is diagnostic-output formatting only — no parsing of untrusted data into privileged operations, no auth/crypto/permissions, no network. The only arithmetic on user-derived values uses saturating_sub and .min(), so no overflow/underflow on adversarial column counts.
Level of scrutiny
Medium — shared diagnostic infrastructure with several downstream consumers (bake overlay, BuildMessage.position, DevErrorPage). The first revision's widened windowing gate would have regressed those consumers for tail-of-line errors; that was reverted after review, and the current gate is byte-identical to base (full_line.len() > 80 + offset_in_line), so those readers see unchanged line_text with line_text_start_column == 0. The new field is wired through every constructor/clone site I could find. The to_error_position change is validated by the new U+2028 test, and the in-file line_column_tracker_tests compare two paths that both go through the changed function, so they remain self-consistent.
Other factors
This is my fourth pass. All three prior findings (the widened gate, undrained stdout in the bun-lock test, and "\n" vs /\r?\n/ splitting) are fixed in the current diff. The tests follow harness conventions (tempDir, bunExe(), bunEnv, concurrent pipe draining, test.concurrent, Buffer.alloc(n, fill) instead of .repeat, /\r?\n/ splitting, exit-code asserted last). bunEnv in test/harness.ts already sets NO_COLOR: "1", so dropping the explicit { ...bunEnv, NO_COLOR: "1" } in the refactored helper is a no-op. No open CHANGES_REQUESTED from human reviewers; the author-resolved github-actions[bot] threads land on lines that changed shape across the intervening commits, consistent with them being addressed. The exit reason is dry_streak.
…t-under-windowed-excerpt
The new usize field grew bun_ast::Msg from 152 to 160 bytes, which put LoadValue::Err(Msg) in the bundler over clippy's large_enum_variant threshold. Store the start column and the highlight length as u32 (both come from i32 ranges), so Location, Data and Msg keep their size.
There was a problem hiding this comment.
LGTM — the follow-up commits since my last pass (the usize → u32 narrowing of Location.length and the new line_text_start_column field) are consistent across every reader and constructor I could find.
What was reviewed:
Location.lengthreaders:src/parsers/json.rs:154(cast tousizebefore the add),src/runtime/bake/dev_server/serialized_failure.rs:232(writes theu32directly),src/jsc/BuildMessage.rs:152(From<u32> for JSValueexists) — no other field reads found; all otherLocationliterals use..Default::default().line_text_start_columnderivation ininit_or_null_impl: the kept-prefix counter runs the sameErrorPositionState::advanceoverfull_line[lo..offset_in_line]as the full-line column scan, so a truncated UTF-8 tail or astral characters count identically in both and the difference is exact in UTF-16 units; the window gate is unchanged, so bake's overlay andBuildMessage.positionsee the same excerpts as base.- Dropping the
-1into_error_position:line_startis set after the terminator in every branch ofadvance, and the tracked and from-scratch scans share the same conversion, so the existingLineColumnTrackerequivalence tests still cover both; for\n/\r\ninputs the result matches the oldtrim_leftoutput byte-for-byte, and the U+2028 case is now correct and tested.
Extended reasoning...
Overview
The PR fixes caret placement under the logger's windowed source excerpts in src/ast/lib.rs: Location gains line_text_start_column: u32 (0-based, UTF-16 units) recording where a left-trimmed line_text begins, Data::write_format subtracts it from the caret indent (clamped to the excerpt length), and ErrorPositionState::to_error_position no longer points line_start one byte before the line (which after a 3-byte U+2028 was a stray continuation byte). length is narrowed from usize to u32 to keep Location/Data/Msg at their prior sizes; the two callers that read it (src/parsers/json.rs, src/runtime/bake/dev_server/serialized_failure.rs) are adapted. Tests: a new printedExcerpts helper in test/js/bun/transpiler/parse-error-column.test.ts with cases for left-trimmed excerpts under build and run, 2-byte and 4-byte UTF-8 prefixes, error + note on one long line, bun install on a long package.json line, and the U+2028 line start; plus a bun-lock.test.ts case asserting 300 carets land under "sha512- in the excerpt and that stderr stays linear in the entry count.
Security risks
None identified. The change only affects how diagnostics are rendered to stderr and the values carried in Location. The new arithmetic is bounded: column_count - kept.column_number uses saturating_sub and both operands are UTF-16 unit counts over a source that is at most i32::MAX bytes, so the u32::try_from(...).expect cannot trip on user input; the caret indent is clamped to line_text.len(). The windowing slice indices (lo, hi) are unchanged from base. No allocation, parsing, or trust-boundary changes.
Level of scrutiny
Moderate. The struct is shared by the bundler, BuildMessage, and the bake dev server, so I enumerated every reader of Location.length and every Location literal in src/ (all others use ..Default::default()), and traced ErrorPosition.line_start to its only two uses (the excerpt slice and offset_in_line). I also checked that the window gate (full_line.len() > 80 + offset_in_line) is unchanged, so consumers that index line_text by the full-line column (bake overlay, BuildMessage.position) receive exactly the excerpts they did on base; the PR explicitly scopes carrying the offset through those consumers to a separate PR. I could not run cargo check or the test suites in this environment, so compile verification rests on the grep audit and CI.
Other factors
The exit reason was dry_streak, no bugs were reported, and the changed files are not covered by CODEOWNERS. My prior reviews raised two items (pipe draining, CRLF split) that are now fixed in the diff (Promise.all over stdout/stderr/exited, split(/\r?\n/)), and the earlier tail-of-line widening concern was dropped by the author. The one outstanding github-actions[bot] inline comment at src/ast/lib.rs:650 was followed four minutes later by the "keep Location.length in place" commit that touches exactly that region, which plausibly addressed it. The tests are specific (toEqual on exact excerpt/caret pairs, Buffer.alloc fills, test.concurrent, hermetic tempDir), and each load-bearing clause of the fix has a test that would fail without it (UTF-16 vs byte vs character counting are distinguished by the astral and U+00E9 cases).
|
Status: ready for review. The diff is green. CI is red on one test that also fails on main. Reproduction: write a CI on 7be9837 (build 118787), a rerun of the same diff:
This diff only changes how the logger prints diagnostics, so it does not reach any of those code paths. The build finished with 180 of 181 jobs passed. |
This branch has not been deployed
2 participants
Problem
bun.lockwith 1000 bad integrity hashes (warn: Unsupported or malformed integrity hash; ignoring) prints 22 MB of stderr.Data::write_formatinsrc/ast/lib.rs. It indents the caret bycolumn - 1.Location::init_or_null_impldrops bytes from the left ofline_textand records nothing about it.Fix
Locationgetsline_text_start_column: the column at whichline_textstarts in the source line.write_formatsubtracts it from the caret indent and bounds the indent by the excerpt length.at file:line:colsuffix still refer to the whole line.test/cli/install/bun-lock.test.ts(one new test) andtest/js/bun/transpiler/parse-error-column.test.ts(seven new tests). All fail on the release build and pass here.Background
Locationcarries a copy of its line's text, taken when the diagnostic is created. The printer drawsN | <text>and a caret line under it.Notes
The window is up to 40 bytes before the error and 80 after, snapped to UTF-8 boundaries. An error in the last 80 bytes of a line keeps the whole line, because the bake overlay and
BuildMessage.positionindexline_textbycolumnalone.LineColumnTrackerfinds line and column for many diagnostics in one file without rescanning from the top, so only the kept bytes before the error are measured, not the dropped prefix. Thetrim_leftof\n\rthat covered for the early line start is gone. On the release build, the longest caret line for N=1000 is 43,800 chars.Repro: a
bun.lockwhosepackagesobject is one line with N entries"pK": ["pK@1.0.0", "", {}, "sha512-x"],package.jsonwith no dependencies, thenbun install.Before the fix the first warning on the repro line (column 44) already showed the caret 4 cells right of the token, because the window started at byte 4 of the line.
Location.lengthis nowu32and the new field isu32. Ausizefield grewbun_ast::Msgfrom 152 to 160 bytes, which putLoadValue::Err(Msg)insrc/bundler/bundle_v2.rsover clippy'slarge_enum_variantthreshold (128). Both values come fromi32ranges, soLocation,DataandMsgkeep their size (96, 120, 152 bytes).A first revision also windowed errors in the last 80 bytes of a long line. Review pointed out that
serialized_failure.rs/overlay.ts,DevErrorPage.rsandBuildMessage.positionexposelineTextwith the full-linecolumnand no start column, so that widening would have misplaced their highlight for tail-of-line errors. It was dropped; carrying the offset through those consumers is #37872's scope.Self-reviewed: 2 concerns raised (the widening above, and the dropped non-ASCII/astral/error+note tests from #37848), both addressed.
Suites run with the debug build:
test/cli/install/bun-lock.test.ts,test/js/bun/transpiler/parse-error-column.test.ts(14 pass),test/bundler/bundler_edgecase.test.ts(170 pass,DeepImportDiamondDAGtimed out at 120 s under ASAN in this container, unrelated),test/js/bun/test/dots.test.ts,test/js/bun/test/only-failures.test.ts,test/regression/issue/12782.test.ts.The ledger note also suggests collapsing N identical warnings into one with a count. This PR does not do that.
no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-lock.test.ts