Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions src/sourcemap/Chunk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,17 +534,19 @@ impl NewBuilder<'_, VLQSourceMap> {
let mut i: usize = 0;
let n: usize = slice.len();
let mut c: i32;
// 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.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

let iter = strings::CodepointIterator::init(slice);
while i < n {
let len = strings::wtf8_byte_sequence_length_with_invalid(slice[i]);
let mut cp_bytes = [0u8; 4];
let take = (len as usize).min(n - i);
cp_bytes[..take].copy_from_slice(&slice[i..i + take]);
c = strings::decode_wtf8_rune_t::<i32>(
cp_bytes,
len,
strings::UNICODE_REPLACEMENT as i32,
);
i += len as usize;
let mut cursor = strings::Cursor {
i: i as u32,
..Default::default()
};
let _ = iter.next(&mut cursor);
c = cursor.c;
i += cursor.width as usize;

match c {
14..=127 => {
Expand Down
32 changes: 14 additions & 18 deletions src/sourcemap/LineOffsetTable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,27 +186,23 @@ impl LineOffsetTable {
// Hoist the base pointer so per-iteration offset math is a single sub + truncate.
let base = contents.as_ptr() as usize;

// `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.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

let iter = strings::CodepointIterator::init(contents);
let mut remaining = contents;
while !remaining.is_empty() {
let b0 = remaining[0];
let len_ = strings::wtf8_byte_sequence_length_with_invalid(b0);
// After the SIMD skip below lands, the loop head
// is overwhelmingly an ASCII '\r'/'\n' or a non-ASCII lead byte, so keep the
// 1-byte path branch-only and confine the zero+copy pad to the cold
// multibyte arm.
// `len_` is the lead byte's *declared* width; a source whose final bytes are
// a truncated multibyte sequence declares more bytes than remain, so every
// slice below (decode, SIMD-skip offset, advance) must use the clamped width.
let cp_len = (len_ as usize).min(remaining.len());
let c: i32 = if len_ == 1 {
b0 as i32
} else {
let mut cp_bytes = [0u8; 4];
cp_bytes[..cp_len].copy_from_slice(&remaining[..cp_len]);
strings::decode_wtf8_rune_t::<i32>(cp_bytes, len_, 0)
};

let offset = (remaining.as_ptr() as usize - base) as u32;
let mut cursor = strings::Cursor {
i: offset,
..Default::default()
};
let _ = iter.next(&mut cursor);
let c: i32 = cursor.c;
let cp_len = cursor.width as usize;

if column == 0 {
line_byte_offset = offset;
Expand Down
85 changes: 85 additions & 0 deletions test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { internalSourceMap } from "bun:internal-for-testing";
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";
import { readFileSync } from "node:fs";
import path from "node:path";

// --- Reference VLQ codec (mirrors the spec; used as the oracle) ---
Expand Down Expand Up @@ -491,6 +492,90 @@
});
});

// Both sides of a mapping come from walking raw bytes: LineOffsetTable::generate
// walks the source and the builder in Chunk.rs walks the printed output. A lead
// byte that is not followed by the continuation bytes it declares (a Latin-1
// file, a sequence cut short) is one character, one byte wide. Stepping over the
// declared width instead swallows the line break after it, and every mapping
// from there on is a line off.
describe.concurrent("sourcemap of a source with an ill-formed UTF-8 sequence right before a line break", () => {
// File contents are given as latin1 text, so "\xe9" is the one byte 0xE9 on
// disk; the output is read back the same way. Lines are 0-based like the map's.
async function bundle(latin1Files: Record<string, string>, entry: string) {
using dir = tempDir(
"sourcemap-ill-formed-utf8",
Object.fromEntries(Object.entries(latin1Files).map(([name, text]) => [name, Buffer.from(text, "latin1")])),
);
await using proc = Bun.spawn({
cmd: [bunExe(), "build", "--sourcemap=external", "--outdir=out", entry],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
expect({ exitCode, stderr }).toEqual({ exitCode: 0, stderr: "" });

Check warning on line 517 in test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts

View check run for this annotation

Claude / Claude Code Review

bundle() helper pipes stdout but never drains it

The `bundle` helper spawns with `stdout: "pipe"` but only awaits `Promise.all([proc.stderr.text(), proc.exited])` — stdout is piped but never drained, which REVIEW.md's "Subprocess tests: drain pipes concurrently" rule and the neighboring "truncated trailing UTF-8 sequence" block in this file both require. Add `proc.stdout.text()` to the `Promise.all` (or drop `stdout: "pipe"`); on these tiny inputs it can't deadlock today, so this is just a convention nit.
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
const outfile = path.join(String(dir), "out", entry);
const outputLines = readFileSync(outfile, "latin1").split("\n");
const map = JSON.parse(readFileSync(outfile + ".map", "utf8"));
const segs = decodeMappings(map.mappings);
// For each statement: the mapping at the start of the output line it was
// printed on, i.e. what a stack trace through that line would be remapped to.
return (statements: string[]) =>
statements.map(statement => {
const generatedLine = outputLines.findIndex(line => line.includes(statement));
expect(generatedLine).toBeGreaterThanOrEqual(0);
const seg = referenceFind(segs, generatedLine, 0);
return { statement, mapped: seg && { source: map.sources[seg.srcIdx], line: seg.origLine } };
});
}

const illFormed = [
["Latin-1 0xE9, a 3-byte lead with no continuation bytes", "\xe9"],
["0xC3, a 2-byte lead with no continuation byte", "\xc3"],
["0xF0 0x9F 0x92, three bytes of a 4-byte sequence", "\xf0\x9f\x92"],
];

// Source lines 0-1 are the comment, `first` is on line 2 and `second` on line
// 5. The output drops the blank lines between them, so the two walks cannot
// each be a line off and still agree with each other by accident.
const rest = '\nb */\nconsole.log("first");\n\n\nconsole.log("second");\n';
const statements = ['console.log("first")', 'console.log("second")'];
const expected = [
{ statement: 'console.log("first")', mapped: { source: "../in.js", line: 2 } },
{ statement: 'console.log("second")', mapped: { source: "../in.js", line: 5 } },
];

// A legal comment is copied into the output, so both walks see the bytes.
test.each(illFormed)("in a legal comment: %s", async (_name, bytes) => {
const mappingsOf = await bundle({ "in.js": "/*! caf" + bytes + rest }, "in.js");
expect(mappingsOf(statements)).toEqual(expected);
});

// A plain comment is dropped from the output, so only the source walk sees them.
test.each(illFormed)("in a plain comment: %s", async (_name, bytes) => {
const mappingsOf = await bundle({ "in.js": "/* caf" + bytes + rest }, "in.js");
expect(mappingsOf(statements)).toEqual(expected);
});

// In a bundle the files' mappings are chained: each file's start where the
// line count of the file printed before it left off, so a file that counts
// one of its own line breaks short also pulls every file after it up a line.
test("in a legal comment of a file printed ahead of the entry point", async () => {
const mappingsOf = await bundle(
{
"in.js": 'import "./dep.js";\nconsole.log("entry");\n',
"dep.js": '/*! caf\xe9\nb */\nconsole.log("dep");\n',
},
"in.js",
);
expect(mappingsOf(['console.log("dep")', 'console.log("entry")'])).toEqual([
{ statement: 'console.log("dep")', mapped: { source: "../dep.js", line: 2 } },
{ statement: 'console.log("entry")', mapped: { source: "../in.js", line: 1 } },
]);
});
});

// `sources` entries in the emitted map are URLs: the path from the chunk
// directory to each source must use forward slashes on every platform, never
// the host path separator.
Expand Down
47 changes: 46 additions & 1 deletion test/js/bun/sourcemap/internal-sourcemap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function extractPositions(stack: string): string[] {
.filter((s): s is string => s !== null);
}

async function run(files: Record<string, string>, env: Record<string, string> = {}) {
async function run(files: Record<string, string | Buffer>, env: Record<string, string> = {}) {
using dir = tempDir("internal-sourcemap", files);
await using proc = Bun.spawn({
cmd: [bunExe(), "index.ts"],
Expand Down Expand Up @@ -227,4 +227,49 @@ describe("InternalSourceMap", () => {
}
expect(exited).toBe(0);
});

// Both line tables behind a remapped frame are built by walking raw bytes: the
// source's by LineOffsetTable::generate, the transpiled output's by the builder.
// A lead byte without the continuation bytes it declares (the file is Latin-1,
// not UTF-8) is one character, one byte wide; stepping over the declared width
// instead swallows the line break after it and every frame below comes out a
// line early. The fixtures are written to disk as latin1, so "\xe9" is the one
// byte 0xE9.
describe.concurrent("an ill-formed UTF-8 byte right before a line break", () => {
async function framePositions(latin1Source: string) {
const { stdout, stderr, exited } = await run({ "index.ts": Buffer.from(latin1Source, "latin1") });
expect(stderr).toBe("");
expect(exited).toBe(0);
return [...stdout.matchAll(/index\.ts:(\d+:\d+)/g)].map(m => m[1]);
}

test("in a comment that is dropped from the output", async () => {
// Lines: 1 "/* caf<E9>", 2 "b */", 3 the Error, 4 console.log.
const positions = await framePositions(
'/* caf\xe9\nb */\nconst err = new Error("x");\nconsole.log(err.stack);\n',
);
expect(positions).toEqual(["3:17"]);
});

test("in a legal comment that is kept in the output", async () => {
// Lines: 1 "/*! caf<E9>", 2 "b */", 3 the Error, 6 console.log. The output
// has no blank lines, so the walks over the source and over the output
// cannot each be a line off and still produce the right answer together.
const positions = await framePositions(
'/*! caf\xe9\nb */\nconst err = new Error("x");\n\n\nconsole.log(err.stack);\n',
);
expect(positions).toEqual(["3:17"]);
});

test("in a template literal, next to a U+FFFD the printer wrote out for another one", async () => {
// Line 1 breaks inside the template literal right after the ill-formed
// byte. The string on line 3 is printed with a real U+FFFD (three bytes,
// one column) ahead of the Error on the same line, which the walk over
// the output must not mistake for an ill-formed byte either.
const positions = await framePositions(
'const s = `caf\xe9\n`;\nconst err = ["caf\xe9", new Error("x")][1];\nconsole.log(err.stack);\n',
);
expect(positions).toEqual(["3:26"]);
});
});
});