Remove the unused function-slot vector from JSCommonJSExtensions - #41405
Conversation
require.extensions has kept custom loader functions alive through Strong handles on the Rust side since #19231. The older mechanism it replaced - a WTF::Vector<WriteBarrier<Unknown>> on the JSCommonJSExtensions cell, the JSCommonJSExtensions__appendFunction / __setFunction / __swapRemove externs that mutated it, and the visitChildren override that walked it - has had no callers since then: the Rust module only declared the externs. Delete the vector, the three externs, the visitChildren override, the never-defined destructor declaration, and the Rust extern block.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe change removes registered-function storage and garbage-collection visitation from ChangesRegistered function cleanup
Merge Risk: ⚪ Minimal · up to The change removes unused extension-function registration plumbing without any identified remaining merge-readiness risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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.
What was reviewed:
- Grepped
src/for the three removed extern names andm_registeredFunctions— no remaining references anywhere. - Confirmed
CustomLoader::Custom(Strong)inNodeModuleModule.rsis what roots user-supplied loader functions;on_require_extension_modifynever called the removed externs, so the vector was always empty andvisitChildrenImplonly ever forwarded toBase. - The removed
~JSCommonJSExtensions()had no definition and nodestroy()override to reach it — it was already dead. NoWriteBarriermembers remain, so droppingDECLARE_VISIT_CHILDRENis safe.
Extended reasoning...
Overview
This PR deletes 70 lines across three files with zero additions. It removes WTF::Vector<WriteBarrier<Unknown>> m_registeredFunctions from JSCommonJSExtensions, the visitChildrenImpl/DEFINE_VISIT_CHILDREN that traced it, a declared-but-undefined destructor, three extern "C" mutators (__appendFunction/__setFunction/__swapRemove), and the matching Rust extern "C" block in NodeModuleModule.rs. The stated reason is that since an earlier PR, custom require.extensions loaders are kept alive by Rust-side Strong handles, making this C++-side rooting mechanism a vestigial leftover with no callers.
Security risks
None. This is deletion of unreferenced native code. No user-facing surface changes, no new parsing, no auth/crypto/permissions paths touched.
Level of scrutiny
Moderate — the REVIEW.md rule "don't silently delete existing [GC roots]" required verifying that the deleted WriteBarrier vector was genuinely redundant, not load-bearing. I confirmed this directly: CustomLoader::Custom(Strong) at src/jsc/NodeModuleModule.rs:116 holds the loader function via a Strong handle created in on_require_extension_modify (lines 148, 156, 158), and that function contains no calls to the removed externs. A repo-wide grep for all four removed identifiers returned zero hits. The vector had no writers, so it was always empty; the removed visitChildren therefore never appended anything beyond Base::visitChildren.
Other factors
The removed ~JSCommonJSExtensions() declaration had no definition anywhere (grep confirms), and the class defines no destroy() override — CREATE_METHOD_TABLE inherits JSDestructibleObject::destroy, which never reached the derived destructor anyway. So the declaration was already dead. The class still derives from JSDestructibleObject despite now having no C++ fields; switching to JSNonFinalObject would be a minor optimization but touches subspace/structure plumbing and is reasonably out of scope for a focused deletion PR. No behavior change, so no new test is expected per REVIEW.md; existing require.extensions coverage in test/js/node/module applies.
|
Updated 4:16 PM PT - Sep 4th, 2026
❌ @dylan-conway, your commit fd9e576 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41405That installs a local version of the PR into your bun-41405 --bun |
2 participants
What does this PR do?
Deletes a leftover from the original
require.extensionsimplementation. Since #19231, custom loader functions are kept alive byStronghandles on the Rust side (CustomLoader::Custom). The mechanism that change replaced — aWTF::Vector<WriteBarrier<Unknown>>member on theJSCommonJSExtensionscell, theJSCommonJSExtensions__appendFunction/__setFunction/__swapRemoveexterns that mutated it, and avisitChildrenoverride that walked it — has had no callers since then;NodeModuleModule.rsonly declared the externs. This removes the vector, the three externs, thevisitChildrenoverride, the declared-but-never-defined destructor, and the Rust extern block.Worth removing rather than leaving: the vector was mutated and visited without
cellLock(), unlike every otherVector<WriteBarrier>member in our cells, so reviving it as-is would have raced the concurrent marker.No behaviour change: the vector was always empty, so the override only ever called
Base::visitChildren.How did you verify your code works?
grepacrosssrc/for the three extern names andm_registeredFunctions: the only references were the definitions and the Rust declarations removed here.JSCommonJSExtensions.cppwith the release flags (-fsyntax-only) after the change; clean. Relying on CI for the full build and the existingrequire.extensionstests (test/js/node/module).