Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions src/runtime/cli/test_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1595,8 +1595,8 @@ impl CommandLineReporter {
// SAFETY: thread-local Box pinned for the thread; sole `&mut` for the
// collection loop below (single-threaded CLI report path).
let map = unsafe { &mut *map.as_ptr() };
// `ByteRangeMapping` owns a `MultiArrayList` and is not `Copy`, so
// collect mutable borrows into the thread-local map instead no
// `ByteRangeMapping` owns its line table and is not `Copy`, so collect
// mutable borrows into the thread-local map instead; there is no
Comment on lines +1598 to +1599

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

// double-free risk.
let mut byte_ranges: Vec<&mut ByteRangeMapping> = Vec::with_capacity(map.len());
for entry in map.values_mut() {
Expand Down
42 changes: 35 additions & 7 deletions src/sourcemap_jsc/CodeCoverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ use core::ptr::NonNull;
use bun_ast::Loc;
use bun_collections::VecExt;
use bun_collections::bit_set::DynamicBitSet;
use bun_core::{self, ZigStringSlice};
use bun_core::{self, ZigStringSlice, strings};
use bun_jsc::{JSGlobalObject, JSValue, VM, bun_string_jsc};
use bun_sourcemap::{
LineOffsetTable, LineOffsetTableColumns as _, Ordinal, ParsedSourceMap, internal_source_map,
line_offset_table,
};

type LinesHits = Vec<u32>;
Expand Down Expand Up @@ -417,7 +416,10 @@ pub struct BasicBlockRange {
}

pub struct ByteRangeMapping {
pub(crate) line_offset_table: line_offset_table::List,
/// Offset of the start of each line, in UTF-16 code units: the unit JSC
/// reports `BasicBlockRange` offsets in. Equal to the byte offset only
/// while the source text is pure ASCII.
Comment on lines +419 to +421

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

pub(crate) line_starts: Box<[u32]>,
pub(crate) source_id: i32,
pub source_url: ZigStringSlice,
}
Expand Down Expand Up @@ -483,7 +485,7 @@ impl ByteRangeMapping {
function_blocks: &[BasicBlockRange],
ignore_sourcemap: bool,
) -> Result<Report, bun_alloc::AllocError> {
let line_starts = self.line_offset_table.items_byte_offset_to_start_of_line();
let line_starts = &*self.line_starts;

let mut executable_lines: Bitset;
let mut lines_which_have_executed: Bitset;
Expand Down Expand Up @@ -808,16 +810,42 @@ impl ByteRangeMapping {
source_id: i32,
source_url: ZigStringSlice,
) -> ByteRangeMapping {
let mut line_offset_table = LineOffsetTable::generate(source_contents, 0)
.unwrap_or_else(|_| bun_alloc::out_of_memory());
let byte_starts = line_offset_table.items_byte_offset_to_start_of_line();

let line_starts: Box<[u32]> = if strings::is_all_ascii(source_contents) {
Box::from(byte_starts)
} else {
// `source_contents` is the UTF-8 form of the string JSC holds
// (8-bit or 16-bit), and JSC's offsets count that string's code
// units, so re-measure each line in those.
Comment on lines +820 to +822

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 mut previous_byte_start: usize = 0;
let mut code_units: u32 = 0;
byte_starts
.iter()
.map(|&byte_start| {
let byte_start = byte_start as usize;
let line = &source_contents[previous_byte_start..byte_start];
code_units += u32::try_from(strings::element_length_utf8_into_utf16(line))
.expect("int cast");
previous_byte_start = byte_start;
code_units
})
.collect()
};
// `MultiArrayList`'s own `Drop` frees the slab only; this drops each
// row's `columns_for_non_ascii` box.
Comment on lines +837 to +838

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

line_offset_table.drop_elements();

ByteRangeMapping {
line_offset_table: LineOffsetTable::generate(source_contents, 0)
.unwrap_or_else(|_| bun_alloc::out_of_memory()),
line_starts,
source_id,
source_url,
}
}
}

// line_offset_table drops automatically.
// source_url is NOT freed (caller owns it).

#[unsafe(no_mangle)]
Expand Down
84 changes: 84 additions & 0 deletions test/cli/test/coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -589,3 +589,87 @@ Ran 1 test across 1 file."
`);
expect(result.exitCode).toBe(0);
});

// JSC reports the offsets of executed and unexecuted blocks in UTF-16 code
// units of the source text it holds, and the line table those offsets are
// looked up in was built in bytes, so line attribution drifted after any
// non-ASCII text. Each non-ASCII variant below has exactly the same number of
// code units on every line as the ASCII twin (an astral character is two code
// units and four bytes), so its lcov record must be identical to the twin's;
// counting bytes or code points shifts it.
//
// Both lengths are in UTF-16 code units.
const coverageBannerUnits = 30;
const coverageTextUnits = 40;
function coverageDemo(banner: string, text: string) {
return `/*! ${banner} */
export const raw = String.raw\`${text}\`;
export const re = /${text}/u;
export function covered() {
return raw.length + re.source.length;
}
export function uncovered() {
return 1;
}
export function alsoUncovered() {
return 2;
}
`;
}
const coverageDemoTest = `
import { expect, test } from "bun:test";
import { covered } from "./demo";

test("source text has the expected length", () => {
expect(covered()).toBe(${2 * coverageTextUnits});
});
`;
function repeatUnits(piece: string, units: number) {
return Buffer.alloc((Buffer.byteLength(piece) * units) / piece.length, piece).toString();
}

// lcov records of `demo.ts` for each variant, in order.
async function coverageRecords(variants: Record<string, string>) {
using dir = tempDir(
"cov-non-ascii",
Object.fromEntries(
Object.entries(variants).flatMap(([name, source]) => [
[`${name}/demo.ts`, source],
[`${name}/demo.test.ts`, coverageDemoTest],
]),
),
);
// `await` here keeps `dir` alive until the processes have finished.
return await Promise.all(
Object.keys(variants).map(async variant => {
const cwd = path.join(String(dir), variant);
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "--coverage", "--coverage-reporter=lcov", "./demo.test.ts"],
cwd,
env: bunEnv,
stdout: "ignore",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
expect(stderr).toContain(" 1 pass\n");
expect(exitCode).toBe(0);
const record = readFileSync(path.join(cwd, "coverage", "lcov.info"), "utf8")
.split("end_of_record")
.find(record => record.includes("\nSF:demo.ts\n"));
expect(record).toBeDefined();
return record!;
}),
);
}

test("a non-ASCII preserved comment does not shift coverage lines", async () => {
const asciiText = repeatUnits("x", coverageTextUnits);
const [ascii, legalComment] = await coverageRecords({
ascii: coverageDemo(repeatUnits("x", coverageBannerUnits), asciiText),
legalComment: coverageDemo(repeatUnits("©🐰", coverageBannerUnits), asciiText),
});
// `uncovered` and `alsoUncovered` start on lines 7 and 10.
expect(ascii).toContain("\nDA:7,0\n");
expect(ascii).toContain("\nDA:10,0\n");
expect(legalComment).toBe(ascii);
});
Loading