Conversation
|
Updated 9:49 AM PT - Jun 18th, 2026
❌ @robobun, your commit ee12296 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32163That installs a local version of the PR into your bun-32163 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Checked #20333: not fixed by this PR, it no longer reproduces on current main at all. The union package's pattern ( |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesConstructor Prototype Wiring and Descriptor Updates
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
I didn't find any issues — the descriptor changes are mechanical and the tty refactor checks out — but since this adds DontDelete in jsFunctionToClass (transitively affecting all 44 $toClass users) and touches a dozen core node-compat modules, it's worth a quick human pass.
Extended reasoning...
Overview
This PR fixes the property descriptor of the prototype own-property on Node.js-compat builtin constructors so it matches the spec ({ writable: true, enumerable: false, configurable: false }) and Node.js. It does so via three kinds of edits across 13 files:
- Mechanical rewrites in 10 JS builtin modules:
Fn.prototype = X→Object.defineProperty(Fn, "prototype", { value: X, writable: true, enumerable: false, configurable: false }). Same value, only the attributes change. - A one-line C++ change in
jsFunctionToClass(ZigGlobalObject.cpp) addingPropertyAttribute::DontDelete, which transitively affects all ~44$toClasscall sites. - A small refactor in
node:tty: since$toClassnow installs a non-configurableprototype, the lazy accessor that follows would fail. The PR inlines$toClass's other effects ($setPrototypeDirect+ name) and lets the lazy accessor ownprototype. I traced this through: the prototype object$toClasspreviously created was immediately discarded by the accessor anyway (and itsconstructorassignment never reached the final lazily-created prototype), so this is behaviour-preserving aside from the descriptor fix.
Security risks
None. This only adjusts property attributes on already-existing prototype properties; no new data flow, parsing, auth, or I/O.
Level of scrutiny
Medium. Each individual edit is trivially equivalent (assignment → defineProperty with the spec attributes), and the new tests cover 15 representative constructors plus the originally-reported defineProperties copy idiom. However:
- The
DontDeleteflag injsFunctionToClassis a one-way door that affects every$toClasscaller. The author states tty was the only one that redefinedprototypeafterward; the test suite run (events/url/crypto/console/http/stream/net/tty/zlib/shell) supports that, but it's the kind of cross-cutting C++ binding change a maintainer should sign off on. - The files touched (events, streams, http, crypto, console, ZigGlobalObject.cpp) are very high-traffic core modules.
Other factors
- The bug-hunting pass found nothing.
- 17 new targeted tests, plus the author reports clean runs across the affected module suites and Node's ported
test-event-emitter-*files. - The fix is spec-cited and matches Node v24's observed behaviour.
I'm deferring purely on breadth (core C++ binding + a dozen hot modules), not on any specific concern with the implementation.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Modify $toClass to support this?
|
Done in 36687ba. $toClass now owns the descriptor logic and every hand-assigned
One catch found while converting: marking the passed objects with |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/tty.ts (1)
24-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the tty-specific
prototype.constructor, not the fs one.These lazy accessors fix the
prototypedescriptor, but they still leave the final constructor/prototype contract wrong.ReadStreaminstalls its ownprototypeaccessor before$toClass(...), andjsFunctionToClassexplicitly skips prototype creation when an ownprototypealready exists, soReadStream.prototype.constructorstays inherited fromfs.ReadStream.prototype.WriteStreamhas the same problem because its accessor aliasesfs.WriteStream.prototypedirectly, soWriteStream.prototype.constructorremainsfs.WriteStream.💡 Possible fix
Object.defineProperty(ReadStream, "prototype", { get() { const Prototype = Object.create(fs.ReadStream.prototype); + Object.defineProperty(Prototype, "constructor", { + value: ReadStream, + writable: true, + enumerable: false, + configurable: true, + }); // Add ref/unref methods to make tty.ReadStream behave like Node.js // where TTY streams have socket-like behavior Prototype.ref = function () { @@ Object.defineProperty(WriteStream, "prototype", { get() { - const Real = fs.WriteStream.prototype; + const Prototype = Object.create(fs.WriteStream.prototype); + Object.defineProperty(Prototype, "constructor", { + value: WriteStream, + writable: true, + enumerable: false, + configurable: true, + }); // Once materialized, match the descriptor of a regular function's "prototype". Object.defineProperty(WriteStream, "prototype", { - value: Real, + value: Prototype, writable: true, enumerable: false, configurable: false, }); - WriteStream.prototype._refreshSize = function () { + Prototype._refreshSize = function () { const oldCols = this.columns; const oldRows = this.rows; const windowSizeArray = [0, 0]; @@ - return Real; + return Prototype; },Based on the
$toClasscontract insrc/jsc/bindings/ZigGlobalObject.cpp, an existing ownprototypeproperty prevents the helper from auto-definingprototype.constructor.Also applies to: 131-207
🤖 Prompt for 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. In `@src/js/node/tty.ts` around lines 24 - 110, The lazy prototype accessor for ReadStream (and similarly for WriteStream) copies fs.*Stream.prototype but leaves Prototype.constructor pointing at fs.ReadStream/WriteStream; before calling Object.defineProperty(ReadStream, "prototype", { value: Prototype, ... }) set the Prototype.constructor to the stream constructor (e.g. ReadStream) with the correct descriptor (non-enumerable, writable/configurable as appropriate) so that $toClass/jsFunctionToClass does not leave the prototype.constructor inherited from fs.ReadStream; apply the same change in the WriteStream accessor.
🤖 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.
Outside diff comments:
In `@src/js/node/tty.ts`:
- Around line 24-110: The lazy prototype accessor for ReadStream (and similarly
for WriteStream) copies fs.*Stream.prototype but leaves Prototype.constructor
pointing at fs.ReadStream/WriteStream; before calling
Object.defineProperty(ReadStream, "prototype", { value: Prototype, ... }) set
the Prototype.constructor to the stream constructor (e.g. ReadStream) with the
correct descriptor (non-enumerable, writable/configurable as appropriate) so
that $toClass/jsFunctionToClass does not leave the prototype.constructor
inherited from fs.ReadStream; apply the same change in the WriteStream accessor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8f535f0-8277-4e3e-bc6f-10ef437e66cd
📒 Files selected for processing (14)
src/js/builtins.d.tssrc/js/builtins/ConsoleObject.tssrc/js/builtins/shell.tssrc/js/internal/streams/readable.tssrc/js/internal/streams/writable.tssrc/js/node/_http_client.tssrc/js/node/_http_incoming.tssrc/js/node/_http_outgoing.tssrc/js/node/crypto.tssrc/js/node/events.tssrc/js/node/tty.tssrc/js/node/url.tssrc/jsc/bindings/ZigGlobalObject.cpptest/js/node/events/event-emitter.test.ts
|
Addressed CodeRabbit's tty finding in a2c8ec9, for the half that's correct:
|
f20ff48 to
bfc3b34
Compare
|
Rebased onto main (bfc3b34) to clear the merge conflicts. The conflicts were all in All other files merged cleanly. The descriptor matrix test (97 pass), the stream, tty, url, console, crypto, and shell suites are green locally. (The one node-http failure is |
There was a problem hiding this comment.
No issues found after the rebase — the change looks correct and is well-tested, but since it reworks jsFunctionToClass (shared by 44+ callers) and swaps setMayBePrototype for didBecomePrototype(), it's worth a human pass on the JSC bits.
Extended reasoning...
Overview
This PR fixes #32160 by making builtin constructors' prototype property report the spec-correct descriptor {writable: true, enumerable: false, configurable: false} instead of all-true. It centralizes the fix in jsFunctionToClass (ZigGlobalObject.cpp), which now: defines prototype with DontEnum | DontDelete, accepts an optional caller-built prototype object (4th arg), preserves a pre-existing own prototype (for tty's lazy accessor), preserves a pre-existing own constructor on a passed prototype, and uses didBecomePrototype() instead of structure()->setMayBePrototype(true) to avoid poisoning shared structures. Ten JS call sites (events, url, crypto, console, shell, tty, internal stream state classes) are converted from Fn.prototype = {} to $toClass(...), plus a builtins.d.ts signature update and a 76-line test matrix covering 15 constructors.
Security risks
None. This is descriptor-attribute correctness for Node.js compat; no auth, crypto primitives, network, or filesystem semantics are touched. The crypto.ts edit only swaps Certificate.prototype = {} for $toClass(Certificate, "Certificate").
Level of scrutiny
Medium-high. The JS-side conversions are mechanical and low-risk on their own, but the C++ change is a shared helper used by 44+ existing $toClass callers (all stream/net/zlib/http classes etc.), so any subtle mistake fans out widely. Two aspects in particular warrant human eyes from someone with JSC internals knowledge:
- The switch from
structure()->setMayBePrototype(true)toprototype->didBecomePrototype(vm). The PR's rationale (object literals / empty-object structure cache sharing tripping a LiteralParser debug assert) is sound anddidBecomePrototype()is the canonical API for this, but it changes structure-transition behavior for every$toClassuser. - Adding
DontDeleteis a tightening — spec-correct and Node-matching, but worth confirming nothing in the ecosystem was relying on reconfiguring these prototypes.
Other factors
- My earlier inline concern about
EventEmitter.namebecoming writable was empirically refuted by the author (JSC's lazy name reification overrides the putDirect attributes) and locked in with a test assertion across all 15 constructors; that thread is resolved. - The post-rebase diff shrank from 14 → 11 files because main independently migrated the three http constructors to
$toClass; the test matrix still covers them. - The bug-hunting pass found nothing; CodeRabbit found nothing actionable.
- Test coverage is thorough (descriptor shape, the failing
definePropertiesidiom from the issue,prototype.constructorshape, andnameshape, all cross-checked against Node v24).
Given the shared-helper blast radius and the JSC structure-handling change, I'm deferring rather than approving.
Functions declared in builtin JS have no own prototype property, so
plain assignments (Fn.prototype = {}) created one via [[Set]] with
default attributes (enumerable: true, configurable: true). Per spec a
function's prototype must be non-enumerable and non-configurable.
Route every such site through $toClass, which now owns the correct
descriptor: it accepts an optional prototype object, keeps a
pre-existing own prototype (node:tty's lazy accessor) or constructor
(Bun.$.Shell's shared class prototype), and defines prototype with
DontEnum | DontDelete. Marking passed objects as prototypes uses
didBecomePrototype so shared structures are not poisoned.
A class declaration also has its own prototype (holding the class
body), so $toClass keeps it instead of overwriting; when a base is
supplied it wires that prototype's [[Prototype]] to the base so
instances still inherit (e.g. perf_hooks PerformanceNodeTiming /
PerformanceResourceTiming extending PerformanceEntry).
5b97edc to
ee12296
Compare
|
CI status on ee12296: the only red lane is The subprocess reaches It's a napi finalizer teardown-ordering leak: finalizers enqueued during the final exit-GC sweep never drain (the event loop has stopped). The test itself forces This PR's diff touches no napi code ( |
|
CI status for ee12296 (build 63337): three jobs are red, none caused by this diff.
The diff itself is unchanged and ready for re-review. |
|
Stale PR review: keep open, rework. The bug is real and still on main. On 1.4.3-canary (367d939), The head commit has the shape that the review from 2026-06-12 asked for:
The merge conflicts are in |
Fixes #32160
Problem
Per spec (https://tc39.es/ecma262/#sec-function-instances-prototype) a function's own
prototypeproperty is{ [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: false }. Because Bun reported it as configurable, the common "copy a function's shape onto a wrapper" idiom threw: the copied descriptor conflicts with the target function's correctly non-configurableprototype.Cause
Functions declared in builtin JS don't get an automatic own
prototypeproperty, soFn.prototype = {}at module setup creates one via[[Set]]with default attributes (all true). The same class of bug existed in$toClass(the internal helper that mimicsclasssetup for function-style constructors), which putprototypewith onlyDontEnum, leaving it configurable.Fix
$toClassis now the one place that knows the correct descriptor, and every hand-assignedFn.prototype = ...site becomes a$toClasscall:jsFunctionToClass(ZigGlobalObject.cpp) definesprototypewithDontEnum | DontDelete, matching the spec for all 44 existing$toClassusers (stream.Readable/Writable/Duplex,net.Socket, zlib classes, ...) plus the converted sites.httpmessage classes andBun.$.Shellpass their existing prototype objects instead of getting a fresh one).prototype(node:tty installs a lazy accessor first;$toClassnow leaves it in place and still wires up static inheritance and the name). tty's accessors materialize into the spec-correct descriptor on first use; previously the materialized descriptor was{ writable: false, enumerable: true, configurable: true }, wrong on all three attributes.constructoron the passed prototype (Bun.$.Shellshares a class prototype whoseconstructormust stay). Where it was absent or hand-assigned,prototype.constructoris now the non-enumerable data property Node has:events.EventEmitter's was enumerable (visible tofor...in), andurl.Url,crypto.Certificate,console.Console, and thehttpclasses either lacked it or defined it enumerable in their prototype literals.didBecomePrototype()instead ofstructure()->setMayBePrototype(true): passed objects (andconstructEmptyObject(globalObject)without a base) can share structures with the literal/empty-object structure caches, and flagging those in place tripsASSERTION FAILED: !newStructure->mayBePrototype()in JSON parsing on debug builds.tty.ReadStream's lazily created prototype also gets its own
constructornow (previously inherited fs.ReadStream's); tty.WriteStream keeps sharingfs.WriteStream.prototypeby design, so itsconstructoris unchanged.Converted sites:
events.EventEmitter(also exported asevents.init),url.Url,crypto.Certificate,console.Console,http.OutgoingMessage/IncomingMessage/ClientRequest, internal streamReadableState/WritableState,Bun.$.Shell, andtty.ReadStream.$toClassnow also handlesclassdeclarations correctly. A class carries its ownprototype(from ClassDefinitionEvaluation), so$toClasskeeps it rather than overwriting, and when a base is supplied it wires that prototype's[[Prototype]]to the base so instances still inherit (perf_hooksPerformanceNodeTiming/PerformanceResourceTimingextendPerformanceEntry). This is checked via a PropertySlot so node:tty's lazy accessor prototype is still left untouched. Previously the overwrite silently dropped the class body's own getters;nodeTiming.name/entryTypenow return"node"andinstanceof PerformanceEntryistrue, matching Node v24. Regression test added intest/js/node/perf_hooks/perf_hooks.test.ts.Out of scope: before first access,
tty.ReadStream/WriteStream.prototypeis still reported as an accessor (the deliberate lazy-initialization mechanism), and native classes likeBuffer(whoseprototypeis non-writable, a different mechanism) are unchanged.Verification
Tests in
test/js/node/events/event-emitter.test.tsassert the exactprototypedescriptor for every fixed constructor, thedefinePropertiescopy idiom from the issue, and theprototype.constructordescriptor; 25 of 96 fail on the unfixed build, all pass with this change. Both descriptor shapes verified against Node v24 for every entry.Also ran the events, url, crypto, console, http, stream, net, tty, and shell suites plus Node's ported
test-event-emitter-*files with the debug build; no new failures (the http proxy test fails identically without this change: network sandboxing in the test environment).