Fix clippy, rustfmt, Miri, and stale tsconfig references on main - #37078
Conversation
clippy -D warnings was failing on the workspace: - yaml.rs bind_anchor takes PendingAnchor by value so binding consumes the must_use token; allow needless_pass_by_value there (from #37055) - uws_handlers.rs wrapped NewSocket::on_close/on_handshake in swallow() after #37067 changed them to return unit; call them directly cargo fmt --check was failing on 4 files (tty.rs, ansi_renderer.rs, bake_body.rs, server_body.rs); ran cargo fmt. tsc --noEmit failed because the root tsconfig still referenced ./src/bake, which moved to ./src/runtime/bake. The test project also fails to parse the deliberately-mismatched JSX fixtures for issue 14477, so exclude those from test/tsconfig.json.
Walks references[] from the root tsconfig.json and fails on any path that is not a tsconfig file or a directory containing one, so a moved project directory (like src/bake -> src/runtime/bake) cannot silently break tsc and editor language services again. Trigger source-lints on tsconfig changes so the lint runs when only a tsconfig moves.
WalkthroughChangesThe pull request adds Miri-safe scalar paths for Highway searches, updates TypeScript project references and exclusions, changes socket callbacks to direct calls, and applies Rust formatting and documentation cleanup. Highway Miri support
TypeScript project-reference validation
Rust runtime updates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/source-lints/tsconfig-references.test.ts`:
- Around line 65-76: Update the path resolution logic around the statSync calls
so catches return null only for expected not-found filesystem errors; rethrow
permission, I/O, and other syscall failures with the affected path context.
Apply this to both the initial stat check and the tsconfig.json lookup while
preserving the existing file and directory resolution behavior.
- Line 55: Replace the manual JSON cleanup and JSON.parse call in the
configuration parsing helper with TypeScript’s ts.parseConfigFileTextToJson or
another established JSONC parser. Preserve the helper’s current
parsed-configuration behavior while ensuring comments and trailing commas are
handled without modifying string contents or accepting otherwise invalid JSON.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4d680a3-2a3a-4b6d-a5a0-f228fa90e35d
📒 Files selected for processing (10)
.github/workflows/source-lints.ymlsrc/bun_core/tty.rssrc/md/ansi_renderer.rssrc/parsers/yaml.rssrc/runtime/bake/bake_body.rssrc/runtime/server/server_body.rssrc/runtime/socket/uws_handlers.rstest/internal/source-lints/tsconfig-references.test.tstest/tsconfig.jsontsconfig.json
Trailing-comma removal now happens in the scanner only when emitting a bracket outside a string, so a string value containing ",]" is left alone; a block comment becomes a single space so it cannot fuse two tokens into accidentally valid JSON. statSync uses throwIfNoEntry so only ENOENT reads as a missing reference and real I/O errors propagate.
|
A note on verification, since these are lint and formatting fixes with no runtime-observable behavior. On main, The tsconfig repairs are exercised by the new lint in The Rust changes compile to identical behavior (an allow attribute, formatting, and dropping a wrapper that was a no-op around calls that now return unit), so no runtime test can fail without them; the CI checks above are the verification. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical lint/format cleanup with no runtime behavior change.
What was reviewed:
- rustfmt hunks in
tty.rs/ansi_renderer.rs/bake_body.rs/server_body.rsare whitespace/import-order only. uws_handlers.rs: confirmedNewSocket::on_close/on_handshakenow return()(socket_body.rs:2035/1666), so droppingswallow(())is a no-op and matches the siblingon_data/on_writable/on_end/on_timeouthandlers in the same impl.- tsconfig: verified
src/runtime/bake/tsconfig.jsonexists andsrc/bakedoes not; the 14477*-mismatch.tsxfixtures exist and are deliberately-invalid JSX. - New source-lint test: string-aware JSONC scanner (not regex-based),
throwIfNoEntry: falsecorrectly lets non-ENOENT stat errors throw; both CodeRabbit threads are addressed and resolved.
Extended reasoning...
Overview
This PR fixes CI lint drift on main: two clippy errors, four rustfmt-unformatted files, a stale tsconfig project reference (./src/bake → ./src/runtime/bake), and adds a source-lint test that walks the tsconfig references[] graph so a moved project directory can't silently break tsc --noEmit again. It also excludes three intentionally-malformed JSX fixtures from test/tsconfig.json and adds tsconfig paths to the source-lints workflow triggers.
Security risks
None. No user-facing runtime code paths are touched. The Rust changes are: whitespace reformatting (rustfmt), import reordering, a targeted #[allow(clippy::needless_pass_by_value)] with a stated reason (matching four existing uses in the tree), and removing a no-op swallow(()) wrapper around two calls whose return type changed to () in #37067.
Level of scrutiny
Low. The rustfmt hunks are mechanically verifiable as whitespace-only. The swallow() removal is behavior-preserving because Swallow for () is already a no-op, and I verified the inherent on_close/on_handshake on NewSocket in socket_body.rs return (). The tsconfig path fix is verified against the filesystem (src/runtime/bake/tsconfig.json exists, src/bake does not). The new test is a source-tree lint that runs against a released bun on a bare checkout — it can't affect the built binary.
Other factors
Both CodeRabbit inline comments were addressed in commit 53f6080 and are marked resolved: the JSONC parser is now a string-aware character scanner (handles escapes, inserts a space for stripped block comments so 1/* */2 stays invalid, only strips trailing commas outside strings), and the file comment explains why ts.parseConfigFileTextToJson isn't used (no bun install in the source-lints workflow). resolveReference uses statSync(..., { throwIfNoEntry: false }) so only ENOENT maps to null; permission/I/O errors still throw with the path. The PR description accurately scopes what is and isn't fixed (pre-existing test/ tsc semantic errors are out of scope). No prior human review comments are outstanding.
Keep the tsconfig repairs; drop the lint test and its workflow triggers.
There was a problem hiding this comment.
LGTM — mechanical lint/format cleanup with no runtime behavior change.
What was reviewed:
- rustfmt hunks in 4 files are pure whitespace/import-order;
uws_handlers.rsdropsswallow(())aroundNewSocket::on_close/on_handshake, which now return()— verified insrc/runtime/socket/mod.rs:143,173, and matches the siblingon_data_no_ext/on_end_no_extcalls in the same impl. #[allow(needless_pass_by_value)]onbind_anchoris justified —PendingAnchoris#[must_use](yaml.rs:3243), so consuming it by value is the intent.- tsconfig:
src/runtime/bake/tsconfig.jsonexists andsrc/bakedoes not; the three14477/*-mismatch.tsxfixtures exist and are deliberate parse-error inputs. - CodeRabbit threads target the source-lint test that was removed in d70c236 per maintainer feedback; both are resolved and moot.
Extended reasoning...
Overview
This PR fixes lint/format drift on main across 8 files: four pure cargo fmt reflows (tty.rs, ansi_renderer.rs, bake_body.rs, server_body.rs — line wrapping and import ordering only), one targeted #[allow(clippy::needless_pass_by_value)] on Parser::bind_anchor in the YAML parser, removal of two swallow(...) wrappers around unit-returning calls in uws_handlers.rs, a stale project-reference path in the root tsconfig.json (./src/bake → ./src/runtime/bake), and an exclude glob in test/tsconfig.json for intentionally-invalid JSX fixtures. An earlier revision also added a tsconfig-reference source-lint test, but the final commit removed it per maintainer feedback — the two CodeRabbit inline threads are on that removed file and are resolved.
Security risks
None. No user-facing surface, no parsing of untrusted input, no auth/crypto/permissions. The only non-formatting Rust changes are an #[allow] attribute and unwrapping a no-op swallow(()) — both compile to identical machine code.
Level of scrutiny
Low. This is the canonical "unbreak main's lint CI" PR: every hunk is either tool-generated (cargo fmt), a lint suppression with a stated reason, or a config path correction. I verified the substantive claims against the tree: PendingAnchor carries #[must_use = "an anchor must be bound to the node it annotates"] at yaml.rs:3243, so pass-by-value is deliberate and the allow is correct. api::NewSocket::on_close / on_handshake in src/runtime/socket/mod.rs return unit, so swallow(unit) was dead — and the four neighboring *_no_ext handlers in the same BunListener impl already call directly without swallow, so this change makes the block internally consistent rather than diverging from it. src/runtime/bake/tsconfig.json exists on disk and src/bake does not. The three test/regression/issue/14477/*-mismatch.tsx fixtures exist and the sibling 14477.test.ts asserts their parse errors, so excluding them from tsc is correct.
Other factors
Verification is the PR's own Clippy/Format CI checks (both red on main, green here) plus tsc --noEmit at the repo root — appropriate for changes with no runtime-observable behavior. There are no outstanding human reviewer comments; the only inline threads (CodeRabbit) target a file no longer in the diff and are marked resolved. No prior claude[bot] review on this PR.
…ernels cargo miri test cannot call foreign functions, and #37052 routed all byte search through the highway FFI, which broke the Miri workflow: bun_ptr's type_base_name hits highway_memrmem via strings::last_index_of. Under cfg(miri) the char and char-set scans take their existing scalar prologue at every length, and the four mem*mem wrappers use a scalar substring search. Kernels with no scalar form here (hashing, hex, sourcemaps, lexer scans) stay FFI-only, so a Miri-tested crate reaching one still fails loudly.
robobun
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical lint/format/config repairs with no production behavior change.
What was reviewed:
- rustfmt reflows in
tty.rs,ansi_renderer.rs,bake_body.rs,server_body.rs— whitespace only. swallow(...)removal inuws_handlers.rs: verifiedNewSocket::on_close/on_handshakereturn()(socket_body.rs:1666, :2035), andimpl Swallow for ()is a no-op — behavior-identical, now matches siblingon_data_no_extetc.highway/lib.rs: under non-Miri,scalar_only(len)folds to exactlylen < SCALAR_CUTOFF(cfg!(miri)is a compile-timefalse), so the FFI paths are byte-identical; thecfg(miri)scalarmemmem/memrmemfallbacks are only reachable under Miri and their callers already guard empty-needle andhaystack.len() < needle.len()so the0..=len-nlenrange can't underflow.- tsconfig: confirmed
src/runtime/bake/tsconfig.jsonexists andsrc/bakedoes not; the 14477*-mismatch.tsxfixtures exist and are intentionally-invalid JSX.
The two comment-cop flags on highway/lib.rs are length-heuristic hits on doc comments; the scalar_memmem one documents the no-underflow precondition and the scalar_only one explains why hashing/hex kernels stay FFI-only under Miri — both read as design docs rather than workaround justifications, but trim if preferred.
Extended reasoning...
Overview
Nine files: four are pure cargo fmt reflows (line-wrapping and import ordering only), one adds a targeted #[allow(clippy::needless_pass_by_value)] with a one-line rationale, one drops two swallow(...) wrappers around calls that now return (), two are tsconfig path/exclude fixes, and one (src/highway/lib.rs) adds cfg(miri)-gated scalar fallbacks so byte-search wrappers work under Miri where FFI is unavailable. An earlier revision's source-lint test was removed per maintainer feedback, so the CodeRabbit threads on that file are moot (and marked resolved).
Security risks
None. No user-input parsing, no auth/crypto/permissions, no FFI signature or ownership changes. The highway change adds Rust-only code paths that are dead under normal compilation.
Level of scrutiny
Low. The rustfmt hunks are mechanical. The #[allow] is a lint suppression with a stated reason. The swallow removal: impl Swallow for () is fn swallow(self) {}, and both api::NewSocket::on_close and on_handshake return () (verified at socket_body.rs:1666/2035) — dropping the wrapper is a no-op and now matches the four neighboring on_*_no_ext bodies in the same impl. The highway change is the only non-trivial hunk, but its non-Miri path is provably identical: scalar_only(len) is #[inline(always)] returning cfg!(miri) || len < SCALAR_CUTOFF, and cfg!(miri) is a compile-time false in every real build, so the branch condition is unchanged. The cfg(miri)-only scalar_memmem/scalar_memrmem are naïve (0..=hlen-nlen).find(...) scans reached only after the caller has handled empty-needle and hlen < nlen, so no underflow. match_at and the four highway_mem*mem* externs are correctly #[cfg(not(miri))]-gated to avoid dead-code warnings.
Other factors
Verification is the CI lints themselves (Clippy/Format/Miri red on main, green here), which is appropriate for changes with no runtime-observable behavior. The tsconfig reference target and 14477 fixture directory both exist on disk. The two open comment-cop bot flags on highway/lib.rs are automated comment-length heuristics on doc comments that document a Miri-specific design decision and a precondition contract; I don't read them as blocking — they're not justifying a workaround, they're stating why the split exists — but the author can shorten them if the maintainers want the bot green.
3 participants
cargo clippyandcargo fmt --checkare red on main, so every PR touching Rust inherits failing Clippy and Format checks. Both workflows only run on pull_request, which is how the drift landed unnoticed.Clippy (2 errors)
src/parsers/yaml.rs:bind_anchortakesPendingAnchorby value on purpose, so that binding consumes the#[must_use]anchor token (landed via yaml: support cyclic anchors/aliases in Bun.YAML.parse #37055, trips-D clippy::needless-pass-by-value). Added a targeted#[allow]with a one-line reason, matching existing usage elsewhere in the tree.src/runtime/socket/uws_handlers.rs: socket: don't leave exceptions pending when a connect promise settle or TLS session/keylog dispatch fails #37067 changedNewSocket::on_close/on_handshaketo return(), leaving twoswallow(...)wrappers passing a unit value (-D clippy::unit-arg). Call them directly, matching the neighboring handlers in the same impl.cargo clippy --workspace --no-depsnow exits 0.rustfmt (4 files)
src/bun_core/tty.rs,src/md/ansi_renderer.rs,src/runtime/bake/bake_body.rs,src/runtime/server/server_body.rshad unformatted hunks. Rancargo fmt --all;cargo fmt --all --checknow exits 0.tsconfig
tsconfig.jsonstill referenced./src/bake, which moved to./src/runtime/bakein the Rust rewrite, sotsc --noEmitfailed immediately with TS6053. Updated the project reference.test/tsconfig.jsonnow excludes the threetest/regression/issue/14477/*-mismatch.tsxfixtures: they contain deliberately mismatched JSX closing tags (the test asserts the parse error), which are unsuppressable TS17002 syntax errors.Note:
cd test && tsc --noEmitstill reports several thousand pre-existing semantic errors across the test suite; that is long-standing drift (not CI-enforced) and out of scope here.Also verified green on this branch: oxlint, clang-format check, prettier.
Miri
cargo miri testis also red on every PR: #37052 routed all byte search through the highway C++ kernels, and Miri cannot call foreign functions. The first caller to hit it isbun_ptr::ref_count::type_base_name(strings::last_index_of->highway_memrmem). Undercfg(miri)the search wrappers insrc/highway/lib.rsnow take their scalar paths: the char and char-set scans reuse their existing short-input scalar prologue at every length, and themem*memwrappers get a scalar substring search. Kernels with no scalar form (hashing, hex, sourcemaps, lexer scans) stay FFI-only so a Miri-tested crate reaching one still fails loudly. Verified locally:bun run rust:mirigreen on bun_ptr (previously failing), bun_ast, bun_base64, bun_clap, bun_collections, bun_dispatch, bun_errno, and bun_hash; this PR's Miri workflow runs the full set.Verification
The changes have no runtime-observable behavior: the proof is this PR's own Clippy and Format CI checks, which run
cargo clippy --workspaceandcargo fmt --all --check(both red on main, both green here), plustsc --noEmitresolving again at the repo root. An earlier revision added a source-lint test walking the tsconfig reference graph; it was removed per maintainer feedback.