Skip to content

node:buffer: define Buffer.of as an own static that never reads this - #45865

Open
robobun wants to merge 9 commits into
mainfrom
robobun/a0a8b7bd/buffer-of-own-static
Open

robobun wants to merge 9 commits into
mainfrom
robobun/a0a8b7bd/buffer-of-own-static

Conversation

@robobun

@robobun robobun commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Add the row of jsBufferConstructorFunction_of Function 0 and a host function. It stores each argument in a new Buffer and never reads this, like Node's arrow function (lib/buffer.js).
  • For --aot builds, list of/wc in own-properties.txt and declare the function in buffer.builtins.ts.
  • Verified: test/js/node/buffer.test.js (19 new tests, 10 fail on bun 1.4.3) and one new test/aot program. Adds no passing Node tests.
  • Self-reviewed: 13 concerns raised, 13 addressed, except a tracker issue for statics no user reported.

Background

  • The table holds Buffer's statics. JSC makes a row a real property on its first read.
  • bun build --compile --aot reads a table of built-ins. own-properties.txt lists each object's own properties, and buffer.builtins.ts declares what each Buffer static does.
  • Considered a JS builtin (a new Buffer(n) per call) and an eager put in finishCreation (the only static outside the table).

Downsides

  • Behaviour change, as in Node: Sub.of() runs no subclass constructor, and Buffer.of.call(Uint16Array, 1) returns a Buffer.
  • Once per realm, the first read of Buffer.of can make JSC recompile optimized Buffer callers. A probe: +28.9M instructions (main: 0).
  • Binary: .text +256 B, file +4,096 B.
Notes

Repro

Each line ran on Node v26.3.0, on bun main (017b243) and on this PR. TypeError stands for TypeError: TypedArray.of requires |this| to be a constructor.

Expression Node v26.3.0 main this PR
const { of } = Buffer; of(1, 2) <Buffer 01 02> TypeError <Buffer 01 02>
[65, 66].map(Buffer.of) [ <Buffer 41 00 00>, <Buffer 42 01 00> ] TypeError same as Node
await Promise.resolve(65).then(Buffer.of) <Buffer 41> TypeError <Buffer 41>
Reflect.apply(Buffer.of, undefined, [1, 2, 3]) <Buffer 01 02 03> TypeError <Buffer 01 02 03>
Object.hasOwn(Buffer, "of") true false true
Object.getOwnPropertyDescriptor(Buffer, "of") { value: [Function: of], writable: true, enumerable: true, configurable: true } undefined same as Node
Buffer.of === Uint8Array.of false true false
Buffer.of.call(Uint8Array, 1, 2) <Buffer 01 02> Uint8Array(2) [ 1, 2 ] <Buffer 01 02>
constructor calls of class Sub extends Buffer in Sub.of(1, 2) 0 1 0
require("safer-buffer").Buffer.of(1, 2) <Buffer 01 02> TypeError: safer.Buffer.of is not a function <Buffer 01 02>

Who this reaches

  • The tracker has no issue for it.
  • No code file in the 1,304 package versions under test/node_modules/.bun references Buffer.of.
  • safer-buffer@2.1.2 is the one concrete carrier found. Its safer.js copies the own enumerable statics of Buffer to a plain object. That copy needs an own enumerable of that does not read this.

Why Node has its own of

Node PR 19682 (buffer: do not emit deprecation notice on Buffer.of) added it. The purpose was to keep Buffer.of away from the deprecated Buffer() constructor (DEP0005). Bun does not emit DEP0005. The basis for this PR is the behaviour that Node shows: an own enumerable property, and a function that ignores the receiver.

Behaviour changes

Each one now matches Node v26.3.0.

  • Sub.of(...) does not run the constructor of a Buffer subclass. If that constructor returns instances of the subclass, Sub.of() gave such an instance before. It gives a plain Buffer now.
  • Buffer.of.call(X, ...) returns a Buffer for each X. Before, it constructed through X.
  • Object.keys(Buffer), for-in and util.inspect(Buffer) show of.
  • Buffer.of !== Uint8Array.of. A patch of Uint8Array.of does not change Buffer.of.
  • A program built with bun build --compile --aot locks every own member of Buffer. There, a store to Buffer.of or a delete of it throws, as for Buffer.from. Before, of was inherited and the delete returned true.

Not changed

Ahead-of-time builds

Main added bun build --compile --aot and its table of built-ins in #45631, after this PR opened. #47129 then moved the AOT programs out of test/aot/programs, and #47942 brought the rewritten compiler. The branch contains a merge of main (2596ac5).

  • src/codegen/builtins/data/own-properties.txt lists the own properties of each built-in object. The generator of the table holds each row of a native property table against it. Without of/wc on the Buffer line, the build stops with Buffer.of is declared and own-properties.txt does not have it. test/aot/builtins/own-properties.test.ts compares the list with the Buffer of the built binary.
  • src/codegen/builtins/host/buffer.builtins.ts declares each static of Buffer. The new entry says that Buffer.of takes any number of operands, runs only their ToPrimitive, can throw, and returns a fresh Buffer. Without it, the row gets the overload of an undeclared row (any result, any use of the operands), and the build lists Buffer.of: the result of Call is anything. The generated table differs in this one entry.
  • The differential check of the declared overload passed on a release build at the base 11c3671. It failed there for a table that says no ToPrimitive runs, and for a table that says the result is not a Buffer. That check is test/aot/builtins/differential.test.ts now (AOT_BUILTINS_FILTER='^Buffer\.of$').
  • The program about locked members had of among the inherited keys of Buffer. It now has of among the members. After AOT: fixes for modules, classes and Workers, WebKit upgrade, shorter compiler docs #47129 it is the IR test test/aot/ir/lower/programs/language/typedarray-buffer-classes-keep-members.js.
  • The new program test/aot/parallel/builtin-modules/buffer-of-with-every-sort-of-operand-and-receiver.js calls Buffer.of with each sort of operand and receiver, in a loop too. It prints the same under bun <file>, under --compile --aot and under Node v26.3.0. On a release build of main at 11c3671 the program failed: the run under bun <file> stops at the detached call.
  • The self-review covered the diff before the first merge of main. The two entries for --aot came after it. The declaration answers a review comment on this PR.
  • The merge of main 2596ac5 had one conflict, in own-properties.txt. AOT: front end, test runs and tests for the rewritten compiler #47942 added the line BroadcastChannel.prototype open directly above the Buffer line. The merge keeps both lines. AOT_BUILTINS_UPDATE=1 on own-properties.test.ts leaves the file as it is. The diff of this PR against main has the same lines as before that merge.
  • On that merge, a debug build with ASAN (Linux x64) gives these results. test/aot/builtins/own-properties.test.ts: 16 pass. The IR test passes with the check of the engine (TESTAOT, AOT_IR_FILTER=typedarray-buffer-classes-keep-members cargo test -p bun_aot --test ir_tests). test/aot/run-parallel.ts passes the new program as written and with its types erased. The differential check of BufferOf/0 passes (AOT_BUILTINS_UPDATE=1 AOT_BUILTINS_FILTER='^Buffer\.of$'), and it fails for a table that says no ToPrimitive runs.
  • AOT: front end, test runs and tests for the rewritten compiler #47942 added two programs that call Buffer.of, in test/aot/parallel/classes-of-views-that-are-only-constructed/. On main that call reaches %TypedArray%.of with Buffer as the receiver. With this PR it reaches the row Buffer.of. Both programs pass in both ways. views-made-by-from-and-of-are-printed-and-bring-little-of-the-library.mjs counts 8 for ImageFunctionsEmitted, and its bound is 24. This PR does not edit the two programs.
  • Not run on that merge: a release build. In test/internal/builtins-table (119 pass, 1 skip, 7 fail on the debug build), 4 tests run the generator of the table with the Bun under test and reach their limit of 300 s. The other 3 failures follow from those. The build runs the same generator against the prebuilt engine, and that step passes.
  • A release build of this branch at the base 11c3671 gave these results. 8 test files of test/aot/builtins: 91 pass, 3 todo, 0 fail. test/internal/builtins-table: 133 pass, 1 skip.

Measurements

Release builds on linux x64: main 017b243 (before #45631), and that commit with the src/jsc change of this PR. The binary size on the current base is at the end of this list. perf, valgrind, bloaty and strace are not installed in the build container. The instruction counts come from a perf_event_open counter (PERF_COUNT_HW_INSTRUCTIONS, user space only, child threads included).

  • Binary (nm -S, size -A): jsBufferConstructorFunction_of is 446 B. jsBufferConstructorTableValues goes from 400 to 440 B. jsBufferConstructorTableIndex stays at 136 B. .text goes from 81,368,037 to 81,368,293 B (+256). .rodata does not change. The stripped file stays at 106,911,456 B.
  • Generated JSBuffer.lut.h: Values[10] becomes Values[11], Index[34] stays. One empty index slot now holds the row. No existing slot moves.
  • Cells per realm (bun:jsc heapStats(), 3 identical runs): the first touch of Buffer adds 133 Function and 125 NativeExecutable cells on both builds. The first read of Buffer.of adds 1 and 1 (main: 0 and 0).
  • Instructions per Buffer.of(...) method call (slope from 200k to 1.2M iterations, empty loop subtracted, median of 3 runs):
Arguments JIT off, main to PR JIT on, main to PR
0 794 to 378 618 to 204
3 1,018 to 589 811 to 385
8 1,362 to 929 1,105 to 674
64 5,242 to 4,754 4,415 to 3,930
5000 363,061 to 357,464 306,852 to 301,381
  • Controls with the same method, equal on both builds within 1 instruction: Buffer.isEncoding("utf8") 304, Buffer.alloc(3) 506, new Buffer(3) 381, Uint8Array.of(1, 8, 15) 624 (JIT off). With the JIT: 132, 322, 269 and 417.
  • Native entries per Buffer.of(1, 2, 3) call (gdb breakpoint counts over 1000 calls): Interpreter::executeConstruct 1 to 0, createJSBufferFromJS 1 to 0, typed-array allocations 1 to 1. A single-step of the host call: 774 to 348 instructions, 3 to 0 indirect calls, 13 to 0 instructions in memset.
  • Startup, empty script, 7 interleaved runs: the median is 3,530,885 instructions on main and 3,528,263 on the PR. The spread on main is 19,197.
  • Late first read. The probe optimizes a function that calls Buffer.alloc, Buffer.isBuffer and Buffer.byteLength. Then it reads one more static for the first time and runs the function again. Each phase has 2e6 calls. The JIT is not concurrent, and each number is the median of 5 runs. With Buffer.of as the late read, the function has 1 DFG compile on main and 2 on this PR, for +28.9M instructions. With Buffer.concat as the late read, it has 2 on both builds, for +27.9M on main and +28.0M on this PR. The run-to-run spread is up to 3.6M.
  • Binary on the first base that has the AOT compiler (release builds, main 11c3671 against 3ba4afe): .text goes from 108,646,665 to 108,646,921 B (+256). .rodata, .data and .bss do not change. The stripped file goes from 138,193,048 to 138,197,144 B (+4,096, one page). On the first base the file size did not change.

Tests

  • describe("Buffer.of") has 19 tests. On bun 1.4.3-canary (367d939), 10 fail and 9 pass. The 9 guard behaviour that must not change: the conversion order, a throw from valueOf, 0 to 5000 arguments, and a GC inside valueOf.
  • The same block runs under Node v26.3.0 through a small bun:test shim: 19 pass.
  • On the current merge with main (2596ac5), buffer.test.js has 739 pass, 1 skip, 0 fail on a debug build with ASAN. Before the first merge of main, the debug build with ASAN had 737 pass, 1 skip, 0 fail (main at that base: 718 pass). With BUN_JSC_validateExceptionChecks=1, the Buffer.of tests report no unchecked scope. A build without the check after setIndex aborts under that flag with Unchecked JS exception.
  • The three vendored Node tests that call Buffer.of pass on main and on this PR: test-buffer-of-no-deprecation.js, test-buffer-copy.js, test-string-decoder-end.js. string-decoder.test.js has 143 pass.

The uninitialized allocation

The host function allocates the buffer without a zero fill. JS cannot reach the buffer before the return, so the loop writes every byte or a throw drops the buffer. JSC's fast path for Uint8Array.of does the same.

Alternatives weighed

  • A JS builtin with Node's source. It constructs through new Buffer(n) on each call, which is the zero-filled path that this PR takes out of Buffer.of.
  • An eager put in JSBufferConstructor::finishCreation. Each realm that creates Buffer allocates the function, and of becomes the only static outside the table. It removes the late first read cost for of only. The other ten rows keep that cost.
  • A zero-filled allocation. The loop writes every byte, so the zero fill is a second pass. JSC's fast path for Uint8Array.of (typedArrayOfFast) also allocates uninitialized.
  • A change to %TypedArray%.of in JSC. That function has the behaviour that the specification gives it.

Related


no test proof · iteration 8 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/buffer.test.js

Buffer had no own `of`, so `Buffer.of` was the inherited %TypedArray%.of.
That function constructs through `this`. A detached call threw
"TypedArray.of requires |this| to be a constructor",
`Buffer.of.call(Uint8Array, ...)` returned a Uint8Array, and a subclass
ran its constructor.

Node defines its own `of` as an arrow function (lib/buffer.js). Add one
row to jsBufferConstructorTable and a host function that allocates a
Buffer of argumentCount bytes and stores each argument in order.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The Buffer constructor now exposes Buffer.of. The method creates a buffer with one byte per argument and assigns each argument by index. Tests cover method properties, call forms, conversion behavior, exceptions, and larger argument counts.

Changes

Buffer.of

Layer / File(s) Summary
Implement and register Buffer.of
src/jsc/bindings/JSBuffer.cpp, src/codegen/builtins/host/buffer.builtins.ts, src/codegen/builtins/data/own-properties.txt
The method allocates a buffer sized to the argument count, assigns each argument by index, and propagates allocation or assignment exceptions. The built-in declaration and property list include Buffer.of. The constructor lookup table registers the method with declared arity 0.
Validate Buffer.of behavior
test/js/node/buffer.test.js, test/aot/ir/lower/programs/language/typedarray-buffer-classes-keep-members.js, test/aot/parallel/builtin-modules/buffer-of-with-every-sort-of-operand-and-receiver.js
Tests cover method properties and call forms, argument conversion and exceptions, argument counts through 5000, and buffer contents when conversion triggers garbage collection. AOT coverage exercises varied operands and receivers. The member test now treats of as a Buffer member.

Suggested reviewers: dylan-conway, jarred-sumner

Merge Risk: Minimal · up to 6b3e5

Buffer.of adds receiver-independent calls with runtime and AOT coverage. No actionable merge-blocking issue remains; the change is mergeable subject to normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely identifies the primary change: adding an own static Buffer.of implementation that ignores its receiver.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The "What does this PR do?" content is covered under …
Full details: Description check

Explanation

The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The "What does this PR do?" content is covered under Problem, Fix, and Background. It states the cause, the fix, and the files touched. The "How did you verify your code works?" content is covered under Verified, Tests, Measurements, and AOT sections. It lists test counts, the Node v26.3.0 comparison, and a debug and release build run on Linux x64. The description is long, and the Notes block is dense. Its core content is complete, so the check passes. One gap remains: the evidence note says test/js/node/buffer.test.js did not run on the reporting machine, and validation is deferred to CI. That limits the verification claim for the reported test runs, but the author documents the local results that were run.

Finishing Touches

Fix failing CI checks

  • Commit to this branch
  • Create a new PR
  • 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

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 7574761 and 6b3e5b5.

Files selected for processing: 6

  • src/codegen/builtins/data/own-properties.txt
  • src/codegen/builtins/host/buffer.builtins.ts
  • src/jsc/bindings/JSBuffer.cpp
  • test/aot/ir/lower/programs/language/typedarray-buffer-classes-keep-members.js
  • test/aot/parallel/builtin-modules/buffer-of-with-every-sort-of-operand-and-receiver.js
  • test/js/node/buffer.test.js

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

@robobun

robobun commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on main (017b243) and on Bun 1.4.3 with:

const { of } = Buffer;
console.log(of(1, 2));
// main:          TypeError: TypedArray.of requires |this| to be a constructor
// this PR:       <Buffer 01 02>
// Node v26.3.0:  <Buffer 01 02>

Promise.resolve(65).then(Buffer.of) and [65, 66].map(Buffer.of) fail in the same way. describe("Buffer.of") in test/js/node/buffer.test.js has these cases: 10 of its 19 tests fail on Bun 1.4.3, and all pass with this change. The same block passes under Node.js v26.3.0.

The branch contains main 2596ac5 (head 367f42f) and has no conflict with it.

CI on 367f42f (build 138891): 183 of the 184 jobs that ran pass. Every build job, cargo-test, verify-baseline, and each test-bun job on Windows and on macOS aarch64 pass. On Linux, 140 of the 141 test-bun jobs pass. On the debian 13 x64-asan lane, buffer.test.js has 738 pass, 2 skip, 0 fail, and the new test/aot program passes. On the Linux x64 release lanes, test/aot/builtins/differential.test.ts and test/aot/builtins/own-properties.test.ts pass.

The one failed job is a test shard of debian 13 x64-asan. It fails in one program: test/aot/parallel/modules/a-data-file-required-before-the-loader-comes-to-its-import/main.mjs. #47942 added that program. It does not use Buffer, and this PR does not change it. Build 138854 of an unrelated branch has the same failure with the same output. So do the builds 139384, 139424, 139451 and 139457 of four more branches. No agent took the 4 macOS x64 test-bun jobs. They expired in the queue four times, and the build ended as failed. macOS x64 last ran for this PR in build 128967, which passed in full. The changes to JSBuffer.cpp and buffer.test.js are the same there.

The diff is ready for review. For a green status, retry the failed shard and the 4 expired jobs.

Pull request: #45865

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — a small, self-contained addition that matches Node's Buffer.of and follows the existing static-table pattern in this file.

What was reviewed:

  • The new host function in src/jsc/bindings/JSBuffer.cpp: throw scope is declared up front, RETURN_IF_EXCEPTION follows both the allocation and each setIndex, and uncheckedArgument(i) is bounded by argumentCount().
  • Reentrant valueOf/toString during setIndex: the buffer is unreachable from JS until return, and JSC's setIndex coerces first and re-checks detached/bounds afterwards, so user code cannot corrupt the write.
  • Realm handling: createUninitializedBuffer resolves the structure through defaultGlobalObject, the same as every sibling static (alloc, concat, ...), so this does not introduce a new inconsistency.
  • The test block imports vm from the existing file header, covers receiver variants, coercion order, throw propagation, the 1000/1001 inline-storage boundary, and GC inside valueOf.
Extended reasoning...

The diff adds one 15-line host function and one lookup-table row to src/jsc/bindings/JSBuffer.cpp, plus a new describe block in test/js/node/buffer.test.js; nothing else changes. It touches no security-sensitive surface (no parsing of untrusted sizes beyond the argument count, no auth or crypto). The function mirrors the structure of the neighbouring statics, has exception checks at every point that can enter user JS, and the behaviour change is deliberately aligned with Node and documented in the PR. No CODEOWNERS entry covers the changed files, the hunt ran dry with no findings, and there are no third-party objections in the timeline.

@robobun

robobun commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:05 PM PT - Nov 8th, 2026

❌ @robobun, your commit 367f42f has 1 failures in Build #138891 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 45865

That installs a local version of the PR into your bun-45865 executable, so you can run:

bun-45865 --bun

The table of built-ins holds each row of a native property table
against src/codegen/builtins/data/own-properties.txt. This branch adds
the row `of` to the Buffer constructor, so the list gets `of/wc`
(writable and configurable), like the other functions of that table.
@robobun

robobun commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main (11c3671) into this branch. The merge has no conflict, but the merged tree did not build. The step that generates the table of built-ins stopped:

error: src/jsc/bindings/JSBuffer.cpp:3797: Buffer.of is declared and own-properties.txt does not have it
  • Main added that step in 8d5672c (Compile TypeScript to machine code ahead of time: bun build --compile --aot #45631). It holds each row of a native property table against src/codegen/builtins/data/own-properties.txt.
  • This PR adds the row of to the table of the Buffer constructor. Commit 20744b6 adds of/wc (writable, configurable) to the Buffer line of the list. That is the only edit.
  • The step fails on the merged tree without the edit and passes with it.
  • Tests, on a local debug build (Linux x64) that contains this merge and the main merges of five other open PRs:
    • test/js/node/buffer.test.js: 739 pass, 1 skip. In the first of three runs one case reached the 5 s limit. It did not repeat.
    • test/aot/builtins/own-properties.test.ts: 17 pass. It holds the list against the properties and the attributes of the Buffer that was built.
    • realm-agreement, module-exports, name-classes, table-regressions and proven-returns in test/aot/builtins: 34 pass, 3 todo.
  • A program built with bun build --compile --aot prints the same as under bun run for const { of } = Buffer; of(1, 2) and for Buffer.of(3, 4, 5).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/jsc/bindings/JSBuffer.cpp Resolved
…d classes

test/aot/programs/language/typed-array-and-buffer-classes-keep-their-members
listed `of` among the keys that Buffer inherits and is closed for. This
branch makes it an own function of Buffer, so a compiled program locks
it like `from`: a `delete` throws where it returned true before. The
program lists `of` as a member, and the stored output follows.
@robobun

robobun commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

One more edit after the merge of main: a program of test/aot had Buffer.of on record as an inherited key.

  • CI build 134236 showed it on Debian 13 aarch64, in test/aot/language-1.test.ts and test/aot/every-program-14.test.ts: language/typed-array-and-buffer-classes-keep-their-members printed Buffer: a key it is closed for: of on a line of its own, with delete TypeError/unchanged where the stored output has delete true/unchanged.
  • The cause is this PR, and the new output is the right one. A compiled program locks every own member of Buffer. of was inherited from %TypedArray%, so a delete Buffer.of returned true and changed nothing. Now of is an own function, and a delete throws, as for Buffer.from.
  • Commit 71a1c5a moves "of" from the inherited keys of Buffer to its members in the program, and rewrites the two lines of typed-array-and-buffer-classes-keep-their-members.expected.txt (AOT_PROGRAMS_UPDATE=1). The program passes on a local debug build with this PR (AOT_PROGRAMS=language/typed-array-and-buffer-classes-keep-their-members TEST_PART=aot/language bun test test/aot/parts.test.ts).
  • The same build 134236 is red for another reason that this PR does not cause. The agents of eight Linux steps were not created (CapacityLimitExceeded from EC2), among them test-aot and test-bun on Debian 13. Those steps did not run.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

The row `of` of jsBufferConstructorTable had no declaration in
buffer.builtins.ts. The table of built-ins gave it the overload of an
undeclared row: any result, and any use of its operands.

Declare what the host function does. It takes any number of operands,
runs only their ToPrimitive, and answers a fresh Buffer. The other ten
statics of that table already have a declaration.

Add a program under test/aot that calls Buffer.of with each sort of
operand and receiver.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the latest commit adds the Buffer.of declaration to buffer.builtins.ts, which closes the one item left open from the earlier review.

What was reviewed:

  • jsBufferConstructorFunction_of in src/jsc/bindings/JSBuffer.cpp: RETURN_IF_EXCEPTION follows the allocation and every setIndex (each can run user valueOf/toPrimitive); the fresh buffer is unreachable from JS until the return, so a mid-loop throw just drops it, and the stack-held cell is rooted the same way as the sibling JSBuffer__bufferFromLengthAsArray path.
  • The new of entry in buffer.builtins.ts mirrors the alloc/concat "js" overloads (newBuffer, throws, runsOnlyOperandCode, converted), and b.any.rest is the form used by the other variadic declarations; own-properties.txt and the AOT locked-members program/expected output were updated consistently.
  • Tests cover receiver independence, detached/callback use, subclass and Proxy non-construction, conversion order with a reentrant valueOf, first-throw semantics (Symbol/BigInt TypeError), the 1000/1001 inline-storage boundary, and a Bun.gc(true) stress inside valueOf.
Extended reasoning...

The change adds an own of static to Node's Buffer constructor: one ~20-line JSC host function and a LUT row in src/jsc/bindings/JSBuffer.cpp, a matching declaration in the AOT builtins table and own-properties list under src/codegen/builtins/, plus 19 new bun:test cases and an AOT program. It touches no security-sensitive surface (no auth, crypto, parsing of untrusted bytes beyond ToNumber coercion of call arguments). The C++ follows the file's existing allocate-then-fill pattern with exception checks after each call that can enter JS, the behaviour change (Sub.of and Buffer.of.call(X) no longer construct through the receiver) is a deliberate match of Node's arrow-function implementation that the tests pin down, no CODEOWNERS entry covers the changed paths, and the only prior review item was addressed by the final commit.

main moved the AOT programs out of test/aot/programs. The Buffer.of
program is now a file of its own under test/aot/parallel/builtin-modules,
and the program about locked members is the IR test
test/aot/ir/lower/programs/language/typedarray-buffer-classes-keep-members.js.
@robobun

robobun commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main (3a9ea00) into this branch. The PR merges clean again.

  • AOT: fixes for modules, classes and Workers, WebKit upgrade, shorter compiler docs #47129 moved the AOT programs out of test/aot/programs, so the three files that this PR edited there are gone. The Buffer.of program is now a file of its own: test/aot/parallel/builtin-modules/buffer-of-with-every-sort-of-operand-and-receiver.js. Its sidecar .json is gone, because the new runner reads no such file.
  • The program about locked members is now the IR test test/aot/ir/lower/programs/language/typedarray-buffer-classes-keep-members.js. Its source lists of as a member of Buffer, as this PR did before. The stored output no longer exists on main.
  • On a debug build of this merge: test/js/node/buffer.test.js (739 pass), test/aot/builtins/own-properties.test.ts (17 pass) and test/internal/builtins-table/mutability.test.ts (4 pass). The new program, built with bun build --compile --aot, prints the same as plain bun.
  • Not run: the IR test. cargo test -p bun_aot --test ir_tests does not compile at 3a9ea00, and Fix the build of bun_bundler with the test_support feature #47299 fixes that on main. The edit moves one string inside the module code, and the check lines of that file do not cover the module code.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the earlier nit is addressed: of is now declared in src/codegen/builtins/host/buffer.builtins.ts alongside the LUT row, so the AOT table has one source of truth for the Buffer statics.

What was reviewed:

  • jsBufferConstructorFunction_of: RETURN_IF_EXCEPTION after createUninitializedBuffer and after every setIndex (which runs ToPrimitive/ToNumber before its own bounds check); the buffer is a stack local never exposed to JS before return, so a throw drops it fully and GC during valueOf is covered by the Bun.gc(true) test.
  • The four mirrored sites (LUT row, own-properties.txt of/wc, buffer.builtins.ts entry, AOT lower-program member list) agree on an enumerable/writable/configurable of with length 0.
  • Tests assert exact bytes, prototype, byteOffset and ArrayBuffer length via toEqual, cover receivers, subclass/Proxy non-construction, argument ordering and throw-stop, and the 1000/1001 in-heap storage boundary.
Extended reasoning...

The diff adds a 15-line host function and LUT row in src/jsc/bindings/JSBuffer.cpp, one entry each in the AOT builtins declaration and own-properties table, a one-line update to an AOT program expectation, a new AOT program, and ~150 lines of tests in test/js/node/buffer.test.js. It touches no auth, crypto, parsing of external data or security-sensitive surface; the only native code is a bounded loop over call-frame arguments with exception checks after each call that can enter JS. The change is small, follows the file's existing createUninitializedBuffer/RETURN_IF_EXCEPTION conventions, has no CODEOWNERS entry, and the single open item from the prior review is resolved in the code itself; the hunt ended on a dry streak with no findings.

@robobun

robobun commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

The IR test that was not run above passes now: AOT_IR_FILTER=typedarray-buffer-classes-keep-members cargo test -p bun_aot --test ir_tests, on this branch merged with main 57f4f19 (87 passed, 0 failed). test/aot/builtins/own-properties.test.ts also passes there (17 tests). The branch has no conflict with main 000ef50.

main has two changes that the previous merge base lacked: the build of
bun_bundler with the test_support feature (#47299), and the scan behind
compile-trim-capabilities.test.ts (#47326). The cargo-test step and two
test-bun jobs failed without them.
One conflict, in src/codegen/builtins/data/own-properties.txt. main
(#47942) added the line `BroadcastChannel.prototype open` directly above
the `Buffer` line, and this branch adds `of/wc` to the `Buffer` line.
The merge keeps both.
@robobun

robobun commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main (2596ac5) into this branch. The PR has no conflict with main again.

  • The merge had one conflict, in src/codegen/builtins/data/own-properties.txt. AOT: front end, test runs and tests for the rewritten compiler #47942 added the line BroadcastChannel.prototype open directly above the Buffer line, and this PR adds of/wc to the Buffer line. The merge keeps both lines. AOT_BUILTINS_UPDATE=1 on test/aot/builtins/own-properties.test.ts leaves the file as it is.
  • The diff against main has the same 6 files and the same changed lines as before the merge.
  • Tests on a debug build with ASAN of this merge (Linux x64):
    • test/js/node/buffer.test.js: 739 pass, 1 skip.
    • test/aot/builtins/own-properties.test.ts: 16 pass.
    • The IR test typedarray-buffer-classes-keep-members passes with the check of the engine (TESTAOT).
    • The differential check of the row Buffer.of (BufferOf/0) passes. It fails for a table that says no ToPrimitive runs.
  • AOT: front end, test runs and tests for the rewritten compiler #47942 added two programs that call Buffer.of, in test/aot/parallel/classes-of-views-that-are-only-constructed/. On main that call reaches %TypedArray%.of with Buffer as the receiver. With this PR it reaches the row Buffer.of. Both programs pass under test/aot/run-parallel.ts, as written and with their types erased, and so does the program of this PR. views-made-by-from-and-of-are-printed-and-bring-little-of-the-library.mjs counts 8 for ImageFunctionsEmitted, and its bound is 24. This PR does not edit the two programs.
  • Not run on this merge: a release build.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant