You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Update the jsc class skill for Bun::allocatePlainObjectCell - #47840
The "Prototype Class" snippet of .claude/skills/implementing-jsc-classes-cpp/SKILL.md allocates with allocateCell<JSFooPrototype>(vm) and declares subspaceFor. Trim ~2 MB from the release binary without touching hot paths #39770 added Bun::allocatePlainObjectCell, and 78 classes use it. No document names the helper.
The snippet calls Bun::allocatePlainObjectCell, has no subspaceFor member, and has STATIC_ASSERT_ISO_SUBSPACE_SHARABLE after the class.
It calls Bun::createClassStructure, Bun::reifyStaticPropertyTable and Bun::putToStringTagWithoutTransition, as the classes on main do. The second commit changes one line of the "Constructor Class" snippet for the same reason.
Two new paragraphs give the reasons and name the header that declares the helpers.
Verified: the snippets, copied byte for byte into a scratch file, compile with the debug build flags. A data member in the prototype class fails at the assert.
Behaviour change: none
Background
subspaceFor tells JSC::allocateCell<T> which GC subspace holds a T. A prototype class adds no field, so all prototype objects share vm.plainObjectSpace(). Bun::allocatePlainObjectCell(vm, size) allocates there and takes no type.
The compiler does not evaluate a static_assert inside a template that nothing instantiates. 23 of the 78 classes have the assert only inside subspaceFor.
#include "root.h" and #include "BunClientData.h", then namespace Bun with using namespace JSC.
A stub JSFoo with DECLARE_INFO.
The "Property Definitions", "Prototype Class" and "Constructor Class" blocks of the document, unchanged.
The two s_info definitions, and empty bodies for the four host functions.
The command is the arguments entry of NodeFSStatBinding.cpp in build/debug/compile_commands.json, with the scratch file as the source. That is clang 23.1.2 with -std=gnu++23 -Werror and WebKit 4d45755fed4a.
Scratch file
Result
Document on main
compiles
Document of this pull request
compiles (-fsyntax-only and -c)
This pull request, plus int m_field in JSFooPrototype
static assertion failed due to requirement 'sizeof(Bun::JSFooPrototype) == sizeof(JSC::JSNonFinalObject)', 24 == 16, at the line after the class
This pull request, plus static void destroy(JSC::JSCell*)
static assertion failed due to requirement 'Bun::JSFooPrototype::destroy == JSC::JSNonFinalObject::destroy'
Shape of the 78 classes (helper, assert inside subspaceFor), plus int m_field
compiles, no diagnostic
Old snippet without its subspaceFor member
no member named 'subspaceFor' in 'Bun::JSFooPrototype'
The object file of the old snippet defines four functions that the object file of the new snippet does not:
JSFooPrototype::subspaceFor<>
JSC::subspaceFor<JSFooPrototype>
JSC::allocateCell<JSFooPrototype>
JSC::tryAllocateCellHelper<JSFooPrototype, ...>
A five-line file checks the static_assert rule alone. clang 23.1.2 and g++ 14.2 accept a false static_assert inside a member template that nothing instantiates. Both reject the same line at file scope. The result is the same with -std=c++17, c++20 and gnu++23.
Bun::allocatePlainObjectCell: 78 calls in 72 files. Each of the 78 classes still declares subspaceFor. 55 have the assert at file scope too.
24 other prototype classes still call allocateCell<T>, for example src/jsc/bindings/JSX509CertificatePrototype.h:21. Their subspaceFor is live. The old snippet had their shape. No commit after Trim ~2 MB from the release binary without touching hot paths #39770 added a class of this kind.
Bun::createClassStructure: 269 calls. 37 of the 42 InternalFunction subclasses use it in createStructure.
Bun::reifyStaticPropertyTable: 108 calls. Bun::putToStringTagWithoutTransition: 89 calls. Outside BunClientData.cpp, no file in src/ calls reifyStaticProperties or JSC_TO_STRING_TAG_WITHOUT_TRANSITION().
root.h does not declare the helpers. BunClientData.h does, and ZigGlobalObject.h includes it indirectly.
The "Prototype Class" snippet allocated with allocateCell<JSFooPrototype>
and declared a subspaceFor member. 78 classes in src/jsc/bindings allocate
their prototype with Bun::allocatePlainObjectCell, which takes a size and
never instantiates subspaceFor.
The snippet now calls the helper, has no subspaceFor member, and has
STATIC_ASSERT_ISO_SUBSPACE_SHARABLE after the class. It also calls
Bun::createClassStructure, Bun::reifyStaticPropertyTable and
Bun::putToStringTagWithoutTransition, as the classes on main do.
Two paragraphs after the snippet say why the class has no subspaceFor
member, why the assert is at file scope, and where the helpers are
declared.
…ippet
The text under the prototype snippet now says to use the out-of-line
helpers. The "Constructor Class" snippet still called
JSC::Structure::create. 37 of the 42 InternalFunction subclasses in src/
build their structure with Bun::createClassStructure, for example
JSStatsConstructor in src/jsc/bindings/NodeFSStatBinding.cpp.
Status: ready for review. CI is red for reasons outside this change.
This pull request has no repro. It changes one document, .claude/skills/implementing-jsc-classes-cpp/SKILL.md: 10 lines added, 8 removed. The build does not compile the document, and no test, script or workflow reads it.
CI, build 137221 (finished):
All build jobs pass. 41 test files are red.
40 of them are the 40 test files that are red in build 137162 of main at the same commit (3bf1a06): 39 files under test/aot/, and test/bundler/compile-aot-size.test.ts.
The other one is test/js/web/request/request-clone-leak.test.ts, on the x64 ASAN lane only. It measured an RSS delta of 64 MB against a bound of < 64 in 4 of 4 attempts.
This branch did not ask for a new CI run. A new run fails the same test/aot/ files until main is green.
How the snippets were checked:
A scratch file holds the "Property Definitions", "Prototype Class" and "Constructor Class" blocks of the document, copied byte for byte. It compiles with the flags that the debug build uses for src/jsc/bindings/NodeFSStatBinding.cpp (clang 23.1.2, -Werror), on main3bf1a06.
The same file with a data member in JSFooPrototype fails at the assert line: sizeof(Bun::JSFooPrototype) == sizeof(JSC::JSNonFinalObject), 24 == 16. With its own destroy in the class, it fails at the same line.
Each line of the new snippets is in a class on main. The Notes of the description list the classes.
No actionable comments were generated in the recent review.
Walkthrough
The JSC class implementation skill updates its prototype and constructor examples to use Bun helpers. It also adds guidance on when a prototype can use the shared plain-object space.
Changes
JSC Class Guidance
Layer / File(s)
Summary
Update prototype and constructor examples .claude/skills/implementing-jsc-classes-cpp/SKILL.md
The prototype example uses Bun allocation, structure, and static-property helpers. The guidance explains when a prototype can use vm.plainObjectSpace() without defining subspaceFor. The constructor example uses Bun::createClassStructure.
Suggested reviewers: alii
Merge Risk: Minimal · up to 33362
The documentation update has no identified merge-blocking issue and does not change runtime code. It is mergeable subject to normal checks.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the change: the jsc class skill document is updated for Bun::allocatePlainObjectCell. The raw summary and objectives confirm that the prototype snippet now uses this helper and the rel…
Description check
The description follows the template sections. The Problem, Fix, and Background sections explain why the change is needed, what changed, and how it was verified. The 'How did you verify your code work…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the change: the jsc class skill document is updated for Bun::allocatePlainObjectCell. The raw summary and objectives confirm that the prototype snippet now uses this helper and the related helpers. The title is specific and concise, and it matches the single documentation file changed.
Full details: Description check
Explanation
The description follows the template sections. The Problem, Fix, and Background sections explain why the change is needed, what changed, and how it was verified. The 'How did you verify your code works?' content is covered in the Fix section, which describes compiling the copied snippets with the debug build flags. The 'What does this PR do?' content is covered by the Problem and Fix sections. The template headings are not used literally, so the author should map the content to the two template headings. This is a minor formatting gap. The description is mostly complete.
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.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a doc-only update that brings the skill's prototype snippet in line with the helpers Bun's bindings already use.
What was reviewed:
Checked the four helper names and signatures against src/jsc/bindings/BunClientData.h lines 143-148; the snippet's call shapes match.
Confirmed the "class has no subspaceFor" claim against BunClientData.cpp: allocatePlainObjectCell allocates through its own PlainObjectCell::subspaceFor, so a member on the caller's class is never consulted.
File-scope STATIC_ASSERT_ISO_SUBSPACE_SHARABLE already appears in real code (e.g. src/jsc/bindings/NodeVMScript.h:42), so the snippet's placement is an existing in-tree pattern, not a new one.
The constructor snippet keeps JSC::allocateCell and its live subspaceFor; only createStructure changed, which is consistent with the InternalFunction subclasses on main.
Extended reasoning...
The PR edits a single Claude skill document, .claude/skills/implementing-jsc-classes-cpp/SKILL.md (+10/-8), replacing inline JSC calls in the example prototype and constructor classes with Bun's out-of-line helpers and adding two explanatory paragraphs; no source, build, or test files change. It touches no security-sensitive surface. Approval was decided by the change being small and documentation-only, by every helper and its semantics being verifiable in BunClientData.h/.cpp, and by the changed path having no CODEOWNER and no outstanding objections in the timeline. The remaining mismatch with the 78 existing classes (which still carry a dead subspaceFor) is a pre-existing state of the source tree that the PR description says is handled in separate PRs, not an error in this document.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Problem
.claude/skills/implementing-jsc-classes-cpp/SKILL.mdallocates withallocateCell<JSFooPrototype>(vm)and declaressubspaceFor. Trim ~2 MB from the release binary without touching hot paths #39770 addedBun::allocatePlainObjectCell, and 78 classes use it. No document names the helper.JSC::allocateCell<T>again. A class copied from one of the 78 keeps theirsubspaceFor, which nothing instantiates:ImportMetaEnv(bun test: Vitest compatibility #44833),JSModuleGraphPrototype(Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590), and six classes in the open Implement the async Clipboard API (navigator.clipboard) #33312, Dispatch unhandledrejection and rejectionhandled on the global scope with PromiseRejectionEvent #45596, Add the QuotaExceededError global #47040 and Implement import.meta.hot for bun --hot #32856.Fix
Bun::allocatePlainObjectCell, has nosubspaceFormember, and hasSTATIC_ASSERT_ISO_SUBSPACE_SHARABLEafter the class.Bun::createClassStructure,Bun::reifyStaticPropertyTableandBun::putToStringTagWithoutTransition, as the classes onmaindo. The second commit changes one line of the "Constructor Class" snippet for the same reason.Behaviour change: none
Background
subspaceFortellsJSC::allocateCell<T>which GC subspace holds aT. A prototype class adds no field, so all prototype objects sharevm.plainObjectSpace().Bun::allocatePlainObjectCell(vm, size)allocates there and takes no type.static_assertinside a template that nothing instantiates. 23 of the 78 classes have the assert only insidesubspaceFor.REVIEW.mdrules out a lint that checks that dead code stays dead.Notes
Each line of the new snippets, and a class on
main3bf1a06 that has itnew (NotNull, Bun::allocatePlainObjectCell(vm, sizeof(T))) T(vm, structure):JSStatsPrototype,src/jsc/bindings/NodeFSStatBinding.cpp:391.Bun::createClassStructure(...)withsetMayBePrototype(true):NodeFSStatBinding.cpp:407.STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(T, T::Base)after the class:src/jsc/bindings/webcrypto/JSSubtleCrypto.cpp:155, and 54 more of the 78 classes.Bun::reifyStaticPropertyTable(vm, JSFoo::info(), table, *this)andBun::putToStringTagWithoutTransition(vm, this, info()):src/jsc/bindings/JSBunRequest.cpp:204.createStructurewithBun::createClassStructure:JSStatsConstructor,NodeFSStatBinding.cpp:494.subspaceFormember: none onmain. All 78 still have the member.JSStatsPrototypeat the head of Remove dead code from the JSC prototype classes, bun_paths, bun_sys, the JSON5 parser, and two code generators #46388 (c5633b3) has the shape of the new snippet, and build 130907 of that head passed.Compile check
The scratch file has these parts, in this order:
#include "root.h"and#include "BunClientData.h", thennamespace Bunwithusing namespace JSC.JSFoowithDECLARE_INFO.s_infodefinitions, and empty bodies for the four host functions.The command is the
argumentsentry ofNodeFSStatBinding.cppinbuild/debug/compile_commands.json, with the scratch file as the source. That is clang 23.1.2 with-std=gnu++23 -Werrorand WebKit 4d45755fed4a.main-fsyntax-onlyand-c)int m_fieldinJSFooPrototypestatic assertion failed due to requirement 'sizeof(Bun::JSFooPrototype) == sizeof(JSC::JSNonFinalObject)',24 == 16, at the line after the classstatic void destroy(JSC::JSCell*)static assertion failed due to requirement 'Bun::JSFooPrototype::destroy == JSC::JSNonFinalObject::destroy'subspaceFor), plusint m_fieldsubspaceFormemberno member named 'subspaceFor' in 'Bun::JSFooPrototype'The object file of the old snippet defines four functions that the object file of the new snippet does not:
JSFooPrototype::subspaceFor<>JSC::subspaceFor<JSFooPrototype>JSC::allocateCell<JSFooPrototype>JSC::tryAllocateCellHelper<JSFooPrototype, ...>A five-line file checks the
static_assertrule alone. clang 23.1.2 and g++ 14.2 accept a falsestatic_assertinside a member template that nothing instantiates. Both reject the same line at file scope. The result is the same with-std=c++17,c++20andgnu++23.Counts on
main3bf1a06Bun::allocatePlainObjectCell: 78 calls in 72 files. Each of the 78 classes still declaressubspaceFor. 55 have the assert at file scope too.allocateCell<T>, for examplesrc/jsc/bindings/JSX509CertificatePrototype.h:21. TheirsubspaceForis live. The old snippet had their shape. No commit after Trim ~2 MB from the release binary without touching hot paths #39770 added a class of this kind.Bun::createClassStructure: 269 calls. 37 of the 42InternalFunctionsubclasses use it increateStructure.Bun::reifyStaticPropertyTable: 108 calls.Bun::putToStringTagWithoutTransition: 89 calls. OutsideBunClientData.cpp, no file insrc/callsreifyStaticPropertiesorJSC_TO_STRING_TAG_WITHOUT_TRANSITION().root.hdoes not declare the helpers.BunClientData.hdoes, andZigGlobalObject.hincludes it indirectly.Left alone
main. They are separate from the helpers of Trim ~2 MB from the release binary without touching hot paths #39770.jsDynamicCastandUNLIKELYas undeclared.WTFMoveis undeclared.initLatercall and onevisitcall per member.ZigGlobalObjectnow uses a member list and an offset table.subspaceFormember of the "Constructor Class" snippet is live: that snippet allocates withJSC::allocateCell<JSFooConstructor>.no test proof · iteration 0 · docs-only change; test-proof not applicable