Repository navigation
TypedArray.prototype.with: keep a BigInt array's elements when argument coercion grows its buffer (WebKit bump for oven-sh/WebKit#601) #42082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
1
commit into
main
Choose a base branch
from
robobun/d6812ecb/typed-array-with-bigint-resizable
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
160 changes: 160 additions & 0 deletions
160
test/js/bun/jsc/typed-array-with-resizable-buffer.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| // %TypedArray%.prototype.with(index, value) reads the length first, then | ||
| // coerces `index` and `value` (which can run user code), then copies every | ||
| // element other than `index` from the source. When a coercion grew the buffer | ||
| // under a length-tracking view, JSC's BigInt64Array / BigUint64Array returned | ||
| // a copy in which every element except the replaced one was 0. | ||
|
|
||
| import { describe, expect, test } from "bun:test"; | ||
|
|
||
| type TypedArrayConstructor = | ||
| | Int8ArrayConstructor | ||
| | Uint8ArrayConstructor | ||
| | Uint8ClampedArrayConstructor | ||
| | Int16ArrayConstructor | ||
| | Uint16ArrayConstructor | ||
| | Int32ArrayConstructor | ||
| | Uint32ArrayConstructor | ||
| | Float16ArrayConstructor | ||
| | Float32ArrayConstructor | ||
| | Float64ArrayConstructor | ||
| | BigInt64ArrayConstructor | ||
| | BigUint64ArrayConstructor; | ||
|
|
||
| const constructors: TypedArrayConstructor[] = [ | ||
| Int8Array, | ||
| Uint8Array, | ||
| Uint8ClampedArray, | ||
| Int16Array, | ||
| Uint16Array, | ||
| Int32Array, | ||
| Uint32Array, | ||
| Float16Array, | ||
| Float32Array, | ||
| Float64Array, | ||
| BigInt64Array, | ||
| BigUint64Array, | ||
| ]; | ||
|
|
||
| const isBigInt = (TA: TypedArrayConstructor) => TA === BigInt64Array || TA === BigUint64Array; | ||
| const isFloat = (TA: TypedArrayConstructor) => TA === Float16Array || TA === Float32Array || TA === Float64Array; | ||
| const convert = (TA: TypedArrayConstructor, n: number): any => (isBigInt(TA) ? BigInt(n) : n); | ||
|
|
||
| function makeBuffer(shared: boolean, byteLength: number, maxByteLength: number) { | ||
| return shared ? new SharedArrayBuffer(byteLength, { maxByteLength }) : new ArrayBuffer(byteLength, { maxByteLength }); | ||
| } | ||
|
|
||
| function growTo(buffer: ArrayBuffer | SharedArrayBuffer, byteLength: number) { | ||
| if (buffer instanceof SharedArrayBuffer) buffer.grow(byteLength); | ||
| else buffer.resize(byteLength); | ||
| } | ||
|
|
||
| // A length-tracking view over a fresh 4-element buffer that can grow to 8, | ||
| // filled with 10, 11, 12, 13. | ||
| function setup(TA: TypedArrayConstructor, shared: boolean, byteOffset = 0) { | ||
| const bpe = TA.BYTES_PER_ELEMENT; | ||
| const buffer = makeBuffer(shared, 4 * bpe, 8 * bpe); | ||
| const whole = new TA(buffer as ArrayBuffer); | ||
| for (let i = 0; i < whole.length; i++) whole[i] = convert(TA, 10 + i); | ||
| const view = byteOffset === 0 ? whole : new TA(buffer as ArrayBuffer, byteOffset); | ||
| return { buffer, view, bpe }; | ||
| } | ||
|
|
||
| const elements = (TA: TypedArrayConstructor, values: number[]) => values.map(v => convert(TA, v)); | ||
|
|
||
| for (const shared of [false, true]) { | ||
| const kind = shared ? "growable SharedArrayBuffer" : "resizable ArrayBuffer"; | ||
|
|
||
| describe.each(constructors.map(TA => [TA.name, TA] as const))(`%s.prototype.with over a ${kind}`, (_, TA) => { | ||
| test("keeps the source elements when coercing value grows the buffer", () => { | ||
| const { buffer, view, bpe } = setup(TA, shared); | ||
| let calls = 0; | ||
| const value = { | ||
| valueOf() { | ||
| calls++; | ||
| growTo(buffer, 6 * bpe); | ||
| return convert(TA, 1); | ||
| }, | ||
| }; | ||
| const copy = view.with(0, value as any); | ||
| expect(calls).toBe(1); | ||
| expect(view.length).toBe(6); | ||
| expect(copy).toBeInstanceOf(TA); | ||
| expect(copy.buffer).toBeInstanceOf(ArrayBuffer); | ||
| expect((copy.buffer as ArrayBuffer).resizable).toBe(false); | ||
| expect([...copy]).toEqual(elements(TA, [1, 11, 12, 13])); | ||
| }); | ||
|
|
||
| test("keeps the source elements when coercing index grows the buffer", () => { | ||
| const { buffer, view, bpe } = setup(TA, shared); | ||
| const index = { | ||
| valueOf() { | ||
| growTo(buffer, 8 * bpe); | ||
| return 2; | ||
| }, | ||
| }; | ||
| const copy = view.with(index as any, convert(TA, 1)); | ||
| expect([...copy]).toEqual(elements(TA, [10, 11, 1, 13])); | ||
| }); | ||
|
|
||
| test("negative index, both coercions grow the buffer", () => { | ||
| const { buffer, view, bpe } = setup(TA, shared); | ||
| const index = { | ||
| valueOf() { | ||
| growTo(buffer, 5 * bpe); | ||
| return -1; | ||
| }, | ||
| }; | ||
| const value = { | ||
| valueOf() { | ||
| growTo(buffer, 7 * bpe); | ||
| return convert(TA, 2); | ||
| }, | ||
| }; | ||
| const copy = view.with(index as any, value as any); | ||
| expect([...copy]).toEqual(elements(TA, [10, 11, 12, 2])); | ||
| }); | ||
|
|
||
| test("length-tracking view with a byte offset", () => { | ||
| const { buffer, view, bpe } = setup(TA, shared, TA.BYTES_PER_ELEMENT); | ||
| expect(view.length).toBe(3); | ||
| const value = { | ||
| valueOf() { | ||
| growTo(buffer, 8 * bpe); | ||
| return convert(TA, 3); | ||
| }, | ||
| }; | ||
| const copy = view.with(1, value as any); | ||
| expect(view.length).toBe(7); | ||
| expect([...copy]).toEqual(elements(TA, [11, 3, 13])); | ||
| }); | ||
|
|
||
| // Shrinking inside the coercion, for contrast. This already behaved | ||
| // correctly: elements past the new length read as undefined, so Number | ||
| // arrays store ToNumber(undefined) and BigInt arrays throw on | ||
| // ToBigInt(undefined). A SharedArrayBuffer cannot shrink. | ||
| test.skipIf(shared)("coercing value shrinks the buffer", () => { | ||
| const { buffer, view, bpe } = setup(TA, shared); | ||
| const value = { | ||
| valueOf() { | ||
| (buffer as ArrayBuffer).resize(2 * bpe); | ||
| return convert(TA, 5); | ||
| }, | ||
| }; | ||
| if (isBigInt(TA)) { | ||
| expect(() => view.with(1, value as any)).toThrow(TypeError); | ||
| } else { | ||
| const fill = isFloat(TA) ? NaN : 0; | ||
| expect([...view.with(1, value as any)]).toEqual([10, 5, fill, fill]); | ||
| } | ||
| // The index itself going out of bounds is a RangeError for every type. | ||
| const again = setup(TA, shared); | ||
| const shrinkPastIndex = { | ||
| valueOf() { | ||
| (again.buffer as ArrayBuffer).resize(2 * again.bpe); | ||
| return convert(TA, 5); | ||
| }, | ||
| }; | ||
| expect(() => again.view.with(3, shrinkPastIndex as any)).toThrow(RangeError); | ||
| }); | ||
| }); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 WEBKIT_VERSION is pinned to
autobuild-preview-pr-601-2b6703ca, an ephemeral preview-PR release tag; merging this would leave main pointing at an artifact that disappears once the WebKit PR closes and that carries unmerged upstream commits. Fix: before merge, swap to the merged oven-sh/WebKit commit SHA (or itsautobuild-<sha>tag) and verify prebuilt archives exist for every platform × flavor, per the Dependencies & vendoring rule and .claude/commands/upgrade-webkit.md. The PR description already flags this as a do-not-merge precondition; this comment tracks it as the merge blocker.Extended reasoning...
.claude/docs/landing-prs.md(Dependencies & vendoring) states: "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:328already warns thatautobuild-preview-pr-*releases only exist while the WebKit PR is open, and.claude/commands/upgrade-webkit.md:34says the preview tag must be bumped to the merge-commit'sautobuild-<sha>before merging the bun PR. If this lands as-is and oven-sh/WebKit#601 is later closed/force-pushed, fresh checkouts and CI will fail to fetch WebKit prebuilts. The base branch pinned a merged SHA (2e2aa2290fac…), so this is a regression the diff introduces. The author notes the same in the PR body; per the vendoring rule it is still a hard merge blocker to track.Verification: normal — acknowledged in diff: the PR description states "The pin is a preview tag, not a merge commit. Do not merge before oven-sh/WebKit#601 lands. I move the pin to the merged commit then." That claim is accurate but does not resolve the hazard; the pin must be swapped before merge. scripts/build/deps/webkit.ts:6 changes
WEBKIT_VERSIONfrom the merged SHA… | normal — acknowledged in diff:…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and intended. The preview tag is only here so CI can run the new test against the engine fix before oven-sh/WebKit#601 merges. Once it lands I push a commit that sets
WEBKIT_VERSIONto the merged 40-character SHA and check that theautobuild-<sha>release carries all prebuilt archives, then resolve this thread. Leaving it open until then as the merge blocker.