diff --git a/src/jsc/bindings/ErrorStackFrame.cpp b/src/jsc/bindings/ErrorStackFrame.cpp index 1e7baf4c2055..e88b20d9732e 100644 --- a/src/jsc/bindings/ErrorStackFrame.cpp +++ b/src/jsc/bindings/ErrorStackFrame.cpp @@ -8,9 +8,10 @@ namespace Bun { using namespace JSC; -/// Adjust a `ZigStackFramePosition` by a number of bytes. This accounts for when the adjustment -/// crosses line boundaries, and thus requires the source code in order to properly compute -/// the result. +/// Adjust a `ZigStackFramePosition` by a number of code units. This accounts for when the +/// adjustment crosses line boundaries, and thus requires the source code in order to properly +/// compute the result. Works on 8-bit and 16-bit sources alike: offsets from JSC are code units +/// either way. void adjustPositionBackwards(ZigStackFramePosition& pos, int amount, CodeBlock* code) { if (pos.byte_position - amount < 0) { @@ -31,20 +32,12 @@ void adjustPositionBackwards(ZigStackFramePosition& pos, int amount, CodeBlock* } auto source = provider->source(); - if (!source.is8Bit()) { - // Debug-only assertion - // Bun does not yet use 16-bit sources anywhere. The transpiler ensures everything - // fit's into latin1 / 8-bit strings for on-average lower memory usage. - ASSERT_NOT_REACHED("16-bit source re-mapping is not implemented here."); - - pos.line_zero_based = 0; - pos.column_zero_based = 0; - pos.byte_position = 0; - return; - } + unsigned length = source.length(); for (int i = 0; i < amount; i++) { - if (source[pos.byte_position - i] == '\n') { + // The position is the end of the text when the callee ends there. + unsigned index = pos.byte_position - i; + if (index < length && source[index] == '\n') { pos.line_zero_based = pos.line_zero_based - 1; } } diff --git a/src/jsc/bindings/ZigException.cpp b/src/jsc/bindings/ZigException.cpp index cfd1059c2e78..bd3946236646 100644 --- a/src/jsc/bindings/ZigException.cpp +++ b/src/jsc/bindings/ZigException.cpp @@ -148,22 +148,22 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr if (flags == PopulateStackTraceFlags::OnlyPosition) return; - if (source_lines_count > 1 && source_lines != nullptr && sourceString.is8Bit()) { - // Search for the beginning of the line - unsigned int lineStart = location.byte_position; + if (source_lines_count > 1 && source_lines != nullptr && !sourceString.isEmpty()) { + unsigned int maxSearch = sourceString.length(); + + // Search for the beginning of the line. The position is the end of the + // text when the expression it belongs to ends there. + unsigned int lineStart = std::min(location.byte_position, maxSearch - 1); while (lineStart > 0 && sourceString[lineStart] != '\n') { lineStart--; } // Search for the end of the line unsigned int lineEnd = location.byte_position; - unsigned int maxSearch = sourceString.length(); while (lineEnd < maxSearch && sourceString[lineEnd] != '\n') { lineEnd++; } - const unsigned char* bytes = sourceString.span8().data(); - // Most of the time, when you look at a stack trace, you want a couple lines above. // It is key to not clone this data because source code strings are large. @@ -177,36 +177,34 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr source_line_numbers[0] = location.line(); if (lineStart > 0) { - auto byte_offset_in_source_string = lineStart - 1; + auto offset_in_source_string = lineStart - 1; uint8_t source_line_i = 1; auto remaining_lines_to_grab = source_lines_count - 1; { - // This should probably be code points instead of newlines - while (byte_offset_in_source_string > 0 && bytes[byte_offset_in_source_string] != '\n') { - byte_offset_in_source_string--; + while (offset_in_source_string > 0 && sourceString[offset_in_source_string] != '\n') { + offset_in_source_string--; } - byte_offset_in_source_string -= byte_offset_in_source_string > 0; + offset_in_source_string -= offset_in_source_string > 0; } - while (byte_offset_in_source_string > 0 && remaining_lines_to_grab > 0) { - unsigned int end_of_line_offset = byte_offset_in_source_string; + while (offset_in_source_string > 0 && remaining_lines_to_grab > 0) { + unsigned int end_of_line_offset = offset_in_source_string; - // This should probably be code points instead of newlines - while (byte_offset_in_source_string > 0 && bytes[byte_offset_in_source_string] != '\n') { - byte_offset_in_source_string--; + while (offset_in_source_string > 0 && sourceString[offset_in_source_string] != '\n') { + offset_in_source_string--; } // We are at the beginning of the line - source_lines[source_line_i] = Bun::toStringView(sourceString.substring(byte_offset_in_source_string, end_of_line_offset - byte_offset_in_source_string + 1)); + source_lines[source_line_i] = Bun::toStringView(sourceString.substring(offset_in_source_string, end_of_line_offset - offset_in_source_string + 1)); source_line_numbers[source_line_i] = location.line().fromZeroBasedInt(location.line().zeroBasedInt() - source_line_i); source_line_i++; remaining_lines_to_grab--; - byte_offset_in_source_string -= byte_offset_in_source_string > 0; + offset_in_source_string -= offset_in_source_string > 0; } } } diff --git a/src/jsc/bindings/bindings.cpp b/src/jsc/bindings/bindings.cpp index e08008192fa1..53edd4926bc7 100644 --- a/src/jsc/bindings/bindings.cpp +++ b/src/jsc/bindings/bindings.cpp @@ -6806,7 +6806,12 @@ extern "C" JSC::EncodedJSValue Bun__REPL__evaluate( auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); - WTF::String source = WTF::String::fromUTF8(std::span { sourcePtr, sourceLen }); + // Same decoder as the module loader: text the transpiler passes through + // verbatim (preserved comments) can be ill-formed UTF-8, which becomes U+FFFD + // here; WTF::String::fromUTF8 returned a null string that evaluated to nothing. + WTF::String source = sourceLen > 0 + ? Zig::convertUTF8ToString(std::span { sourcePtr, sourceLen }) + : WTF::emptyString(); WTF::String filename = filenameLen > 0 ? WTF::String::fromUTF8(std::span { filenamePtr, filenameLen }) : "[repl]"_s; diff --git a/src/sourcemap_jsc/CodeCoverage.rs b/src/sourcemap_jsc/CodeCoverage.rs index 72308d3e3f87..3e617e2e577b 100644 --- a/src/sourcemap_jsc/CodeCoverage.rs +++ b/src/sourcemap_jsc/CodeCoverage.rs @@ -6,11 +6,10 @@ use std::borrow::Cow; use bun_ast::Loc; use bun_collections::VecExt; use bun_collections::bit_set::DynamicBitSet; -use bun_core::{self, Utf8Bytes}; +use bun_core::{self, Utf8Bytes, 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; @@ -641,10 +640,13 @@ 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. + pub(crate) line_starts: Box<[u32]>, /// JSC records coverage per `SourceProvider`, and each load of the file makes one. pub(crate) source_ids: Vec, - /// Of the text `line_offset_table` was built from. + /// Of the text `line_starts` was built from. source_hash: u64, pub source_url: Utf8Bytes<'static>, } @@ -710,7 +712,7 @@ impl ByteRangeMapping { ignore_sourcemap: bool, ) -> Result, bun_alloc::AllocError> { let source_url = self.source_url.slice(); - 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; @@ -1037,9 +1039,36 @@ impl ByteRangeMapping { source_id: i32, source_url: Utf8Bytes<'static>, ) -> 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. + 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. + 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_ids: vec![source_id], source_hash, source_url, diff --git a/test/bundler/bundler_compile.test.ts b/test/bundler/bundler_compile.test.ts index ac147a1d9dbd..fc9694b2bc1c 100644 --- a/test/bundler/bundler_compile.test.ts +++ b/test/bundler/bundler_compile.test.ts @@ -1360,6 +1360,38 @@ error: Hello World`, }, }, }); + // The non-ASCII comment makes JSC hold the module as a 16-bit string, and the + // argument list below `new` makes the position fix-up read that string. + itBundled("compile/NoSourceMapNonAsciiSource", { + target: "bun", + compile: true, + files: { + "/entry.ts": /* js */ ` + /*! © café 中 */ + function code() { + throw new (class Boom extends Error { + constructor(message: string) { + super(message); + } + })("boom"); + } + code(); + `, + }, + run: { + exitCode: 1, + validate({ stderr }) { + expect(stderr).toInclude("| /*! © café 中 */\n"); + expect(stderr).toInclude( + `5 | throw new class Boom extends Error { + ^ +error: boom +`, + ); + expect(stderr).toMatch(/at code \(.*:5:9\)\n/); + }, + }, + }); itBundled("compile/SourceMapBigFile", { target: "bun", compile: true, diff --git a/test/cli/test/coverage.test.ts b/test/cli/test/coverage.test.ts index 3be7a44e6635..41015a470be3 100644 --- a/test/cli/test/coverage.test.ts +++ b/test/cli/test/coverage.test.ts @@ -1006,3 +1006,87 @@ test("b", () => { }); }); }); + +// 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) { + 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); +}); diff --git a/test/js/bun/repl/repl.test.ts b/test/js/bun/repl/repl.test.ts index 5947a8d30a3f..4f3e5ca9b075 100644 --- a/test/js/bun/repl/repl.test.ts +++ b/test/js/bun/repl/repl.test.ts @@ -431,6 +431,28 @@ describe.concurrent("Bun REPL", () => { expect(exitCode).toBe(0); }); + test(".load of a file with an ill-formed byte in a preserved comment still evaluates it", async () => { + // The transpiler passes `/*! */` comments through verbatim, so the program + // handed to the evaluator can contain ill-formed UTF-8. + using dir = tempDir("repl-load-ill-formed", { + "bad.js": Buffer.concat([ + Buffer.from("function tagged() {\n /*! "), + Buffer.from([0xe9]), + Buffer.from(" */\n}\nvar loadedWithComment = 1;\n"), + ]), + }); + const filePath = path.join(String(dir), "bad.js"); + const { outputs, stderr, exitCode } = await runRepl([ + `.load ${filePath}`, + 'JSON.stringify([loadedWithComment, tagged.toString().split("\\uFFFD").length])', + ".exit", + ]); + // `.load` echoes the value of the file's last statement. + expect(outputs).toEqual([`Loading ${filePath}...\n1`, '"[1,2]"']); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + }); + test(".load with a nonexistent file shows the error and keeps running", async () => { // A relative path: Windows rejects a forward-slash absolute path with EINVAL. const { outputs, stderr, exitCode } = await runRepl([ diff --git a/test/js/bun/util/inspect-error.test.js b/test/js/bun/util/inspect-error.test.js index 355ea17b2683..eff3aed5fb04 100644 --- a/test/js/bun/util/inspect-error.test.js +++ b/test/js/bun/util/inspect-error.test.js @@ -510,3 +510,29 @@ describe.concurrent("AggregateError whose errors cannot be walked", () => { expect(exitCode).toBe(1); }); }); + +// The position of `xyz is not defined` is the end of the identifier, which is +// the end of the text here. `Malloc=1` lets ASAN see a read past the source. +describe("source excerpt for an error at the end of the source text", () => { + test.concurrent.each([ + ["8-bit", "xyz"], + ["16-bit", "中文"], + ])("%s source", async (_, source) => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `(0, eval)(${JSON.stringify(source)});`], + env: { ...bunEnv, NO_COLOR: "1", Malloc: "1" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, excerpt: stderr.split("\n").slice(0, 3) }).toEqual({ + stdout: "", + excerpt: [ + `1 | ${source}`, + Buffer.alloc(4 + source.length, " ").toString() + "^", + `ReferenceError: ${source} is not defined`, + ], + }); + expect(exitCode).toBe(1); + }); +});