-
Notifications
You must be signed in to change notification settings - Fork 4.3k
inspect: show real class name for DOM constructors, not [class Function]
#29229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+156
−7
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
78a4e7d
inspect: show real class name for DOM constructors, not [class Function]
robobun 8c9469c
[autofix.ci] apply automated fixes
autofix-ci[bot] 9dcdcbd
test #29225: concurrent, stdout-before-exitCode, trim prose
robobun 4f694c2
ci: retrigger darwin-aarch64 test (infra flake, expired)
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| // https://github.com/oven-sh/bun/issues/29225 | ||
|
|
||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
| import { ReadableStreamBYOBReader } from "node:stream/web"; | ||
|
|
||
| const streamWebClasses = [ | ||
| "ByteLengthQueuingStrategy", | ||
| "CompressionStream", | ||
| "CountQueuingStrategy", | ||
| "DecompressionStream", | ||
| "ReadableByteStreamController", | ||
| "ReadableStream", | ||
| "ReadableStreamBYOBReader", | ||
| "ReadableStreamBYOBRequest", | ||
| "ReadableStreamDefaultController", | ||
| "ReadableStreamDefaultReader", | ||
| "TextDecoderStream", | ||
| "TextEncoderStream", | ||
| "TransformStream", | ||
| "TransformStreamDefaultController", | ||
| "WritableStream", | ||
| "WritableStreamDefaultController", | ||
| "WritableStreamDefaultWriter", | ||
| ]; | ||
|
|
||
| test.concurrent("node:stream/web classes inspect as [class X], not [class Function]", async () => { | ||
| const source = ` | ||
| const sw = require("node:stream/web"); | ||
| const names = ${JSON.stringify(streamWebClasses)}; | ||
| for (const name of names) { | ||
| const klass = sw[name]; | ||
| if (typeof klass !== "function") { | ||
| console.log(name + ": MISSING"); | ||
| continue; | ||
| } | ||
| // Bun.inspect() uses the same formatter as console.log. | ||
| console.log(name + ": " + Bun.inspect(klass)); | ||
| } | ||
| `; | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", source], | ||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| const lines = stdout.trim().split("\n"); | ||
| expect(lines.length).toBe(streamWebClasses.length); | ||
|
|
||
| for (let i = 0; i < streamWebClasses.length; i++) { | ||
| const name = streamWebClasses[i]; | ||
| // Must not be "MISSING" (sanity check for the test itself) and | ||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // must report the real class name, not "Function". | ||
| expect(lines[i]).not.toContain("MISSING"); | ||
| expect(lines[i]).toBe(`${name}: [class ${name}]`); | ||
| } | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test.concurrent("other DOM / WebCore constructors inspect as [class X]", async () => { | ||
| // Sanity: the inspect formatter should work for any `isConstructor` | ||
| // InternalFunction exposed as a global. Keep this list small — it's | ||
| // a regression guard, not an audit. | ||
| const code = ` | ||
| console.log("URL: " + Bun.inspect(URL)); | ||
| console.log("Request: " + Bun.inspect(Request)); | ||
| console.log("Response: " + Bun.inspect(Response)); | ||
| console.log("Blob: " + Bun.inspect(Blob)); | ||
| console.log("Event: " + Bun.inspect(Event)); | ||
| `; | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", code], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe( | ||
| "URL: [class URL]\n" + | ||
| "Request: [class Request]\n" + | ||
| "Response: [class Response]\n" + | ||
| "Blob: [class Blob]\n" + | ||
| "Event: [class Event]\n", | ||
| ); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test.concurrent("user-defined classes and extends still render correctly", async () => { | ||
| const code = ` | ||
| class Foo {} | ||
| class Bar extends Foo {} | ||
| const Anon = class {}; | ||
|
|
||
| console.log("Foo: " + Bun.inspect(Foo)); | ||
| console.log("Bar: " + Bun.inspect(Bar)); | ||
| console.log("Anon: " + Bun.inspect(Anon)); | ||
| `; | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", code], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| // `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); | ||
| }); | ||
|
|
||
| test.concurrent("instanceof and prototype identity still work", async () => { | ||
| // Functional behavior must not regress — the fix is cosmetic only. | ||
| const stream = new ReadableStream({ | ||
| type: "bytes", | ||
| start(c) { | ||
| c.enqueue(new Uint8Array([1, 2, 3])); | ||
| c.close(); | ||
| }, | ||
| }); | ||
| const reader = stream.getReader({ mode: "byob" }); | ||
| expect(reader).toBeInstanceOf(ReadableStreamBYOBReader); | ||
| expect(Object.getPrototypeOf(reader)).toBe(ReadableStreamBYOBReader.prototype); | ||
| reader.releaseLock(); | ||
|
|
||
| class Sub extends ReadableStreamBYOBReader {} | ||
| expect(Object.getPrototypeOf(Sub.prototype)).toBe(ReadableStreamBYOBReader.prototype); | ||
| }); | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The new
proto_is_classcheck 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 checkproto === Function.prototypeinstead ofproto.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:
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
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
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.