Print every native function's source on one line - #545
Conversation
#155 changed the InternalFunction and callable-object branches of Function.prototype.toString to the V8 shape: function Map() { [native code] } Host functions (NativeExecutable::toStringSlow), builtin functions written in JavaScript (FunctionExecutable::toStringSlow), bound functions and remote functions (JSFunction::toString) still printed the old three-line shape: function hasOwnProperty() { [native code] } lodash's isNative() builds a RegExp from the source of Object.prototype.hasOwnProperty and tests Map, Set, WeakMap, DataView and Promise against it. With two shapes in one engine, the RegExp did not match, lodash decided Map was not native, and MapCache and Stack fell back to ListCache, a linear scan. cloneDeep, memoize, uniq and isEqual became quadratic under Bun. json-schema-to-typescript measured cloneDeep of a 10 000 definition schema at 11 s under Bun and 76 ms under node (bcherny/json-schema-to-typescript#871). This changes the remaining four sites to the one-line shape, so all native functions print the same way, and updates the inspector's native-getter sniff and the testapi.mm expectation to match. JSTests/stress/function-toString-native-one-line.js pins the shape for each kind of function, re-runs lodash's RegExp, and walks the global object to check that no native function prints on more than one line.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
WalkthroughThe runtime now stringifies native, bound, and remote functions on one line. API and inspector checks use the new format. A stress test validates native function categories, caching, compatibility, and global traversal. ChangesNative function stringification
Suggested reviewers: Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| auto throwScope = DECLARE_THROW_SCOPE(vm); | ||
|
|
||
| JSValue value = jsMakeNontrivialString(globalObject, "function "_s, name(), "() {\n [native code]\n}"_s); | ||
| JSValue value = jsMakeNontrivialString(globalObject, "function "_s, name(), "() { [native code] }"_s); |
There was a problem hiding this comment.
🔴 Switching host-function toString to one line breaks several enabled ChakraCore baseline tests that pass on the base branch (they print only NativeExecutable-backed functions, so #155 did not touch them): Function/toString.js, strict/05.arguments_sm.js, Array/protoLookup.js, es6/letconst_global_shadow_builtins.js. run-javascriptcore-tests will report new failures after merging. Fix: rebaseline every .baseline-jsc still containing the multi-line shape — sweep:\{\n \[native code\]\n\} under JSTests/ChakraCore — replacing each three-line block with { [native code] }.
Extended reasoning...
On the base commit d71031a, parseFloat, String.prototype.indexOf, Array.prototype.shift, JSON.stringify and the strict-mode arguments.callee throw-type-error accessor are all JSFunctions backed by a NativeExecutable, so their toString still goes through NativeExecutable::toStringSlow and prints the three-line form. Their ChakraCore baselines (toString.baseline-jsc lines 3-6/34-41, 05.arguments_sm.baseline-jsc lines 4-9, protoLookup.baseline-jsc lines 41-43, letconst_global_shadow_builtins.baseline-jsc lines 14-16) contain only these host functions and no InternalFunction constructors, so PR #155 left them green. After this diff NativeExecutable::toStringSlow emits function name() { [native code] }, the printed output no longer matches the checked-in baselines, and runChakra :baseline in ChakraCore.yaml (lines 138, 608, 1585, 2151) reports each as a new failure. Baselines that also print an InternalFunction (e.g. Function/prototype.baseline-jsc, typedarray/set.baseline-jsc, ErrorCtorProps_v3.baseline-jsc) were already failing before this PR and…
Verification: normal — The change at Source/JavaScriptCore/runtime/NativeExecutable.cpp:111 switches host-function toString from "() {\n [native code]\n}" to "() { [native code] }", but the enabled ChakraCore baseline tests were not rebaselined and still expect the multi-line form. Base branch (d71031a) still emits multi-line for NativeExecutable (git show d71031a973:...NativeExecutable.cpp…
|
|
||
| if (nativeGettersAsValues) { | ||
| if (@String(descriptor.get).@endsWith("[native code]\n}") || (!descriptor.get && @Object.@hasOwn(descriptor, "get") && !descriptor.set && @Object.@hasOwn(descriptor, "set"))) { | ||
| if (@String(descriptor.get).@endsWith("[native code] }") || (!descriptor.get && @Object.@hasOwn(descriptor, "get") && !descriptor.set && @Object.@hasOwn(descriptor, "set"))) { |
There was a problem hiding this comment.
🟡 (optional) The injected-script native sniff was updated to "[native code] }" but the frontend counterpart isFunctionStringNativeCode in Source/WebInspectorUI/UserInterface/Base/Utilities.js:1690 still checks endsWith("{\n [native code]\n}"); after this change no native function's toString ends that way, so the Web Inspector object tree stops recognizing any native function (previously host/builtin/bound functions still matched). Fix: update isFunctionStringNativeCode to test the single-line suffix (e.g. endsWith("{ [native code] }")) so both inspector halves agree. Pre-existing for InternalFunction only; this diff extends the breakage to every native function.
Extended reasoning...
Before this diff, Object.prototype.hasOwnProperty.toString() and other NativeExecutable/FunctionExecutable builtins produced function name() {\n [native code]\n}, so isFunctionStringNativeCode (WebInspectorUI/UserInterface/Base/Utilities.js:1688-1691) returned true for them and ObjectTreePropertyTreeElement.js:299/312 and ObjectTreeBaseTreeElement.js:222 rendered them as native (simplified function, no jump-to-source context menu). After this diff every native path emits function name() { [native code] }, which never ends with {\n [native code]\n}, so isFunctionStringNativeCode returns false for all functions and the inspector frontend treats every native function as user code. The PR updated the backend sniff in InjectedScriptSource.js but not the matching frontend helper. Note: the JSCOnly port Bun builds does not ship WebInspectorUI, so this only affects consumers of this fork that build the full inspector UI.
Verification: nit — The mismatch is real. Source/WebInspectorUI/UserInterface/Base/Utilities.js:1688-1691 still reads return str.endsWith("{\n [native code]\n}");, and its callers at Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:299,312 and Source/WebInspectorUI/UserInterface/Views/ObjectTreeBaseTreeElement.js:222 gate the native-parameter-string display and the…
Preview Builds
|
oven-sh/WebKit#545 completes oven-sh/WebKit#155. NativeExecutable, builtin functions written in JavaScript, bound functions and remote functions now print `function name() { [native code] }`, the same one-line shape as InternalFunction constructors and V8. lodash's isNative() builds a RegExp from hasOwnProperty's source and tests Map against it. With one shape the RegExp matches, so cloneDeep, memoize, uniq and isEqual use a native Map instead of a linear-scan ListCache under Bun. cloneDeep of 8 000 objects: 1 971 ms before, linear after. The pin moves from the oven-sh/WebKit#541 preview to the fork's main at 01de4c1d, which also carries oven-sh/WebKit#529, #542 and #543. #543 moved the dynamic-import TLA deadlock check into the engine and removed JSModuleLoader::asyncEvaluationOrderForKey and the referrerAsyncOrder parameter of JSC::importModule, so moduleLoaderImportModule no longer computes or passes it. Tests: test/js/bun/jsc/function-prototype-tostring.test.ts and the oven-sh/WebKit#545 stress file as a jsc-stress fixture. Both fail on Bun 1.4.1.
This reverts the move to fork main 01de4c1d (2918d3c). That commit carries oven-sh/WebKit#543, which replaces the referrer-based skip of the top-level-await wait in dynamic imports with a walk over the import promise's reactions. The Nitro fixture in test/js/bun/http/bun-server.test.ts does import("./chunks/stream.mjs") from a Bun.serve handler while index.mjs, which the chunk imports back, is awaiting the response. The walk cannot see through the HTTP hop, the wait is taken, and the request idles out. Bun built at 742c886cdc (the #543 merge) fails the test on every platform; at 167a4cef86 it passes. 167a4cef86 is upstream c119008088 merged into the fork's main and nothing after it. The oven-sh/WebKit#545 tests leave with the pin; they need that engine change.
2 participants
Problem
Function.prototype.toStringprints native functions in two shapes. After Fix native Function.prototype.toString() format to match V8/Node.js #155,InternalFunctionsubclasses and callable objects printfunction Map() { [native code] }. Host functions (NativeExecutable::toStringSlow,runtime/NativeExecutable.cpp:111), builtin functions written in JavaScript (FunctionExecutable::toStringSlow,runtime/FunctionExecutable.cpp:192), bound functions and remote functions (JSFunction::toString,runtime/JSFunction.cpp:251and:257) still printfunction hasOwnProperty() {\n [native code]\n}.isNative()builds a RegExp from the source ofObject.prototype.hasOwnPropertyand testsMap,Set,WeakMap,DataViewandPromiseagainst it. The RegExp does not match, so lodash decidesMapis not native andMapCacheandStackfall back toListCache, a linear scan.cloneDeep,memoize,uniqandisEqualbecome quadratic under Bun. json-schema-to-typescript measuredcloneDeepof a 10 000 definition schema at 11 s under Bun and 76 ms under node (Copy compile()'s input with a plain-data clone instead of lodash cloneDeep (no output change; large schemas under bun 17× faster) bcherny/json-schema-to-typescript#871). Under Bun 1.4.1,_.cloneDeepof 8 000 small objects takes 1 971 ms, node 52 ms.Fix
function name() { [native code] }. Every native function now prints the same way, and the same way V8 does. Names do not change (accessors keepget size, bound functions keep the target name).inspector/InjectedScriptSource.js(nativeGettersAsValues) fromendsWith("[native code]\n}")toendsWith("[native code] }"), and thetestapi.mmexpectation forString(TestObject), which Fix native Function.prototype.toString() format to match V8/Node.js #155 already moved to one line.JSTests/stress/function-toString-native-one-line.jspins the shape for host functions, InternalFunction constructors, JSFunction constructors (Promise,Number), builtin functions, accessors, bound functions and callable proxies, re-runs lodash's RegExp against them, and walks the global object to check that no native function prints on more than one line. It fails on Bun 1.4.1 at the first host function.Background
Function.prototype.toStringon a built-in function must return a string with theNativeFunctionsyntax:function name() { [native code] }with any whitespace. JSC historically used three lines, V8 one line. Fix native Function.prototype.toString() format to match V8/Node.js #155 (Function.constructor.toString() is not compliant with V8 runtime bun#26698) moved JSC to the V8 shape but only infunctionProtoFuncToString, which handlesInternalFunctionand callable non-function objects. AJSFunctiondispatches to its executable instead, and those paths were not touched.NativeExecutablebacks aJSFunctionwhose body is a C++ function (JSFunction::create(vm, globalObject, length, name, nativeFunction)).FunctionExecutablebacks functions parsed from JavaScript. Builtins such asArray.prototype.mapareFunctionExecutables withisBuiltinFunction()set and print[native code]instead of their source.m_asString), so the change costs nothing at runtime.