inspect: show real class name for DOM constructors, not [class Function]#29229
inspect: show real class name for DOM constructors, not [class Function]#29229
[class Function]#29229Conversation
`console.log(ReadableStreamBYOBReader)` printed `[class Function]`. The `.Class` formatter used `calculatedClassName`, which walks the prototype chain looking for a `.constructor`. For WebCore InternalFunction constructors that walk resolves to `Function.prototype.constructor === Function`, so every DOM class in `node:stream/web` rendered as `[class Function]`. Use the constructor's own `.name` property instead — it's set to the real class name at constructor init. Also only report `extends` when the parent is itself a class, so built-ins don't render as `[class X extends Function]`. Fixes #29225
|
Updated 10:47 PM PT - Apr 12th, 2026
❌ @robobun, your commit 4f694c2 has some failures in 🧪 To try this PR locally: bunx bun-pr 29229That installs a local version of the PR into your bun-29229 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughConsole class formatting now uses the constructor's Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29225.test.ts`:
- Line 67: In the test file update each spawned-process assertion to check
stdout before the exit code: for the occurrences that currently call
expect(exitCode).toBe(0) (variables named exitCode and stdout in
test/regression/issue/29225.test.ts), move the corresponding
expect(stdout).toBe(...) assertions to appear before expect(exitCode).toBe(0) so
stdout mismatches are reported first (apply this swap at the three spots that
currently assert exitCode first).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc331170-8c14-4ca0-b000-ab388420987e
📒 Files selected for processing (2)
src/bun.js/ConsoleObject.zigtest/regression/issue/29225.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/29225.test.ts (1)
63-63:⚠️ Potential issue | 🟡 MinorReorder assertions to check
stdoutbeforeexitCodein spawned-process tests.
expect(exitCode).toBe(0)is currently placed before output assertions in all three spawned-process tests. Move exit-code checks to the end of each test block so output diffs are surfaced first.Suggested diff
- expect(exitCode).toBe(0); - const lines = stdout.trim().split("\n"); expect(lines.length).toBe(streamWebClasses.length); @@ for (let i = 0; i < streamWebClasses.length; i++) { @@ expect(lines[i]).toBe(`${name}: [class ${name}]`); } + expect(exitCode).toBe(0); @@ - expect(exitCode).toBe(0); expect(stdout).toBe( @@ "Event: [class Event]\n", ); + expect(exitCode).toBe(0); @@ - expect(exitCode).toBe(0); // `Anon` picks up the "Anon" name from the variable binding, matching // JSC's naming inference for `const Anon = class {};`. expect(stdout).toBe("Foo: [class Foo]\n" + "Bar: [class Bar extends Foo]\n" + "Anon: [class Anon]\n"); + expect(exitCode).toBe(0);As per coding guidelines: "Expect stdout assertions before exit code assertions - expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages".
Also applies to: 98-99, 128-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29225.test.ts` at line 63, In the spawned-process tests in test/regression/issue/29225.test.ts, move the exit code assertions to after the stdout assertions so output diffs are shown first: for each test that currently calls expect(exitCode).toBe(0) before checking stdout, reorder to assert expect(stdout).toBe(...) (or other stdout expectations) first, then call expect(exitCode).toBe(0); update all occurrences that use the exitCode and stdout variables (including the other two blocks referencing exitCode around the same test functions) so stdout assertions run before exit code assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29225.test.ts`:
- Around line 2-12: Remove the multi-line bug-history narration comment block
that begins with "Before the fix, `console.log(ReadableStreamBYOBReader)`
printed" and trim it down to a single-line GitHub issue URL comment; keep only
the one-line issue reference and any minimal test-design rationale, removing the
lengthy prose. Apply the same trimming to the other similar comment block(s) in
this test (the short explanatory narration that follows the URL), ensuring test
files under test/regression/issue/*.test.ts contain only the single-line issue
URL and concise comments.
---
Duplicate comments:
In `@test/regression/issue/29225.test.ts`:
- Line 63: In the spawned-process tests in test/regression/issue/29225.test.ts,
move the exit code assertions to after the stdout assertions so output diffs are
shown first: for each test that currently calls expect(exitCode).toBe(0) before
checking stdout, reorder to assert expect(stdout).toBe(...) (or other stdout
expectations) first, then call expect(exitCode).toBe(0); update all occurrences
that use the exitCode and stdout variables (including the other two blocks
referencing exitCode around the same test functions) so stdout assertions run
before exit code assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13664b16-a832-482d-9cee-965b693ce52b
📒 Files selected for processing (1)
test/regression/issue/29225.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean, focused fix with good test coverage; the two inline nits are test-style only and don't affect correctness.
Extended reasoning...
Overview
The PR modifies src/bun.js/ConsoleObject.zig (the .Class formatter branch) and adds a new regression test file. The Zig change is small: ~12 net lines replacing getClassName/calculatedClassName with getName (the .name property), and adding a guard so extends is only emitted when the proto is itself a class.
Security Risks
None. This is a cosmetic formatting fix in the debug/inspect path with no auth, crypto, or permission implications.
Level of Scrutiny
Low. The fix is a one-for-one substitution that mirrors what the .Function formatter branch already does. Memory management follows the existing defer printable.deref() pattern used elsewhere in the file. The proto_is_class guard correctly prevents spurious extends Function output for built-ins.
Other Factors
The two inline bugs are both nits about test-file conventions (missing test.concurrent, stdout-before-exitCode ordering). Neither affects test correctness or the fix itself. The core Zig logic is sound and well-covered by the new test (17 stream/web constructors, 5 DOM globals, user-defined class scenarios, and a functional regression guard).
| const proto_is_class = !proto.isEmptyOrUndefinedOrNull() and proto.isCell() and proto.isClass(this.globalThis); | ||
| var printable_proto: bun.String = if (proto_is_class) try proto.getName(this.globalThis) else bun.String.empty; |
There was a problem hiding this comment.
🟡 The new proto_is_class check silently drops the extends clause when a class extends a regular function (not a class-keyword constructor): class Derived extends someRegularFunction {} now renders as [class Derived] instead of [class Derived extends someRegularFunction]. While the old behavior was also wrong (it showed [class Derived extends Function]), Node.js correctly shows the actual base name; a more precise fix would check proto === Function.prototype instead of proto.isClass(), which would suppress the noisy 'extends Function' case for DOM/built-ins while preserving the extends name for plain-function bases.
Extended reasoning...
What the bug is and how it manifests
In ConsoleObject.zig at lines 2345-2346, the new code computes:
const proto_is_class = \!proto.isEmptyOrUndefinedOrNull() and proto.isCell() and proto.isClass(this.globalThis);
var printable_proto: bun.String = if (proto_is_class) try proto.getName(this.globalThis) else bun.String.empty;
The isClass() C++ implementation checks isClassConstructorFunction() for JS functions. Regular JS functions (declared with the function keyword) return false from isClassConstructorFunction(), so proto_is_class is false, printable_proto is set to bun.String.empty, and the extends clause is omitted entirely.
The specific code path that triggers it
- User writes class Derived extends regularFunction {} where regularFunction = function() {}.
- proto = Derived.getPrototype(globalThis) → the regular function regularFunction.
- proto.isClass() → calls isClassConstructorFunction() → returns false.
- printable_proto is assigned bun.String.empty.
- Output: [class Derived] — extends clause silently dropped.
- Node.js output: [class Derived extends regularFunction] — correct.
Why existing code does not prevent it
The code comment explicitly documents this as an intentional tradeoff: 'Only report extends when the parent is itself a class — otherwise built-in and DOM constructors have Function.prototype as their prototype, which would render as [class X extends Function] and is noise.' There are no tests for the edge case of extending a plain function. The old behavior was wrong too (calculatedClassName walked the chain and returned "Function", not the base name), but at least it preserved the existence of inheritance.
Addressing the refutation
The refutation correctly notes that the old behavior was also wrong (showing [class Derived extends Function] instead of the actual base name) and that the new behavior is arguably less misleading. Both behaviors diverge from Node.js. However, the new behavior is a regression in a specific dimension: it loses all inheritance information rather than showing an incorrect name. A developer inspecting code where class Derived extends someFactory() would see no indication that inheritance is present at all, which is harder to debug than a wrong name.
What the impact would be
This is cosmetic only. Functional behavior (instanceof, prototype chain, subclassing) is unaffected. The edge case of extending a plain function with class syntax is uncommon in modern JS. Severity is nit.
How to fix it
Rather than proto.isClass(), check whether proto is exactly Function.prototype. This correctly suppresses the noisy 'extends Function' for DOM/built-in constructors (whose prototype chain leads to Function.prototype) while still showing the extends name for plain-function bases — matching Node.js behavior for this edge case.
Step-by-step proof
function Animal(name) { this.name = name; }
class Dog extends Animal {}
console.log(Bun.inspect(Dog));
// Current (after PR): [class Dog]
// Node.js: [class Dog extends Animal]
// Before this PR: [class Dog extends Function] // wrong name but shows inheritance exists
No test in the PR suite covers this pattern; all class-extends tests use class-keyword bases (class Foo, class Bar extends Foo), which pass the isClass() check correctly.
There was a problem hiding this comment.
LGTM — prior feedback addressed, fix is correct.
Extended reasoning...
Overview
Modifies src/bun.js/ConsoleObject.zig (the .Class branch of the inspect formatter) and adds a regression test test/regression/issue/29225.test.ts. The Zig change swaps getClassName/calculatedClassName (which walks the prototype chain and resolves DOM InternalFunction constructors to "Function") for getName (which reads the constructor's own .name property). A new proto_is_class guard suppresses the spurious extends Function suffix that appeared for all DOM built-ins.
Security risks
None. The change is entirely cosmetic — it affects only the textual output of Bun.inspect / console.log for class constructors. No execution semantics, auth, crypto, or data exposure are involved.
Level of scrutiny
Low. This is a targeted, single-code-path fix with narrow scope: one conditional branch in the inspect formatter, matched by a comprehensive regression test covering 17 node:stream/web constructors, 5 global DOM constructors, user-defined classes with inheritance, and an instanceof/prototype-identity check. The existing inspect test suite (test/js/bun/util/inspect.test.js, 71 cases) also exercises the affected paths.
Other factors
My two prior concerns — missing test.concurrent and stdout-before-exitCode ordering — were resolved in commit 9dcdcbd. The remaining open inline comment (the proto_is_class check silently dropping the extends clause for class X extends regularFunction {}) is a cosmetic-only nit in an uncommon edge case; the tradeoff is documented in a code comment and the main regression it fixes is significantly more impactful.
|
buildkite: retrying darwin-aarch64 — only |
There was a problem hiding this comment.
LGTM — the core fix (using .name instead of calculatedClassName) is correct, tests follow conventions, and the previous feedback has been addressed.
Extended reasoning...
Overview
This PR modifies src/bun.js/ConsoleObject.zig (the .Class branch of the inspect formatter) and adds a regression test test/regression/issue/29225.test.ts. The fix replaces getClassName()/calculatedClassName() (which walks the prototype chain and returns "Function" for DOM InternalFunction constructors) with getName() on the constructor's own .name property. A proto_is_class guard is added to suppress the noisy "extends Function" label for DOM/built-in constructors.
Security risks
None. This is a purely cosmetic change to the Bun.inspect/console.log output formatter. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low. The change is confined to the console/inspect formatter, a non-critical display path. The root cause is well-understood, the fix is targeted, and the PR description explains the mechanism clearly. The test suite covers all 17 node:stream/web constructors plus 5 WebCore globals and user-defined class hierarchies.
Other factors
All prior feedback was addressed: test.concurrent is used for all subprocess-spawning tests, stdout assertions precede exitCode assertions, and prose comments were trimmed. A minor nit about the proto_is_class check silently dropping the extends clause for plain-function bases (e.g. class Dog extends Animal {} where Animal is a regular function) is already documented in an inline comment; it is a cosmetic edge case where the old behavior was also incorrect, and it does not block the core fix.
|
Second retry also expired on the same two darwin-aarch64 test lanes. Unrelated PRs #29231 and #29228 have the identical failure ( |
1 participant
Fixes #29225
Repro
Before
After
Cause
The
.Classformatter inConsoleObject.zigcalledvalue.getClassName(), which under the hood falls through toJSObject::calculatedClassName. That helper walks the prototype chain looking for.constructor. For any WebCoreInternalFunctionconstructor (ReadableStream,WritableStream,TextDecoderStream, …), its own.constructorlookup traverses the proto chain and resolves toFunction.prototype.constructor === Function, so the class name came out as"Function".Every DOM /
node:stream/webclass was affected:Functional behavior (
instanceof, prototype identity, subclassing) was fine — the bug was cosmetic.Fix
Use the constructor's own
.nameproperty (whichJSDOMBuiltinConstructor::initializePropertiessets to the real class name) instead ofcalculatedClassName. This is the same approach the.Functionformatter already uses.Also only report
extendswhen the prototype is itself a class — otherwise built-ins with aFunction.prototypeparent would render as[class X extends Function].Verification
test/regression/issue/29225.test.tscovers:node:stream/webclass constructors inspect as[class X]URL,Request,Response,Blob,Event) inspect as[class X]class Foo,class Bar extends Foo, and variable-bound anonymous class expressions still render correctlyinstanceofand prototype identity still work forReadableStreamBYOBReadertest/js/bun/util/inspect.test.js(71 pre-existing cases incl.[class A],[class B extends A],[class (anonymous) extends A],[class (anonymous)]) still passes.