Conversation
When the source text has no trailing newline and the target line is the last (unterminated) line, index_of_line_ranges would exit its scan loop without ever pushing that line's range. remap_zig_exception assumes the last returned range is the target line and numbers backward from there, so the code frame showed the wrong line numbers and omitted the line that actually threw.
WalkthroughThe line-range index now includes non-empty unterminated final lines. CLI tests cover error code frames and ChangesFinal-line error reporting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:59 AM PT - Aug 1st, 2026
⏳ @robobun, your commit 29fd565 is still building in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix to the error code-frame preview. The one inline nit (undrained stdout pipe) is cosmetic; the child writes nothing to stdout so there's no deadlock risk.
Checked both callers of index_of_line_ranges (VirtualMachine.rs remap path and css/error.rs) — both assume the last returned range is target_line and number backward, so both benefit. Traced the trailing-newline case: prev_end + 1 == text.len() leaves behavior unchanged. Dropping the old current_line <= target_line guard is safe since that invariant always holds at the fall-through point.
Extended reasoning...
Overview
Fixes an off-by-one in the error code-frame preview when source has no trailing newline. The change is 11 net lines in index_of_line_ranges (src/bun_core/string/immutable.rs): after the newline-scan loop exits, if there is content past the last \n, push it as the final LineRange. The old code only made room in the sliding window without pushing anything. Four new tests in run-eval.test.ts cover -e, --print, running a file, and Bun.inspect(new Error(...)).
Security risks
None. This is display-only formatting for error previews; it reads bytes from source text already in memory and produces line-range indices. No parsing of untrusted input beyond what already existed, no allocation changes, no FFI.
Level of scrutiny
Low-to-medium. The function is not on a hot path (only runs when formatting an error's code frame) and has exactly two callers, both of which I checked:
VirtualMachine.rs:5469— numbers lines backward fromtarget_line, assuming the last returned range is that line. The fix makes this assumption hold for unterminated final lines.css/error.rs:185— fetches a single line for CSS diagnostics; same assumption, same benefit.
I traced the invariants: at the fall-through point current_line <= target_line always holds (the loop returns early otherwise), so dropping that condition from the make-room block is correct. For inputs that do end with \n, prev_end + 1 == text.len() and the new block is skipped, preserving prior behavior. The pushed range's start: prev_end (index of the last \n) matches the leading-newline convention used by the in-loop ranges. The as u32 wrapping cast follows the file's existing pattern.
Other factors
Tests follow REVIEW.md conventions (spawnSync + stderr assertions before exitCode, tempDir, test.concurrent for the async subprocess pair) and cover the variant matrix: both eval flags, file execution, and Bun.inspect. The one nit (stdout piped but not drained in one test) is harmless here since the child writes 0 bytes to stdout — worth fixing for consistency with the sibling test but not blocking. No CODEOWNERS on these paths, no outstanding reviewer comments.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/bun_core/string/immutable.rs`:
- Around line 1733-1736: Guard both line-range eviction conditions, including
the block around new_ranges, with LINE_RANGE_COUNT != 0 before slicing from
index 1; preserve existing eviction behavior for nonzero capacities. Add
regression coverage for zero-capacity handling of inputs such as b"a\nbc" and
b"a\nb\nc".
In `@test/cli/run/run-eval.test.ts`:
- Around line 51-61: Update test/cli/run/run-eval.test.ts at lines 51-61,
141-146, and 161-166 to assert complete normalized code-frame output in the
eval, file-error, and Bun.inspect() tests. Pipe and verify empty stdout in the
eval test, replace independent toContain checks with one ordered exact frame
assertion per test, and run bun bd test test/cli/run/run-eval.test.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5af85acf-3537-4721-9ed6-d995274109dc
📒 Files selected for processing (2)
src/bun_core/string/immutable.rstest/cli/run/run-eval.test.ts
…, window-full test The bare-CR arm of the scan loop pushed a range but left prev_end stale (and computed the range end from the advanced cursor, not the CR position). Update prev_end and the end index so the tail-push guard does not re-push an overlapping range for sources that mix LF and bare CR. Tests now assert the code-frame lines as one ordered block and assert empty stdout, and a new 8-line case covers the full-window shift branch.
There was a problem hiding this comment.
LGTM — focused fix for the off-by-one code-frame preview when the target line is unterminated.
What was reviewed:
- Traced
index_of_line_rangestail push against both callers (VirtualMachine.rsnumbers backward fromlast_line;css/error.rswithLINE_RANGE_COUNT=1) — both now get the correct final range. - Verified the bare-CR else-arm change (
current_end + 1+prev_end = current_end) fixes the overshoot when the peek advances the cursor, and is a no-op when the peek hits EOF. - Confirmed the dropped
&& current_line <= target_lineguard was always true at that point (early return inside the loop guarantees it). - Tests drain both pipes, assert the frame as one ordered block, cover
-e/--print/file/Bun.inspect, and exercise the window-shift path.
Extended reasoning...
Overview
The PR fixes index_of_line_ranges in src/bun_core/string/immutable.rs so the unterminated final line of a source is included in the returned range window. Previously the scan loop pushed a range per newline and fell through without pushing the tail, so remap_zig_exception (which numbers backward from the last returned range assuming it is target_line) mislabeled every preview line by one and omitted the throwing line whenever the source lacked a trailing newline. A secondary fix in the same function updates prev_end in the bare-CR branch and corrects its range end from cursor.i + 1 (which overshoots by one when the peek advances) to current_end + 1. Four tests are added to test/cli/run/run-eval.test.ts.
Security risks
None. This is a crate-private helper feeding a cosmetic error-preview renderer. Ranges are bounded by text.len(); the only new arithmetic is (prev_end as usize) + 1 < text.len(), which cannot overflow (prev_end is a valid byte index into text). No user-controlled sizing or allocation.
Level of scrutiny
Low-medium. The change is ~15 net lines in a function with exactly two callers. I traced both:
VirtualMachine.rs:5469(SOURCE_LINES_COUNT = 6): iterates the reversed result assigninglast_line, last_line-1, ...— now correctly aligned since the last range is actuallytarget_line.css/error.rs:185(LINE_RANGE_COUNT = 1): takes[0]after reversal — previously got line N-1 when the target was the unterminated line N; now gets N.
The dropped current_line <= target_line guard on the post-loop shift was dead: the in-loop if current_line >= target_line { return } runs before the increment, so falling out of the loop implies current_line <= target_line.
Other factors
Two rounds of review feedback were addressed (stdout draining in the file test; the bare-CR/prev_end interaction I flagged; comment length; ordered-block assertions per coderabbit). All threads are resolved. Tests follow harness conventions (tempDir, bunEnv, concurrent subprocess drain, exitCode asserted last) and include a window-full case that exercises the ranges.len() == LINE_RANGE_COUNT shift. The bare-CR change is a strict improvement over pre-PR behavior (which never updated prev_end in that arm) and does not affect the first-newline block, which handles CR-only single-newline inputs separately.
|
CI on 29fd565: the new tests in
This diff is ready for review; all review threads are resolved. |
This branch has not been deployed
1 participant
When a source file or
bun -einput has no trailing newline and an error is thrown on the last line, the code-frame preview labels lines off by one and never shows the throwing line.Repro
The stack frame
[eval]:3:11is correct; only the preview is wrong. Appending a trailing\nto the input makes the preview correct. The same happens for files without a trailing newline and forBun.inspect(new Error(...))when thenew Erroris on the last line.Cause
index_of_line_rangesscanstextand pushes aLineRangeeach time it crosses a newline. When the loop runs out of newlines it falls through and returns, so the final unterminated line is never pushed.remap_zig_exceptionassumes the last returned range istarget_lineand numbers backward from there, so every label is off by one and the target line's content is missing.Fix
After the scan loop, if there is content past the last newline (
prev_end + 1 < text.len()), push{prev_end, text.len()}as the final range, making room in the sliding window first. The old after-loop block only made room but never pushed.After
Verified for
-e/--print, files, CRLF, non-ASCII on the final line, and inputs longer than the preview window. Tests inrun-eval.test.tscover-e/--print, a file, andBun.inspect.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-eval.test.ts