Skip to content

Remove dead code from helpers.h and MimallocWTFMalloc.h - #45548

Open
robobun wants to merge 2 commits into
mainfrom
robobun/4273fcc0/dead-code-sweep
Open

robobun wants to merge 2 commits into
mainfrom
robobun/4273fcc0/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Delete both. The diff has deletions only: 2 files, 35 lines.
  • Correct because a -O0 build emits a symbol for every function it uses. Neither function has a symbol in any object of the debug build of main. The compiler does not report them: the build passes -Wno-unused-function.
  • A --gc-sections link keeps the same 1,415,903 symbols before and after, and drops the same functions.
  • Verified: bun bd, and four test files on the debug build (293 pass, 0 fail, listed in Notes).

Background

Downsides

  • None found. Checked: the symbols of every object in the debug build, and git grep over the sources and the generated code. Also checked: the added lines of all 5599 open pull requests.

Behaviour change: none

Notes

Method

Tests

  • Debug build, --timeout 90000: test/js/web/fetch/headers.test.ts, test/js/web/fetch/fetch_headers.test.js, test/internal/bindgen.test.ts, test/js/web/web-globals.test.js. 293 pass, 0 fail.
  • After the merge of main (017b243): git grep finds no use of either function, bun bd builds, and the same four files pass (314 pass, 0 fail).

Self-review

  • A review of the first version of this diff (699 lines) ran for 88 minutes and was stopped before it produced a verdict. The scan of open pull requests below took its place and removed 664 lines from the diff.

Found without a user on main, but not deleted: an open pull request uses or rewrites it

A first version of this diff deleted 699 lines of WebIDL converter code. The scan of open pull requests showed that most of it has a user in flight, so those deletions are not here.

Scanned and clean, or already claimed

  • Rust: a name scan over 3160 files, every .rs file against the build's dep-info, Cargo features, and the functions that the --gc-sections link drops. Each candidate is a proc-macro entry point, has a user on another target, is in an open pull request, or is in the mordant baseline.
  • TypeScript: tsc --noUnusedLocals over src/js, src/codegen, src/runtime/bake, scripts and six packages reports only unused parameters. No internal module has an export that nothing reads.
  • C and C++: clang -Wunused-function -Wunused-template -Wunused-macros over 591 of 602 translation units (11 do not compile alone). Each finding has a user on another platform or is in an open pull request.
  • 48 dead-code pull requests are open. 34 of them no longer merge into main. Their deletions were not repeated here.

Follow-up candidates, not in this diff

Delete two functions under src/jsc/bindings whose last callers are gone:

- Zig::toStringCopy(EncodedSlice, StringPointer) in helpers.h. #45461 removed its two callers in WebCore__FetchHeaders__createValueNotJS.
- MimallocMalloc::alignedMalloc in MimallocWTFMalloc.h. #45104 removed its caller in Bindgen/ExternVectorTraits.h.

The diff has deletions only: 2 files, 35 lines.
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 77265409-de62-4b31-a2c4-ae85eaee4a88
📥 Commits

Reviewing files that changed from the base of the PR and between 017b243 and 9946129.

📒 Files selected for processing: 2
  • src/jsc/bindings/MimallocWTFMalloc.h
  • src/jsc/bindings/helpers.h
💤 Files with no reviewable changes: 2
  • src/jsc/bindings/MimallocWTFMalloc.h
  • src/jsc/bindings/helpers.h

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

The change removes MimallocMalloc::alignedMalloc and the toStringCopy overload that accepts a StringPointer. tryAlignedMalloc and the one-argument toStringCopy remain.

Changes

Aligned allocation helper

Layer / File(s) Summary
Remove aligned allocation wrapper
src/jsc/bindings/MimallocWTFMalloc.h
alignedMalloc was removed. tryAlignedMalloc remains available. The removed method called tryAlignedMalloc and crashed if allocation returned null.

Encoded slice conversion helper

Layer / File(s) Summary
Remove subrange conversion overload
src/jsc/bindings/helpers.h
The toStringCopy overload accepting an EncodedSlice and StringPointer was removed, along with its empty-range check, length-limit checks, and UTF-8, UTF-16, and Latin-1 conversion paths. The one-argument overload remains.

Suggested reviewers: dylan-conway

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 99461

No concrete behavior change or merge-blocking risk is identified. Merge after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description clearly explains what the PR changes and how the changes were verified. It does not use the exact template headings, but it provides the required information in equivalent sections.
Title check Passed The title clearly and concisely identifies the removal of dead code from both affected files, which matches the main change.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:53 AM PT - Oct 17th, 2026

✅ @robobun, your commit 9946129e08b5e245105f2b60bc83d9e8a192aafa passed in Build #127546! 🎉


🧪   To try this PR locally:

bunx bun-pr 45548

That installs a local version of the PR into your bun-45548 executable, so you can run:

bun-45548 --bun

@robobun

robobun commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

How the two functions were shown to be dead:

  • A debug build of main, then the same link with --gc-sections --print-gc-sections. Neither function has a symbol in any object. A -O0 build emits a symbol for every function it uses.
  • The same build and link with this diff. The kept symbols (1,415,903) and the dropped function symbols are identical to main.
  • git grep -w finds no other mention of alignedMalloc, and no call of toStringCopy with two arguments.
  • No open pull request adds a call to either function (added lines of all 5599 open pull requests).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, straightforward dead-code removal.

What was reviewed:

  • MimallocMalloc::alignedMalloc: repo-wide grep finds no references; git show af5743d7 confirms its only caller in Bindgen/ExternVectorTraits.h was dropped by #45104, and the surviving traits code now static_asserts alignof(ExternElement) <= maxAlign rather than allocating over-aligned buffers. tryAlignedMalloc (still used by bun_icu_decompress.cpp) is untouched.
  • Two-argument toStringCopy(EncodedSlice, StringPointer): a multiline grep across C++, codegen and Rust sources finds no call with a second argument; all 22 call sites use the one-argument overload that remains. StringPointer is still used elsewhere (bindings.cpp), so nothing is left orphaned.
  • Both symbols were header-inline statics with no #if-gated references, so no platform-specific compile or link dependency is affected.
Extended reasoning...

The change deletes 35 lines from two headers in src/jsc/bindings: a crash-on-null aligned-malloc wrapper on the MimallocMalloc allocator policy, and a sub-slice overload of Zig::toStringCopy. It touches no security-sensitive surface and no tests, which is appropriate for a pure deletion. The deciding facts were that git history confirms the sole prior caller of alignedMalloc was removed in #45104, and repo-wide (including multiline) greps find no remaining caller of either removed function in C++, codegen, or Rust sources. The vendored WTF headers are not in this checkout, but the alignment members are Bun-local additions outside WTF's FastMalloc policy contract and the retained traits code no longer relies on them. CODEOWNERS does not cover the changed paths and there are no outstanding reviewer objections.

@robobun

robobun commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Two more deletions from a sweep of the same base (5866b5f) are in no open dead-code pull request, this one included. The patch is below, in case a maintainer wants them here and not in a pull request of their own.

  • CryptoAlgorithmAES_CBC::Padding and the fourth parameter of platformEncrypt/platformDecrypt (src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBC.h, CryptoAlgorithmAES_CBCOpenSSL.cpp, +4/-9). The two callers pass three arguments, and the definitions never name the parameter. Remove dead code: 17 log scopes nothing logs to and four fields nothing reads, in 10 crates #44050 lists it as verified and held. Upstream WebKit still has the parameter.
  • impl From<bun_sys::Error> for bun_spawn::Error (src/spawn/error.rs, -6). A --gc-sections link of the debug build drops it, and nothing names it on any target. The Sys variant keeps its three other constructors.

Verified on 5866b5f with only these two hunks: cargo check --workspace --all-targets, and cargo check --workspace for x86_64-pc-windows-msvc and aarch64-apple-darwin. A larger diff that contained both hunks also passed bun run rust:check-all (12 triples), a release check with --cfg bun_codegen_embed, bun bd, and test/js/web/crypto/web-crypto.test.ts (95 pass). The patch also applies to main at b28212a. None of the 183 open pull request heads that I checked adds a user of Padding. I did not scan the added lines of every open pull request.

The same sweep found three more things without a user on main. They stay, for the reasons that this PR already gives for the converters:

Patch (3 files, +4/-15)
diff --git a/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBC.h b/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBC.h
index c4a5ad044a..c80d18cc01 100644
--- a/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBC.h
+++ b/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBC.h
@@ -36,18 +36,13 @@ class CryptoKeyAES;
 
 class CryptoAlgorithmAES_CBC final : public CryptoAlgorithm {
 public:
-    enum class Padding : uint8_t {
-        Yes,
-        No
-    };
-
     static constexpr ASCIILiteral s_name = "AES-CBC"_s;
     static constexpr CryptoAlgorithmIdentifier s_identifier = CryptoAlgorithmIdentifier::AES_CBC;
     static Ref<CryptoAlgorithm> create();
 
     // Operations can be performed directly.
-    WEBCORE_EXPORT static ExceptionOr<Vector<uint8_t>> platformEncrypt(const CryptoAlgorithmAesCbcCfbParams&, const CryptoKeyAES&, const Vector<uint8_t>&, Padding padding = Padding::Yes);
-    WEBCORE_EXPORT static ExceptionOr<Vector<uint8_t>> platformDecrypt(const CryptoAlgorithmAesCbcCfbParams&, const CryptoKeyAES&, const Vector<uint8_t>&, Padding padding = Padding::Yes);
+    WEBCORE_EXPORT static ExceptionOr<Vector<uint8_t>> platformEncrypt(const CryptoAlgorithmAesCbcCfbParams&, const CryptoKeyAES&, const Vector<uint8_t>&);
+    WEBCORE_EXPORT static ExceptionOr<Vector<uint8_t>> platformDecrypt(const CryptoAlgorithmAesCbcCfbParams&, const CryptoKeyAES&, const Vector<uint8_t>&);
 
 private:
     CryptoAlgorithmAES_CBC() = default;
diff --git a/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBCOpenSSL.cpp b/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBCOpenSSL.cpp
index 793b97da88..3f88ec1a37 100644
--- a/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBCOpenSSL.cpp
+++ b/src/jsc/bindings/webcrypto/CryptoAlgorithmAES_CBCOpenSSL.cpp
@@ -117,7 +117,7 @@ static std::optional<Vector<uint8_t>> cryptDecrypt(const Vector<uint8_t>& key, c
     return plainText;
 }
 
-ExceptionOr<Vector<uint8_t>> CryptoAlgorithmAES_CBC::platformEncrypt(const CryptoAlgorithmAesCbcCfbParams& parameters, const CryptoKeyAES& key, const Vector<uint8_t>& plainText, Padding)
+ExceptionOr<Vector<uint8_t>> CryptoAlgorithmAES_CBC::platformEncrypt(const CryptoAlgorithmAesCbcCfbParams& parameters, const CryptoKeyAES& key, const Vector<uint8_t>& plainText)
 {
     auto output = cryptEncrypt(key.key(), parameters.ivVector(), Vector<uint8_t>(plainText));
     if (!output)
@@ -125,7 +125,7 @@ ExceptionOr<Vector<uint8_t>> CryptoAlgorithmAES_CBC::platformEncrypt(const Crypt
     return WTF::move(*output);
 }
 
-ExceptionOr<Vector<uint8_t>> CryptoAlgorithmAES_CBC::platformDecrypt(const CryptoAlgorithmAesCbcCfbParams& parameters, const CryptoKeyAES& key, const Vector<uint8_t>& cipherText, Padding)
+ExceptionOr<Vector<uint8_t>> CryptoAlgorithmAES_CBC::platformDecrypt(const CryptoAlgorithmAesCbcCfbParams& parameters, const CryptoKeyAES& key, const Vector<uint8_t>& cipherText)
 {
     auto output = cryptDecrypt(key.key(), parameters.ivVector(), cipherText);
     if (!output)
diff --git a/src/spawn/error.rs b/src/spawn/error.rs
index 233800d59f..7751d62b42 100644
--- a/src/spawn/error.rs
+++ b/src/spawn/error.rs
@@ -35,10 +35,4 @@ impl bun_core::output::ErrName for Error {
     }
 }
 
-impl From<bun_sys::Error> for Error {
-    fn from(e: bun_sys::Error) -> Self {
-        Self::Sys(e.into())
-    }
-}
-
 pub type Result<T, E = Error> = core::result::Result<T, E>;

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants