Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (2)
Comment |
|
Status: restacked on #38517, waiting on CI. Reproduced with a two-file ESM fixture ( This PR's base is #38517's branch so the diff here is only the instance half (it reuses that PR's |
There was a problem hiding this comment.
LGTM — the pointer-identity check against starDefaultPrivateName.impl() mirrors what JSFunction::reifyName does, and the test proves a class literally named starDefault is left alone. The inline nit about CallSite::formatAsString is a narrow pre-existing sibling, not a blocker.
Extended reasoning...
Overview
Seven-line change in JSC__JSValue__getClassName (src/jsc/bindings/bindings.cpp) that maps JSC's private *default* identifier string to "default" before returning it to the formatter, so Bun.inspect(new Cls()) on an anonymous export default class {} prints default {} instead of leaking the internal starDefault name. One new subprocess test in test/js/bun/util/inspect.test.js.
Security risks
None. This is cosmetic output formatting on the inspect path — no parsing of untrusted input, no allocation changes, no new user-reachable code paths. The only new call is JSC::getVM(arg1), which is already used pervasively in this file.
Level of scrutiny
Low. The change is a targeted string substitution guarded by a pointer-identity comparison against a VM-singleton SymbolImpl. The PR description correctly explains why identity (not string equality) is the right test — it's the same check JSFunction::reifyName performs, and the test's export class starDefault {} case verifies a user class with that literal name is not remapped. The two other calculatedClassName callers in bindings.cpp (lines 957, 1714) only compare names for deepEquals and don't display them, so they're correctly out of scope.
Other factors
- The test is thorough: plain class, explicit constructor, parenthesized expression form,
Number/Booleansubclasses,.prototype, a named subclass (unchanged), and the negative case. It follows harness conventions (tempDir,bunEnv, concurrent pipe drain, exit code asserted last). - The one finding (sibling
calculatedClassNamecall inCallSite::formatAsString) is a nit: it requires a sloppy-mode function called with an anonymous-default-class instance asthisto trigger, is pre-existing behavior this PR doesn't worsen, and the PR already references #38517 for the stack-frame path. It's worth noting the "only Bun consumer" claim in the description is slightly overstated, but that doesn't affect the correctness of the fix.
|
Updated 12:40 PM PT - Aug 14th, 2026
❌ @robobun, your commit 1f1d9ab has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38530That installs a local version of the PR into your bun-38530 --bun |
1f1d9ab to
83d56af
Compare
…ault", not "starDefault"
The Foo {} prefix the formatter prints for an instance comes from
JSObject::calculatedClassName, which returns the constructor's display
name. For export default class {} that falls back to the parser's private
*default* identifier, whose string is "starDefault", while the class's own
.name (and node) say "default". Run the result through
Zig::functionNameForDisplay, the same filter the class value itself and
stack frames use, in JSC__JSValue__getClassName and in the CallSite
receiver prefix that reads the same name.
83d56af to
8b730c5
Compare
There was a problem hiding this comment.
LGTM — the description now matches the code (CallSite site swept via the shared helper), so my earlier note is resolved.
What was reviewed:
- Both
calculatedClassNamedisplay sites now route throughZig::functionNameForDisplay; the twoBun__deepEqualscallers only compare and are correctly left alone. - Verified the header include chain:
CallSite.handbindings.cppboth pull inErrorStackTrace.h, so the unqualified call inCallSite.cpp(insidenamespace Zig) and the qualified one inbindings.cppresolve. - Test covers the false-positive guard (a class really named
starDefaultkeeps its name) and the named-subclass case stayingSub {}. - The Windows
test/bake/deinitialization.test.tssegfault in build #96116 is unrelated to this change.
Extended reasoning...
Overview
Stacked on #38517. Two one-line C++ changes wrap JSObject::calculatedClassName results through the existing Zig::functionNameForDisplay filter (from #38517) at the two display sites: JSC__JSValue__getClassName in bindings.cpp (drives the Foo {} prefix in Bun.inspect/console.log) and CallSite::formatAsString in CallSite.cpp (the Type. prefix in formatted stack frames). One new test in test/js/bun/util/inspect.test.js sits next to #38517's test and covers plain / explicit-ctor / parenthesized-expression default classes, Number/Boolean subclasses, Cls.prototype, a named subclass (unchanged), and a class actually named starDefault (unchanged — proving the identity-compare filter does not false-positive on the string).
Prior feedback
I raised two points on earlier revisions: (1) the sibling calculatedClassName site in CallSite::formatAsString should be swept or explicitly excluded, and (2) after it was swept, the description still said it was left alone. Both are now resolved: the site is swept via the shared helper, and the current description states that and explains why the branch is unreachable today (CallSites are built from JSCStackTrace::fromExisting, whose frames carry no call frame, so m_thisValue is always undefined) — which is why no test covers that line. The comment-cop nit about the long inline comment in bindings.cpp was also addressed (8b730c5).
Security risks
None. This is a display-name mapping for inspect output and stack-frame formatting; no parsing of untrusted input, no allocation shape change, no lifetime change. functionNameForDisplay is a pure identity check on a VM-owned StringImpl returning either the input or the "default" literal.
Level of scrutiny
Low. The diff is two lines of C++ that call an already-reviewed helper from the base PR, plus a hermetic subprocess test that follows harness conventions (tempDir, bunEnv, bunExe, concurrent stdout/stderr/exited drain, exact inline snapshot, stderr and exitCode asserted last). I confirmed the include chain resolves functionNameForDisplay at both call sites (CallSite.h → ErrorStackTrace.h; bindings.cpp:173 includes it directly). The remaining calculatedClassName callers in Bun__deepEquals compare rather than display and are correctly untouched.
Other factors
CI build #96116 has one failure — test/bake/deinitialization.test.ts segfault on Windows 2019 x64 — which is in the bake/dev-server area and unrelated to inspect or bindings. This PR is stacked on #38517 and should merge after it; the diff shown here is only the delta on top.
|
oven-sh/WebKit#950 proposes the same rule in JavaScriptCore, for functions, classes and their instances in one place. This pull request and #38517 stay open as the other option until a maintainer says on oven-sh/WebKit#950 which place is wanted. If that one merges, the Bun pull request with the tests (branch |
Stacked on #38517 (this PR's base branch); the diff shown here is only the instance half. Merge #38517 first, GitHub retargets this one to main.
Problem
console.log(new Cls())/Bun.inspect(new Cls()), whereClsis an anonymousexport default class {}, printsstarDefault {}. Node printsdefault {}, andCls.nameis already"default"in Bun.{ inst: starDefault {} }),export default class extends Number {}([Number (starDefault): 1]),Booleansubclasses,Cls.prototype.JSC__JSValue__getClassName(src/jsc/bindings/bindings.cpp), which returns JSC'sJSObject::calculatedClassName. That resolves the constructor and returnsJSFunction::calculatedDisplayName, which falls back to the executable'secmaName. For an anonymousexport defaultthe parser sets that name to the private*default*identifier (starDefaultPrivateName), whose string is"starDefault".JSFunction::reifyNamemaps it to"default"when it creates.name;calculatedDisplayNamedoes not.Zig::functionNameForDisplay(the*default*to"default"filter) and applies it where Bun prints the function value itself ([class default],[Function: default], stack frames). With Name anonymous export default functions and classes "default" in stack traces and inspect #38517 alone the same program prints[class default] starDefault {}: the instance path reads the name throughcalculatedClassName, which Name anonymous export default functions and classes "default" in stack traces and inspect #38517 leaves out.Fix
JSC__JSValue__getClassName: pass thecalculatedClassNameresult throughZig::functionNameForDisplay, so the class value and its instances go through the one filter.CallSite::formatAsString(src/jsc/bindings/CallSite.cpp): theType.prefix it builds from the receiver reads the samecalculatedClassName, so it gets the same one-line treatment. It is not reachable today (CallSites are built from recordedJSC::StackFrames viaJSCStackTrace::fromExisting, whose frames carry no call frame, so the receiver is alwaysundefined), which is why the test cannot cover it; it is changed so that every display consumer ofcalculatedClassNameis filtered, not just the one with a repro.calculatedClassNameis, for these objects, exactly the constructor'scalculatedDisplayName, the string Name anonymous export default functions and classes "default" in stack traces and inspect #38517 already filters when printing the constructor; this makes the instance agree with the class value, withCls.name, and with node. The filter is an identity test on the*default*symbol'sStringImpl, so a class that is really namedstarDefaultis untouched (covered by the test).calculatedClassNamecallers (two inBun__deepEquals) compare the names of both operands and are left alone.instances of an anonymous export default class print as default), next to Name anonymous export default functions and classes "default" in stack traces and inspect #38517's test. Built with src/ reverted to the Name anonymous export default functions and classes "default" in stack traces and inspect #38517 state it fails with[class default] starDefault {}and the seven other instance lines readingstarDefault; with this branch it passes. It covers a plain class, an explicit constructor,export default (class {}),NumberandBooleansubclasses,Cls.prototype, a named subclass (unchanged) and a class really namedstarDefault(unchanged).JSFunction::calculatedDisplayName/getCalculatedDisplayNamein oven-sh/WebKit would also cover JSC's ownError.prototype.stackand would let these two call sites drop the filter; it would not remove the filter itself, since Name anonymous export default functions and classes "default" in stack traces and inspect #38517 also applies it to Bun-side reads of the rawecmaName. So the Bun-side filter is the shape either way, and this PR only adds callers to it.Background
ecmaName: the name JSC records on a function's executable for.namepurposes (the class name for a class,fforconst f = () => {}).JSFunction::name()is the syntactic name and is empty for these, so display-name helpers fall back toecmaName.*default*: the spec's binding name for an anonymousexport default. JSC represents it as the private identifierstarDefaultPrivateName; it is not meant to be user visible, soreifyNameturns it into"default"when materializing.name.JSObject::calculatedClassName: JSC helper that names an object after itsconstructor(own property first, then through the prototype chain), falling back toSymbol.toStringTagand the native class name. Bun's formatter uses it for theFoo {}prefix.Zig::functionNameForDisplay(Name anonymous export default functions and classes "default" in stack traces and inspect #38517): takes a name string and returns"default"if the string is the*default*identifier's ownStringImpl, otherwise the string unchanged.Earlier shape of this PR
The first revision was independent of #38517 and did the identity compare inline in
JSC__JSValue__getClassName(calculated.impl() == vm.propertyNames->starDefaultPrivateName.impl()), leavingCallSite.cppalone because it is unreachable. #38517 has since moved to a string post-filter with the same compare, so this revision is stacked on it and calls that helper instead of carrying a second copy of the mapping.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/bun/util/inspect.test.js