Remove cross-crate layering workarounds - #33152
Jarred-Sumner wants to merge 12 commits into
Conversation
|
Updated 12:48 AM PT - Jul 7th, 2026
❌ @autofix-ci[bot], your commit 9f9f9b5 has 8 failures in
🧪 To try this PR locally: bunx bun-pr 33152That installs a local version of the PR into your bun-33152 --bun |
c64e7a0 to
e278f95
Compare
c3813ef to
59b1313
Compare
|
@claude review |
ffe1992 to
eae70e2
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if js_lexer::is_identifier_start(text[0] as i32) { | ||
| let mut i: usize = 1; | ||
|
|
||
| while i < text.len() && js_lexer::is_identifier_continue(text[i] as i32) { | ||
| while i < text.len() && js_lexer::is_identifier_part(text[i] as i32) { | ||
| i += 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Rewiring the highlighter from the deleted fmt::js_lexer stub (which returned true for every byte > 0x7F) to crate::string::identifier::{is_identifier_start,is_identifier_part} changes the predicate to expect a Unicode codepoint, but the highlighter still feeds it raw UTF-8 bytes (text[i] as i32 at fmt.rs:1704/1707/2159/2162/2203). Continuation bytes 0x80..0xBF mostly map to non-ID_Continue Latin-1 codepoints, so an identifier like café (bytes …C3 A9) now splits mid-sequence — 0xC3→Ã (true) but 0xA9→© (false) — emitting caf\xC3 + ANSI reset + orphaned 0xA9. Cosmetic only (REPL / markdown fenced-code / bun:internal-for-testing highlightJavaScript, plus the recursive template-literal arm at fmt.rs:1884, all of which pass check_for_unhighlighted_write: false and so bypass the is_all_ascii guard); fix by restoring a local c > 0x7F ⇒ true byte-level fast path here, or decode via CodepointIterator before calling the predicate as FormatValidIdentifier (fmt.rs:1172) already does.
Extended reasoning...
What changed
The PR deletes bun_core::fmt::js_lexer — a local stub whose is_identifier_start/is_identifier_continue had a c > 0x7F fallthrough that returned true for every non-ASCII byte — and rewires QuickAndDirtyJavaScriptSyntaxHighlighter to crate::string::identifier::{is_identifier_start, is_identifier_part} (src/bun_core/string/identifier.rs:1-19). Those functions interpret their i32 argument as a Unicode codepoint: for 0x80..=0x10FFFE they call is_id_{start,continue}_es_next(cp) against the two-stage ID_Start/ID_Continue bitmap tables. But the highlighter still passes individual UTF-8 bytes — text[i] as i32 at fmt.rs:1704, 1707, 2159, 2162, 2203 — not decoded codepoints.
Why this mis-tokenizes multi-byte identifiers
UTF-8 continuation bytes are 0x80..=0xBF. Interpreted as codepoints, 0x80..=0x9F are C1 control characters (never ID_Continue) and 0xA0..=0xBF are mostly Latin-1 punctuation (NBSP, ¡, ¢, §, ©, «…¿); only 0xAA (ª), 0xB5 (µ), and 0xBA (º) are ID_Continue letters. So for essentially every multi-byte UTF-8 character in an identifier, the lead byte may pass (e.g. 0xC3 → U+00C3 Ã, category Lu → true) but the following continuation byte fails, and the identifier scan stops mid-sequence.
Step-by-step proof
Take const café = 1 and trace café = bytes [0x63, 0x61, 0x66, 0xC3, 0xA9] through the identifier arm at fmt.rs:1704-1707:
text[0] = 0x63(c):is_identifier_start(0x63)→ true, enter the loop withi = 1.i=1,2:0x61,0x66→ ASCII letters → true.i=3:0xC3→is_identifier_part(0xC3)→ not in0x00..=0x7F, sois_id_continue_es_next(0xC3)→ U+00C3 LATIN CAPITAL LETTER A WITH TILDE, category Lu → true. Loop continues.i=4:0xA9→is_identifier_part(0xA9)→is_id_continue_es_next(0xA9)→ U+00A9 COPYRIGHT SIGN, category So → false. Loop exits withi = 4.- The highlighter emits
text[..4]=[0x63, 0x61, 0x66, 0xC3]— a torn UTF-8 sequence ending in a lone lead byte — viabstr::BStr::new(...)(lossy →caf�), followed by an ANSI reset (<r>). textis advanced to[0xA9, ...].0xA9is not an identifier start and matches no punctuation arm, so it falls through towriter.write_char(c as char)(fmt.rs:2228), which widens the byte to U+00A9 and re-encodes it as UTF-8C2 A9.
Net output: caf� + ANSI escape + © — garbled highlighting with a replacement character and a spurious © where é should be. With the old stub, both 0xC3 and 0xA9 returned true (> 0x7F), the whole 5-byte run was treated as one identifier, and the bytes were emitted verbatim as valid UTF-8.
Why the is_all_ascii guard does not prevent this
The guard at fmt.rs:1690 (if !strings::is_all_ascii(text) { return write_bytes(writer, text); }) is inside if self.opts.check_for_unhighlighted_write { … }. Four callers set check_for_unhighlighted_write: false and therefore reach the byte-indexed loop with non-ASCII input:
src/runtime/cli/repl.rs:1242— REPL syntax highlightingsrc/md/ansi_renderer.rs:1516— markdown fenced-code renderingsrc/jsc/fmt_jsc.rs:37/48— thebun:internal-for-testinghighlightJavaScriptbinding (exercised byhighlighter.test.ts)fmt.rs:1884— the recursive${…}template-literal arm (inherits the outer opts)
The default (check_for_unhighlighted_write: true) path — used by the error/log formatters — is unaffected because it bails out to plain write_bytes on any non-ASCII byte.
Impact
Cosmetic only: the REPL, markdown code-fence renderer, and highlightJavaScript test binding emit garbled output (torn UTF-8 + misplaced ANSI resets) for source containing non-ASCII identifiers. No effect on the transpiler, printer, or any code that ships to disk — FormatValidIdentifier (fmt.rs:1172) already decodes via CodepointIterator before calling these predicates and is unaffected. This is a real regression introduced by this PR (the old stub was a deliberate byte-level approximation), but the blast radius is display-only, hence nit.
Fix
Either restore a local byte-level fast path in the highlighter (e.g. wrap the call sites: c > 0x7F || js_lexer::is_identifier_part(c)), which matches the old stub's "quick and dirty" semantics; or decode WTF-8 codepoints via strings::CodepointIterator before calling is_identifier_*, as FormatValidIdentifier at fmt.rs:1172 already does. The former is a two-line change and preserves the highlighter's existing byte-indexed structure.
Delete legacy re-export shims, mirrored type definitions, and hook/vtable indirection left over from the crate split. Types move to a single canonical home; upward call indirection becomes direct calls or properly-tiered dependencies. No behavior change.
Declare the bindgen step's Rust output as an input of the cargo step so rust-only builds generate it, and gate a debug-only import to debug builds.
The shim_standalone configuration compiles without the host feature, so the read_without_launch payload and the debug_assert-only helpers are conditionally dead there; gate them explicitly and drop an unused feature attribute from the bin crate root.
Cross-crate hooks with a single concrete implementation now link directly: per-slot extern "Rust" functions replace the fn-pointer tables, installed vtable statics, and single-implementation trait objects, and typed opaque handles replace the erased pointers they carried. Remaining re-export facades, mirrored option/framework types, a shell crate, and duplicated helpers are deleted, and a few types move to the crate that owns them.
Keep the top-level-dir bytes owned by a static instead of Box::leak (leak checkers see them as reachable), cfg-gate the POSIX-only DNS hint helpers instead of allowing dead code, and record the new per-file allow counts.
The generated bun:main wrapper moved from the runtime-state box onto the VirtualMachine, but only the timer's reclaim followed; drop it in destroy at the same point the old box died so worker VMs stop leaking it.
Point the rebased install code at the canonical type paths, and make the never-lossy libuv return-code mapping retry an unmapped negative code as a raw errno magnitude before collapsing to UNKNOWN, matching the previous translation.
bb8d87f to
9f9f9b5
Compare
This branch has not been deployed
2 participants
Removes the cross-crate layering workarounds left over from the crate split. Four classes of code are deleted, each replaced by the one canonical definition or a direct call:
pub usechains and module facades (crate::jsc, the sql-sidejscfacade,bun_pathspath-buffer re-exports, install's legacy re-exports, thebun_apishell crate, …) are gone; callers name the owning crate directly.Framework/BuiltInModule, the resolver-visible bundler options, the libuvUV_E*table, parseroptionsmirrors, theJsErrormirror variants ofToJSError,BufferedStdin, …) now have a single home; theFrom/field-copy glue between the copies is deleted.extern "Rust"functions resolved at link time, withbun_opaque-typed handles instead of*mut c_void/*mut (): the runtime hook table, the JS event-loop and event-loop-ctx vtables, the dev-server, macro, wake-handler, bundle-completion, plugin-resolver, printer-callback, OOM and duplex seams. Single-implementation trait objects (dyn MacroRunner,dyn JsCompletion,dyn PluginResolver,dyn AutoInstaller) are gone.bun_dispatch,bun_output,bun_transpiler,bun_errno,bun_sql_jsc,bun_api, …) are folded into their owners.Three places intentionally keep a registration or accessor rather than a link call, each for a concrete reason: the pre-exit callback list (lower-tier test binaries can't resolve a symbol defined in
bun_runtime), the standalone-graph accessor (a stored&'staticcannot soundly produce the required&mut), and theBlobextension traits (static dispatch, not a pointer table).Net diff: +32.7k / −42.2k lines (−9.5k). Most of the insertions are existing code landing at its canonical home (file moves/splits), not new code.
This is intended to be behavior-preserving. The deliberate, user-visible changes that fall out of deduplicating diverged copies:
Bun.inspect/console.lognow quote object keys containing non-identifier, non-ASCII codepoints (the printer and lexer share one identifier predicate).inspectoutput escapes control characters as\u{…}consistently.bun build(bake) reports an out-of-memory failure asOutOfMemoryinstead of a genericJSError.user-agent/bun pm whoamireport the actual CI vendor name (previously hard-coded"ci"), andCI=falseis honored.BUN_INSTALLandXDG_CACHE_HOMElike the rest of the install cache chain.os_signpostintervals are emitted again (they had become silent no-ops).Verification on Linux x64 (debug):
cargo check --workspacefor all supported target triples,cargo clippy --workspace(0 warnings), rustfmt/prettier clean, fullbun bdbuild.origin/mainbinary at the same commit: install (incl. lifecycle scripts), bundler/compile (incl. bytecode), macros, transpiler, shell, spawn/IPC, timers + fake timers, node fs/process/child_process, fetch/Blob, streams, WebSocket (incl. permessage-deflate),Bun.serve, sqlite, sourcemaps, bake dev server, hot reload,bun run. Every remaining local failure reproduces identically with theorigin/mainbinary in the same environment.