Repository navigation
Conversation
…ray cannot hold matches nothing (oven-sh/WebKit#604) Pin WebKit to the preview build of oven-sh/WebKit#604 and add a test. Float32Array / Float16Array indexOf, lastIndexOf and includes matched the element an int32 needle rounds to (16777217 found 16777216, 65520 found Infinity in a Float16Array, 2049 found 2048). The double-encoded needle path already rejected a value the element type cannot represent.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe PR switches the selected WebKit build to an autobuild preview and adds regression tests for floating-point typed-array search methods. ChangesTyped-array regression coverage
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This updates the WebKit dependency and adds regression coverage for floating-point typed-array search behavior. No merge-blocking risk is currently identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduced on bun 1.4.3 (Linux x64) with: const f32 = new Float32Array([16777216, 2147483648]);
console.log(f32.indexOf(16777217), f32.includes(2147483647), f32.indexOf(16777216.5 + 0.5)); // bun: 0 true 0 node v26.3.0: -1 false -1
const f16 = new Float16Array([Infinity, 2048, 65504]);
console.log(f16.indexOf(65520), f16.indexOf(2049), f16.includes(65505), f16.lastIndexOf(2 ** 31 - 1)); // bun: 0 1 true 0 node: -1 -1 false -1
The engine fix is oven-sh/WebKit#604. This PR carries the test and the preview pin, and stays blocked until that PR merges. Then I move the pin to the The first CI run (build 113311) failed on every build lane with The automated review raised one point: the preview pin must not merge as is. That is already the plan above. The thread stays open as the merge blocker until oven-sh/WebKit#604 merges and the pin moves. No code change from the review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "2e2aa2290fac856d6f451ceacb58f7f5b44dd057"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-604-a9c83095"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-604-a9c83095, an ephemeral preview release that is deleted once oven-sh/WebKit#604 merges or closes — after that every fresh build/CI without a cached tarball fails at the WebKit fetch (download.ts:323-332 already special-cases this exact failure). Fix: before merge, replace with the 40-hex merge-commit SHA and confirm the autobuild-<sha> release has every {os,arch,musl,debug|lto,asan} artifact prebuiltSuffix can request. The PR description already says "Do not merge before oven-sh/WebKit#604 lands"; this comment is the merge blocker that enforces it.
Extended reasoning...
REVIEW.md → Dependencies & vendoring: "Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge." scripts/build/download.ts:323-332 documents that autobuild-preview-pr-* releases "only exist while the WebKit PR is open". If this PR merges as-is and the WebKit PR later merges (deleting the preview release), any dev or CI agent without webkit-preview-pr-604-a9c83095* already in cfg.cacheDir hits a 404 at prebuiltUrl() and the build aborts. On the base branch (2e2aa22…, a merged-commit SHA) the release is permanent and this cannot happen. The author's description acknowledges the hazard but the pin still needs to be swapped before merge — the description is not a code guard.
Verification: normal — acknowledged in diff: the PR description states "The pin is a preview tag. Do not merge before oven-sh/WebKit#604 lands. I move the pin to the autobuild-<sha> release of the merge commit then", which accurately describes the hazard but does not resolve it; the pin as committed is still ephemeral. scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from the 40-hex SHA… | normal —…
There was a problem hiding this comment.
Agreed, and that is the plan stated in the PR body. This pin is only here so CI can run the test against the engine fix before oven-sh/WebKit#604 merges. When it merges I push the 40-hex merge-commit sha here, after checking that its autobuild-<sha> release carries all 42 artifacts. I leave this thread open until then as the merge blocker.
…ows from #42083 and #42089 Adds the rows those two PRs pinned that this file did not have yet: negative int32 needles and arithmetic results on Float32Array, the 65505..65519 and 65535 needles on Float16Array, more wrapped BigInt needles (negative many-digit, -(2^64) +/- 1, -(2^63) - 1 on BigUint64Array), and the same searches through a subarray, a resizable buffer, a shared buffer, and with a fromIndex.
|
Superseded by #42093, which pins the oven-sh/WebKit#609 preview. That change includes this fix (the |
2 participants
Problem
Float32Array/Float16ArrayindexOf,lastIndexOfandincludesmatch the element an integer needle rounds to when the element type cannot hold it:new Float32Array([16777216]).indexOf(16777217)is0andnew Float16Array([Infinity]).includes(65520)istrue. V8 and the spec (IsStrictlyEqual / SameValueZero on Numbers) say -1 / false.parseInt, integer arithmetic). The same value read from aFloat64Arrayalready missed.FloatTypedArrayAdaptor::toNativeFromInt32WithoutCoercionandFloat16Adaptor::toNativeFromInt32WithoutCoercionin JavaScriptCore'sruntime/TypedArrayAdaptors.hwere a barestatic_cast. The double path checks that the conversion round-trips. The int32 path did not.Fix
autobuild-preview-pr-604-a9c83095, the preview build of [JSC] TypedArray indexOf/lastIndexOf/includes: an int32 needle that a Float16Array or Float32Array cannot hold exactly matches nothing WebKit#604, which adds the round-trip check to both int32 conversions.Float64Arrayis unaffected: every int32 is a double.autobuild-<sha>release then. The preview is built on WebKitmain10350ddde9, three commits ahead of the current pin.test/js/bun/jsc/typed-array-indexof.test.tsfails 3 of 5 tests on 1.4.3 (the double-needle andFloat64Arraycontrols pass) and passes on a debug build with this pin. JSC-side runs are in the notes.Background
Notes
f32ids.includes(id)is true for a neighbouring id once ids pass 2^24 (2048 for Float16), andf16.indexOf(65520)returns the position of an Infinity.jscshell built from the branch: the newJSTests/stress/typed-array-index-of-int32-needle-not-representable.js(fails on the current pin's shell at the first case), the existingtypedarray-indexOf.js,typedarray-includes.js,typedarray-lastIndexOf.js,non-suitable-typed-array-index-of.jsstress files, and the 130 test262 files underbuilt-ins/TypedArray/prototype/{indexOf,includes,lastIndexOf}(130 pass before and after, test262 has no case for this).mainhas the same two functions.toNativeFromValueWithoutCoercionare the three search builtins inJSGenericTypedArrayViewPrototypeFunctions.h, and each already maps "not representable" to not found (with the existing special case for anundefinedneedle inincludes). There is no DFG/FTL intrinsic for typed array search, only forArray.prototype.new Float16Array([-1]).indexOf(-1)and every other negative finite half. That is a V8 bug. JSC finds them before and after this change, and the new test keeps a-2048case.Uint8ClampedArraydouble path (#45441) and the BigInt path (#45461). They are separate functions and not touched here.[policy-decision:webkit] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file