Repository navigation
Conversation
|
Updated 6:05 PM PT - Oct 21st, 2026
❌ @robobun, your commit ac2ecc5 has some failures in 🧪 To try this PR locally: bunx bun-pr 45735That installs a local version of the PR into your bun-45735 --bun |
|
Status: ready for review (#45735). The diff is green on every CI lane that ran. How I reproduced it. On main and on 1.4.3, with Node.js v26.3.0 as the reference: import util from "node:util";
const o = { *gen() {}, asyncStorage() {} };
console.log(util.inspect(o.gen)); // bun: [Function: gen] GeneratorFunction node: [GeneratorFunction: gen]
console.log(util.inspect(o.asyncStorage)); // bun: [AsyncFunction: asyncStorage] Function node: [Function: asyncStorage]
console.log(util.inspect(Object.setPrototypeOf(function f() {}, { __proto__: null })));
// bun: Error: Invalid object node: [Function: f] Function <[Object: null prototype] {}>The new tests fail on main (50 in CI on ac2ecc5 (build 130293): 179 jobs passed. The two |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughNative async and generator function checks now support function labels in ChangesFunction Kind Classification and Inspection
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the C++ side of the change: jsFunctionIsAsyncFunction keeps the isHostFunction() guard ahead of jsExecutable(), and dropping the getPrototype walk removes the only call that could re-enter JS, so no exception scope is needed there; jsFunctionIsGeneratorFunction is unchanged apart from the new header declaration. The behavior change to util.types.isAsyncFunction for re-prototyped and promisify-wrapped functions was also looked at and ruled out as a bug — the new values match the Node.js expectations recorded in test/js/node/util/test-util-types.test.js.
Extended reasoning...
The change replaces the toString-parsing polyfills in src/js/internal/util/inspect.js with $newCppFunction bindings, switches jsFunctionIsAsyncFunction in src/jsc/modules/NodeUtilTypesModule.cpp from a prototype-chain check to the executable's parse mode, and lets internalGetConstructorName accept functions; it touches no auth, crypto, or input-parsing surface. Two confirmed findings about the new constructor-less-chain path are posted inline, so a human look is already signaled; this note only records what else was examined.
|
Related: oven-sh/WebKit#834 (a draft, meant for WebKit/WebKit first) makes With the prototype test that this PR removes, |
fb24e84 to
14dc79a
Compare
There was a problem hiding this comment.
I re-reviewed the two new commits and found no bugs; the constructor-less prototype branch now reads only own data properties through captured primordials, which covers what I raised earlier. Because this changes the observable answer of the public util.types.isAsyncFunction (e.g. util.promisify(asyncFn) now reports false) and the labels of a few builtins, a human look at that semantic shift would still be worthwhile.
What was reviewed:
jsFunctionIsAsyncFunction: host-function guard still precedesjsExecutable();isAsyncFunctionParseModeincludes async arrows, methods and async generators; bound functions and Proxies fall tofalse, matching the new Node-derived test rows.internalGetConstructorName(val, true): the chain walk usesObjectGetOwnPropertyDescriptor/ObjectGetPrototypeOf/SymbolToStringTagcaptured at load, no.constructororSymbol.matchlookups run; the subprocess test replacestoString,Symbol.matchandutil.typesbefore loadingnode:utiland checks zero calls.FunctionPrototypeToStringandStringPrototypeMatchkeep other callers ininspect.js, so no dead primordials remain; the removedAsyncFunctionPrototype.hinclude had no other use.
Extended reasoning...
The diff touches src/js/internal/util/inspect.js (function-kind labelling and a new own-data-property name walk for functions inside a prototype chain), src/jsc/modules/NodeUtilTypesModule.cpp/.h (isAsyncFunction now reads the FunctionExecutable parse mode instead of the prototype chain, plus two forward declarations for $newCppFunction), and three test files with Node v26.3.0-cited expectations. No security-sensitive surface is involved beyond tamper-resistance of builtin module code, which the new code honours. The two new commits address both inline comments from the prior run, and the hunt ran dry with no findings. Deferring rather than approving because the PR intentionally changes the result of a public API (util.types.isAsyncFunction) for 13 probed function shapes and leaves part of V8's GetConstructorHelper deliberately unported, which is a product judgement a maintainer should confirm.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
There was a problem hiding this comment.
I reviewed the three new commits and did not find any bugs; since this changes the answer of the public util.types.isAsyncFunction and ports only part of V8's GetConstructorHelper (the PR itself notes the direct-vs-prototype split is an approximation), a human look at that compat trade-off would still be worthwhile.
What was reviewed:
- The prototype-walk in
internalGetConstructorNamenow reads only own data descriptors and skipsconstructoron the receiver itself, which matches V8's GetConstructorHelper (it has the same "B.prototype.constructor = B" exclusion), so the earlier concern about user getters orSymbol.matchsteering a label no longer applies to the new code. isAsyncFunctionParseModeis the same predicateBunCPUProfiler.cppuses for the "async " display prefix and covers async arrows, methods and async generator wrapper modes; the host-function guard still precedesjsExecutable()in both native checks, andgetFunctionBasepicks up the same$newCppFunctionbindings.- The ruled-out candidate (an own
constructoron a prototype function printing differently from Node) is consistent with V8, not a deviation. - Tests spawn with
bunEnv, drain stdout/stderr concurrently, and useawait using.
Extended reasoning...
The diff rewires util.inspect's function-kind detection from source-text heuristics to two native host functions bound with $newCppFunction, changes jsFunctionIsAsyncFunction in NodeUtilTypesModule.cpp to answer from the executable's parse mode instead of the prototype chain, and adds an isPrototype branch to internalGetConstructorName that walks own data properties; it touches no auth, crypto, or input-parsing surface. Both prior inline findings were addressed by commits 11a73ee and 14dc79a (own-descriptor walk, isPrototype flag), and the threads were resolved by this bot's session rather than a person. What keeps this from an approve is that it is a user-visible semantic change to a public node:util/types API plus a deliberately partial port of V8 behavior (the PR description states 467 of 817 probed constructor-less shapes still print a different word than Node), which is a compat judgment call rather than a mechanical change; no CODEOWNERS entry covers these files and no third-party CHANGES_REQUESTED review is outstanding.
|
A note on landing order. #46111 replaces the JS If #46111 lands first, this PR can drop its hunk in I ran the 20 rows that this PR pins for a function on a chain with no constructor on the #46111 branch. All 20 print the same name after the label. Two still print the wrong label there ( |
f212527 to
e19dd58
Compare
util.inspect decided the kind in a function's label from the first characters of its source text. A generator method printed `[Function: gen] GeneratorFunction`, and a method named `asyncStorage` printed `[AsyncFunction: asyncStorage] Function`. - util.types.isAsyncFunction reads the parse mode of the function's executable. Before, it read the current prototype of the function. - inspect.js binds the native isAsyncFunction and isGeneratorFunction with $newCppFunction and no longer tests the source text. - internalGetConstructorName accepts a function. Before, it threw `Error: Invalid object`, so util.inspect threw for a function whose prototype chain has no constructor.
…rties A function that is the prototype of the inspected value fell through to the lines that name an object. Those lines read `constructor` with a normal get and match a regular expression through RegExp.prototype[Symbol.match], so a getter or a replaced method decided the name. V8 reads own data properties only: a string Symbol.toStringTag, then a named constructor below the function, then "Function".
e19dd58 to
ac2ecc5
Compare
Problem
util.inspectreads the kind of a function from its source text. A generator method prints[Function: gen] GeneratorFunction, Node.js prints[GeneratorFunction: gen]. A method namedasyncStorageprints[AsyncFunction: asyncStorage] Function.src/js/internal/util/inspect.js:147-150, a 2023 workaround. The nativeutil.types.isAsyncFunction(src/jsc/modules/NodeUtilTypesModule.cpp:954) still reads the current prototype of the function.util.inspectthrowsError: Invalid objectfor a function whose chain has noconstructor.Fix
util.types.isAsyncFunctionreads the parse mode of the function, asisGeneratorFunctiondoes.inspect.jsbinds both with$newCppFunction. The text tests are gone.internalGetConstructorNameaccepts a function: no throw.test/js/node/util/node-inspect-tests/internal-inspect.test.js(50 fail on main) andtest/js/node/util/test-util-types.test.js(11 fail on main), expected values from Node.js v26.3.0. Alsotest/js/node/util/,test/js/bun/util/inspect.test.js.Background
inspect.jsafter user code, so it binds private copies (as util.inspect: don't take the iterator guards from the writablenode:util/types#45291).inspect.jsonly: it adds a host function and leavesutil.types.isAsyncFunctionwrong.Downsides
util.types.isAsyncFunctionchanges for 13 of 76 probed forms, always to Node's answer (util.promisify(asyncFn)givesfalse). 12 of 4,638 builtin labels change.util.inspect(fn)runs 5,705 instructions per call, before 7,296.Notes
Origin. A fuzzer that compares the label of
util.inspect(fn)withutil.typesfound the wrong labels. There is no issue and no user report. The change toutil.types.isAsyncFunctionand the fix of the throw are not part of that report. They are here because Node.js takes the label from these two checks, and because the second place whereinspect.jsnames the kind of a function needs them.Repro (Node.js v26.3.0 prints the second column)
The same labels come out of
util.format("%O"), and for a function inside an object or an array.Why
util.types.isAsyncFunctionchanges too. Node.js uses one pair of checks for the label and forutil.types. With the old native check the ported Node.js testtest/js/node/util/node-inspect-tests/parallel/util-inspect.test.jsfails: it inspects a generator function whose prototype isAsyncFunction.prototypeand expects[GeneratorFunction (anonymous)] AsyncFunction. #34872 fixed otherutil.typeschecks in place in the same way. No builtin module readsutil.types.isAsyncFunction.Why not a better regular expression. Text cannot see builtins, whose source is hidden, and a script can replace
Function.prototype.toStringandRegExp.prototype[Symbol.match].The two private bindings. The two text tests were private to
inspect.js. A read from the writablenode:util/typeswould let a script change the kind in a label for the first time. The other 20 checks thatinspect.jsreads fromnode:util/typesstay as they are, as decided in #45291 (comment).The 2023 note. The note deleted from the ported test came with #4493 (e60b360). It says that the test relies on the text tests and that the native checks were flaky. Two defects were live then.
isAsyncFunctionanswered from the prototype, which this change removes. Both checks calledjsExecutable()on host functions, which c49547c (#20972) guards. That guard stays first in both checks. Every assertion of the ported test stays.Deleted as dead. The two text tests, the comment with the two commented-out names, the note in the ported test, and the
AsyncFunctionPrototype.hinclude, which had one use.Measurements. Release builds of the merge base ea6a2c7 and of this branch (e19dd58), same WebKit, linux x64.
jsFunctionIsAsyncFunction193 to 86 B,jsDollarLazy3,959 to 3,998 B,JS2NATIVE_COUNT262 to 264, bundledinspect.js75,054 to 75,560 B. No new host function. The growth is the walk that names a function that is a prototype.util.types.isAsyncFunction(async function f() {}): 47 to 24 executed instructions per call (gdbstepi), 55 to 26 static instructions, indirect calls 1 to 0.util.inspect(function f() {}), JIT off, N=300 (bun:jscheapStats): 28.25 to 19.25. RegExp 1 to 0, Array 10.01 to 6.01, Arguments 7 to 3.util.inspect({ a: 1, b: "x" }): 36.26 to 36.26.util.inspect(class K {}): 29.25 to 29.25.Function.prototype.toString6 to 2,RegExp.prototype[Symbol.match]2 to 0,RegExp.prototype.exec2 to 0, a replacedutil.types.isAsyncFunction0 to 0. Node.js: 0, 0, 0, 0.require("node:util").inspectin a realm: 1,021 to 1,020 cells (NativeExecutable 64 to 66, FunctionExecutable 90 to 88).require("node:util")alone still does not loadinspect.js.node:util/typeshas 44 keys.util.inspect(fn)over 8 forms whose label was right before, 2e6 iterations, JIT on: 7,296 to 5,705 instructions per call (perf_event_openinstructions:u, 10 interleaved runs, spread 0.38% and 0.37%). Wall clock, 10 interleaved runs: median 1,760 to 1,168 ns per call, noise floor 790 ns and 1,135 ns, so the wall clock does not resolve the difference.perf,valgrindandbloatyare not installed here.The 13 forms where
util.types.isAsyncFunctionchanges (of 76 probed forms).isGeneratorFunctiondoes not change.falsetotrue(7): an async function withFunction.prototype, with a null prototype, withObject.create(AsyncFunction.prototype), and with a prototype that has no constructor. An async arrow withFunction.prototype. An async method with a prototype that has no constructor. An instance of a subclass ofAsyncFunction.truetofalse(6): a generator function, a plain function, a class and a method withAsyncFunction.prototype.Reflect.construct(Function, [body], AsyncFunction).util.promisify(async function).The 12 labels of builtin functions that change. The walk covers every function reachable from
globalThisand the builtin modules. It does not cover functions that exist only on an instance.Bun.inspectlabels changed: 0.Bun.$.ShellPromisemethods:[Function: text] AsyncFunctionto[AsyncFunction: text].process.allowedNodeEnvironmentFlags:[Function: values] GeneratorFunctionto[GeneratorFunction: values]. Node.js prints[Function: values], its methods are not generators.fs.Dir.prototype[Symbol.asyncIterator]:[AsyncFunction: [Symbol.asyncIterator]] AsyncGeneratorFunctionto[AsyncGeneratorFunction: [Symbol.asyncIterator]].AsyncResource.prototype.asyncId:[AsyncFunction: asyncId] Functionto[Function: asyncId].A function on a chain with no constructor. Node.js asks V8 for the name (
GetConstructorHelper, linked in the code). V8 gives an ordinary function the name of the intrinsic constructor of its kind. For a function that is or was a prototype, a class, a bound function, and a function made by a subclass, V8 takes a name from own data properties of the chain: a stringSymbol.toStringTag, then a namedconstructorbelow the function, thenFunction.inspect.jsknows one of these states: the function is the prototype of the value that it prints. Then it does the same walk, with own data properties only, so no getter runs and no replaced method decides the name. A matrix of 817 shapes (19 kinds of function on 4 chains, never a prototype or made one in 6 ways, through the 3 paths into the code):Error: Invalid objecton 817. This branch throws on 0.Reflect.constructwith anothernew.target. No occurrence of these shapes is known outside tests.Outside this matrix, one more prototype shape differs. A function with a null prototype and an own property, used as a prototype, is printed by a nested
inspectcall that does not know that the value is a prototype.util.inspect(Object.create(Object.assign(Object.setPrototypeOf(async function h() {}, null), { a: 1 })))printsObject <[AsyncFunction: null prototype]> {}, and Node.js printsObject <[Function: null prototype]> {}.Not in this change
console.logandBun.inspect(src/jsc/ConsoleObject.rs) label a function by the name of its current prototype. They print the reported methods correctly. It is Bun's own format.getFunctionBasedecides[class X]from the source text, with the code of Node.js. The text in Bun is the transpiler's print, soclass X extends (Base) {}prints[class X extends Base]and Node.js prints[Function: X]. A replacedFunction.prototype.toStringcan still change this word.timersPromises.setInterval(node:timers/promises: implement setInterval as async generator (lazy arm + iterator protocol) #34619) andfsPromises.watch(fs.promises.watch: implement as a real async generator #34718).Map <...>,[Foo: null prototype]), theTODO: null prototypeslines of the ported test. util.inspect: recover constructor name for null-prototype class instances #34755 was a try. These lines also readconstructorwith a normal get, so a getter on such a chain runs and its error passes on. They are the lines of main and are not changed here.src/jsc/bindings/ErrorCode.cpp) prints[Function: name]for every kind. node errors: render functions in ERR_INVALID_ARG_VALUE messages like util.inspect #38637 is open for it.util.inspectdoes not print theProxy(...)wrapper of Node.js v26. util.inspect: print proxies as Proxy(...) and unwrap nested proxies like node v26 #38652 is open for it.Suites run with the debug build: the three touched test files (also with
BUN_JSC_validateExceptionChecks=1),test/js/node/util/,test/js/bun/util/inspect.test.js,test/bundler/bundler_browser.test.ts,test/js/web/console/console-log.test.ts,test/js/node/assert/assert.test.cjs, and the upstreamtest/js/node/test/parallel/test-util-types.js,test-process-binding-util.js,test-util-inspect-proxy.js,test-util-inspect-namespace.js,test-util-inspect-getters-accessing-this.js,test-util-inspect-regexp.js,test-util-types-exists.js.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js