Skip to content

Built-in modules: bind the engine's Date functions through @natives, not the Date found at first run - #47058

Open
robobun wants to merge 6 commits into
mainfrom
robobun/1d9779e2/date-builtins-first-load
Open

robobun wants to merge 6 commits into
mainfrom
robobun/1d9779e2/date-builtins-first-load

Conversation

@robobun

@robobun robobun commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Six built-in modules keep a Date mock that is on when they first run. After the restore, util.inspect(date) throws TypeError: DatePrototypeGetTime.@call is not a function, and a keep-alive node:http server dies with DateNow is not a function.
  • Cause: nine module-scope statements such as const DateNow = Date.now (src/js/node/v8.ts:34). Built-in modules first run after user code. Node.js is not affected.
  • Since 1.4.1 (Defer module-init work in node:fs, util, http, net, vm, assert, worker_threads #39628) the module behind util.inspect first runs at its first read, not when node:util loads. No user reported this: a fuzzer found it.

Fix

  • The nine statements read $DateNow and three $DatePrototype* names.
  • bun run: a define turns each name into @natives.<name>, a copy of JavaScriptCore's static table row, which a script cannot change.
  • --aot: four rows of the built-ins table bind the names. A build check rejects new captures.
  • Verified: test/js/bun/test/mock-fn.test.js and ten more files (Notes). Self-reviewed: 27 concerns raised, 25 addressed.

Background

  • @natives (bun test: Vitest compatibility #44833) is where built-in modules read globals that a script cannot patch.
  • JavaScriptCore makes Date.now from a compiled table row. A copy has the same native code and JIT intrinsic.
  • Considered $cpp copies per statement (the first version): the --aot compiler cannot see their result.

Downsides

  • @natives now also reads a JavaScriptCore-owned table, for Date only. A changed row fails a RELEASE_ASSERT at first load.
  • util.inspect(date): +15 instructions per call (+0.7%) in programs that read fewer than three Date.prototype members. .text: +1,536 B.
  • About 500 other first-load captures remain: Number.isNaN, Object.keys or JSON.stringify replaced around the first util.format still fail.
Notes

Who hits it. A test run that puts a mock on Date, Date.now or a member of Date.prototype while one of the six modules first runs. setSystemTime() and jest.useFakeTimers() do not. Each row is bun test ./a.test.ts ./b.test.ts, and b.test.ts mocks nothing. I ran the rows on the release binaries and on release builds of main b1305dd and of this branch.

a.test.ts 1.3.14 1.4.0 1.4.1, 1.4.4 main this branch
spyOn(Date.prototype, "toISOString") around a first util.format, then mock.restore() 4 pass 4 pass 3 fail 3 fail 4 pass
jest.spyOn(globalThis, "Date"), then jest.restoreAllMocks() 4 pass 4 pass 1 fail 3 fail 4 pass
spyOn(Date, "now") that no test restores, first use of node:v8 not a fair row 3 pass 3 pass 1 fail 3 pass

On main the failures are in b.test.ts: util.inspect({ createdAt: new Date(0) }) throws DatePrototypeGetTime.@call is not a function, or v8.GCProfiler reports no time. The third row is new on main: since #44833 Bun restores a spy at the end of its test file, and the built-in modules stay shared between files.

Short repro. Node.js v26.3.0 and Bun 1.4.0 print 1970-01-01T00:00:00.000Z. Bun 1.4.1 to 1.4.4 throw TypeError: undefined is not a function. main throws TypeError: DatePrototypeGetTime.@call is not a function.

import util from "node:util";
const Real = Date, d = new Real(0);
globalThis.Date = function Mock() { return new Real(0); };
util.inspect(5);
globalThis.Date = Real;
console.log(util.inspect(d));

Which moment decides for util.inspect. It is the first run of internal/util/inspect. a5e30c9 (#39628) made inspect, format, formatWithOptions and stripVTControlCharacters lazy getters of node:util. The three statements are the same at the tags bun-v1.4.0 and bun-v1.4.1. Each row puts a plain function on globalThis.Date, does the first use, restores Date and calls util.inspect(realDate).

before the mock first use under the mock 1.3.14, 1.4.0 1.4.1 to 1.4.4, main this branch, Node.js
import { format, inspect } from "node:util" (the import reads the getter) format("%s", 5) ok ok ok
import util from "node:util" util.format("%s", 5) ok throws ok
node:assert and node:util imported a failed assert.deepStrictEqual() ok throws ok
nothing console.log() of an object with a custom inspect function throws throws ok
import util from "node:util" under the mock, first call after the restore throws ok ok

The nine statements, and what Node.js does there.

statement on main used for Node.js
src/js/node/v8.ts:34 Date.now v8.GCProfiler startTime, endTime a native clock
src/js/node/inspector.ts:12 Date.now timestamp of Runtime.consoleAPICalled a native clock
src/js/internal/repl/node-primordials.ts:24 Date.now, and the read of the global Date on each call at :93 readline, the time between a CR and an LF the DateNow primordial
src/js/internal/test_runner/mock_timers.ts:27 Date.prototype.getTime mock.timers.enable({ now }) the DatePrototypeGetTime primordial
src/js/internal/util/inspect.ts:59-61 getTime, toISOString, toString util.inspect(date), util.format three primordials
src/js/node/_http_server.ts:123 Date.now and its second text src/js/ahead-of-time/node/_http_server.ts:228 the idle time of a keep-alive socket no JavaScript clock: the timer of the socket starts at libuv's loop time (lib/_http_server.js:1229, lib/internal/timers.js:387 in v26.3.0)

The two node:http statements are a stopgap. #46528 and #37974 replace them with the clock of the timers. Until then this PR keeps the clock they read today.

How the two backings work.

  • src/codegen/replacements.ts has the list dateFunctions. A define turns $DateNow into @natives.DateNow, as it turns Blob into @natives.Blob.
  • NativeGlobals::getOwnPropertySlot (src/jsc/bindings/ZigGlobalObject.cpp) has one new arm behind a miss in Bun's own table. It takes the table from the name (DatePrototype... or Date...), finds the row with staticPropHashTable->entry() and calls JSFunction::create(vm, globalObject, entry->functionLength(), name, entry->function(), ImplementationVisibility::Public, entry->intrinsic()). These are the arguments of reifyStaticProperty() in Lookup.h, and the same call that the function rows of @natives already make. The existing putDirect keeps one function per realm.
  • Four RELEASE_ASSERTs check what the accessors of HashTableValue only ASSERT: the row exists, it is a native function, and it is not a JS builtin or a DOMJIT function. So a name that JavaScriptCore does not have stops the first read. It does not leave undefined in a module.
  • The defines that read @natives do not reach the text that the --aot compiler reads (src/codegen/builtins-manifest.ts:64). There the four names are rows of src/codegen/builtins/host/bun-syntax.builtins.ts, next to $Array and $Promise. collect-syntax-uses.ts counts a name whose define reads @natives as used, or the table step refuses the rows as unused.
  • assertNoCapturedDateFunction runs on the bundled output of each module and function, and on each second text. It rejects an uncalled Date.now and any Date.prototype. A call-time Date.now() stays legal. With a capture put back into a first text or a second text, the bundle step fails with the message. It does not catch const { now } = Date. The new test in test/js/bun/globals.test.js does: it lists every module that reads the global Date when it first runs.
  • The arm is a backing for Date only. Array.prototype, Math, Object.prototype and Function.prototype have no static table in the pinned WebKit, so it cannot serve the whole class. When engine constants land (internal/primordials: Node.js parity, generated and engine-backed #35567 with Node.js primordials: tamper-proof built-in references for builtin JS WebKit#341, which declare the same four names), four things go: the define loop of dateFunctions, createDateFunction with its arm, the filter in collect-syntax-uses.ts, and the four lines of builtins.d.ts. The statements, the tests, the compiled rows and the build check stay.

Precedent. REVIEW.md asks built-in modules for "globals captured at module load". #45291 fixed the same first-load defect for two guards of inspect. #40271 did it for MessagePort. The nativeFunctions of #44833 give built-in modules a copy of setTimeout that fake timers do not replace.

Questions for a maintainer.

  1. Is a Date-only second source in @natives acceptable until engine constants land? The other first-load captures are not in this PR, and a wider set of names in @natives waits for this answer. On fix(js): better error messages when makeSafe fails #16263 Jarred-Sumner wrote that JavaScriptCore should have private symbols for what built-in modules need. internal/primordials: Node.js parity, generated and engine-backed #35567 is that route.
  2. AOT: front end, test runs and tests for the rewritten compiler #47942 owns src/js/ahead-of-time, src/codegen/builtins and test/aot. Which lands first? If AOT: front end, test runs and tests for the rewritten compiler #47942 does, the compiled half of this PR is done again on it. AOT: front end, test runs and tests for the rewritten compiler #47942 also removes the loop in bundle-modules.ts that holds the build check for second texts, so that call needs a new place.
  3. Do the two node:http statements stay until http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528?

Measurements. Two release builds with LTO on the same machine, linux-x64: main b1305dd and this branch (6955356). Instructions are user-mode counts of the first thread from a perf_event_open(PERF_COUNT_HW_INSTRUCTIONS) program. A per-call number is the difference of a run of 2N and a run of N, divided by N, with BUN_JSC_useConcurrentJIT=0 BUN_JSC_useConcurrentGC=0 BUN_JSC_numberOfGCMarkers=1, median of 7 interleaved runs.

  • release binary .text: +1,536 B, .rodata: +0 B. NativeGlobals::getOwnPropertySlot: 2,519 B -> 4,215 B (llvm-size, llvm-nm). The stripped binary has the same size (140,478,752 B).
  • new host functions: 0. JS2NATIVE_COUNT: 327 on main, 327 here. define keys: 152 -> 156.
  • Function cells per realm after all six modules and one use each: main 3469, here 3469. When the program reads the four public functions first: 3469 and 3472 (four copies of 32 B, one arrow function less). NativeExecutable: +0 in every case.
  • instructions per call, main / here: GCProfiler start and stop 1769.7 / 1769.6, console.debug under Runtime.enable 2497.1 / 2497.3, readline CR and LF chunk pair 3819.1 / 3823.7, mock.timers.enable({ now }) and reset() 24071 / 24137, one keep-alive node:http request and response 279,956 / 279,298. The spread of one build is 0.2% to 1.2%.
  • util.inspect(new Date(0)): median 2042.2 / 2057.2, mean 2035.4 / 2056.6, 40 interleaved runs, standard error 2.5. The difference follows the number of members that the public Date.prototype has made: the modules no longer read three of them. With three members read by the program first: 2038.7 / 2039.0. In the interpreter alone: 16701 / 16708.
  • bun empty.js: 3,639,481 -> 3,633,880 instructions (spread 0.9%), heap cell counts identical.
  • compiled executables, the six sites in a --compile --aot executable, main / here (at 2c84d69, before the last commit, which changes only the first read of a name under bun run): util.inspect(date) 2431.1 / 2433.3, GCProfiler 1768.7 / 1753.1, readline 4510.4 / 4369.4, inspector 3962.0 / 3899.5, mock timers 34142 / 34364 (+0.65%), node:http 82,762 / 82,864.
  • test/aot/perf/compare-builds.ts at 2c84d69: 56 server rows (node, bun, hono, elysia, express, fastify, koa). One row, server bun json, was 2.2% slower in one run and 1.8% faster in the next: it costs one of two amounts (about 35,030 or 35,740 instructions) on both builds. No other row moved beyond the 2% threshold in both of its readings.
  • test/aot/perf/plain-bun-costs.measure.ts at 2c84d69, 68 rows: -0.34% to +0.29%, start rows at most +0.13%.
  • compiled node:http server round (test/aot/servers/budgets-of-node.test.ts): 878 -> 878 accesses by name, 148 -> 148 runtime operations, 4,092,876 -> 4,092,876 code bytes.
  • compiled code, test/aot/tools/same-output.ts over 289 programs: in the programs that do not hold internal/util/inspect, no function changes size. In one that holds it, compared by name (595 functions), only the module body of internal/util/inspect changes: 4537 B -> 4522 B.
  • aot_builtin_syntax.rs: 4 Date rows. Undescribed results: 2103 lines (main 2103), native:Cpp: 19 (main 19). With the $cpp copies of the first version the table listed four more, each as the result of Call is anything.
  • key-order fixture: 2 stored rows rewritten (Date, Date.prototype), identical under bun run and --aot.
  • 15 of 15 fixture rows equal Node.js 26.3.0 with 0 mock calls and 0 reads of globalThis.Date. The nine new tests of the eight plain test files fail on the release build of main.
  • calls of a replacement after the restore: plain 0, compiled 0. The two new programs and the key-order fixture pass through test/aot/run-parallel.ts on the release build of this branch and fail on the release build of main.
  • fs._toUnixTimestamp(-1) under a Date.now spy that returns 5000: 5 with 1 call on both builds.
  • Tools: perf, valgrind and bloaty are not installed.

What stays. Each row is checked on this branch.

Other open PRs on these lines. #47942 (see the questions): a trial merge with its head 1bbb52e conflicts in bundle-functions.ts, bundle-modules.ts and replacements.ts, where it adds a build check of its own at the same place, and it deletes the key-order fixture that this PR edits. #46528 and #37974 delete the two node:http statements. #35567 declares the same four names. #45274, #40291 and #36546 touch neighbouring lines.

Tests.

  • test/js/node/v8/v8-module.test.ts, test/js/node/inspector/inspector.test.ts, test/js/node/readline/readline.node.test.ts, test/js/node/test_runner/node-test.test.ts, test/js/node/util/util.test.js, test/js/node/http/node-http.test.ts: one new test each. Each writes a fixture. runInBunAndNode(), new in test/harness.ts, runs it in a fresh process for each mock, under Bun and under Node.js 26.3.0 or later when node is installed. The fixture counts the calls of the mock and the reads of the global Date.
  • The node:http fixture sets response.sendDate = false. The Date header reads the clock anew for each response, which is a call-time read.
  • test/js/bun/test/mock-fn.test.js: two rows. A spy on Date in one test file, then a second file that prints and times dates. The jest.spyOn(globalThis, "Date") row also fails on the installed 1.4.3.
  • test/js/bun/globals.test.js: no built-in module reads the global Date when it first runs. main lists 8 reads (_http_server, inspector four times, readline, v8, the mock timers). Node.js lists none.
  • test/aot/a-module-that-first-runs-after-a-test-replaced-date-functions-uses-the-engines-own.test.ts: a test replaces getTime, toISOString and toString before util.inspect first runs. Plain bun test and bun test --debug-aot print the same lines, with no call of a replacement.
  • test/aot/parallel/library-modules/date-now-and-to-string-replaced-while-modules-first-run.cjs and global-date-replaced-while-modules-first-run.cjs: each holds its expected line and must print it under bun <file> and as a --compile --aot executable. Together they cover the four names in node:v8, node:http, util.inspect, readline and the mock timers.
  • test/aot/parallel/library-modules/keys-of-built-in-objects-behind-modules-that-read-them.mjs: the stored orders of Date and Date.prototype change, because the modules no longer read now, getTime, toISOString and toString on the public objects.
  • On the debug ASAN build the eight plain test files pass whole (11, 25, 82, 46, 224, 315, 850 and 25 tests), and the new test/aot test file passes. Of 192 vendored Node.js tests (test/js/node/test/parallel/test-{util-inspect,util-format,runner-mock-timers,readline,v8-,inspector,repl}* and test-http-*{keep*alive,timeout}*) 191 pass. test-http-client-timeout-option.js, which times a 1 ms timeout of the client, fails 2 of 5 runs on that build and passes on both release builds. test/internal/builtins-table, test/internal/source-lints and test/internal/second-texts-of-modules.test.ts pass.
  • test/aot/the-library-that-bun-holds-compiled-is-the-lowest-layer.test.ts fails here on the release build of main and on this branch in the same way: its SEEN setTimeout line is false under plain Bun and true in layers. Its SEEN Date.now | 5,1 line holds.

Self-review. Of the 27 concerns, two are not done.

  • A tracking issue for the other captures. The class and its repros are listed above.
  • A Date.now row in the compiled test run. With a write of Date.now and node:v8 the run of that one case took 3 min 20 s on the ASAN debug build, and the test file passed its limit of 270 s. The two executable programs pin $DateNow under a replaced Date.now and under a replaced global Date. By hand, the compiled test run printed the same lines as plain Bun, with no call.

The first version of this PR. Up to 9213a7b the seven statements of five modules called $cpp("DatePrimordials.cpp", "Bun::createDateNow") and three more creators, which made the same copy from the same rows. On today's main the table step of the --aot compiler refused the four bindings (runs native code nobody has classified), and the key-order fixture, which landed one day after that version's last CI run, failed. This version keeps the copy and the five tests, moves the copy into @natives, and adds node:http.

Credit. The four names are those of #35567. The mechanism is the @natives object of #44833.


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

… `Date` found at first load

node:v8, node:inspector, node:readline, the node:test mock timers and
util.inspect read Date.now or a Date.prototype method from the global
Date when the module first evaluates. A built-in module loads after
user code. So a Date mock that is active at that moment stays inside
the module after the test restores it.

Each of the seven statements now binds a function that
DatePrimordials.cpp makes from the static property table of
JavaScriptCore's DateConstructor or DatePrototype. The copy has the
native code and the intrinsic of the property, and a script cannot
reach it.
@robobun

robobun commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:21 AM PT - Nov 7th, 2026

❌ @robobun, your commit 6955356 has 38 failures in Build #138236 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 47058

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

bun-47058 --bun

@robobun

robobun commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced, fix pushed in this PR (#47058).

Reproduced with the six-line script in the description. A plain function stands on the global Date while util.inspect runs for the first time. After the restore, util.inspect(date) throws TypeError: DatePrototypeGetTime.@call is not a function on a release build of main (b1305dd), and TypeError: undefined is not a function on Bun 1.4.1 to 1.4.4. Node.js v26.3.0, Bun 1.4.0 and this branch print 1970-01-01T00:00:00.000Z.

As a test suite: jest.spyOn(globalThis, "Date") in one test file, restored after each test, then a second file that prints dates. It passes on 1.4.0, fails on 1.4.1 to 1.4.4 and on main, and passes on this branch. The nine new tests of the eight plain test files fail on that build of main and pass on this branch.

CI for 6955356 is build #138236. It failed on tests that also fail on main. The test files that this PR adds or changes pass on every lane.

  • 28 test files are red: 27 under test/aot/ and test/bundler/compile-aot-size.test.ts.
  • 27 of them are red in the builds of main at b1305dd (#137822) and at 7ca0042 (#138109).
  • The other one is test/aot/engine-tests-flow-under-other-schedules-2.test.ts. It fails in the job log of #138109 on each of its four attempts, for the same IR test that fails in test/aot/engine/ir-1.test.ts. testaot with the table of built-ins of main and with the table of this branch gives the same result for that IR test under each of the 32 rule orders.
  • compile-aot-size.test.ts: the trimmed hello world maps 25,978,077 bytes here and 25,986,099 bytes on main. The limit is 25,880,000.
  • 41 more test files failed once and passed on retry, most of them in a parallel batch on the ASAN lane. Four other pull requests of the same day have 34 to 40.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The change adds native bindings for Date functions and uses them in Bun’s internal modules. The code generator checks for captured Date functions. New regression tests cover replaced Date APIs across module loading, runtime behavior, and AOT execution.

Changes

Date primordial bindings

Layer / File(s) Summary
Add native Date functions
src/js/builtins.d.ts, src/codegen/builtins/host/bun-syntax.builtins.ts, src/codegen/replacements.ts, src/jsc/bindings/ZigGlobalObject.cpp, src/codegen/builtins/collect-syntax-uses.ts
The change declares and maps four Date functions to native bindings. JavaScriptCore lookup resolves Date and Date prototype functions, and syntax-use collection includes intrinsic native names.
Check bundled code for captured Date functions
src/codegen/replacements.ts, src/codegen/bundle-functions.ts, src/codegen/bundle-modules.ts
The code generator checks bundled outputs and selected transpiled AOT sources for captured Date.now and Date.prototype references.
Use native Date functions
src/js/internal/repl/node-primordials.ts, src/js/internal/test_runner/mock_timers.ts, src/js/internal/util/inspect.ts, src/js/node/inspector.ts, src/js/node/v8.ts, src/js/node/_http_server.ts, src/js/ahead-of-time/node/_http_server.ts
Internal modules use native Date bindings in place of direct JavaScript Date function references. The AOT HTTP server source hash also changes.
Test behavior with replaced Date APIs
test/harness.ts, test/js/node/inspector/inspector.test.ts, test/js/node/readline/readline.node.test.ts, test/js/node/test_runner/node-test.test.ts, test/js/node/util/util.test.js, test/js/node/v8/v8-module.test.ts, test/js/node/http/node-http.test.ts, test/js/bun/globals.test.js
The harness can run fixtures in Bun and an eligible Node.js version. New tests cover module loading and behavior with mocked Date methods, replaced global Date, or mocked timers.
Test AOT and test-process behavior
test/aot/a-module-that-first-runs-after-a-test-replaced-date-functions-uses-the-engines-own.test.ts, test/aot/parallel/library-modules/*, test/js/bun/test/mock-fn.test.js
New tests check Date method replacement during AOT and module loading, global Date replacement across Node modules, built-in member ordering, and Date behavior across test files.
Merge Risk: Minimal · up to 69553

The change makes built-in modules use the engine's own Date functions, so mocked Date no longer affects them. No actionable merge-blocking risk was identified.

Pre-merge checks: Passed 4

Check name Status Explanation
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.
Title check Passed The title clearly describes the main change: built-in modules use engine Date functions instead of capturing the mutable global Date.
Description check Passed The description follows the repository template with both required sections, "What does this PR do?" (as "Problem", "Fix", "Background", "Downsides") and "How did you verify your code works?" (as "Ver…
Full details: Description check

Explanation

The description follows the repository template with both required sections, "What does this PR do?" (as "Problem", "Fix", "Background", "Downsides") and "How did you verify your code works?" (as "Verified", "Tests", "Notes" and "Measurements"). It states the bug, the cause, the fix, the affected modules, and the tests added for each. It also lists the open questions and the remaining first-load captures that are out of scope. The verification section is detailed, with a reproduction table and measured results. One gap: the robobun evidence line says node-http.test.ts has no test proof on this machine, and the verification section does not explain how that test was run or why it was not run here.

  • 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 9213a7b and 6955356.

Files selected for processing: 24

  • src/codegen/builtins/collect-syntax-uses.ts
  • src/codegen/builtins/host/bun-syntax.builtins.ts
  • src/codegen/bundle-functions.ts
  • src/codegen/bundle-modules.ts
  • src/codegen/replacements.ts
  • src/js/ahead-of-time/node/_http_server.ts
  • src/js/builtins.d.ts
  • src/js/internal/repl/node-primordials.ts
  • src/js/internal/test_runner/mock_timers.ts
  • src/js/internal/util/inspect.ts
  • src/js/node/_http_server.ts
  • src/js/node/inspector.ts
  • src/js/node/v8.ts
  • src/jsc/bindings/ZigGlobalObject.cpp
  • test/aot/a-module-that-first-runs-after-a-test-replaced-date-functions-uses-the-engines-own.test.ts
  • test/aot/parallel/library-modules/date-now-and-to-string-replaced-while-modules-first-run.cjs
  • test/aot/parallel/library-modules/global-date-replaced-while-modules-first-run.cjs
  • test/aot/parallel/library-modules/keys-of-built-in-objects-behind-modules-that-read-them.mjs
  • test/harness.ts
  • test/js/bun/globals.test.js
  • test/js/bun/test/mock-fn.test.js
  • test/js/node/http/node-http.test.ts
  • test/js/node/util/util.test.js
  • test/js/node/v8/v8-module.test.ts

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

@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.

Beyond the inline findings, I also checked: the uncurryThis(fn)(value) -> fn.$call(value) rewrite in inspect.js and DateNowFn.$call(Date) -> bare native in node-primordials.js are behavior-preserving (uncurryThis is Function.prototype.call.bind, and the native Date.now ignores this); the new C++ path in DatePrimordials.cpp only calls Identifier::fromString and JSFunction::create, neither of which enters JS, so there is no missing exception check; and a sweep of src/js for other module-scope Date.now / Date.prototype.* captures finds only _http_server.ts:123, which the PR says is removed by a separate change. The reading of JSC's static property hash table is a new mechanism in this tree, so a maintainer's sign-off on that design is still worthwhile.

Extended reasoning...

The change replaces seven module-load captures of Date functions in five built-in JS modules with four new $cpp bindings that construct fresh JSFunctions from JSC's static property tables for DateConstructor and DatePrototype, and adds five spawned-fixture tests that also run under Node when available. It touches no auth, crypto, or input-parsing surface; the only native code is a small table lookup guarded by RELEASE_ASSERTs. Two inline findings were already posted (duplicated test runner block, and the readline time-sentinel colliding with a mocked clock at 0). The decisive facts against approving are the two posted findings and that this is the first use of staticPropHashTable in src/jsc, which the author flags as needing maintainer acceptance.

Comment thread test/js/node/v8/v8-module.test.ts Resolved
runInBunAndNode() in test/harness.ts runs a fixture in a fresh process
for each argument, under Bun and under Node.js when it is installed.
The five new tests call it, so the runner is written once.

@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.

The runInBunAndNode helper in 56e6bda addresses the duplication I noted on the first push. Beyond the inline note on that helper, I also checked that every remaining use of the rebound identifiers (DateNow in interface.js, readline.js, v8.ts and inspector.ts; DatePrototypeGetTime at mock_timers.ts:548 and :724; the three Date functions at inspect.js:1410-1412) is called with no receiver or via .$call, so no uncurryThis-style call survives the switch, and that the js2native codegen includes DatePrimordials.h from the $cpp filename, so the new file needs no extra registration. The read of JavaScriptCore's static property table in DatePrimordials.cpp is the design choice a maintainer should still weigh.

Extended reasoning...

The change adds src/jsc/bindings/DatePrimordials.{h,cpp}, which builds fresh JSFunctions from JSC's DateConstructor/DatePrototype static hash tables, rebinds seven Date captures in five built-in JS modules to those functions, and adds a shared test harness helper plus five spawned-fixture tests. It touches no security-sensitive surface. Not approved because the static-table read is a new mechanism with RELEASE_ASSERTs that fail at first load on a WebKit table change, which the author themselves flags as needing maintainer acceptance, and one inline nit is being posted.

Comment thread test/harness.ts Resolved
It returned an empty stderr for a run that exits 0, so the `stderr: ""`
that the five Date mock tests expect could not fail. The 13 fixture
runs print nothing to stderr under Bun and under Node.js v26.3.0.

@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.

Thanks, both earlier nits are addressed: the five fixtures now share runInBunAndNode, and it returns the real stderr of every run. This automated review ran on the latest push and found no bugs; since src/jsc/bindings/DatePrimordials.cpp is the first Bun binding to reify a function from a JSC-owned staticPropHashTable, a maintainer should still sign off on that mechanism.

What was reviewed:

  • createDateFunctionFromStaticTable: asserts the row exists and is a plain native (not Builtin/DOMJIT) before reading function()/intrinsic(); the JSFunction::create arguments match reifyStaticProperty, and it runs once per module load, so the per-load Identifier::fromString is off the hot path.
  • The seven JS sites: DateNow is handed through as a bare function (readline calls it with no receiver, which Date.now ignores); inspect.js calls the three prototype copies with .$call(value) on an isDate-checked value.
  • Sibling captures: src/js/node/_http_server.ts:123 is the only remaining const DateNow = Date.now in src/js and the PR names it as excluded.
  • Tests: the toEqual(runs.map(...)) shape pins stdout/stderr/exitCode to literals, so a differing run still fails; fixtures mock before the first require and assert calls:0, reads:0.
Extended reasoning...

The change adds a 48-line C++ binding (DatePrimordials.cpp/.h) that builds fresh JSFunctions from JSC's DateConstructor/DatePrototype static property tables, swaps seven module-scope Date captures in five src/js modules to $cpp calls to it, adds a runInBunAndNode harness helper, and adds one spawned fixture test per module that also runs under Node 26.3.0+. It touches no auth, crypto, or input-parsing surface; the tamper-resistance direction is the point of the change. No CODEOWNERS entry covers the changed paths and both of my prior inline comments were addressed by commits 56e6bda and 9213a7b. Defer rather than approve because the PR introduces a new mechanism (first read of a JSC-owned staticPropHashTable, which the author also flags as needing maintainer acceptance) and the WebKit headers it includes (DatePrototype.h is not included anywhere else in src/) are not in this checkout, so I could not confirm the build locally.

Conflicts: main moved src/js/internal/repl/node-primordials.js and
src/js/internal/util/inspect.js to TypeScript and dropped uncurryThis
from inspect and node:v8. The seven Date bindings are the same
statements in the new files.
…Date*` names

Nine module-scope statements read `Date.now` or a member of
`Date.prototype` when their module first runs, which is after user code.
They now read `$DateNow`, `$DatePrototypeGetTime`,
`$DatePrototypeToISOString` and `$DatePrototypeToString`.

- `bun run`: a define turns each name into `@natives.<name>`, and
  `NativeGlobals` makes the function from the row of JavaScriptCore's
  own static table.
- `bun build --compile --aot`: four rows of the table of built-ins bind
  the names to the engine's functions.
- The bundler of the built-in modules rejects an uncalled `Date.now`
  and any `Date.prototype` in `src/js`.

This replaces the `$cpp` bindings of `DatePrimordials.cpp`, which the
compiler could not see through, and adds the two texts of
`node:_http_server`.
…k shapes of a test run

- `NativeGlobals` finds the row from the name itself (`DateNow` is `now` of
  `DateConstructor`, `DatePrototypeGetTime` is `getTime` of `DatePrototype`),
  so `dateFunctions` is the only list for `bun run`.
- The build check says what to write and which members have no row.
- `mock-fn.test.js`: a spy on `Date` in one test file, then a second file
  that prints and times dates.
- `globals.test.js`: no built-in module reads the global `Date` when it
  first runs.
- The two programs of `test/aot` also replace `Date.prototype.toString`
  and cover `util.inspect`, readline and the mock timers. The case of a
  compiled test run has a file of its own.
@robobun
robobun requested a review from alii as a code owner November 7, 2026 10:21
@robobun robobun changed the title Built-in modules: bind copies of the engine's Date functions, not the Date found at first load Built-in modules: bind the engine's Date functions through @natives, not the Date found at first run Nov 7, 2026

@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.

I reviewed the rewritten version of this PR (the two commits since my last review) and found no bugs; the earlier points about the duplicated spawn block and the blanked stderr are addressed by runInBunAndNode in test/harness.ts. Because the mechanism is now a fallback in NativeGlobals::getOwnPropertySlot that derives a row from JSC's DateConstructor/DatePrototype static hash tables plus a new build-time assertNoCapturedDateFunction check, a maintainer's look at that design is still worthwhile.

What was reviewed:

  • createDateFunction in src/jsc/bindings/ZigGlobalObject.cpp: name derivation (DateNow -> now, DatePrototypeGetTime -> getTime), the RELEASE_ASSERTs, and that a non-Date unknown name still returns false as before; the @ natives object is reachable only from bundled builtins, so the asserts are not user-reachable.
  • Codegen: $DateNow and friends map to __intrinsic__natives.* for bun run, are excluded from the compiler's syntax-use set, and are declared as builtin("Date.now")-style rows alongside the existing Object.getPrototypeOf/Math.min entries; the remaining Date.now uses in src/js are all immediate calls, so the new build check passes.
  • Tests: the per-module fixtures run under Bun and Node, assert exact {stdout, stderr, exitCode}, and globals.test.js counts global Date reads across every builtin module (Date is not in globalsToPrefix, so the old const DateNow = Date.now captures would trip it).
  • One sweep item: the PR description still describes the earlier DatePrimordials.cpp / $cpp(...) / "4 more $lazy rows" design, which no longer exists in the diff.
Extended reasoning...

The change replaces seven module-scope captures of Date.now / Date.prototype methods in src/js with four new private intrinsics ($DateNow, $DatePrototypeGetTime, $DatePrototypeToISOString, $DatePrototypeToString) resolved lazily by a new fallback in NativeGlobals::getOwnPropertySlot that reads JSC's own DateConstructor/DatePrototype static property tables and creates a fresh JSFunction from the table row, plus a codegen assertion that rejects future captures and declarations in bun-syntax.builtins.ts and builtins.d.ts. It touches no auth, injection, or data-exposure surface; the hostile-userland concern it addresses (a mock active at a builtin module's first load) is the fix itself. The deciding facts for defer rather than approve: this is the first Bun read of a JSC-owned static table and the author explicitly asks a maintainer to accept it, the ahead-of-time compiler semantics of the new builtin("Date.now") rows live in vendored WebKit not present in this checkout, src/js/builtins.d.ts is covered by a CODEOWNERS entry, and the PR description still describes the superseded DatePrimordials.cpp design.

@robobun

robobun commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator Author

The description is current. I rewrote it for this version at 10:22 UTC, after the push and before this review was posted. It describes the $Date* names, the @natives arm in NativeGlobals::getOwnPropertySlot and the build check. DatePrimordials.cpp and $cpp(...) now appear only as the first version of the PR.

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.

require('fs') evaluates Set prototype function

2 participants