[robobun]

Problem

• A class field typed as a function that stores the resolve or reject function of new Promise(executor) is read behind a test: HasBirthFieldsFilled, then GetSlot level=Expected, with a by-name arm. bun build --compile --aot --explain reports each read, the store and the declaration as missed-optimization (AOT: missed-optimization for a function-typed class field holding a Promise's resolve bun#47599, the request-batching pattern).
• applyConstructionOfPromise (aot/valueflow/AOTValueFlowRulesPromises.cpp:180) hands the executor two objects that have no class, so ValueFlowPositions::addUnlessInstanceOfClass types them anyObject|anyFunction. A store of that into a function slot may leave the slot, so the field never gets IsNeverStoredWithOtherType and never becomes AlwaysFilled.

Fix

• A class %PromiseResolvingFunction% with hasClosureInstances, beside %BoundFunction% (promise.builtins.ts). computeClassesOfBuiltinResults notes it on the second object of every construction whose callees are a subset of {Promise}, and on one new object of the universe, resolvingFunctionOfPromises(), which the rule uses for reject and, where the construction has no site of its own, for resolve.
• The new object replaces resultOf(Promise, 1) in the rule: that object is also what the table's rows allocate on behalf of Promise (an allocatedAs write naming AsyncFunction was found there), so a class noted on it was withdrawn.
• The ResultOfBuiltin arm of addUnlessInstanceOfClass types an object of such a class as a function, as the Site arm does. The two checks that named %BoundFunction% ask for hasClosureInstances instead.
• Verified: five .ir tests in aot/tests/valueflow/protocols/ (three fail on main, two negative cases pass on both). testaot run tests: 5803 passed, 0 failed. testaot native: 3053 passed, 0 failed (on the merge commit of AOT: crashes and miscompilations found since #971, Map/Set index slots, a bounds test on split() #972, where AOT: hangs, wrong answers, failed builds; bun test --aot follows #44833's mocks bun#47605 pins the engine). Self-reviewed: 11 concerns raised, 9 addressed (Notes).

Background

• Flow gives every object a class where it can. A class with hasClosureInstances stands for function objects that a built-in makes and that flow does not follow into: %BoundFunction% was the only one. Such an object becomes Type::function(), never a class part, so no instance test is ever generated for it.
• The universe numbers its objects by kind. resultOf(builtin, n) are the two objects a built-in answers with where a call has no site. The new object sits at the end of that range: one id per compile.
• Considered: typing the objects as the compiled closures of PromiseConstructor.ts (wrong for a native construction, and it would re-dispatch every r(x) call), a "surely a function" set beside the universe (more code, same result), and a new object kind (+28% universe ids per compile). The class row is the one mechanism the engine already has.

Downsides

• A program that stores a resolving function into an object literal pushed into a queue (the DataLoader record form) can lose one inlining on the old heat threshold: on a 25-line program load() drops from the hottest fifth of the code to warm, because the executor now carries two AssumeType values and the heat graph counts values, not code. +2,000 branches of 14,011 there. With five more small functions in the program both engines compile it the same (16,457 branches). Follow-up: the heat graph's size should not count values that emit no code.
• The table grows by one class row: +150 bytes in Bun's table objects (AOTBuiltinTableBuiltins.cpp.o +120, AOTBuiltinTableNames.cpp.o +30), 0 host functions. One universe object more per compile.
• A construction whose callee may be Promise or another constructor keeps the test, as before (the negative tests pin it).

Notes

Measurements (engine 4d45755 with and without this change, Bun 0903892715 built against each with the same recipe, x64 release without LTO)

• The program of AOT: missed-optimization for a function-typed class field holding a Promise's resolve bun#47599: --explain reads 9 free, 0 tested, 0 by name, was 6 free, 3 tested. The three missed-optimization entries are gone. The unknown-callee entry at new Promise stays: it is the executor(...) call inside the compiled Promise constructor, the same on both engines, and out of scope here.
• HasBirthFieldsFilled in settle and in the executor: 1 + 1 > 0 + 0 (testaot print --until=PlaceTypeTests).
• Branches of the executable (--aot-count-branches, BUN_JSC_aotCountsPath): the issue program 38 > 32, run-time calls 15 > 14. The three companion programs (16 objects, 10,000 settle turns, 160,000 reads): resolve 830,158 > 670,158, reject 500,253 > 340,253, the | undefined spelling 830,158 > 670,158. One branch per read, as expected.
• The executor's EntryChecks wrapper and generic version (two functions) go, for every typed executor: its parameters are proven.
• compare-builds.ts on the idioms and everyday benchmarks, three sizes, no servers: 12,612 rows compared, 0 slower, 0 faster; 6 rows moved among the benchmarks the tool lists as unsteady (3 up, 3 down, 2.7% to 6.5%), 27 moved once and not the second time.
• same-output.ts: all 289 programs hand over another IR (the class ids are renumbered); the machine code of 283 is identical function by function. Six differ: four node:events programs in one library function (createOnWarning, 316 > 354 bytes) and in functions that moved, stream-new-readable in node:stream's operators (onCatch 98 > 68, afterItemProcessed 208 > 111, maybeResume 160 > 70 bytes: they store the resolve of a new Promise), and instanceof-response in bytes only.
• Compile time of test/aot/servers/app/api-bun.ts, five interleaved runs each: median 806 ms > 810 ms (runs 725 to 858 ms against 734 to 910 ms, the first cold run of each left out). Wall clock, so within the noise; the change adds one object id to the universe and one class row to the table, no pass.
• Not measured with perf, valgrind or bloaty: none is installed here. Sizes are from size -A, counts from the run time's own counters.

The reject object

The plan was to note the class on resultOf(Promise, 1), which the rule used for reject. On the issue program the class was withdrawn at once: the reconciliation loop at the end of computeClassesOfBuiltinResults found an allocatedAs write on that object naming AsyncFunction, from useByHost on behalf of Promise (actWithoutOverload makes resultOf(m_builtin, 1) the contents object of whatever the host's rows allocate). So the rule now has an object of its own. It also stands for resolve where the construction has no site object: Reflect.construct(Promise, [executor]), a bound Promise, a construction by native code. Before, those took resultOf(Promise, 1) for resolve and stayed tested.

The DataLoader record form

new Promise((resolve, reject) => queue.push({ key, resolve, reject })), settled by queue[j].resolve(v): flow proves the literal's fields on both engines (5 free, 0 tested). On the 25-line probe the PR engine runs 16,011 branches against 14,011 and the run time grows queue eleven times (no planned capacity). The cause is not the field: load() is Hot on main (heat percentile 82) and Warm here (81), and with it the executor's predicted call loses the budget that would have copied it into the loop. The heat graph sizes a function by its number of values (AOTHeatGraph.cpp, blockSize = block.values.size() + 1); the executor has 16 values now, 14 before (two AssumeType, which emit no code), which moves load() across the threshold on a program of about 350 values in all. The same program with five more small functions: 16,457 branches on both engines, 58 run-time calls against 76.

Tests

• The branch is based on the merge commit of AOT: crashes and miscompilations found since #971, Map/Set index slots, a bounds test on split() #972, the engine that the open Bun train (AOT: hangs, wrong answers, failed builds; bun test --aot follows #44833's mocks bun#47605) pins: Bun's table generator of that train reads this engine's built-ins, and Bun main's does not read AOT: follow-up to #972: crashes, wrong answers, bun test --aot, build time #974's yet. Rebasing onto main is one line in AOTValueFlowBodiesLink.cpp.
• the-resolving-functions-of-a-promise-fill-the-function-typed-fields-that-store-them.ir: resolve and reject stored into two fields of a class. Fails on main.
• ...-constructed-by-super-fill-the-fields-that-store-them.ir: class P extends Promise { constructor(executor) { super(executor) } }. Fails on main.
• ...-constructed-without-a-site-fill-the-fields-that-store-them.ir: Reflect.construct(Promise, [executor]). Fails on main.
• ...-are-not-known-where-the-callee-may-be-another-built-in.ir: new (flag ? Promise : Map)(executor). Passes on both: pins the subset test.
• ...-are-not-known-where-a-constructor-of-the-program-may-call-the-executor.ir: new (flag ? Promise : Foo)(executor) where Foo calls the executor with a number. Passes on both.
• Companion (oven-sh/bun): three programs under test/aot/parallel/field-writers/ with COUNT: branches bounds that the engine of main fails.

Self-review

11 concerns, 9 addressed: the rebase onto main (one conflict in AOTValueFlowBodiesLink.cpp, resolved by hand), the COUNT bounds (the field read now dominates the count), the negative tests, the super() test, the siteless construction (Reflect.construct) and the comment that described it, the unknown-callee entry named as out of scope, the measured delta in place of the count of reports, the DataLoader record form (measured and explained above). Not done: a fix of the heat graph's size measure (a change of its own), and a truthful --explain cause for stores of callables that built-ins make (named as follow-up in oven-sh/bun#47599).

Follow-ups named, not done here

• Promise.withResolvers(): its resolve/reject come from a table row with b.callable and are still objects nobody knows.
• The then(res, rej) parameters of a custom thenable.
• The unknown-callee entry at every new Promise(executor).
• A number field stored by a method or a closure that the constructor calls (ConstructionWalk in AOTValueFlowUnobservedDefinitions.cpp observes the birth undefined).

[github-actions]

Preview build of e16b7a5: autobuild-preview-pr-977-e16b7a5e

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: e5cf7ace-dec3-4f30-8302-982cffcd9ab7

Commits

Reviewing files that changed from the base of the PR and between 9b16286 and e16b7a5.

Files selected for processing: 12

• Source/JavaScriptCore/aot/builtins/promise.builtins.ts
• Source/JavaScriptCore/aot/tests/valueflow/protocols/the-resolving-functions-of-a-promise-are-not-known-where-a-constructor-of-the-program-may-call-the-executor.ir
• Source/JavaScriptCore/aot/tests/valueflow/protocols/the-resolving-functions-of-a-promise-are-not-known-where-the-callee-may-be-another-built-in.ir
• Source/JavaScriptCore/aot/tests/valueflow/protocols/the-resolving-functions-of-a-promise-constructed-by-super-fill-the-fields-that-store-them.ir
• Source/JavaScriptCore/aot/tests/valueflow/protocols/the-resolving-functions-of-a-promise-constructed-without-a-site-fill-the-fields-that-store-them.ir
• Source/JavaScriptCore/aot/tests/valueflow/protocols/the-resolving-functions-of-a-promise-fill-the-function-typed-fields-that-store-them.ir
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowBodies.h
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowBodiesLink.cpp
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowBuiltins.cpp
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowPositions.cpp
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowResultsBuiltins.cpp
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowRulesPromises.cpp

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

The value-flow model now identifies Promise resolving functions and tracks them during Promise construction. New fixtures check resolver fields across direct, subclass, and conditional construction cases.

Changes

Promise resolving function value flow

Layer / File(s)	Summary
Resolver class and object identity Source/JavaScriptCore/aot/builtins/promise.builtins.ts, Source/JavaScriptCore/aot/valueflow/AOTValueFlowBodies.h, Source/JavaScriptCore/aot/valueflow/AOTValueFlowBodiesLink.cpp	The builtin declarations add PromiseResolvingFunction. ValueFlowUniverse reserves and identifies a rule-created object for Promise resolving functions.
Promise resolver classification and construction Source/JavaScriptCore/aot/valueflow/AOTValueFlowResultsBuiltins.cpp, Source/JavaScriptCore/aot/valueflow/AOTValueFlowRulesPromises.cpp, Source/JavaScriptCore/aot/valueflow/AOTValueFlowBodiesLink.cpp, Source/JavaScriptCore/aot/valueflow/AOTValueFlowBuiltins.cpp, Source/JavaScriptCore/aot/valueflow/AOTValueFlowPositions.cpp	Builtin-result analysis classifies Promise resolver functions. Promise construction uses a site object for the resolve function when available and the shared resolving-function object otherwise. Function-class handling now uses closure-instance metadata.
Resolver field value-flow fixtures Source/JavaScriptCore/aot/tests/valueflow/protocols/the-resolving-functions-of-a-promise-*.ir	New fixtures check resolver fields across conditional constructors, Promise subclasses, and construction without a site object.

Possibly related PRs

• oven-sh/WebKit#906: Changes value-set queries and AOT field-type inference used by the builtin-result class assignment updated here.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to e16b7

No identified issue prevents merging after normal checks.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check		The author-provided description is substantive and technical. It states the problem, the fix, the verification, and the trade-offs. It explains why the change is needed, names the files and functions …	Rewrite the description to match the repository template. Specifically: (1) add a WebKit Bugzilla bug link in the form https://bugs.webkit.org/show_bug.cgi?id=##### and a bug title line; (2) add a 'Reviewed by' line (set to NOBODY (OOPS!) u…

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title clearly identifies the main change: improving AOT flow analysis for Promise resolving functions. It is concise and specific.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The author-provided description is substantive and technical. It states the problem, the fix, the verification, and the trade-offs. It explains why the change is needed, names the files and functions involved, and reports test results. It does not follow the WebKit template. It has no Bugzilla bug link (the template requires one under 'File a Bug'). It has no 'Reviewed by' line, and it does not use the template's commit-message layout with a bug title and a changed-files list. The description references an oven-sh/bun issue and a Bun PR instead of a WebKit bug. The content is strong, but the structural elements the repository template requires are missing.

Resolution

Rewrite the description to match the repository template. Specifically: (1) add a WebKit Bugzilla bug link in the form https://bugs.webkit.org/show_bug.cgi?id=##### and a bug title line; (2) add a 'Reviewed by' line (set to NOBODY (OOPS!) until a reviewer is assigned, or name the reviewer); (3) add the explanation section in the template's style, which can keep the current Problem/Fix/Background content; (4) add the changed-files list in the template format (for example, '* Source/JavaScriptCore/aot/builtins/promise.builtins.ts:' followed by the affected class or function entries). Keep the measurements and downsides, since they are useful to reviewers.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[claude]

I reviewed this PR and didn't find any bugs. Because it changes what the value-flow analysis is willing to prove about executor parameters (a wrong proof here would be a miscompile rather than a missed optimization), a human familiar with aot/valueflow should still take a look.

What was reviewed:

• The new universe object at m_firstClosure - 1: kindOf classifies it as ResultOfBuiltin; the only reverse mappings over that range (answersFunctionBoundToReceiver bounds check, firstHostObject comparison in AOTValueFlowResultsAccesses.cpp) exclude it or treat it conservatively, and numberOfObjectsMadeWithClasses already counts it.
• Mixed-callee constructions (new (flag ? Promise : Foo), Promise-or-another-built-in): the subset check in computeClassesOfBuiltinResults plus the executor's other argument flow keep the slot tested, pinned by the two negative .ir tests.
• The ResultOfBuiltin arm of addUnlessInstanceOfClass now mirrors the Site arm exactly; the %BoundFunction% special cases replaced by hasClosureInstances have no other class with that flag besides the new one, and the generator's checkClosureInstances requires extends Function, which the new class satisfies.

Extended reasoning...

The change adds a %PromiseResolvingFunction% built-in class, one rule-owned object in the value-flow universe, and makes applyConstructionOfPromise hand the executor objects of that class so function-typed fields storing resolve/reject keep their stability facts; five new .ir protocol tests cover the direct, super, no-site and two negative cases. It touches no security-sensitive surface (no auth, injection or data exposure), only the AOT compiler's type-proof machinery. The source delta is small (about 30 lines across six files) and the reasoning checked out, but the code is specialized compiler analysis where an over-eager proof becomes a miscompile, the commit comes from a bot author, and there is no independent human review on the timeline, which is why this is a defer rather than an approve.