Replace bun_core::Error intern machinery with per-crate thiserror enums - #33909
Conversation
|
Updated 9:05 PM PT - Jul 10th, 2026
@autofix-ci[bot], your commit be52741 is building: |
|
This is a behavior-preserving type-level refactor: the verification is the three-step compiler ladder ( |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/PackageInstall.rs:2150-2160— Therealpath_errclosure now mapsE::ENOENTtocrate::Error::Sys(SystemErrno::ENOENT), whose.name()is"ENOENT"— but the comment directly above saystest/cli/install/bun-link.test.ts asserts onFileNotFound:``, and that test (line 462, not touched by this PR) checksexpect(err4).toContain("FileNotFound: failed linking dependency/workspace to node_modules..."). Either restore the Zig-style names in this closure (e.g. via local variants like `crate::Error::FileNotFound`) or update bun-link.test.ts in this PR; the same user-visible rename applies to `AccessDenied`→`EACCES`, `NotDir`→`ENOTDIR`, and `NameTooLong`→`ENAMETOOLONG` in this closure.Extended reasoning...
What changed
Before this PR,
realpath_errininstall_with_link_runmapped errno values to Zig-style named errors:E::ENOENT => bun_core::err!("FileNotFound"), E::EACCES => bun_core::err!("AccessDenied"), E::ENOTDIR => bun_core::err!("NotDir"), E::ENAMETOOLONG => bun_core::err!("NameTooLong"),
The PR replaces these with:
E::ENOENT => crate::Error::Sys(bun_errno::SystemErrno::ENOENT), E::EACCES => crate::Error::Sys(bun_errno::SystemErrno::EACCES), E::ENOTDIR => crate::Error::Sys(bun_errno::SystemErrno::ENOTDIR), E::ENAMETOOLONG => crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG),
The comment immediately above the closure was left in place and still reads: "so map the openat errno to the named error tag to preserve the user-visible error tag (test/cli/install/bun-link.test.ts asserts on
FileNotFound:)" — i.e. the whole point of this closure was to preserve the Zig-style tag name, and the PR undid exactly that while keeping the comment.Why the name changes
install::Error::name()atsrc/install/error.rs:390implementsSelf::Sys(e) => <&'static str>::from(e).SystemErrnoderivesstrum::IntoStaticStr, so<&'static str>::from(SystemErrno::ENOENT)yields the variant name"ENOENT", not"FileNotFound". LikewiseEACCES,ENOTDIR,ENAMETOOLONG.How this reaches user output
When the realpath
openatfails,realpath_err(err)is wrapped asInstallResult::fail(…, Step::LinkingDependency, None)(PackageInstall.rs:2177). InPackageInstaller.rs:2079-2088, the else-branch of the failure handler calls:Output::err(cause.err, "failed {} for package <b>{}<r>", (cause.step.name(), …))
Output::erruses theErrNametrait for the prefix, andStep::LinkingDependency.name()is"linking dependency/workspace to node_modules". So the printed line becomes:error: ENOENT: failed linking dependency/workspace to node_modules for package <name>The
DanglingSymlinkandEACCESspecial-case branches at PackageInstaller.rs:1978/1994 don't intercept ENOENT, so it reaches the else-branch.Step-by-step proof of the test failure
test/cli/install/bun-link.test.ts:462runsbun installagainst a linked package whose target directory does not exist, and asserts:expect(err4).toContain(`FileNotFound: failed linking dependency/workspace to node_modules for package ${link_name}`);
- The realpath
openatfails withENOENT;realpath_errreturnscrate::Error::Sys(SystemErrno::ENOENT). cause.err.name()→"ENOENT".- Printed:
ENOENT: failed linking dependency/workspace to node_modules for package …. toContain("FileNotFound: …")fails.bun-link.test.tsis not in this PR's changed-files list (grep ofpr_diff.txtshows only the source comment referencing it), so nothing updates the assertion.
The PR description also claims "Every
#[error("...")]string preserves the originalerr!("...")literal verbatim" — this site is a counterexample.Fix
Either:
- Add
FileNotFound/AccessDenied/NotDir/NameTooLongvariants toinstall::Error(or reuse existing local variants) and return those fromrealpath_errso.name()matches the old strings; or - Update
test/cli/install/bun-link.test.ts:462(and any siblings asserting onAccessDenied:/NotDir:/NameTooLong:from this path) to expect the errno-style names in the same PR, and update the stale comment aboverealpath_err.
Per CLAUDE.md's Landing PRs guidance ("When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR"), one of these needs to land with this change.
|
Pushed fixes for the review findings and the CI crashes (263e54c): Review findings:
CI crashes (the 19 segfaults + ASAN stack-buffer-overflow):
Also fixed Verified locally: |
|
Pushed 21d0fe7 addressing the round-5 review findings and the CI test failures at 6090ffe:
Gate: the smoke test in this PR is intentionally behavior-pinning, not fail-before. This is a type-level refactor (1,641 |
|
Status at ff387ce: Green: compiler ladder (check/build/rust:check-all 10/10), no crashes in CI (ABI + Blob fixes landed), enum sizes down to 4-8 bytes, all intern machinery deleted. Remaining (known, mechanical):
Stopping per the brief's "if the error count isn't strictly decreasing, stop and report" instruction rather than continue the fix→new-lint cascade. The tail is tractable; every remaining failure is the same class. |
|
Updated remaining-work list with the two new review findings (same bug class): Lossy
These plus the ~7 CI test failures are the complete known tail. All are the same mechanical fix pattern demonstrated in commits 6090ffe and 7b0d093. |
|
Merge-conflict notification received (8 commits behind main; #33613 and #33925 likely touch files this PR modifies in Holding the rebase until the documented remaining-work tail is addressed. Rebasing a known-red 540-file PR before that would restart the gate/CI loop without changing the outcome, and conflict resolution in the ~40 |
|
@robobun rebase and resume |
7b0d093 to
a7c9b08
Compare
|
Rebased onto main (clean, no conflicts) and resumed per request. Pushed a7c9b08:
|
|
CI at 6395780: the two remaining refactor-caused test failures ( Only failure is |
|
CI 71695 at cacb44a: all refactor-caused test failures are green. Two remaining failures are single-platform flake unrelated to this PR:
Neither touches anything this refactor changes (error enum types / |
|
Thanks for the numbers. Honest assessment of what this refactor could plausibly explain: Install +12.5% — most plausibly mine. Express/Fastify −2-3% rps, +4% sys CPU — less clear. Fastify +53 MB RSS / +27% minor faults — I can't attribute this to the refactor. The error enums are stack/register values; nothing here changes heap allocation patterns, and Express doesn't show the same RSS delta. This looks like a separate issue (or a different build — the table says If you want me to pursue the size shrink (drop cross-crate nesting → install::Error back to ≤2 bytes), I can, but it's the "selective flattening" we deferred earlier and will re-open a handful of the |
|
@robobun sorry this is a different PR please ignore. |
|
Understood, no worries. PR state unchanged: |
|
@robobun fix comments + conflicts |
cacb44a to
c2408ed
Compare
|
Rebased onto main (1 conflict in jsc_hooks.rs, 2 new |
|
CI 71735 at 75631c0 (rebased + dead-code cleanups): all refactor-caused test failures green. Single remaining failure is Re-roll already spent. Diff is green on cargo check/build/clippy, rust:check-all 10/10, and all other CI lanes. Ready for merge past the gate + this one unrelated lane. |
Resolved src/bun_core/string/mod.rs: #33909 switched return types to crate::CrateResult<()> in write_pre_quoted_string and quote_for_json, which this branch restructures into the const-generic write_pre_quoted_string_inner; carried CrateResult<()> into both the adapter and _inner signatures. Resolved src/router/lib.rs and src/sourcemap/Chunk.rs: #33909 touched return types inside blocks this branch deletes (orphaned router test scaffolding, uncalled print_source_map_contents); kept the deletions after re-verifying zero callers on current main. Restored Node::find_last in src/collections/pool.rs: gained a test caller via #33311.
Resolved src/bun_core/string/mod.rs: #33909 switched return types to crate::CrateResult<()> in write_pre_quoted_string and quote_for_json, which this branch restructures into the const-generic write_pre_quoted_string_inner; carried CrateResult<()> into both the adapter and _inner signatures. Resolved src/router/lib.rs and src/sourcemap/Chunk.rs: #33909 touched return types inside blocks this branch deletes (orphaned router test scaffolding, uncalled print_source_map_contents); kept the deletions after re-verifying zero callers on current main. Restored Node::find_last in src/collections/pool.rs: gained a test caller via #33311.
## What this adds **A spec-exact TOML v1.1.0 parser** (`src/parsers/toml.rs`), replacing the previous JS-lexer-derived implementation, plus the **official [toml-lang/toml-test](https://github.com/toml-lang/toml-test) conformance suite** as a generated `bun:test` file, following the same pattern as the YAML and JSON5 conformance suites. ### Conformance: 100% of the official suite, before → after Measured against the toml-test v1.1.0 manifest (commit `4d77658d`): 217 valid + 481 invalid + 1 out-of-range-integer + 9 invalid-encoding cases — every case in the manifest, zero exclusions. | | before | after | |---|---|---| | total | 160/699 (23%)¹ | **708/708 (100%)** | | valid documents parsed correctly | 148/217 | 217/217 | | invalid documents rejected with the asserted SyntaxError | 12/481 | 481/481 | | invalid documents wrongly **accepted** | 144 | 0 | | invalid-encoding documents (raw byte input) | untestable¹ | 9/9 | | date/time literals | all rejected as syntax errors | parse as strings of their source text | ¹ The 9 invalid-encoding cases contain ill-formed UTF-8 that a JS string cannot carry; they became testable when `TOML.parse` gained binary input in this PR. ### Parser - All four TOML date/time types (offset/local date-time, local date, local time), returned as strings of the source text - Correct `inf`/`nan`, underscore and leading-zero validation, `0x`/`0o`/`0b` integers - Integers outside `Number.MAX_SAFE_INTEGER` throw instead of silently losing precision (TOML requires lossless handling or an error) - Table / array-of-tables / dotted-key / inline-table definition-state rules enforced; duplicate keys rejected, including non-ASCII keys (the old byte-view comparison of UTF-16 keys missed duplicates and falsely rejected distinct keys) - Control characters, bare carriage returns, and ill-formed UTF-8 rejected (simdutf-validated); leading BOM handled - Multi-line string delimiter/trimming rules, CRLF→LF normalization, exact escape validation including TOML 1.1's `\xHH` and `\e` - TOML 1.1 additions: optional seconds in times, multi-line inline tables with trailing commas - Dotted keys are parsed iteratively (the old parser capped them at 512 segments) ### `Bun.TOML.parse` - Throws `SyntaxError` with precise messages (previously a `BuildMessage`), matching `JSON.parse`, `Bun.YAML.parse`, and `Bun.JSON5.parse` - Converts the AST directly to JS values (previously printed to JSON and re-parsed) - Accepts `Blob`/`TypedArray`/`DataView`/`ArrayBuffer` input like the YAML and JSON5 siblings - Parse errors carry the redaction flag so secrets in malformed config files stay out of logs ### `Bun.TOML.stringify` (new) Serializes a JavaScript object to a TOML document — this API did not exist before. Same surface as the YAML/JSON5 siblings: `(value, replacer, space)`, where `replacer` throws and `space` is accepted but ignored (TOML output is line-oriented). - Idiomatic layout: scalar keyvals first, then `[table]` and `[[array-of-tables]]` sections; mixed arrays use inline tables; keys are bare when possible, quoted otherwise - `Date` values become TOML offset date-times - `null`, `BigInt`, circular structures, and non-object top-level values throw (TOML cannot represent them); `undefined`/function/symbol properties are skipped - Integral doubles beyond ±(2^53 − 1) are emitted as floats so documents round-trip through any TOML reader; unpaired surrogates get the same USVString replacement as `TOML.parse` string input ### Tests - `test/js/bun/toml/toml-test-suite.test.ts` — generated official suite (708 tests); every rejection test asserts the exact full error message - `test/js/bun/toml/generate_toml_test_suite.ts` — pins the upstream commit, decodes the suite's tagged-JSON expectations, inlines every case; `--check` mode fails if the committed suite is stale - `test/js/bun/toml/toml.test.ts` — hand-written coverage beyond the official suite: input types (Buffer/TypedArray subviews/DataView/ArrayBuffer/SharedArrayBuffer/Blob), JS value mapping (`__proto__` safety, key ordering, no Unicode normalization), safe-integer boundaries, source-text date/time preservation, multi-line string edge cases, recursion-limit and GC robustness, the SyntaxError message contract, and `TOML.stringify` (exact layout output, escaping, round-trips, every error contract, and a GC stress test) ### Performance 1.15×–1.49× faster than the previous parser at every document size, growing with size. Release builds from this pipeline (darwin-aarch64), interleaved A/B, best of 3 rounds (each sample = median of 15 timed batches), real-world inputs both parsers accept: | document | size | old | new | change | |---|---|---|---|---| | `uv.lock` (apache/airflow) | 2.9 MB | 246 MB/s (12.0 ms) | **367 MB/s (8.0 ms)** | 1.49× | | `uv.lock` (langgenius/dify) | 662 KB | 237 MB/s | **338 MB/s** | 1.43× | | `poetry.lock` (python-poetry/poetry) | 201 KB | 249 MB/s | **341 MB/s** | 1.37× | | `Cargo.lock` (zed-industries/zed) | 495 KB | 194 MB/s | **262 MB/s** | 1.35× | | `Cargo.lock` (this repo) | 71 KB | 199 MB/s | **259 MB/s** | 1.30× | | `pyproject.toml` (python-poetry/poetry) | 5.7 KB | 168 MB/s | **221 MB/s** | 1.31× | | `poetry.lock` (truncated at a package boundary) | 2.4 KB | 16.2 µs/op | 13.5 µs/op | 1.20× | | `bunfig.toml` | 126 B | 3.29 µs/op | 2.85 µs/op | 1.16× | Basic strings borrow the source bytes when no escape requires decoding (the same approach the YAML parser uses for plain scalars), so string-dominated documents — lockfiles in particular — see the largest gains. ### Behavioral note: stricter parsing of invalid config files The old parser silently accepted some invalid TOML, so a small set of existing `bunfig.toml` files (about 1% in a sample of 99 real-world bunfigs from GitHub) will go from silently tolerated to a startup `SyntaxError` that states the fix. The one pattern observed in the wild is unquoted string values — easy to carry over from `.npmrc` syntax — which now produce a purpose-built message: ``` TOML Parse error: Strings must be quoted: "isolated" ``` The other formerly-tolerated patterns (backslash escapes in basic strings being silently dropped, missing newlines between key/value pairs, bare keys containing `:`) were silent data corruption or non-portable syntax that every other TOML implementation already rejects; none appeared in the sample. ### Deletions The old TOML lexer (`src/parsers/toml/lexer.rs`), and the now-caller-less `E::Object::set_rope`, `get_or_put_array`, and `set`. The bake error reporter's JS-escape decoder (which borrowed the old TOML lexer) is extracted standalone with its exact semantics. Closes #22426 Closes #28680 Closes #28681 Closes #28687 Also addresses the TOML half of #32025 (`\u{…}` is now correctly rejected; the JS-lexer half remains). --- ### Merge with main (38cfba6) One conflict, in `src/ast/e.rs`. Main's hardening round (#33072) added `flags: own_key_property_flags(&key)` to the property-construction sites, which marks a `__proto__` key as computed so it becomes an own property rather than setting the prototype. This branch deletes `set`, `set_rope`, and `get_or_put_array`, replacing `set` with `append_property`. Git merged the new flags into `append_property` on its own; the conflict was only the bodies of the two functions this branch removes. Kept them removed, after checking that neither has a caller anywhere in the tree and that every live construction site (`put`, `get_or_put_object`, `append_property`) carries the new flags, so the hardening is preserved on every path that still exists. Verified on the merge result: the TOML suites pass (810 tests, including the full conformance suite), and the INI and `bun init` tests pass (73 tests) since they are the remaining `get_or_put_object` callers. `test/bake/dev/production.test.ts` also passes again now that #33204 pinned the React build the bake harness installs. ### Merge with main (dbac342) Conflicts with #33722, which fixed `\u{...}` overflow and unterminated-brace handling in the old TOML lexer's copy of the JS escape loop. - `src/parsers/toml/lexer.rs` (modify/delete): kept deleted. The new parser rejects `\u{...}` at the opening brace (`\u{…}` is JavaScript syntax, not TOML), so neither the overflow nor the unterminated-brace path is reachable. - `test/js/bun/resolve/toml/toml-parse.test.ts`: kept this branch's content and folded the #30825 crash-regression inputs into the existing `rejects JS-style \u{XX} escapes` test. Main's `still accepts in-range \u{...}` test asserted `\u{41}` decodes to `"A"`, which contradicts spec-compliant TOML and is already asserted to throw by this file. - `src/runtime/bake/DevServer/js_escape.rs`: ported the one-line unterminated-brace fix, since that decoder was extracted from the code #33722 patched and is documented as preserving its exact semantics. Overflow was already prevented by the existing `0x10FFFF` clamp. ### Merge with main (aba8757) Conflicts with #33925 (repo-wide snake_case directory cleanup), which renamed `src/runtime/bake/DevServer/` to `dev_server/` and replaced the `#[path = "../DevServer/..."]` module-path attributes with plain `mod` declarations. - `src/runtime/bake/dev_server/mod.rs`: adopted main's simplified submodule block and added the `js_escape` declaration. - `src/runtime/bake/DevServer/js_escape.rs`: moved to `dev_server/js_escape.rs` following the rename. Git tracked the rename of this branch's `ErrorReportRequest.rs` changes into `error_report_request.rs`, and its `super::js_escape::decode_js_escape_sequences` call is intact. ### Merge with main (ace42f9) Conflicts with #33909, which replaced `bun_core::Error` with per-crate `thiserror` enums across the codebase. - `src/parsers/toml.rs`: main changed the old file's error handling; this branch replaces the whole file. Kept the new parser and adapted `TOML::parse` to return `crate::Result<Expr>`, mapping the internal `PErr` to `parsers::Error::{SyntaxError, Alloc, StackOverflow}`. - `src/parsers/toml/lexer.rs` (modify/delete): kept deleted. - `src/parsers/error.rs`, `src/runtime/error.rs`: removed the `From<toml::lexer::Error>` impls that referenced the deleted file. - `src/runtime/bake/dev_server/js_escape.rs`: adapted to return `Result<(), crate::Error>` with `crate::Error::SyntaxError`, since `bun_core::err!` is gone. - `src/runtime/api/TOMLObject.rs`: matched on `bun_parsers::Error::Alloc(_)` in place of the removed `bun_core::err!("OutOfMemory")`. - `src/ast/lexer_log.rs`: doc comment combining both sides. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Replaces the runtime-interned global error set
bun_core::Error(#[repr(transparent)] NonZeroU16) and theerr!()macro with idiomatic per-cratethiserrorenums.Why
err!(Name)cached a globalRwLock<Vec<&'static str>>intern lookup in a per-siteAtomicU16. Not aconst, so it could never be a pattern; every dispatch was an if-else guard chain with no exhaustiveness check (worst case ~40 arms inFetchTasklet::to_body_value_error). This was Zig'sanyerror, ported.Design
One
#[derive(thiserror::Error)]enum per crate at<crate_dir>/error.rs, re-exported ascrate::Error+crate::Result<T, E = Error>:err!("OutOfMemory")(92 sites, 17 crates) routes tobun_alloc::AllocErrorENOENT,FileNotFound,AccessDenied,NoSpaceLeft,NameTooLong,NotDir,EISDIR,BadPathName, allE*) route tobun_errno::SystemErrno(2 bytes). NOTbun_sys::Error(40 bytes; the context tier).css::Error::InvalidCharacter!=sql::Error::InvalidCharacter.#[error(transparent)] X(#[from] ...)for pass-through;?composes viaFrom.bun_core::output::ErrNamesoOutput::err(e, ...)keeps working.#[error("...")]string preserves the originalerr!("...")literal verbatim.thiserror = "2"was already a workspace dep). Nodyn Error/Box<dyn Error>/anyhow.Deleted
macro_rules! err+.bun_errlink-section (src/bun_core/lib.rs)SEED,EXTRA,intern_slow,intern_cached,Error::intern,Error::from_errno,Error::from_raw,Error::name,Error::as_u16, theRwLock<Vec<&'static str>>intern table (src/bun_core/result.rs)named_error_set!macro (all 31 call sites deleted; enums now derivethiserror::Errordirectly)OrWriteFailedtraitErrorCode::from(bun_core::Error)/ErrorCode::to_error()(src/jsc/ErrorCode.rs): these aliased the internu16onto the JS error-code table index. Callers ofErrorable::err()now passErrorCode(ErrorCode::JS_ERROR_OBJECT)directly.impl From<bun_sys::Error> for bun_core::Error(src/sys/lib.rs); replaced withFrom<bun_sys::Error> for bun_errno::SystemErrno.bun_core::Errorstill exists but is now bun_core's own local 13-variant thiserror enum (for thebun_core::io::Writetrait's return type).Foundation
bun_errno::SystemErrnonow implsDisplay+core::error::Error(viastrum::IntoStaticStr).bun_alloc::AllocErrornow implsDisplay+core::error::Error.Scope
err!()call sites across 40 crates, 449 distinct namesVerification
cargo check --workspace --keep-going: greencargo build --workspace --keep-going: green (post-mono const asserts)bun run rust:check-all: green across all 10 targets (linux/macos/windows x x64/aarch64 + android)Deferred
bun_runtime(192 local variants) andbun_install(117) kept as flat per-crate enums rather than module-level split. Splitting would change==semantics for the 9 names that span modules insidebun_runtime; both enums are still <=4 bytes withSysnesting.bun_sys::Error(40 bytes), variant payloads,$ERR_*JS machinery: out of scope.