Skip to content

sourcemap: fix the CRLF peek when counting generated lines - #38449

Open
robobun wants to merge 1 commit into
mainfrom
farm/e927bb38/sourcemap-lone-cr
Open

robobun wants to merge 1 commit into
mainfrom
farm/e927bb38/sourcemap-lone-cr

Conversation

@robobun

@robobun robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Every mapping that follows a carriage return in the printed output lands on the wrong generated line, in both bun build --sourcemap output and the maps the runtime uses to remap error.stack: a lone \r followed by a one-character line is not counted as a line break, and an actual \r\n is counted as two.
  • Repro: bundle /*! a\rb\nc */\nconsole.log(1);\nthrow new Error("x");\n with --sourcemap=external. The output puts console.log on its 5th line (the \r ends a line, as it does for every JS engine), but mappings is ;AAGA;AAAA;AAAA,QAAQ,IAAI,CAAC;..., with console.log in the 4th line group. Running a file with that comment reports a new Error() created on line 4 as line 5.
  • Cause: update_generated_line_and_column_slow in src/sourcemap/Chunk.rs advances i past the decoded character before matching on it, but the \r arm peeked at last_generated_update + i + 1, which is two bytes past the CR. (The + 1 is from esbuild's version of this loop, where i is the index of the rune itself.) So \rX\n skips the CR because the byte two ahead is a \n, and \r\nX counts the CR and then the \n again.

Fix

  • The \r arm peeks at slice[i], the byte directly after the CR. That condition is the whole change; the rest of the hunk is the comment.
  • This is the line terminator rule JSC uses for the positions that get looked up in these maps (\r, \n, U+2028 and U+2029 each end a line, \r\n ends one), and it is the rule LineOffsetTable::generate already applies to the original side of every mapping, so the two sides of a mapping now count lines the same way.
  • This miscount is what Sourcemap reported location in browser does not match actual location #18814 ran into (CRLF legal comments in tabster's dist shifted every later mapping in the bundle, one line per comment line); Ensure we add sourcemappings for S.Comment #23871 fixed that report by rewriting \r\n to \n inside legal comments in the printer, which left the builder wrong for the CRs that still get through (the shapes tested here). Fixing the peek where the count happens covers those without more printer-side normalization.
  • Tests, all of which fail on the unfixed build:
    • test/bundler/bundler_comments.test.ts: three bundles (lone CR in a legal comment, CRLF left in a legal comment, CRLF in an inlined enum member comment) check that the generated line holding each statement after the comment maps back to that statement's line in the entry point. Unfixed, the statement after the lone CR maps to the line below it and the one after the CRLF maps to the line above it.
    • test/js/bun/sourcemap/internal-sourcemap.test.ts: the same two shapes through bun run and error.stack, which go through the same builder but the runtime's internal map format. Unfixed, they report line 5 instead of 4 and line 4 instead of 5.
  • Also ran test/js/bun/sourcemap/, bundler_edgecase, the compile source map tests and the node:module source map tests against the debug build.

Background

  • The printer calls the source map builder once per token it maps. update_generated_line_and_column scans whatever was printed since the previous call to advance the generated line and column, emitting one line separator (a ; in VLQ maps, a separator entry in the runtime's internal format) per line break it sees. bun build maps and the runtime's stack trace maps both come out of this one loop.
  • A raw CR reaches the printed output in two ways. Legal comments (/*! ... */) are copied verbatim except that a \r\n inside them becomes \n, so a lone \r survives and \r\r\n comes out as a real \r\n. Accessing an inlined enum member prints 1 /* member name */ with the name verbatim. Strings and template literals escape or normalize CRs, so the tests use those two shapes.
  • The runtime CRLF test uses the legal comment shape rather than the enum one because the runtime prints a single-member enum as an arrow function ending in a multi-line template literal, and JSC reports wrong line numbers after that construct regardless of source maps (tracked separately).
  • Not changed here: the same loop (and LineOffsetTable::generate) steps over the declared width of an invalid UTF-8 lead byte, so a Latin-1 byte directly before a newline still swallows that line break. Different cause, tracked separately.
Repro output before and after
$ printf '/*! a\rb\nc */\nconsole.log(1);\nthrow new Error("x");\n' > in.js
$ bun build ./in.js --outdir out --sourcemap=external

# out/in.js (cat -A)
// in.js$
/*! a^Mb$
c */$
console.log(1);$
throw new Error("x");$

# mappings before (6 groups, console.log in group 4, it is on line 5 of the file)
;AAGA;AAAA;AAAA,QAAQ,IAAI,CAAC;AACb,MAAM,IAAI,MAAM,GAAG;
# mappings after (7 groups)
;AAGA;AAAA;AAAA;AAAA,QAAQ,IAAI,CAAC;AACb,MAAM,IAAI,MAAM,GAAG;

$ printf 'enum E { "a\\r\\nb" = 1 }\nconsole.log(E["a\\r\\nb"]);\nthrow new Error("x");\n' > in.ts
$ bun build ./in.ts --outdir out --sourcemap=external
# out/in.js contains `console.log(1 /* a<CR><LF>b */);` (3 lines of code after the path comment)
# before (6 groups, the CRLF counted twice)
;AACA,QAAQ,IAAI;AAAA;AAAA,IAAW;AACvB,MAAM,IAAI,MAAM,GAAG;
# after (5 groups)
;AACA,QAAQ,IAAI;AAAA,IAAW;AACvB,MAAM,IAAI,MAAM,GAAG;

$ printf '/*! a\rb\nc */\nconst err = new Error("x");\nconsole.log(err.stack.split("\\n")[1]);\n' > rt.js
$ bun rt.js
    at /tmp/rt.js:5:17      # before (the Error is on line 4)
    at /tmp/rt.js:4:17      # after

update_generated_line_and_column_slow advances `i` past the decoded
character before matching on it, so the '\r' arm was inspecting the byte
two positions after the carriage return instead of the one right after
it. A lone '\r' followed by a one-character line was not counted as a
line break, and a real "\r\n" was counted as two, so every mapping after
it landed on the wrong generated line.

Peek at slice[i], which is the byte following the '\r'.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1e5320a5-febf-45a9-828f-0a9d14a7eddf

📥 Commits

Reviewing files that changed from the base of the PR and between 9c629a4 and 5c7d4c1.

📒 Files selected for processing (3)
  • src/sourcemap/Chunk.rs
  • test/bundler/bundler_comments.test.ts
  • test/js/bun/sourcemap/internal-sourcemap.test.ts

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

@robobun

robobun commented Aug 14, 2026 •

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

❌ @robobun, your commit 5c7d4c1 has some failures in Build #95838 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38449

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

bun-38449 --bun

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on 1.4.0 and main with the commands in the PR body (bun build --sourcemap=external of a file whose legal comment contains a lone \r: one line group short; an inlined enum member comment with \r\n: one line group too many; bun run of the first file reports the Error one line late). The five new tests fail on the unfixed build and pass with this branch; the fix is the one-line condition in src/sourcemap/Chunk.rs.

@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 — the CRLF peek was checking two bytes past the CR because i had already been advanced; peeking at slice[i] is the correct byte-after-CR.

What was reviewed:

  • Traced the loop: i += len runs before the match, so slice[i] is the byte after \r; the i < n bound covers CR-at-end.
  • Checked the two sibling CRLF loops in src/sourcemap/lib.rs and LineOffsetTable.rs — both index the rune itself pre-advance, so neither shares this bug.
  • Tests cover both failure directions (lone CR under-counted, CRLF double-counted) through both the VLQ bundler path and the runtime internal-map path; readFileSync sidesteps api.readFile()'s CRLF normalization.
Extended reasoning...

Overview

One-line fix in src/sourcemap/Chunk.rs::update_generated_line_and_column_slow: the \r arm now peeks at slice[i] (the byte immediately after the CR) instead of output[last_generated_update + i + 1] (two bytes past it, since i is post-advanced by i += len before the match). The old expression was a straight port from esbuild's loop where i indexes the rune itself. Five new tests across test/bundler/bundler_comments.test.ts and test/js/bun/sourcemap/internal-sourcemap.test.ts pin both failure modes.

Security risks

None. This is line-counting logic for sourcemap generation over already-printed output; no user-controlled sizes drive allocation, and the new i < n bound guards the only added indexed read.

Level of scrutiny

Low-to-medium. The native change is a single condition whose correctness is verifiable by tracing the loop locally, and the PR body derives the exact off-by-one from the esbuild reference. I confirmed the two other CRLF-peek sites in src/sourcemap/ (lib.rs:193-195 and LineOffsetTable.rs:256) use pre-advance indexing and are already correct, so there are no unfixed siblings.

Other factors

The tests are well-constructed against the review guide: they assert exact original-line values via SourceMap.findEntry / error.stack rather than string shape, cover both the bundler VLQ format and the runtime internal format (both flow through this one loop), read the outfile with readFileSync to avoid the harness's CRLF normalization, and exercise both the lone-CR-followed-by-one-char and the actual-CRLF shapes that the old code got wrong in opposite directions. robobun confirmed all five fail on the unfixed build.

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