Repository navigation
Conversation
…k traces and inspect JSC binds an anonymous `export default` to the private `*default*` identifier, whose string form is "starDefault". JSFunction::reifyName maps it to "default" when the `name` property is materialized, but the fallbacks Bun uses when that has not happened yet (ecmaName() in ErrorStackTrace.cpp, JSC::getCalculatedDisplayName, and the executable name in JSC__JSValue__getNameProperty) returned the raw identifier, so a frame rendered as `at new starDefault (...)` until something read `.name`, and Bun.inspect printed `[class starDefault]`. Route every one of those fallbacks through one helper that renders the `*default*` identifier as "default", and share the displayName/name chain between the regular and the finalizer-safe frame name lookups instead of keeping two copies of it.
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status Reproduced on bun 1.4.0 (linux x64) with a two-file ESM fixture: Fix is in this PR (#38517): the five places that take a function's name from its executable ( Not covered here: |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it refactors the C++ JSC name-lookup paths — including the FinalizerSafety::MustNotTriggerGC branch — and intentionally shifts Bun.inspect output for the empty-displayName and empty-name builtin cases, a human look at the GC-safety and behavioral-unification claims would still be worthwhile.
What was reviewed:
calculatedDisplayNameuses onlygetConcurrently/getDirect/tryGetValueWithoutGC/nameWithoutGCand identifier reads — no JS-heap allocation or getter execution, matching the finalizer-safe contract of the inline code it replaces.- The hoisted
FunctionNameFlags::Functionwrite in theMustNotTriggerGCbranch fires on the same set of callee types as the old per-return lambda. functionNameForDisplaycompares againststarDefaultPrivateName(the private identifier), so a user function literally namedstarDefaultis unaffected — covered by thenamed-star-default.mjscontrol case.- Tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, exact-value assertions on structured output).
Extended reasoning...
Overview
The PR fixes anonymous export default functions and classes rendering as starDefault in stack frames and Bun.inspect before their .name property is reified. It adds two helpers in ErrorStackTrace.cpp — functionNameForDisplay(vm, identifier) (maps the private *default* identifier to "default") and calculatedDisplayName(vm, object) (unified own-displayName → nameWithoutGC → mapped ecmaName chain) — and routes four previously-separate lookups through them: functionName(vm, codeBlock), functionName(vm, globalObject, object), the MustNotTriggerGC branch of the frame-based functionName, plus JSC__JSValue__getName and the jsExecutable()->name() fallback in JSC__JSValue__getNameProperty in bindings.cpp. Tests cover error.stack, CallSite#getFunctionName, uncaught/unhandled-rejection frame printing, and Bun.inspect for class/subclass/arrow/JSX shapes.
Security risks
None identified. This is display-name computation for diagnostics; no untrusted input parsing, auth, or network paths are touched.
Level of scrutiny
High. Per REVIEW.md, native JSC bindings are the most-blocked category, and this change specifically rewrites the FinalizerSafety::MustNotTriggerGC path — code that runs during error finalization where running getters or allocating on the JS heap is UB. The new calculatedDisplayName is asserted to preserve those guarantees (structure-slot reads only via getConcurrently + getDirect + tryGetValueWithoutGC, nameWithoutGC, and Identifier::string()), and traces cleanly to me, but a maintainer with JSC internals context should confirm. The change also replaces JSC::getCalculatedDisplayName (which had a TOP_EXCEPTION_SCOPE wrapper because it could enter JS) with the GC-free helper in the non-finalizer functionName path and in JSC__JSValue__getName — a semantic shift the PR description acknowledges (empty-string displayName now falls through; empty-name builtins fall through to inferred name), described as moving toward node's output.
Other factors
The PR description is unusually thorough: it names the mechanism (JSFunction::reifyName), enumerates every affected call site, documents what is intentionally not fixed (JSObject::calculatedClassName inside WebKit for new D()), and lists both new tests and the existing suites re-run under debug ASAN. The tests include a control case for a real function named starDefault, before/after .name reification, and the finalizer-safe path via uncaught-throw / unhandled-rejection subprocesses. The refactor deduplicates three hand-written copies of the name-lookup chain, which is exactly what REVIEW.md's "fix the whole class" guidance asks for — but that same unification is why this exceeds the "simple, mechanical, or obvious" bar for auto-approval.
|
Two notes for whoever picks up the points the review above asks a human to look at:
|
…reimplementing getCalculatedDisplayName The previous revision replaced JSC::getCalculatedDisplayName with a local copy of the displayName/name chain that used Bun's stack-trace rule for builtin functions. JSC__JSValue__getName therefore started printing the inferred ecmaName of functions defined in Bun's built-in modules, which are JSC builtins: Bun.inspect(zlib.gzip) became "[Function: fn]" and fs.promises wrappers became "[AsyncFunction: wrapped]". Keep every existing lookup as it was and pass its result through functionNameForDisplay(), which swaps the private *default* identifier's string (compared by StringImpl identity) for "default". Add a test pinning the inspect output of builtin-module functions.
|
Pushed a reduced revision (1dda628, cfd9586), description updated to match. Self-review of the first revision found that replacing |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The native change is minimal — one identity-compare post-filter applied at five existing return sites — but it sits in the JSC bindings for error stack formatting and inspect, including the MustNotTriggerGC path, so a human look would still be worthwhile.
What was reviewed:
functionNameForDisplayin theMustNotTriggerGCbranch:defaultKeyword.string()is a VM-lifetimeCommonIdentifiersstring and the check is a pointer compare, so no JS-heap allocation is introduced there.- Identity vs equality:
starDefaultPrivateNameis a uniqueSymbolImpl, so a user function literally namedstarDefaulthas a differentimpl()and is left alone (covered by thenamed-star-default.mjscontrol). - The
JSC__JSValue__getNamePropertyfallback wrap only fires afterfunction->name(vm)returned empty for a non-host/non-builtin function, so builtin-module functions are unaffected (pinned by the newgzip/readdirtest).
Extended reasoning...
Overview
The PR fixes anonymous export default functions/classes rendering as starDefault in stack traces, CallSite#getFunctionName(), Bun.inspect, and JSX element names. It adds Zig::functionNameForDisplay(vm, name) in ErrorStackTrace.cpp/.h — an 8-line helper that returns "default" when name.impl() is pointer-identical to vm.propertyNames->starDefaultPrivateName.impl(), otherwise returns the input unchanged — and wraps five existing name-lookup return values through it (three in ErrorStackTrace.cpp, two in bindings.cpp). ~160 lines of tests are added to stack.test.ts and inspect.test.js.
Security risks
None. This is display-name formatting only; no parsing of untrusted input, no allocation-size arithmetic, no auth/crypto/permissions.
Level of scrutiny
Medium-high. Although the diff is small and the helper is a pure post-filter, it touches core JSC binding code in ErrorStackTrace.cpp (used by every error stack, including the finalizer-safe path where GC must not be triggered) and bindings.cpp (JSC__JSValue__getName/getNameProperty, used by Bun.inspect). The MustNotTriggerGC invariant is exactly the kind of thing a maintainer should confirm; the analysis holds (both propertyNames reads are pointer/refcount operations on VM-lifetime atomized strings), but I'd rather a human sign off on it than approve JSC-bindings changes autonomously.
Other factors
- The PR already went through one self-correction: the first revision reimplemented
getCalculatedDisplayNameand regressed builtin-module inspect output; the current revision reduced to a post-filter and added a test pinningBun.inspect(zlib.gzip)/Bun.inspect(fs.promises.readdir). - Test coverage is thorough: all four export-default shapes, before/after
.namereification, astarDefault-named control for the identity check, both theerror.stackand the uncaught-error-printer code paths, and the JSX tag-name path. Tests follow harness conventions (tempDir, concurrent pipe drains, combined-object assertions). - The comment-cop bot's two "paragraph-long comment" flags were addressed in cfd9586 (both threads resolved).
- The PR description notes a fuller fix belongs in the WebKit fork (
getCalculatedDisplayName/calculatedClassName, tracked in #38530); that layering call is worth a maintainer's opinion.
|
This PR conflicts with main now. Two groups of readers still show the private name after the mapping. I checked them on a release build of main bd599f5 with the mapping applied:
So the rule belongs one layer down, in the display name fall-throughs of the WebKit fork ( #34932 depends on this. It moves every anonymous default function from |
Problem
export default class { ... }renders asat new starDefault (p.mjs:1:55)until something reads the class's.name; after that the same frame rendersat new default (...), which is also what node prints.D.nameitself is always"default", so the frame name depends on evaluation order.export default class extends Base {}(synthesized constructor),export default () => ...andexport default (function () { ... }), and on every surface that shows a frame name:error.stack,CallSite#getFunctionName()underError.prepareStackTrace, and the frames Bun's own uncaught error / unhandled rejection printer writes to stderr.Bun.inspect/console.logof the class or function prints[class starDefault],[class starDefault extends Base],[Function: starDefault], and a JSX element whose type is such a class prints<starDefault />(node:[class default],[Function: default]). These stay wrong even after.nameis read.*default*identifier (vm.propertyNames->starDefaultPrivateName, whose string is"starDefault") and only substitutes"default"when it reifies thenameproperty (JSFunction::reifyName). Bun's name lookups use an already reified ownnameproperty when there is one and otherwise take the name from the executable, where it is still the raw identifier:src/jsc/bindings/ErrorStackTrace.cpp:functionName(vm, codeBlock)and theFinalizerSafety::MustNotTriggerGCbranch readecmaName()directly;functionName(vm, globalObject, object)gets it throughJSC::getCalculatedDisplayName, whose last fallback isecmaName()too.src/jsc/bindings/bindings.cpp:JSC__JSValue__getName(inspect of functions and classes,describe(Class)names) throughJSC::getCalculatedDisplayName, andJSC__JSValue__getNameProperty(JSX tag names) throughjsExecutable()->name(). That fallback is only reached whenJSFunction::name()returned"", which it does precisely for*default*, so it could only ever produce""or"starDefault".Fix
Zig::functionNameForDisplay(vm, name)returns"default"whennameis the*default*identifier's string and returnsnameunchanged otherwise. The five fallbacks above pass their result through it; nothing else about any lookup changes, so every other name comes out exactly as before (verified below for the builtin-module functions that the first revision of this PR got wrong).name.impl() == starDefaultPrivateName.impl(), the same comparisonIdentifier::operator==performs inJSFunction::reifyName. The private identifier's string is its own uniqueStringImpl, so a function that is really namedstarDefaultis left alone."default"is the name JSC itself reifies for these functions (JSFunction::reifyNameandJSFunction::originalNameapply the same substitution), so the lookups now return the same string before and after.nameis materialized, and they match node for every shape above.JSFunction::calculatedDisplayName/getCalculatedDisplayName(and the callers ofecmaName()inStackFrame,SamplingProfilerand the inspector) have the same missing substitution, which is also whyconsole.log(new D())still printsstarDefault {}(JSObject::calculatedClassName, tracked in inspect: print instances of an anonymous export default class as "default" #38530) and why--cpu-profand the debugger show the same name. Fixing it in the fork would cover all of those and reduce this PR to its tests; that needs a WebKit PR and a version bump, and the two directecmaName()reads inErrorStackTrace.cppare Bun's own code either way. This PR fixes the surfaces Bun formats itself now; when the fork applies the substitution,functionNameForDisplaybecomes an identity function and can be deleted along with its five call sites.Received function starDefaultsuffix of nativeERR_INVALID_ARG_TYPEmessages, sincedetermineSpecificTypeusesfunctionName(vm, globalObject, object); node errors: render a callable's name in determineSpecificType like node #38473 independently reworks that call site to read.name.test/js/bun/test/stack.test.ts("anonymous export default is named 'default'"):error.stackfor all four shapes plus a control function really namedstarDefault,CallSite#getFunctionName(), before and after.nameis read, and the frames printed for an uncaught throw and an unhandled rejection (those go through theMustNotTriggerGCbranch; confirmed by reverting only that line, which fails exactly those two cases). All three fail on the released bun withstarDefaultand pass with this change.test/js/bun/util/inspect.test.js("anonymous export default class and function are named 'default'"):[class default],[class default extends Base],[Function: default],<default />. Fails on the released bun, passes with this change.test/js/bun/util/inspect.test.js("functions from built-in modules do not inspect as their internal variable name"):Bun.inspect(zlib.gzip)stays[Function]andBun.inspect(fs.promises.readdir)stays[AsyncFunction]. Passes on the released bun and with this change; fails on the first revision of this PR ([Function: fn],[AsyncFunction: wrapped]).Bun.inspectofzlib.gzip,zlib.gunzipSync,fs.promises.readFile/writeFile,Readable.prototype.map,util.promisify,events.once,Bun.serve,Array.prototype.map,new Promiseresolvers, functions with an empty or non-emptydisplayName, inferred names and bound functions is byte-identical between the released bun and this change.test/js/node/v8/capture-stack-trace.test.js,test/js/bun/util/inspect-error.test.js,reportError.test.ts,error-name-preservation.test.ts,test/js/bun/test/describe.test.ts,test/js/node/util/{bun-inspect,custom-inspect}.test.*, the twoprepare-stack-trace/bindings-stack-traceregression tests and the rest ofinspect.test.jspass (the two "minified file" cases ininspect-error.test.jsfail identically on main under debug builds, whereBUN_DEBUGenablesshowPrivateScriptsInStackTraces).error-gc-test.test.jsandinspect-error-leak.test.js, which stress these lookups underBun.gc, pass under the debug ASAN build with their timeouts raised.Background
*default*: the spec's name for the module-internal binding created byexport default <anonymous declaration or expression>. JSC represents it with a private symbol whose description isstarDefault; that symbol is also theStringImplbehind the identifier's string, which is what the identity comparison relies on.ecmaName(): the name JSC's parser records on a function's executable for the purposes of the ECMAScriptnameproperty (for examplefinconst f = () => {}).JSFunction::name()returns the binding name instead, and returns""for*default*.namereification: JSC does not create a function's ownnameproperty until it is first looked up. Bun's lookups read that property when it exists (soObject.defineProperty(fn, "name", ...)wins) and otherwise fall back to the executable, which is why the output used to depend on whether.namehad been read.FinalizerSafety::MustNotTriggerGC: Bun formats some stack traces while JSC is finalizing an error, or when printing an error that still owns its raw frames, where running getters or allocating on the JS heap is not allowed. That branch reads structure slots directly; this PR only wraps its existingecmaName()return, which allocates nothing (defaultKeyword.string()is a VM-lifetime string).First revision of this PR (superseded)
The first revision replaced
JSC::getCalculatedDisplayNamein bothErrorStackTrace.cppandJSC__JSValue__getNamewith a Bun copy of the displayName / name / ecmaName chain that used the stack-trace rule for builtin functions (isHostFunction()instead of JSC'sisHostOrBuiltinFunction()). Functions defined in Bun's built-in modules are JSC builtins, soBun.inspectstarted printing their inferred internal names:zlib.gzipbecame[Function: fn]and thefs.promiseswrappers became[AsyncFunction: wrapped]. Review caught it; the current revision leaves every lookup as it was and only post-filters the result, and the new builtin-module test pins that output.