Repository navigation
Conversation
When the printer has no kept frame for a place, it asks for the text of the module to cut the source lines above the error. The module loader answered that with a print mode: it made a string of the whole file, and the printer turned the string back into bytes. `VirtualMachine::original_source_text` gives the printer the bytes: a virtual source, the body of a `data:` URL, or the file. It transpiles nothing and makes no string. `FetchFlags` and the arena flag of `zig_exception::Holder` have no user left and are gone. A line of the frame that is not UTF-8 becomes a string, so that it shows U+FFFD as before. The text above the frame is not looked at for that.
|
Status How to reproduce:
PR: #47770, stacked on #47659. CI: build 136973 is red on 28 test files, 27 under |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A line of the frame that is not UTF-8 grows when it becomes a string: U+FFFD is three bytes. If the lines then pass 4 GiB, their ranges would wrap, so the frame is not shown. Tests: the rows for a carriage return alone check that the printer writes UTF-8, not which bytes the line cutter puts in the line. The row for a text that a string cannot hold lowers the limit of a string in place of a 2 GiB file. The executable test runs alone, so it needs no timeout.
There was a problem hiding this comment.
I re-reviewed after 89bc108 and found no new bugs; because this rewires the error printer's source-loading path through the module loader and deletes the parser-arena reset that Holder::deinit used to perform, a human look would still be worthwhile.
What was reviewed:
- The u32-overflow guard in
code_frame_lines_as_utf8: clearing bothrangesandlinesdrops the already-wrapped ranges, and the empty-code path falls through tocollect_source_linesas before. fetch_without_on_load_pluginsnow resets the arena unconditionally viaArenaResetGuard;original_source_textnever touches the transpiler, so no path is left depending on the deleted deferred reset.read_source_textcloses the fd the resolver cache leaves open for the watcher, andoriginal_source_textcaps text atString::max_length()before theu32line ranges are cut.- The reworked tests replace the 2 GiB sparse-file case with
BUN_FEATURE_FLAG_SYNTHETIC_MEMORY_LIMITand move the compile test out of the concurrent block without a timeout.
Extended reasoning...
The diff touches src/jsc/VirtualMachine.rs (about 300 lines), src/runtime/jsc_hooks.rs, ModuleLoader.rs, ZigException.rs, ConsoleObject.rs and a 500-line test block, replacing the transpiler-backed PrintSource fetch with a raw byte read for code frames and deleting FetchFlags and the Holder-driven parser arena reset. It touches no auth, crypto or injection surface; the sensitive part is memory ownership (arena reset timing, fd handling, u32 line ranges over possibly multi-GiB text). The latest commit addresses the blocking overflow finding from the prior review and the hunt ran dry, but the change is large, authored by a bot account, and alters unconditional-vs-conditional behaviour in the module loader (watcher registration and vitest checks no longer gated), so a maintainer should still look rather than this being approved automatically.
Stacked on #47659. It is the load behind a frame that the cache of #47659 does not hold. No user reported it.
Problem
clone_utf8,src/runtime/jsc_hooks.rs:2882) and turns it back into bytes.Fix
VirtualMachine::original_source_textreturns the bytes.FetchFlags::PrintSourceis deleted. The same print: 9.6 us, 115,825 to 88,245 instructions.test/js/bun/util/inspect-error.test.js(46 new tests, 5 fail on the base). Other suites: Notes.Background
.stackwas read still load.parse_internal: it needs a 12-argument macro.Downsides
\rprints U+FFFD, not the character's first byte.read()calls, not 2. The third returns 0. resolver: read a file of 16 KB or more without a last empty read() #47771 removes it.Notes
Builds. Release builds, Linux x64, one machine: main 3f3a9ff, the base (#47659 at 5708972), this PR, and the released 1.3.14 and 1.4.3. Instruction counts are user-mode from
perf_event_open,(I(600 prints) - I(200 prints)) / 400, three runs, lowest to highest.Instructions per print that loads. The error's
.stackwas read first, so no build keeps its frame and each print loads.éin the frame, line 3BUN_DISABLE_SOURCE_CODE_PREVIEW=1What is left past line 3 is the line scan (85 instructions per line above the error). It is not in this PR.
A print that uses a kept frame (the same place again), base then this PR: 111 B 24,637-25,187 then 24,140-24,777. 48 KB 26,592-27,516 then 27,190-27,909. 4 MB line 30,003 27,101-27,616 then 27,095-27,636. The ranges overlap.
Wall time per print that loads, microseconds, lowest of 5 interleaved rounds. Main against main in the same run differs by up to 2.4%.
Per print that loads, from
/proc/self/ioandheapStats().mimalloc, base then this PR:read()calls: 2 then 2 for a file under 16,383 bytes, 2 then 3 for a longer one. Bytes read: the file size on both.Callers that never print.
parse_internalis not touched:parse_internal::<false, false, false>is 8,451 bytes on main and here.transpile_source_code_of_any_provenance::<false>: 18,870 to 17,523 bytes.fetch_without_on_load_plugins: 1,446 to 1,284.require()of 2,000 generated modules, five runs, main / base / this PR:.ts166,199-166,658 / 166,096-166,649 / 166,164-166,698..json31,598-32,197 / 31,668-33,027 / 31,733-32,414.#[inline(always)]ontranspile_source_code_innerkeeps the shape that main compiles to. Without it LLVM inlines the other way once the print-mode branches are gone, and each load copies the 136-byteResolvedSourceonce more.size): 140,289,809 to 140,289,020 bytes (-789).original_source_text1,966 bytes,code_frame_lines_as_utf81,026,remap_zig_exception10,186.Output. 66 fixtures on
Bun.inspect,console.errorand the uncaught-error output, base against this PR: 64 equal. The 2 that differ are a lone CR beforeéand before a byte that is not UTF-8, in a line of the frame. The line cutter ends such a line one byte after the CR. On the base that byte is printed alone (C3, orEFof the U+FFFD that the string made). Here the line is made a string, so it prints U+FFFD. Fixtures: file, JSON,.txtand extension-lesssourceURL, eval, stdin,blob:,data:, import suffix, node fallback,macro:, builtin, UTF-8 BOM, UTF-16 BOM, bytes that are not UTF-8 above and inside the frame, CRLF, lone CR, deleted / emptied / shortened file.How a line that is not UTF-8 is handled. On the base the whole file goes through a string, which puts U+FFFD for such bytes. Here only the lines of the frame are looked at. If one is not UTF-8, the lines of the frame are copied as strings show them, and the frame that #47659 keeps holds that copy. A print with only UTF-8 lines copies nothing.
What the self-review changed. The first form of this change was cut from main. The review asked for: the stack on #47659, no macro around
parse_internal(the printer reads its three kinds of source itself), the reader change ofsrc/resolver/fs.rsout of this PR, no whole-file redo for one bad byte, tests that still reach the load with the cache in front (they readerror.stackfirst and countcodeFrame_sourceLoads), tests for the entry-point loader, for an executable and for a source that a string cannot hold, no test comment that states a defect of main as a rule, and no multi-line comments. The test for a source that a string cannot hold lowers the limit of a string in place of a 2 GiB file.Tests.
inspect-error.test.js: 46 new tests, 45 of them in the block "the text that a code frame is cut from". On the base 5 fail: the two allocator tests (50 heaps per 50 prints), the two rows for a lone CR (the printer writes half a character), and the row for a text longer than a string can be. The rest hold the output for each kind of source.BUN_FEATURE_FLAG_SYNTHETIC_MEMORY_LIMIT. The base makes the string in C++, which does not read the lowered limit, so it shows the frame there. With the real limit (a sparse file of 2 GiB, release builds) both builds show none.filename. The printer reads that file today (error: do not read a frame's source URL from disk unless the loader loaded it #41477 is about whether it should). The rows hold the output, so that a change of the read does not change it.inspect-error.test.js107 pass,vm-sourceUrl.test.ts,reportError.test.ts,inspect.test.js,bun-inspect.test.ts,run-eval.test.ts,test/js/bun/sourcemap/(367),import-query.test.ts,bun-build-compile-sourcemap.test.ts,source-too-large.test.ts,test/cli/inspect/inspect.test.ts. clippy on bun_jsc, bun_runtime, bun_bundler.Not in this PR.
read(): the heap branch ofread_file_with_handle_implasks for one byte more than the file has. resolver: read a file of 16 KB or more without a last empty read() #47771 is that change.macro:, an executable's modules) keep a string.original_source_textfor a second consumer. jsc_hooks: move owned source bytes into a JSC external string instead of cloning #31103 moves owned source bytes into a JSC string at the clone site that this PR deletes for the printer. Its JSON import case stands.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/inspect-error.test.js