Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The encoding math checks out and the test is well-constructed, but since this is a runtime logic change with a struct field replacement and explicit drop_elements() handling, a human sign-off would be worthwhile.
What was reviewed:
- Verified the code-unit re-measurement is correct for both Latin-1 and UTF-16 JSC strings (© = 1 unit / 2 UTF-8 bytes; 🐰 = 2 units / 4 bytes;
element_length_utf8_into_utf16via simdutf agrees). - Checked
find_indexis a plain binary search over sortedu32offsets, so feeding it code-unit offsets instead of byte offsets is sound. - Confirmed no other consumers of the removed
line_offset_tablefield;Box::from(byte_starts)copies beforedrop_elements(), and the borrow ends before the&mutcall. - The comment at
test_command.rs:1598("owns aMultiArrayList") is now stale but the code it justifies is unchanged and still correct.
Extended reasoning...
Overview
This PR fixes coverage line misattribution for source files containing non-ASCII characters. ByteRangeMapping in src/sourcemap_jsc/CodeCoverage.rs previously stored a LineOffsetTable (byte offsets of line starts) and looked up JSC's code-unit block offsets against it — the two units only agree for pure ASCII. The struct now stores line_starts: Box<[u32]> in UTF-16 code units, computed by re-measuring each line with strings::element_length_utf8_into_utf16 when the source is not all-ASCII. The full LineOffsetTable (including per-line columns_for_non_ascii boxes) is now freed immediately via drop_elements() rather than living in the thread-local map. A regression test in test/cli/test/coverage.test.ts runs identical modules with ASCII vs. ©🐰 banners of equal code-unit length and requires identical lcov DA: records.
Security risks
None. This is pure offset arithmetic on already-decoded module source text; no new inputs, no parsing of untrusted data, no allocation sizing driven by external values beyond what already existed.
Level of scrutiny
Medium. This is a real algorithmic change to runtime code, not a mechanical tweak. The reasoning (JSC 8-bit Latin-1 and 16-bit UTF-16 both round-trip through UTF-8 to the same code-unit count JSC reports) is correct but subtle enough that a maintainer should confirm the approach — particularly since the column_position computed in the sourcemapped branch is now in code units rather than the previous mixed-unit subtraction, which is more correct but interacts with the internal-sourcemap column semantics that PR 4 of the stack (#33866) addresses.
Other factors
- The struct field change is fully contained: grep confirms no other reader of
.line_offset_tableonByteRangeMapping; the only external references to the type (test_command.rs, C++ FFI inZigSourceProvider.cpp/InternalModuleRegistry.cpp) go through unchanged accessors. - Memory handling is sound:
Box::from(&[u32])deep-copies beforedrop_elements()runs;drop_elements()setslen = 0so the subsequentMultiArrayListDrop only frees the slab. - The test follows harness conventions (
tempDir,bunEnv, concurrent spawns with drained stderr, content assertion before exit-code assertion) and asserts specificDA:7,0/DA:10,0lines on the ASCII baseline before the equality check, so it cannot pass vacuously. - Minor: the loop variable
byte_offsetandline_start_byte_offsetingenerate_report_from_blocksare now misnomers (they hold code units), andtest_command.rs:1598's comment is stale — neither affects correctness.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
f856ca8 to
785825f
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The encoding logic checks out (UTF-16 code-unit count of the UTF-8 re-encoding equals JSC's offset for both 8-bit Latin-1 and 16-bit strings), and the test exercises both BMP and astral characters. Because it's part of a 4-PR stack and the correctness argument hinges on JSC string-representation invariants, a human look would still be worthwhile.
What was reviewed:
compute()conversion: verifiedelement_length_utf8_into_utf16yields the right count for Latin-1→UTF-8 (© = 2 bytes → 1 unit) and surrogate pairs (🐰 = 4 bytes → 2 units), matching JSC's index unit in both storage modes.drop_elements()ordering:byte_startsis copied into theBox<[u32]>before the mutable borrow; the slab itself is freed byMultiArrayList::Dropat scope exit, so no leak or UAF.- No other consumers of the removed
line_offset_tablefield;find_indexis agnostic to the offset unit. - The comment-cop bot flags look like false positives — the flagged comments are 2-3 line field/unit documentation, not workaround justifications.
Extended reasoning...
Overview
This PR changes ByteRangeMapping in src/sourcemap_jsc/CodeCoverage.rs to store line-start offsets in UTF-16 code units instead of UTF-8 bytes, fixing coverage line misattribution when source text contains non-ASCII characters. The struct's line_offset_table: MultiArrayList<...> field becomes line_starts: Box<[u32]>; compute() now branches on is_all_ascii and re-measures each line via element_length_utf8_into_utf16 when needed. A comment in test_command.rs is reworded to match the new field, and a new differential test in coverage.test.ts compares lcov records for an ASCII vs. ©🐰 banner of equal code-unit length.
Security risks
None. This is offline coverage-report post-processing over source text the runtime already loaded; no new parsing of untrusted input, no auth/crypto/network paths.
Level of scrutiny
Medium-high. The change is small in line count but rests on a non-obvious encoding invariant: that for both JSC string kinds (8-bit Latin-1 and 16-bit UTF-16), the UTF-16 code-unit length of the UTF-8 re-encoding equals the index unit JSC reports. I verified this holds (Latin-1 chars are all BMP → 1 unit each; surrogate pairs round-trip to 2 units), and the PR description states it clearly, but it is exactly the kind of claim a maintainer familiar with the JSC source-provider path should confirm. The column_position calculation in the sourcemap branch now subtracts two code-unit values instead of the previous mixed byte/code-unit subtraction — strictly more correct, but worth a maintainer glance since sourcemap column semantics are involved. The drop_elements() addition changes when per-row column boxes are freed; borrow ordering is correct (the &[u32] is consumed before the &mut call) and the slab is still freed by the list's own Drop.
Other factors
- Second of a 4-PR stack split from #33866; the description notes the helper is shaped for a follow-up variant test, so a human should confirm the split boundaries are what they want.
- The new test is well-constructed: asserts specific
DA:7,0/DA:10,0lines on the ASCII baseline before comparing, uses an astral character so code-unit ≠ codepoint ≠ byte, and drains stderr/exit concurrently. - comment-cop left five automated "paragraph-long comment" flags; on inspection each is a short (2-3 line) unit-documentation comment rather than a workaround justification, so I read them as bot noise, but they are technically outstanding on the thread.
|
Rebased onto #38708's head a865431 (main 4b02e10) as aa48a4a on 2026-10-02. One commit.
CI for aa48a4a, #122756: 181 of 182 jobs green. The one red job is the x64-asan test lane, where |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Checked the code-unit invariant for both string shapes JSC can hold here: an 8-bit string with characters above 0x7F (to_utf8 re-encodes each byte to two, element_length_utf8_into_utf16 counts one unit each) and a 16-bit string (astral pairs four bytes / two units, lone surrogates U+FFFD / one unit); both are 1:1 with JSC's offsets. Every other offset use in CodeCoverage.rs now compares against the unit table, including the sourcemap-branch column, which matches what Bun's own maps emit. drop_elements is a real method, frees the per-row column boxes MultiArrayList's Drop leaves behind (a small pre-existing leak), and the later Drop is slab-only. The test fails on 1.3.14 exactly as described (DA:7/DA:10 vs DA:4/5/6, FNH identical). Fine to land from my side.
Nits: the body says the diff is two files but the second commit also touches src/runtime/cli/test_command.rs (a comment); is_all_ascii re-scans text LineOffsetTable::generate already classified (its first-non-ASCII column would do), once per file under --coverage so not worth blocking on; repeatUnits silently needs units % piece.length == 0, both call sites satisfy it, a one-line note would guard the follow-up variants.
|
Thanks. Body now mentions the |
6f9b751 to
6f9b53b
Compare
785825f to
fabdf83
Compare
There was a problem hiding this comment.
LGTM — focused fix that rebuilds the coverage line table in UTF-16 code units so JSC's block offsets map to the right lines when the source contains non-ASCII text.
What was reviewed:
- Verified the code-unit re-measurement is correct for both JSC string kinds: 8-bit Latin-1 (each char = 1 UTF-16 unit) and 16-bit UTF-16 —
element_length_utf8_into_utf16on the UTF-8 re-encoding recovers JSC's offset unit in both cases. - Confirmed
MultiArrayList::Dropis slab-only, so the explicitdrop_elements()correctly frees each row'scolumns_for_non_asciibox with no double-free (it zeroeslen); the old code held the full table for the mapping's lifetime and leaked those boxes on drop. - Checked all four
find_indexcall sites now compare like units (code-unit offsets vs. code-unit line starts); no other consumers of the removedline_offset_tablefield exist. - Test design: twin lcov comparison with a
©🐰banner (30 code units either way; astral char distinguishes code units from both bytes and code points), non-vacuous baseline check on lines 7/10,using dirkept alive by the awaitedPromise.all. Fails on main, passes on branch per the gate output.
Extended reasoning...
Overview
The PR fixes bun test --coverage line misattribution when module source text contains non-ASCII characters. JSC reports BasicBlockRange offsets in code units of the string it holds (Latin-1 bytes for 8-bit strings, UTF-16 units for 16-bit strings), but ByteRangeMapping::compute was building its line-start table from UTF-8 byte offsets of the re-encoded source. The two agree only for pure ASCII. The fix stores line starts in UTF-16 code units — using byte offsets directly when is_all_ascii holds, otherwise re-measuring each line via element_length_utf8_into_utf16. The struct field changes from a full line_offset_table::List to a plain Box<[u32]>; the intermediate MultiArrayList is drained with drop_elements() so its per-row columns_for_non_ascii boxes don't leak. test_command.rs gets a comment-only update to stop referring to the removed field.
Security risks
None. This is coverage-report line-number arithmetic in the CLI test runner; no untrusted input reaches new allocation sizes or indexing beyond what the existing code already did, and the only new arithmetic is a running u32 code-unit sum bounded by the source length.
Level of scrutiny
Medium. Encoding/offset math is subtle, but the change is small (~40 net lines of Rust), confined to ByteRangeMapping::compute plus a field-type swap, and the consumer loops in generate_report_from_blocks are unchanged — they already treated line_starts as an opaque monotone [u32] for binary search. I traced both JSC string encodings through the new path and they both reduce to the same UTF-16 code-unit count that JSC's offsets index. I also confirmed via multi_array_list.rs that Drop is slab-only and drop_elements() sets len = 0, so the explicit drain is correct and the subsequent implicit Drop only frees the slab.
Other factors
- The new test is well-constructed: it compares full lcov records between an ASCII twin and a
©🐰variant with identical per-line code-unit lengths, asserts the ASCII baseline reports the expected uncovered lines (7 and 10) so the equality check isn't vacuous, and uses an astral character so counting bytes or code points would fail. The PR body shows it failing on main and passing on the branch under both debug+ASAN and release. - The comment-cop bot warnings on the PR are a length heuristic; the flagged comments document the unit of
line_startsand whydrop_elementsis needed — both are non-obvious and worth keeping. - Second of a four-PR stack; the description states nothing here depends on #38708 and the diff is self-contained.
…ix-up and the REPL A module whose text contains any non-ASCII character (a preserved comment is enough) is held by JSC as a 16-bit string. Three readers assumed 8-bit: - ZigException.cpp only rendered the source preview for 8-bit sources, reading them through span8(); it now indexes the string, which works for both widths, so such modules get a preview again. - adjustPositionBackwards in ErrorStackFrame.cpp gave up on 16-bit sources and reset the frame to 1:1 whenever moving a constructor call's position back to its new keyword crossed a line; the offsets it works with are code units in either width, so the 8-bit-only check is removed. - Bun__REPL__evaluate decoded the program with WTF::String::fromUTF8, which returns a null string for ill-formed input; a preserved comment passes through the transpiler verbatim, so a stray byte in one made the whole input evaluate to nothing. It now uses the same decoder as the module loader, which substitutes U+FFFD.
6f9b53b to
6b80bea
Compare
…text The excerpt scan in populateStackFramePosition starts at the error's position, which is the length of the source when the expression it belongs to ends the text (an identifier that is not defined, for example). The scan then read one code unit past the source. Start it at the last code unit, and skip the excerpt for an empty source. adjustPositionBackwards gets the same bound for the same position. 8-bit sources had this read before. 16-bit sources reach it now that they get an excerpt.
JSC reports the offsets of executed and unexecuted blocks in code units of the source string it holds, but ByteRangeMapping looked them up in a table of byte offsets built from the UTF-8 form of that string, so line attribution drifted after any non-ASCII text in the module. Today a non-ASCII preserved comment is enough: every line after it is misattributed and functions that never ran are reported as covered. The table now stores each line start in code units (the byte offsets are kept as-is while the text is ASCII), and the per-row column tables the line offset table allocates, which were never used here, are freed instead of being kept for the life of the mapping.
fabdf83 to
aa48a4a
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/sourcemap_jsc/CodeCoverage.rs— Users runningbun test --coverageon a module whose JSC text does not end in a newline get no line coverage for its final line, so an unexecuted last-line function never appears in the lcov record. In the loops at src/sourcemap_jsc/CodeCoverage.rs:762 and :808 every offset on the final line is fed toLineOffsetTable::find_index, which at src/sourcemap/LineOffsetTable.rs:134-149 only returnsSomewhen ani + 1entry exists or the offset equals the line start, and the equal case is then skipped by the>=guard at :771. Fix: treat offsets at or past the last entry ofline_startsas belonging to the last line (fall back to the last index instead ofNone) in all four lookup loops, and add a fixture whose module lacks a trailing newline.Why this was flagged
Trigger: a coverage-instrumented module whose text as JSC holds it has content on the last line with no trailing newline. Entry point:
ByteRangeMapping::generate_report_from_blocksat src/sourcemap_jsc/CodeCoverage.rs:708, called for every covered file. For an offset on the last line,LineOffsetTable::find_index(src/sourcemap/LineOffsetTable.rs:121-150) findsbyte_offset < loc_startbuti + 1 < lenis false, so it advances past the end and returnsNone; theelse { continue; }at CodeCoverage.rs:767 drops the offset. The only offset that returnsSomeis the exact line start, which the>=guard at CodeCoverage.rs:771 then skips. Result:executable_linesandline_hitsnever see the last line, so lcov omits itsDA:record and the function-level!did_fn_executefill at :830-837 cannot mark it either. The base used the samefind_indexon the byte table, but this PR rewrote the table these loops consume without fixing the lookup that still drops the final line.Verification: Pre-existing. Trigger: the text JSC holds for a covered file has content on its last line and no trailing newline. src/sourcemap/LineOffsetTable.rs:121-150
find_index: for an offset strictly after the last entry's start,i + 1 < lenis false, so it returnsNone; the onlySomeis the exact line-start hit, which src/sourcemap_jsc/CodeCoverage.rs:770-773 skips.
|
On the review's note about a last line with no newline after it: I could not reproduce it. The text that JSC holds for a transpiled module ends with a newline, because the printer writes one after the last statement. Probe: a |
a865431 to
ec41078
Compare
|
Closing: #45677 fixes this on main.
|
Split out of #33866. Based on #38708 for ordering only.
Problem
bun test --coveragereports the wrong lines for a file whose text has a non-ASCII character. A licence banner (/*! © ... */) is enough: each line after it shifts, and functions that never run vanish from the lcov record.ByteRangeMapping(src/sourcemap_jsc/CodeCoverage.rs) looked them up in a table of byte offsets into the UTF-8 form of that string. The two agree only for ASCII text.Fix
element_length_utf8_into_utf16.test/cli/test/coverage.test.ts. It fails on main.Background
DA:<line>,<count>record is what--coverage-reporter=lcovwrites for each line.Downsides
--coverage, per loaded file: one more scan of the text (is_all_ascii) and one copy of 4 bytes per line. Non-ASCII text gets a second scan.Notes
What the test does
©🐰banner of the same length in code units (an astral character makes code units differ from both bytes and code points). It requires identical lcov records, after a check that the ASCII record has the two functions that never run on lines 7 and 10.DA:7,0andDA:10,0becomeDA:4,0,DA:5,0andDA:6,0.Names
byte_offsetlocals in the report loops and the nameByteRangeMappingfollow JSC's own "byte range" term for these offsets. They stay as they are. The unit is documented on the field they are compared with.Rebase of 2026-10-02
ByteRangeMappingin coverage: count every load of a file, not only the last one #43158 and bun test --parallel: send coverage and test results to the coordinator as data, not rendered LCOV/XML #40678: it now keeps a list of source ids and a hash of the text. The conflict resolution keeps both and replaces only the line table.src/runtime/cli/test_command.rs. Main rewrote that function, the comment no longer exists, and the commit is dropped.--paralleltests (the container's load average was above 500). CI runs with a 90 s limit per test.[review] gate passed · iteration 18 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 18
evidence per changed file