Repository navigation
Bump WebKit (oven-sh/WebKit#977 preview): a function-typed field that holds a promise's resolve is read without a test - #47683
Conversation
…mise is read without a test Bump WebKit to the preview build of oven-sh/WebKit#977: the engine of #972, where this branch pins it, plus that change. Flow knows the two resolving functions that `new Promise(executor)` hands its executor as functions, so a field typed as a function that stores one keeps its declared type from birth, and `bun build --compile --aot` reads it with one load. Three programs under test/aot/parallel/field-writers/ pin the count of branches: the engine of main runs one test more for each read.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
test/aot/harness/flow-claims.ts— CI running test/aot/what-flow-says-of-an-entered-function-holds.test.ts fails after this bump for the "an executor" way, where the base passes. The bumped engine types the resolve and reject thatnew Promise(T)handsTas a new class with closure instances, butadmitsat test/aot/harness/flow-claims.ts:114 accepts only a class named Proxy, Function or BoundFunction for a value seen as a function. Fix: makeadmitsaccept the engine's new resolving-function class as well (extend the regex at flow-claims.ts:114, or accept any class token the engine marks as having closure instances), and run that test against the pinned testaot before landing. Condition: the class prints under a name not starting with those three words, as%PromiseResolvingFunction%does.Why this was flagged
test/aot/ways-to-enter-a-function.ts:396 lists the way
new Promise(T);. fileOf in test/aot/harness/flow-claims.ts:34-58 makesw_<name>(a, b)hand a and b tok_<name>, and also calls w_ with symbols (lines 47-50), so at run time a is seen as "symbol" and "function". test/aot/what-flow-says-of-an-entered-function-holds.test.ts:66-81 reads the flow type of k_'s parameters fromtestaot printand calls admits(type, value) for each seen kind. With the engine of oven-sh/WebKit#977 the executor's parameters carry the new class. admits at flow-claims.ts:108-115 returns true for kind "function" only when a part is "function", "object", f, builtin(...), or matches /^(Proxy|Function|BoundFunction)\b/; a part such as PromiseResolvingFunction matches none, so withoutRoom gets "an executor (strict): the first parameter was function, the analysis says ..." and expect(withoutRoom).toEqual([]) at line 81 fails. On the base engine the same parameters were "objects it knew nothing of", which admits as "object". The test is not skipped on CI: harness/printed-ir.ts:33-39 skips only when no testaot is found, and the prebuilt tarball ships one.Verification: test/aot/harness/flow-claims.ts:108-115: for kind "function" the only accepted parts are
function,object,/^f\d+$/,builtin(, or/^(Proxy|Function|BoundFunction)\b/.PromiseResolvingFunctionmatches none, so for the "an executor" way the first parameter's type has no part that admits "function", and the test fails. On the base pin the engine typed these as objects it knew nothing of, which line 111 admits.
Four stored IR dumps print the executor's parameters, a captured resolve and an array of resolving functions as what the engine of oven-sh/WebKit#977 proves: undefined|function, not an expected undefined|null|function or an object.
|
On the two findings:
The CI failures of the first run that are not on the base branch's own build (#136420) were four stored IR dumps under |
Fixes #47599
Problem
bun build --compile --aot --explainreportsmissed-optimizationthree times for a class field typed as a function that holds theresolveof anew Promise(executor): the read in the settle loop, the store in the executor, and the declaration. The field is read behind aHasBirthFieldsFilledtest with a by-name arm. This is the request-batching pattern.aot/valueflow/AOTValueFlowRulesPromises.cpp:180andAOTValueFlowPositions.cpp:495).Fix
WEBKIT_VERSIONis the preview build of [JSC] AOT: the resolving functions of a promise are functions to flow WebKit#977: the engine of AOT: crashes and miscompilations found since #971, Map/Set index slots, a bounds test on split() WebKit#972, where this branch pins it, plus that change. This PR is based on AOT: hangs, wrong answers, failed builds;bun test --aotfollows #44833's mocks #47605 and moves its pin. Move it to a commit of oven-sh/WebKitmainwith that change before this lands.bind()'s result has one, so flow knows they are functions. The field becomesAlwaysFilledandIsNeverStoredWithOtherType. Details and measurements are in [JSC] AOT: the resolving functions of a promise are functions to flow WebKit#977.test/aot/parallel/field-writers/(resolve, reject, the| undefinedspelling) pin the branch count of 160,000 reads. Each reads 160,000 branches less than on the engine of AOT: hangs, wrong answers, failed builds;bun test --aotfollows #44833's mocks #47605 (830,158 > 670,158 and 500,253 > 340,253), one test per read.test/aot/engine-tests/change where they print the executor's parameters, a capturedresolveor an array of resolving functions:undefined|function, proven, in place of an expectedundefined|null|functionorobject.test/aot/engine-testswith the pinned engine (1879 passed, 0 failed),what-flow-says-of-an-entered-function-holds.test.ts, and the three programs withrun-parallel.ts, compiled and with types erased, on a Bun built against the engine with and without the change. The issue's program:9 free, 0 tested, 0 by name, was6 free, 3 tested.Background
WEBKIT_VERSION(scripts/build/deps/webkit.ts) names the oven-sh/WebKit release the build links.autobuild-preview-pr-<n>-<sha>is the build of a pull request there.--aotcompiles the program to machine code at build time. The value-flow analysis of the engine gives each object a class where it can. A class with closure instances stands for function objects a built-in makes that flow does not follow into:%BoundFunction%was the only one, and this adds%PromiseResolvingFunction%.Downsides
unknown-calleeentry that--explainprints at everynew Promise(executor)stays. It is the executor call inside the compiled Promise constructor, on every engine, and not this change's subject.src/back on main, the programs still pass. The proof is the pin.Notes
The report
@romulovalez, bun
1.4.4-canary.1+da867c60b, darwin arm64.class Call { resolve!: (v: number) => void; promise: Promise<number>; constructor() { this.promise = new Promise<number>((r) => { this.resolve = r; }); } }andsettle(list, values)callinglist[j].resolve(values[j]).--explainreports threemissed-optimizationentries (UnprovenCause::LeftByPasses), withtypes: 'Call'.What was found on the way
undefinedof a non-nullable field counts as observed once the constructor handsthisto a method or a closure (AOTValueFlowUnobservedDefinitions.cpp,ConstructionWalk). Not fixed here.Promise.withResolvers()and a custom thenable'sthen(res, rej)still hand objects nobody knows. Not fixed here.{ key, resolve, reject }literals into a queue runs 2,000 branches more with the change:load()moves from the hottest fifth of the code to warm because the executor now carries twoAssumeTypevalues, and the heat graph counts values, not code. With five more small functions in the program both engines compile it the same. Follow-up in the engine.Measurements (engine 4d45755fed with and without the change, Bun 0903892 built against each with the same recipe)
test/aot/perf/compare-builds.ts, 12,612 rows, no servers: 0 slower, 0 faster.test/aot/tools/same-output.ts: 283 of 289 programs compile to identical code function by function; the other 6 store the resolve of a promise in library code (node:streamoperators) or moved with it.test/aot/servers/app/api-bun.ts: median 806 ms > 810 ms, within the noise.testaot run tests5803 passed, 0 failed;testaot native3053 passed, 0 failed.Self-review
11 concerns raised, 9 addressed (the base, the count bounds, negative tests, the
super()andReflect.constructpaths, the measured delta in place of a count of reports, the DataLoader record form). Not done: the heat graph's size measure, a truthful--explaincause for stores of callables that built-ins make.no test proof · iteration 0 · no src or test change; test-proof not applicable