Skip to content

strings: drop the '\r' from line 1 of CRLF files in error code frames - #38338

Open
robobun wants to merge 1 commit into
mainfrom
farm/c5dd398c/crlf-first-line-code-frame
Open

robobun wants to merge 1 commit into
mainfrom
farm/c5dd398c/crlf-first-line-code-frame

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • In the code frame bun prints above an uncaught error (and in Bun.inspect(err) / console.error(err) output) from a file with CRLF line endings, line 1 keeps its carriage return: 1 | 'L1';\r. Lines 2 and later print cleanly. Only visible when line 1 is inside the preview window (the error is on one of the first 6 lines).
  • The same \r shows up as position.lineText ("}\r") of a CSS diagnostic reported on line 1 of a CRLF stylesheet, since the CSS parser locates its diagnostics with the same helper.
  • Cause: index_of_line_ranges in src/bun_core/string/immutable.rs. The scan for the first line (line 1799 on main) ends the range at the \n of a \r\n pair, so the \r stays inside it. The main loop (line 1842 on main) ends every later line at the \r, which is why only line 1 is affected. The display helper (trimmed_text() in src/jsc/ZigStackTrace.rs) strips \n only, and the inspector's LifecycleReporter.error payload and BuildMessage.position.lineText use the ranges as they are.

Fix

  • End the first line's range at the \r when the first line break is \r\n, the same boundary the main loop already uses for every other line.
  • Keep resuming the scan from the \n (prev_end = cursor.i, which is the \n in both the \n and \r\n cases, so the main loop is unchanged). Later ranges deliberately start at the \n that ended the previous line because trimmed_text() strips \n from both ends; starting them at the \r instead would put \r\n at the front of line 2, which trimmed_text() does not strip.
  • Fixing the range rather than adding \r to trimmed_text() fixes every consumer of the helper at once (code frame, Bun.inspect, CSS lineText, inspector payload) and matches what the helper already does for lines 2+. LF files are unaffected: their first range already ended at the \n exclusive, and prev_end has the same value as before.
  • The open strings: include unterminated final line in error code-frame preview #36683 changes the tail of this function (unterminated final line); this change is confined to the first-line scan and does not overlap with it. The vm/eval code frame comes from a different producer (ZigException.cpp, handled in error printer: fix the code frame lines above errors thrown from vm/eval sources #38244) and is not affected either way.
  • Verified with test/js/bun/util/inspect-error-crlf.test.ts: uncaught error below line 1, on line 1 (the target_line == 0 early return), empty line 1, Bun.inspect(err), and a CSS diagnostic on line 1. All 5 fail on the released binary and on a debug build of main with src/ stashed; all pass with the fix. The existing inspect-error.test.js, reportError.test.ts, test-test.test.ts, run-eval.test.ts, and the CSS error tests still pass. These tests are a sibling file because inspect-error.test.js pins its own line numbers in inline snapshots, so adding an import there rewrites unrelated snapshots.

Background

  • Code frame: the numbered source lines, caret and name: message bun prints above a stack trace. For transpiled files remap_zig_exception (src/jsc/VirtualMachine.rs) re-reads the original file and calls get_lines_in_text, which wraps index_of_line_ranges, to cut out the error line and up to 5 lines above it.
  • index_of_line_ranges returns one (start, end) byte range per line. It scans the first line separately from the rest: the first range starts at byte 0, and each later range starts where the previous line's terminator was found (prev_end), so lines 2+ carry a leading \n that the printers trim (trimmed_text() in ZigStackTrace.rs, and the logger's own trimming for CSS diagnostics printed by bun itself).
  • Consumers that do not trim: InspectorLifecycleAgent.cpp sends the strings to debugger frontends as sourceLines, and BuildMessage.rs exposes a CSS diagnostic's line as position.lineText.
Repro
printf "'L1';\r\n'L2';\r\nthrow new Error('x');\r\n" > crlf.js
bun crlf.js 2>&1 | head -3 | cat -A

Before:

1 | 'L1';^M$
2 | 'L2';$
3 | throw new Error('x');$

After:

1 | 'L1';$
2 | 'L2';$
3 | throw new Error('x');$

CSS, before and after (printf '}\r\na {}\r\n' > a.css, then Bun.build({ entrypoints: ["./a.css"], throw: false })): logs[0].position.lineText is "}\r" before and "}" after. Diagnostics on lines 2+ of a CSS file still carry the leading \n described above; that is a separate shape in the CSS consumer and is not changed here.

…frames

index_of_line_ranges ended the first line's range at the '\n' of a CRLF
pair while every later line's range ends at the '\r', so line 1 of a CRLF
file was the only line printed with a trailing carriage return in the code
frame above an error, in Bun.inspect(err), and in the lineText of CSS
diagnostics reported on line 1.
@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:05 AM PT - Aug 14th, 2026

❌ @robobun, your commit 9ad41a7 has some failures in Build #95359 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38338

That installs a local version of the PR into your bun-38338 executable, so you can run:

bun-38338 --bun

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on the released binary and on a debug build of main (printf "'L1';\r\n'L2';\r\nthrow new Error('x');\r\n" > crlf.js && bun crlf.js prints 1 | 'L1';\r). Fix is in index_of_line_ranges; test/js/bun/util/inspect-error-crlf.test.ts fails without the src/ change (5/5) and passes with it. Waiting on CI.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eaca0bdc-4690-437f-9aa2-405a8e8f0bb8

📥 Commits

Reviewing files that changed from the base of the PR and between e697804 and 9ad41a7.

📒 Files selected for processing (2)
  • src/bun_core/string/immutable.rs
  • test/js/bun/util/inspect-error-crlf.test.ts

Walkthrough

CRLF line-range scanning now preserves carriage-return positions and resumes later scans from the newline cursor. New tests cover JavaScript and CSS diagnostics, error inspection, empty lines, normalized frames, and exit codes.

Changes

CRLF line-range handling

Layer / File(s) Summary
Preserve CRLF range positions
src/bun_core/string/immutable.rs
CRLF ranges end at the carriage return. Subsequent scans resume from the newline cursor.
Validate CRLF diagnostics
test/js/bun/util/inspect-error-crlf.test.ts
Tests cover JavaScript and CSS diagnostics, Bun.inspect(error), empty lines, normalized frames, position text, and exit codes.

Suggested reviewers: jarred-sumner

Merge Risk: ⚪ Minimal · up to 9ad41

This removes an unintended carriage return from first-line CRLF diagnostics and code frames while preserving existing behavior for other line endings and consumers; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the CRLF line-ending fix for error code frames.
Description check ✅ Passed The description explains the problem, fix, scope, and verification results, although it does not use the template headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — targeted fix that ends line 1's range at the \r of a \r\n pair, matching what the main loop already does for lines 2+.

What was reviewed:

  • Traced prev_end = cursor.i for both the LF arm (cursor.i == first_newline_range.end, unchanged) and the CRLF arm (still the \n index, so line 2's start is unchanged and trimmed_text() still strips it).
  • Confirmed trimmed_text() in ZigStackTrace.rs:129 trims only \n, so the range-level fix is the right layer; checked callers (VirtualMachine.rs, css/error.rs).
  • Checked the bare-\r-at-EOF and \r-not-followed-by-\n paths in the first-line scan — both fall through unchanged (they don't reach prev_end).
  • Tests cover error below/on line 1, empty line 1, Bun.inspect, and the CSS lineText consumer; harness conventions (tempDir, bunEnv, concurrent pipe drain, describe.concurrent) look right.
Extended reasoning...

Overview

Two-line logic change in index_of_line_ranges (src/bun_core/string/immutable.rs): the first-line scan now ends the range at the \r of a \r\n pair (via a saved cr_i) instead of at the \n, and prev_end is initialized from cursor.i rather than first_newline_range.end so the main loop's resume point is unchanged. A new test file test/js/bun/util/inspect-error-crlf.test.ts adds 5 subprocess/build tests.

Security risks

None. This affects only the byte range boundaries used to slice source text for error code frames and diagnostic lineText. No parsing of untrusted input, no allocation-size arithmetic, no auth/crypto surface.

Level of scrutiny

Low-to-moderate. The helper is used only for diagnostic output (error code frames, CSS position.lineText, inspector sourceLines), so a mistake would degrade error messages, not runtime behavior. The change is mechanical: I walked each arm of the first-line 'brk block and confirmed that for the NL case cursor.i == first_newline_range.end (so prev_end is byte-identical to before), and for the CRLF case cursor.i is still the \n index (so prev_end is byte-identical to before). The only observable change is first_newline_range.end moving one byte left on CRLF, which is exactly the bug fix. The paths that return before reaching prev_end (no-newline fallback, bare \r at EOF) don't touch the new code.

Other factors

  • Fix is at the layer that owns the invariant (the range producer) rather than patching each consumer, and it makes line 1 consistent with the existing lines-2+ behavior in the same function.
  • The PR description explains why prev_end must stay at the \n (so trimmed_text()'s \n-only trim keeps working for line 2's leading char); I verified trimmed_text() at src/jsc/ZigStackTrace.rs:129-131 and it indeed trims only \n.
  • Tests follow harness conventions: tempDir/bunEnv/bunExe, describe.concurrent, concurrent Promise.all drain of stdout/stderr/exited, exact .toEqual assertions on the combined {stdout, stderr, exitCode} shape. The separate-file justification (inspect-error.test.js pins its own line numbers in inline snapshots) is sound.
  • The PR states all 5 tests fail on the released binary and on debug main without the src/ change, and pass with it; and that adjacent suites (inspect-error.test.js, reportError.test.ts, CSS error tests) still pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant