bun_alloc: delete unused MimallocHeapRef, TypedArena, BumpWithFallback#31542
Conversation
…k, and Zig-port stubs
|
Updated 12:08 AM PT - May 29th, 2026
❌ @dylan-conway, your commit 3b1c8cc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31542That installs a local version of the PR into your bun-31542 --bun |
|
Warning Review limit reached
More reviews will be available in 13 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR removes several unused or deprecated public APIs from the ChangesAllocator API cleanup and deprecation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure dead-code removal; just clean up the two leftover doc-comment lines in memory.rs noted inline.
Extended reasoning...
Overview
This PR is a dead-code sweep of the bun_alloc crate, removing six items: the MimallocHeapRef struct and its Allocator impl, the TypedArena<T> type alias, the BumpWithFallback back-compat alias, StackFallback::fallback_mut, the trivial init_default<T> wrapper, and MaybeOwned::into_parent. The diff is purely subtractive (no logic added or modified), plus a corresponding trim of the pub use stack_fallback::{...} re-export list in lib.rs.
I grepped the full repo for MimallocHeapRef|TypedArena|BumpWithFallback|fallback_mut|init_default|into_parent and confirmed there are no remaining references outside unrelated libarchive patch text and a stale comment. The PR description's claim that cargo check --workspace passes is consistent with this — if any caller had remained, the build would have failed on the removed re-exports.
Security risks
None. This is removal of unused allocator-adapter types and helpers; no input handling, auth, crypto, or permission surfaces are touched. Deleting an unused unsafe impl Allocator strictly reduces the unsafe surface area.
Level of scrutiny
Low. Pure deletions where the compiler is the ground truth — if anything were still referenced, cargo check would fail. The only behavioral question ("is this really dead?") is mechanically verifiable and I verified it.
Other factors
The bug hunter flagged one nit: the init_default deletion left the first two lines of its /// doc comment behind in memory.rs, which now silently attach to rebase_slice via rustdoc's outer-attribute rules. This is a real but cosmetic docs-only issue — worth fixing, but not a reason to block. There's also a now-stale comment at src/bun_alloc/maybe_owned.rs:84 that still mentions into_parent(self); harmless, but could be tidied in the same pass. Neither affects runtime behavior, so I'm approving.
There was a problem hiding this comment.
LGTM — thanks for fixing the orphaned doc-comment in 3b1c8cc; the two remaining inline notes are non-blocking nits (stale comment text + unused Cargo dep).
Extended reasoning...
Overview
This PR is a pure dead-code sweep of bun_alloc: it deletes MimallocHeapRef (a speculative Allocator impl superseded by ArenaPtr), the TypedArena<T> re-export alias, the BumpWithFallback back-compat alias, StackFallback::fallback_mut, memory::init_default<T>, and MaybeOwned::into_parent. The diff is deletion-only across four files plus narrowing one pub use line. The PR description includes a per-item provenance table tracing each removal to its origin commit and confirming zero external callers.
Security risks
None. Every change is a removal of an unreferenced public item; no remaining code path is modified. The retained unsafe code in stack_fallback.rs (StackFallback, ArenaPtr) and memory.rs (rebase_slice) is unchanged. No allocator behavior, FFI surface, or memory-safety invariant is touched.
Level of scrutiny
Low. This is mechanical dead-code deletion in an internal allocator support crate, verified by cargo check --workspace and confirmed by grep — the only remaining hit on any deleted name in the workspace is the stale prose comment flagged inline. There are no design decisions, no behavioral changes, and no test regressions possible from removing code that nothing calls.
Other factors
I previously flagged leftover init_default doc-comment lines on commit 7c5c330; the author addressed that in follow-up commit 3b1c8cc, and the current memory.rs is clean. The two new inline findings from this run are both explicitly nits: (1) a trailing comment clause in maybe_owned.rs that still mentions the deleted into_parent, and (2) the now-unused typed-arena.workspace = true entry in src/bun_alloc/Cargo.toml. Neither affects compilation, runtime, or docs rendering — they're cleanup-completeness suggestions that don't block merging.
| /// Used by smart pointer types and allocator wrappers. See `crate::borrow`. | ||
| pub fn borrow(&self) -> MaybeOwnedBorrowed { | ||
| // Borrowed view carries no allocator state — just the owned/borrowed bit. |
There was a problem hiding this comment.
🟡 The trailing comment block at the bottom of this file still says "keeping one would also forbid moving self._parent out in into_parent(self)" — but into_parent is deleted in this PR. Drop that clause; the preceding sentence about field drop glue stands on its own. Same class of leftover as the init_default doc-comment fixed in 3b1c8cc.
Extended reasoning...
What the issue is
This PR deletes MaybeOwned::into_parent(self) -> Option<A> from src/bun_alloc/maybe_owned.rs. However, the rationale comment at the bottom of the file (post-diff lines 81-84) still references it:
Per PORTING.md (Idiom map:
pub fn deinit), that is exactly field drop glue on_parent: Option<A>, so no explicitDropimpl — keeping one would also forbid movingself._parentout ininto_parent(self).
The bolded clause cites into_parent(self) as a second reason for omitting an explicit Drop impl. With into_parent gone, that reason no longer exists in the codebase, so the comment now points readers at a method they cannot find.
Step-by-step proof
- Pre-PR,
maybe_owned.rsdefinedpub fn into_parent(self) -> Option<A> { self._parent }at lines 72-76. - The diff for this PR shows that block removed (the
- pub fn into_parent(self) -> Option<A> {… hunk). - Post-PR, the file ends with the comment block at lines 81-84, whose final clause reads verbatim: "keeping one would also forbid moving
self._parentout ininto_parent(self)." grep -rn into_parent src/after the PR returns only this comment line — there is no longer anyinto_parentdefinition or call site for it to refer to.- Therefore the clause is dangling: it justifies a design choice by appeal to a constraint that has been deleted.
Why nothing catches it
This is a plain // line comment, not a doc comment, so neither cargo check nor cargo doc parses its contents. There is no intra-doc link to break. The PR description's "cargo check --workspace passes" is consistent with this being purely a prose artifact.
Impact
No runtime or compile-time effect. The cost is reader confusion: someone auditing why MaybeOwned has no Drop impl will go looking for into_parent and not find it. Since this PR is explicitly a dead-artifact sweep — and the same class of leftover (the orphaned init_default doc-comment lines in memory.rs) was already cleaned up in follow-up commit 3b1c8cc — it's worth removing for consistency.
Fix
Trim the trailing clause so the comment ends after the first reason, e.g.:
// Zig `deinit` only forwarded to `bun.memory.deinit(parent_alloc)` on the owned field.
// Per PORTING.md (Idiom map: `pub fn deinit`), that is exactly field drop glue on
// `_parent: Option<A>`, so no explicit `Drop` impl.The field-drop-glue rationale is sufficient on its own; the into_parent clause was always a secondary "and besides" point.
| __s | ||
| }}; |
There was a problem hiding this comment.
🟡 Removing the TypedArena<T> alias drops the only reference to the typed_arena crate inside bun_alloc, but src/bun_alloc/Cargo.toml:30 still declares typed-arena.workspace = true. Since this PR is a dead-code sweep, drop the now-unused crate-level dependency too (the workspace root entry stays — ast, js_parser, css, etc. depend on it directly).
Extended reasoning...
What the issue is
The PR deletes pub type TypedArena<T> = typed_arena::Arena<T>; from src/bun_alloc/lib.rs. That line was the only reference to the typed_arena crate anywhere under src/bun_alloc/. However, the crate manifest still lists it:
# src/bun_alloc/Cargo.toml:30
typed-arena.workspace = trueAfter this PR, that dependency entry is dead weight in bun_alloc's Cargo.toml.
Step-by-step proof
- Before the PR,
rg -i 'typed.arena' src/bun_alloc/returned two hits: theCargo.tomldependency line and thepub type TypedArena<T> = typed_arena::Arena<T>;re-export inlib.rs. - This PR's diff removes the
lib.rsline (the only.rsreference). - After the PR, the same grep returns only
src/bun_alloc/Cargo.toml:30:typed-arena.workspace = true— verified against the checked-out PR head. Nouse typed_arena, no path references, noextern crate, nothing inlib.rsor any submodule. - Therefore
typed-arenais compiled intobun_alloc's dependency graph but never used by it.
Why nothing catches it
cargo check --workspace (which the PR description cites as passing) does not warn on unused [dependencies] entries — that's the domain of cargo-udeps / cargo-machete, neither of which runs in this CI. So the leftover entry silently survives the build.
Also note: typed-arena remains a legitimate workspace dependency — ast, js_parser, css, and others list it in their own Cargo.tomls and use typed_arena::Arena directly (not via the removed bun_alloc::TypedArena alias, which the PR description confirms was "never referenced"). So only the bun_alloc crate-level entry should go; the [workspace.dependencies] root entry stays.
Impact
None at runtime. The only cost is a spurious edge in bun_alloc's build graph and a stale manifest line that contradicts the PR's stated goal ("dead-code sweep of bun_alloc. None referenced outside their own definitions"). Flagging it because incomplete cleanup in a cleanup PR is exactly the kind of thing that's easier to fix now than to rediscover later.
Fix
Delete line 30 from src/bun_alloc/Cargo.toml:
-typed-arena.workspace = true
2 participants
Dead-code sweep of
bun_alloc. None referenced outside their own definitions.cargo check --workspacepasses.Why this code exists
MimallocHeapRef*.zig)Allocatorimpl over raw*mut mi_heap_t, added in 9532c84; superseded byArenaPtr, never referenced outsidesrc/bun_alloc/TypedArena<T>typed_arena/TypedArenain*.zig)typed_arena::Arenaadded in 76a5089, never referencedBumpWithFallbackStackFallbackin 4075846 leaving this "back-compat alias"; no external caller ever existedStackFallback::fallback_mutstd.heap.StackFallbackAllocatorexposesfallback_allocatoras a public field (e.g.src/js_printer/renamer.zig:496,532), no method accessorfallback()is used, this&mutvariant never wasinit_default<T>src/bun_alloc/memory.zig:36(initDefault)src/ptr/owned.zig:58,64,111,147,src/ptr/shared.zig:100,src/collections/array_list.zig:70,81,165,src/threading/guarded.zig:19T::default()over 1f86fa6 / 87d8efd / 98ba4b1 / 21daace; the wrapper is now identityMaybeOwned::into_parentsrc/bun_alloc/maybe_owned.zig:76(intoParent)deinitat:48