Repository navigation
sourcemap: count an ill-formed UTF-8 lead byte as one byte so the line break after it is not skipped - #38593
sourcemap: count an ill-formed UTF-8 lead byte as one byte so the line break after it is not skipped#38593robobun wants to merge 4 commits into
Conversation
LineOffsetTable::generate (original side) and the builder's update_generated_line_and_column (generated side) advanced by the width a lead byte declares even when the decode failed, so a Latin-1 byte right before a line break swallowed the line break and every mapping after it was one line off. Both loops now decode through strings::CodepointIterator, which yields one U+FFFD one byte wide for an ill-formed sequence, the same as LineColumnOffset::advance.
|
Warning Review limit reached
Next review available in: 1 minute 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 |
|
Status: fix and tests pushed, review feedback addressed, waiting on CI.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: CodepointIterator::next never sets cursor.width = 0 (wtf8_byte_sequence_length returns ≥ 1 for every byte and the ASCII fast path sets 1), so neither loop can stall; the fresh-Cursor-per-iteration pattern with width defaulted to 0 reads at exactly i/offset as intended; and the sentinel change in LineOffsetTable (invalid decode was 0, now U+FFFD) only adds columns_for_non_ascii entries for lines with ill-formed bytes — which is a correction, not a regression.
Extended reasoning...
The fix replaces two hand-rolled WTF-8 decode loops with the shared strings::CodepointIterator, which is exactly what REVIEW.md's "grep for the in-tree helper" rule asks for and what LineColumnOffset::advance already does. I traced next() in src/bun_core/string/immutable.rs and wtf8_byte_sequence_length / decode_wtf8_rune_t to confirm width is always ≥ 1 (so no infinite-loop risk from the while i < n / while !remaining.is_empty() heads), that continuation bytes 0x80-0xBF and 0xF8-0xFF decode identically to before (len==1 → T::from(p[0])), and that well-formed multibyte sequences still return the same codepoint and width. The u32 casts on i/offset are pre-existing constraints (both loops already stored offsets as u32). The one behavioral side effect beyond the fix itself — LineOffsetTable now sees c = 0xFFFD instead of c = 0 for a bad decode, so c > 0x7F becomes true and the line's columns_for_non_ascii table is populated — makes columns after an ill-formed byte match what JSC actually sees, so it's a secondary correctness improvement. This is a well-tested, correctly-layered fix, but it rewrites decode logic in two hot builders and interacts with two other open PRs (#38262, #38449), so a human look is still worthwhile.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Same decoder as `LineOffsetTable::generate` and `LineColumnOffset::advance`: | ||
| // a lead byte whose continuation bytes are missing (a Latin-1 byte in a legal | ||
| // comment) is one U+FFFD, one byte wide, so a line terminator right after it is | ||
| // still seen below instead of being skipped with the width the lead byte declared. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `CodepointIterator` is also what `LineColumnOffset::advance` and the | ||
| // generated-side walk in `Chunk.rs` use, so all three agree on what a byte | ||
| // sequence is: a lead byte whose continuation bytes are missing (Latin-1 text) | ||
| // is one U+FFFD, one byte wide, and the bytes after it (a line terminator, say) | ||
| // are looked at on their own rather than skipped as part of the width the lead | ||
| // byte declared. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // An ill-formed sequence decodes as one U+FFFD one byte wide, so a line | ||
| // terminator right after a bad lead byte still reaches the match below. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Same decoder as the generated-side walk in `Chunk.rs`: an ill-formed sequence | ||
| // is one U+FFFD one byte wide, so a line terminator right after it is not skipped. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
LGTM — the earlier stdout-drain nit and the comment-cop length complaints have both been addressed, and no new issues were found on re-review.
Checked that CodepointIterator::next (src/bun_core/string/immutable.rs:106) sets width = 1 and c = U+FFFD on decode failure, so both loops now advance one byte and hit the following \n on the next iteration; the while i < n / !remaining.is_empty() guards make the discarded iter.next() return unreachable-false. The sentinel change in LineOffsetTable (0 → U+FFFD) now trips the c > 0x7F non-ASCII column-tracking arm for ill-formed bytes — traced that through the columns_for_non_ascii extend and add_source_mapping lookup and it produces the same column, so it's a no-op for correctness. The removed .min(remaining.len()) clamp is reproduced inside next() at line 126, and the six truncated-tail cases from #32774 are still in the file.
Extended reasoning...
Overview
Two source-map byte walkers — update_generated_line_and_column_slow in src/sourcemap/Chunk.rs and LineOffsetTable::generate_in in src/sourcemap/LineOffsetTable.rs — are switched from hand-rolled wtf8_byte_sequence_length_with_invalid + decode_wtf8_rune_t to the shared strings::CodepointIterator. The old code advanced by the lead byte's declared width whether or not the decode succeeded, so a Latin-1 byte like 0xE9 (declared width 3) right before \n swallowed the newline and shifted every mapping below by one line. Ten new tests cover both walks independently (legal comment kept in output, plain comment dropped), three ill-formed shapes (bare 2-/3-byte lead, truncated 4-byte), the multi-file chaining case, and the runtime error.stack path.
Security risks
None. Input is source bytes already being parsed; the change replaces one bounded decoder with another. The removed .min(remaining.len()) truncation clamp lives inside CodepointIterator::next (take = (len as usize).min(tail.len())), so no new out-of-bounds read is introduced.
Level of scrutiny
Moderate. The two touched functions feed every source map Bun emits (bundler and runtime stack traces), but the diff is ~30 native lines that swap a bespoke decode loop for the crate's shared iterator — exactly the "grep for the in-tree helper before hand-writing anything" pattern REVIEW.md asks for, and the same iterator LineColumnOffset::advance already uses for the third walk over the same bytes. I verified: the loop guard makes the discarded iter.next() bool unreachable; Cursor { i, ..Default } gives width = 0 so pos = i on entry; the ASCII fast path is preserved inside next()'s first < 0x80 branch (#[inline(always)], so it folds in as before). The LineOffsetTable sentinel changing from 0 to U+FFFD means an ill-formed byte now sets byte_offset_to_first_non_ascii, but since width = 1 and it counts one column, the recorded per-byte columns match what the old byte-offset path would have produced — verified by walking the columns_for_non_ascii extend arithmetic for a single 0xE9 at offset 5.
Other factors
My prior nit (undrained stdout in the bundle helper) was fixed in 34634fc and is visible in the current diff. The comment-cop length flags on both .rs files were addressed in 92d29b1/a492bab — the added comments are now one line each. Test coverage is thorough per REVIEW.md's variant-matrix rule (three ill-formed shapes × two comment kinds, plus the dependency-chaining and template-literal/U+FFFD-column cases), the fixtures use blank lines so the two off-by-ones can't cancel, and the PR notes the tests fail on the released build. _slow is #[cold] and generate_in runs once per source, so the fresh-Cursor-per-iteration pattern (vs. reusing one) is not on a hot path and the 8-byte struct is trivially SROA'd.
This branch has not been deployed
1 participant
Problem
bun build --sourcemapoutput and in the line numbersbun runputs inerror.stack.printf '/*! caf\xe9\nb */\nconsole.log(1);\nconsole.log(2);\n' > in.js && bun build ./in.js --outdir out --sourcemap=external. The output hasconsole.log(1)on its 4th line, butmappingsis;AACA;AAAA,QAAQ,IAAI,CAAC;AACb,...(5 line groups, the statement in the 3rd), and the original line recorded for it is line 1 instead of line 2. With a well-formedéthe same file produces;AAEA;AAAA;AAAA,QAAQ,...(6 groups, original line 2). At runtime, a file starting with/* caf\xe9\nb */reports an error created on line 3 at line 2.wtf8_byte_sequence_length_with_invalid, 3 for0xE9) whether or not the decode succeeded.update_generated_line_and_column_slowinsrc/sourcemap/Chunk.rsdidi += lenafterdecode_wtf8_rune_thad returned the replacement character, andLineOffsetTable::generate_ininsrc/sourcemap/LineOffsetTable.rsdidremaining = &remaining[cp_len..]the same way, so the\ninside the supposed 3-byte sequence never reaches the line terminator arm of either match.Fix
strings::CodepointIteratorinstead of their ownwtf8_byte_sequence_length_with_invalid+decode_wtf8_rune_tcopies, and advance by the width it reports. An ill-formed sequence comes back as one U+FFFD, one byte wide, so the line break is looked at on its own on the next iteration. Well-formed input decodes exactly as before.String::fromUTF8ReplacingInvalidSequences, which turns the bad byte into U+FFFD and keeps the\n, so the positions it reports are what these tables have to line up with; esbuild, where both loops come from, walks the bytes with Go's range decoding, which also steps one byte past an invalid one.CodepointIteratoris also whatLineColumnOffset::advancealready uses, and that is the third walk feeding the same line separators (the glue between files in a bundle, and the positions of the placeholder substitutions insrc/bundler/Chunk.rs), so the three walks now count the same bytes the same way by construction. Open PR lexer: decode ill-formed UTF-8 in JS source as U+FFFD #38262 refinesCodepointIterator's ill-formed handling to one U+FFFD per maximal subpart; these loops pick that up as is, and since a line break is never part of such a subpart the line count is the same either way..min(remaining.len())clamp from Don't panic generating a sourcemap for a source ending in a truncated UTF-8 sequence #32774 lives inside the iterator now; the six truncated-tail cases it added still pass.test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts:bun build --sourcemap=externalon Latin-1 files with an ill-formed sequence (a bare0xE9, a bare 2-byte lead, three bytes of a 4-byte sequence) right before a line break, once in a legal comment (kept in the output, so both loops see it) and once in a plain comment (dropped, so only the original-side table does), decoding the mappings and checking the line each following statement maps back to. The source has blank lines between the statements that the output does not, so the two off-by-ones cannot cancel out. One more case puts the comment in a dependency and checks the entry point's statement is still placed right.test/js/bun/sourcemap/internal-sourcemap.test.ts: the same two comment shapes throughbun runanderror.stack(unfixed they report line 2 and line 5 for an error on line 3), plus an ill-formed byte inside a template literal with, on the error's line, a string the printer re-emits with a well-formed U+FFFD, which still has to count as one column (3:26; unfixed2:26).test/js/bun/sourcemap/,bundler_comments, the source map tests inbundler_edgecase, thebun build --compilesource map tests,test/js/node/module/*sourcemap*,test/cli/test/coverage.test.ts, andtest/bake/dev/{sourcemap,server-sourcemap}.test.ts.Background
LineOffsetTable::generatewalks the source file once and records where every line starts, plus a byte-to-UTF-16-column table for lines containing non-ASCII bytes;add_source_mappingturns a token's byte offset into an original line and column with it. The builder inChunk.rsis called once per mapped token and walks whatever the printer emitted since the previous call, bumping the generated line at each line terminator. The same builder producesbun build's VLQ maps and the compact maps the runtime uses to rewrite stack traces.wtf8_byte_sequence_length_with_invalid(b)returns the number of bytes a UTF-8 sequence starting withbwould have (2 for0xC0..0xDF, 3 for0xE0..0xEF, 4 for0xF0..0xF7), judging by the lead byte alone.decode_wtf8_rune_tthen checks that the following bytes are continuation bytes and returns a caller-supplied sentinel when they are not. The bug was advancing by the first number after the second call had said the sequence was not there.strings::CodepointIterator(src/bun_core/string/immutable.rs) is the shared way of walking raw bytes as code points: it returns the code point and the number of bytes it took up, reporting an ill-formed sequence as U+FFFD with a width of 1, so a caller never steps over bytes that were not part of a valid sequence./*! ... */, copied verbatim, and the only place one can sit right before a line break) and regex literals (which cannot contain a line break); strings and template literals are re-encoded by the printer and other comments are dropped. They get into the source-side table from anywhere in the file.Repro output before and after