Repository navigation
Conversation
JSC__JSValue__getNameProperty read Symbol.toStringTag (through the prototype chain) before looking at the function itself, so every async, generator and async generator function came back as "AsyncFunction", "GeneratorFunction" or "AsyncGeneratorFunction" and the test runner's formatter printed all of them as [Function: AsyncFunction] in snapshots, toEqual diffs and JSX tags. Look up the JSFunction / InternalFunction name first and only fall back to Symbol.toStringTag when the function has no name of its own, so anonymous functions print exactly as before.
|
Warning Review limit reached
Next review available in: 15 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 |
|
Updated 10:37 AM PT - Aug 15th, 2026
✅ @robobun, your commit fbdb43ff5822cb557913b8d6f189262b1166d843 passed in 🧪 To try this PR locally: bunx bun-pr 38959That installs a local version of the PR into your bun-38959 --bun |
|
Status: fixed, ready for review.
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — reorders existing name-lookup branches so a function's own name wins over the inherited Symbol.toStringTag.
What was reviewed:
- Confirmed the
JSFunction/InternalFunction/toStringTagbranches are the same code as before, only reordered, with an addedRETURN_IF_EXCEPTIONaftertoWTFString. - Checked all four
get_name_propertycallers (pretty_format.rsTag::Function/ JSX /handle_first_property,ConsoleObject.rsJSX) — the non-functionhandle_first_propertypath still hitstoStringTagfirst since both downcasts fail. - Grepped
test/for[Function: AsyncFunction]/GeneratorFunction/[class ...]in snapshots — no existing snapshot output changes beyond the new tests.
Extended reasoning...
Overview
The PR reorders three existing branches inside JSC__JSValue__getNameProperty in src/jsc/bindings/bindings.cpp (~30 lines) so that JSFunction::name() / jsExecutable()->name() and InternalFunction::name() are consulted before the Symbol.toStringTag lookup, and adds a missing RETURN_IF_EXCEPTION after toWTFString. Two new tests in moremore.test.ts cover the fixed cases (named async/generator/bound/method functions, a class with a static Symbol.toStringTag, a JSX element) and pin the deliberately-unchanged cases (arrows and anonymous expressions with only an inferred name).
Security risks
None. This is display-only logic in the test-runner pretty formatter and getClassName; no user input parsing, no allocation sizing, no privilege boundaries.
Level of scrutiny
Low-to-medium. The change is a reorder of code that already existed in the function — no new name sources, no new lookup calls beyond what was already there. The one semantic tweak beyond ordering is that empty-named JSFunction/InternalFunction values now fall through to toStringTag instead of returning an empty string, which is what preserves the [Function: AsyncFunction] output for nameless async arrows and host builtins (verified by the second new test and the fs.promises.readFile example in the PR body).
Other factors
- All four Rust callers of
get_name_propertywere checked: thehandle_first_propertypath inpretty_format.rsis gated on!is_function(), so plain-objecttoStringTagprefixing is unaffected by the reorder; the JSX andTag::Functionpaths handlelen == 0explicitly. - A grep across
test/for[Function: AsyncFunction|GeneratorFunction|AsyncGeneratorFunction]and.snapfiles for[classfound no existing snapshots that would flip. - The PR description enumerates every affected surface (
describe(Class),expect.any,toThrow(Class), JSX tags) viagetClassName, and the author ran the full snapshot/expect/inspect suites under a debug build. - No prior reviewer comments; bug-hunting system found nothing.
|
Not a duplicate of #38939. That PR changes the two JSX printers ( |
There was a problem hiding this comment.
LGTM — a straightforward reorder of two existing lookups in getNameProperty, with an added exception check.
What was reviewed:
- Traced the behavioral delta for each object shape: named JSFunction, host/builtin with empty name, InternalFunction, and non-function objects with
Symbol.toStringTag— the only changed outputs are the ones the tests pin. - Confirmed the
else ifis safe (JSFunction and InternalFunction are disjoint hierarchies) and that the newRETURN_IF_EXCEPTIONaftertoWTFStringcloses a pre-existing gap. - Checked callers (
getClassName,Tag::Function/Tag::JSXinpretty_format.rs,ConsoleObject.rs) — non-function callers still reach theSymbol.toStringTagpath unchanged.
Extended reasoning...
Overview
This PR reorders JSC__JSValue__getNameProperty in src/jsc/bindings/bindings.cpp so that a function's own name (JSFunction::name() / InternalFunction::name()) is consulted before Symbol.toStringTag. Previously the tag was read first via prototype-chain lookup, so async/generator/async-generator functions — which inherit Symbol.toStringTag from their prototype — always printed as [Function: AsyncFunction] etc. in bun:test snapshots and diffs, hiding their actual name. The change is a pure reorder of existing lookup blocks plus one added RETURN_IF_EXCEPTION after toWTFString. Two new tests in moremore.test.ts cover the fix (named async/generator functions, bound functions, object methods, a class with a static Symbol.toStringTag, toEqual diff text, JSX tag) and pin the backward-compat behavior for functions without an own name.
Security risks
None. This is display-formatting logic for the test runner and inspectors. No parsing of untrusted input, no allocation-size arithmetic, no auth/crypto/permission code.
Level of scrutiny
Medium-low. The function is ~35 lines and the change is a reorder, not new logic. I traced each object shape through both old and new code: named JSFunctions now return their name (the fix); host/builtin functions with empty names fall through to the tag exactly as before; InternalFunctions with empty names now fall through to the tag instead of returning empty (a harmless improvement); non-function objects skip both function branches and hit the tag lookup unchanged, so the Foo { prefix path in handle_first_property is unaffected. The else if between JSFunction and InternalFunction is safe because they are disjoint JSC class hierarchies. No memory-safety concerns: no new allocations, same Zig::toZigString return pattern as before.
Other factors
The PR author verified the new test fails on bun 1.4.0 with USE_SYSTEM_BUN=1 and passes on the debug build, and ran the wider snapshot/expect/inspect/console suites. The comment-cop bot's feedback about the long code comment was addressed in fbdb43f (now one line). The interaction with #38939 (JSX printers) is documented and composes cleanly. No CODEOWNERS restrictions apply.
Problem
toMatchSnapshot()stores"load": [Function: AsyncFunction]forasync function load() {}([Function: GeneratorFunction]/[Function: AsyncGeneratorFunction]for the other kinds), andtoEqual/toStrictEqualfailure diffs show the same text, so two async functions in one object are indistinguishable. A plainfunction load() {}prints[Function: load], andconsole.log(load)prints[AsyncFunction: load]; only the test runner's formatter is affected. Found by inspection while working on Print JSX component tags with the component's inferred name or displayName #38939 (the same lookup names JSX tags there); there is no tracker issue for it, and the behaviour dates back to the formatter's first version.<AsyncFunction />, in both bun:test output andBun.inspect) and, throughgetClassName, the[class X]printer,describe(Class),expect.any(Class)and the "Expected constructor:" line oftoThrow(Class).JSC__JSValue__getNameProperty(src/jsc/bindings/bindings.cpp, the binding behindJSValue::get_name_property, used by theTag::Functionarm ofsrc/runtime/test_runner/pretty_format.rsand bygetClassNamefor functions) readsSymbol.toStringTagwithgetIfPropertyExists, which walks the prototype chain, before it looks at the function.AsyncFunction.prototype[Symbol.toStringTag]is"AsyncFunction"(likewiseGeneratorFunction/AsyncGeneratorFunction), so for these functions the name is never consulted.Fix
getNamePropertynow takes theJSFunction/InternalFunctionname first and readsSymbol.toStringTagonly when the object has no name of its own. The name sources and the tag lookup are the ones the function already used; only the order changes (plus an exception check after resolving the tag string).Symbol.toStringTagis never its name, it is the kind inherited from the prototype (or, for a class with a static[Symbol.toStringTag], a description of the class object); the name is whatconsole.log, node'sutil.inspectand jest's[Function name]all print first. Non-function objects never reach the name branches, so theSymbol.toStringTagbehaviour thathandle_first_propertyrelies on for theFoo {prefix of tagged plain objects is unchanged.const save = async () => {},{ update: async () => {} }), still prints exactly as today,[Function: AsyncFunction], through the tag fallback, just asconst f = () => {}still prints[Function]. This formatter has never printed inferred names (Fix missing function names in console.log and Bun.inspect #6612 changed that forconsole.logonly) and existing.snapfiles with such functions depend on it, so moving this printer toconsole.log's resolution ([Function: save], or[AsyncFunction: save]) is a snapshot-format change for a separate decision; this PR only fixes the output that was wrong under the formatter's existing rule. The second new test pins the unchanged cases so that a later change to the rule is made on purpose.<Page />instead of<AsyncFunction />for an async JSX component in both printers (Print JSX component tags with the component's inferred name or displayName #38939 reworks JSX tag naming separately and composes with this), and a class with a staticSymbol.toStringTaggetter now prints as[class Tagged]/Any<Tagged>/Expected constructor: Taggedinstead of the tag, matching whatconsole.logprints for the class. inspect: display a class or function name set with Object.defineProperty #38922 and Name anonymous export default functions and classes "default" in stack traces and inspect #38517 add name sources inside the same branch of this function; whichever of those and this lands second needs a small textual rebase, and the reorder keeps their additions ahead of the tag lookup.test/js/bun/test/snapshot-tests/snapshots/moremore.test.ts, "async and generator functions print their own name": declaration, generator, async generator, named function expression, bound async function, object-literal async / generator methods, a class with a staticSymbol.toStringTag, thetoEqualdiff text, and a JSX element whose type is an async function. Fails on bun 1.4.0 (every one of those prints as its kind), passes with this change.snapshot-tests/,describe.test.ts,expect.test.js,expect-extend.test.js,jest-extended.test.js,spyMatchers.test.ts,test-test.test.ts,util/inspect.test.js,console/console-log.test.tspass.printing/diffexample.test.tsdiffers only in an unstripped[4.22s]duration andpretty-format-overflow.test.tsoverflows the stack, both pre-existing under debug builds and both already tracked by open PRs; the ~700 lines of diff output in the former are byte-identical.Symbol.toStringTaggetter that throws while a diff is being formatted trips a JSC exception-scope assertion in debug builds before and after this change alike (the diff formatter discards the formatter's error atdiff_format.rs:47); that is a separate pre-existing bug and has been reported on its own.Background
Symbol.toStringTag: the propertyObject.prototype.toStringreads to produce[object Foo]. JSC defines it onAsyncFunction.prototype,GeneratorFunction.prototypeandAsyncGeneratorFunction.prototype, so every function of those kinds inherits it; ordinary functions inherit nothing fromFunction.prototype. Bun's formatters also use it to prefix plain objects that carry one (Foo {}).JSFunction::name(): the name a function was declared with (function load() {},async get() {}in a literal, a bound function's target name). A name that was only inferred from a binding (const f = () => {}) is stored separately (ecmaName) and is whatf.nameandgetCalculatedDisplayName(used byconsole.log) report;getNamePropertynever read it, and still does not.InternalFunctionis the base class of native constructors such asPromiseorError, which carry their name directly.getClassName(bindings.cpp) delegates togetNamePropertywhenever the cell's class isFunction, i.e. for every function and class value, which is how the[class X]printer and the matcher messages above share this code path.Before / after (bun:test snapshot output)