[robobun]

Related to oven-sh/bun#46342

Problem

• The date setters return NaN when the year alone is outside ±275760 or the month count outside ±3309120: new Date(0).setUTCFullYear(275761, -12, 1) is NaN, Date.UTC(275761, -12, 1) is 8639977881600000. One report, for dates over 275,760 years apart.
• MakeDay range-checks only year + floor(month/12). fillStructuresUsingDateArgs() (runtime/DatePrototype.cpp) bounds each argument and narrows it to int.

Fix

• normalizeYearAndMonthForMakeDay() (runtime/DateConstructor.h) splits an int64_t month count by 12 exactly and checks that the year fits int32_t. makeDay() (Date.UTC, new Date, Temporal) and the date setters call it.
• The time setters compile to the same instructions.
• Verified: JSTests/stress/date-setters-year-month-range.js (fails on the base). 7813 fuzzed calls equal a BigInt MakeDay. test262 Date: 0 new failures.
• Self-reviewed: 14 concerns raised, 8 addressed, 2 in part, 4 follow-ups (Notes).

Background

• MakeDay folds the month count into the year, then adds the day count. Only the final time value has a range.
• This replaces the guards of [JSC] Fix tc39 a test case for Date.prototype.setUTCFullYear WebKit/WebKit#43285, [JSC] Ensure Date.prototype.setYear Respects TimeClip Range by Guarding GregorianDateTime::years Overflow WebKit/WebKit#46001 and [JSC] Ensure Date.prototype.setMonth and Date.prototype.setUTCMonth Respects TimeClip Range by Guarding months of GregorianDateTime Overflow WebKit/WebKit#47036, and is not filed upstream. A maintainer has to choose: this, or a wider per-argument bound that fixes the report and keeps two normalisations.

Downsides

• Above |month| = 12×2^53, Date.UTC and new Date(y, m) answer NaN where ECMA-262 defines a date (this fork's limit): 3,431 of 300,000 fuzzed calls, 859 were right. 882 wrong dates become exact.
• No speed cost found: per x64 call, date setters run 22 to 125 fewer instructions and Date.UTC 31. .text: +224 bytes (-2,176 with ThinLTO).

Notes

Measurements (base = 680ef47cec, the tip of main, which bun pins since oven-sh/bun#46690. Shells: Release -march=nehalem, and a ThinLTO + frame pointer build with the flags of the shipped lto lane, both clang 23.1.2, both built with and without this change. An earlier round on 79618395ee gave the same figures within the noise, except setMonth on one Date: -13.7% there, -4.7% here.)

• time setters: 0 differing instructions of 420 in setNewValueFromTimeArgs and 0 of 107 in DateCache::gregorianDateTimeToMS (non-LTO), 0 of 749 and 0 of 108 (ThinLTO) (objdump, base vs PR, x64)
• instructions per call vs base (JIT off, x64 nehalem, 1M calls, median of 3): setUTCHours(4 args) +0.0, setUTCMilliseconds -0.0, setHours(0,0,0,0) +0.1, setUTCDate -68.0, setDate -67.0, setMonth(0..11) -100.9, setUTCMonth -102.0, setFullYear(2024) -97.0, setUTCFullYear(2024) -97.9, setUTCFullYear(3 args) -125.1, setYear -22.0, Date.UTC(3 args) -30.8. The time-setter figures are counter noise: the code is identical.
• cycles per call vs base, ThinLTO x64, 13 interleaved runs, minimum of the runs, chained on one Date (A/A floor -0.7% to +0.7%): time setters -0.6% to -0.0%. Date setters with a month argument in 0..11: -4.7% to -23.7%. Date setters whose month argument carries into the year: setUTCMonth(17) / setUTCMonth(-7) -4.6%, setUTCMonth(getUTCMonth() ± 12) -7.9%, setUTCFullYear(y, 12 + m, d) -10.0%, setMonth(17) / setMonth(-7) -3.3%. Date.UTC(3 args) -22.0%, Date.UTC(7 args) -7.0%, new Date(y, m, d) -6.1%.
• the same on 1024 independent Dates (A/A floor -1.2% to +1.1%): time setters -0.6% to +1.1%, date setters -5.5% to -18.9%, carrying month arguments -5.3% to -10.6%, Date.UTC(3 args) -22.3%.
• arm64 instructions per call vs base (the Date translation units cross-compiled and driven by a harness under a qemu-user instruction trace): generic toInt32 flavour: time setters +0.0, date setters -123.8 to -30.0, with double arguments -87.8 and -66.8, with a carrying month -74.0, Date.UTC(3 args) -39.8. fjcvtzs flavour (-mcpu=apple-m1): time setters +0.0, date setters -86.0 to -10.0, with double arguments -50.0 and -46.0, with a carrying month -52.0, Date.UTC(3 args) -2.0. These are from the previous base, 79618395ee, and were not run again: the Date functions have the same x64 instruction counts on both bases, with and without the change.
• jsc .text: +224 bytes non-LTO, -2,176 bytes ThinLTO (llvm-size -A, base vs PR)
• ns per call, JIT on, ThinLTO, 13 interleaved runs, minimum (the base against itself moves by up to 0.6, and by 2.0 on setMonth): setUTCHours(4 args) 34.9 -> 34.8, setHours(0,0,0,0) 42.2 -> 42.0, setUTCMilliseconds 35.4 -> 35.2, setUTCDate 34.8 -> 30.8, setUTCMonth 43.7 -> 41.1, setMonth 79.2 -> 77.0, setUTCFullYear(3 args) 44.6 -> 37.1, Date.UTC(3 args) 23.9 -> 18.3
• Date.UTC / new Date: 4,313 of 300,000 fuzzed calls change. 3,431 go from a number to NaN, all with |year| or |month| above 12 × 2^53 (859 of them were MakeDay's value before). 882 go from a wrong date to MakeDay's value. 0 change for |year| and |month| up to 2^53 - 1. Time setters: 0 of 300,000 results differ from the base.

Results that change

• The reported cases: new Date(0).setUTCFullYear(275761, -12, 1) 8639977881600000 (was NaN). new Date(Date.UTC(-1, 0, 1)).setUTCMonth(12 * 275761) 8639977881600000 (was NaN). new Date(NaN).setUTCFullYear(-271821, 3600000) 827076182400000 (was NaN). The lines of Date#setMonth returns Invalid Date when the result lies more than 8.64e15 ms from the date bun#46342: d.setFullYear(-187631, 8, 27); d.setMonth(3472364) is +101732-09-27 (was Invalid Date). On released bun, dayjs and moment diff() return 0 for two such dates, and date-fns differenceInMonths is off by one.
• The day count is no longer multiplied by msPerDay and added to the milliseconds first. Beyond 2^53 that sum lost a millisecond: new Date(8.64e15 - 999).setUTCDate(-199000000) was -8553601036801000 and is -8553601036800999, as Date.UTC(275760, 8, -199000000, 23, 59, 59, 1) is. This is not a second fix: without the per-argument bound, a day count can be that large and still give a date.
• new Date(0).setUTCFullYear(275761, 0, -200) (a day count that pulls the year back) equals Date.UTC(275761, 0, -200) now, as in V8. A strict reading of MakeDay step 8 does not require this one: no time value has the year 275761.
• Date.UTC(-6004799503158661, 2 ** 56, 1) was 2000-01-01 where MakeDay gives 2000-05-01: the double split floor(month / 12) and month - years * 12 rounds beyond 2^53. It is 2000-05-01 now. Date.UTC(2000 - 2 ** 52, 12 * 2 ** 52 + 8, 1) was 2000-05-01 and is 2000-09-01. MakeDay floors the real quotient month / 12. A double quotient has no fraction from 2^52, so its floor is one more for that month count.
• Above |month| = 12 × 2^53 the answer is NaN. There floor(month / 12) is not always a Number and MakeDay adds the rounded value: for Date.UTC(2000 - 2 ** 53, 12 * 2 ** 53 + 16, 1) MakeDay gives 2000-05-01, exact arithmetic gives 2001-05-01, and this change gives NaN. V8 gives NaN from |month| = 1e7.
• makeDay() truncates a year or month count that has a fraction, as MakeDay's ToIntegerOrInfinity does, where it answered NaN. No caller passes one.

Tests

• The stress test: 191 assertion lines, 17 of 17 configurations of run-jsc-stress-tests (which forces TZ=US/Pacific), and 8 time zones. An expected value that is NaN fails it, because Date.UTC and the Date constructor share MakeDay with the setters. A NaN is expected through shouldBeNaN() only. 24 of its lines are outside V8's own caps (|year| above 1e6 or |month| above 1e7), where V8 answers NaN. V8 agrees with all the others.
• 50 single-edit mutants of the added decisions (split divisor and remainder, a double quotient in the split, the int32 check, the range check with a bound of 2^53 - 1, of 12 × 2^53 + 16 and of 2^62, each field a setter stores on the int32 and on the double path, the terms of the setters' day sum, setYear's 0 to 99 rule, the local offset, TimeClip): all fail the stress test under TZ=US/Pacific. One of them showed that the test did not pin Date.UTC beyond int32. It does now. The double quotient came from a review: the test passed with it for every month count, and now fails from 12 × 2^52.
• 40,000 fuzzed setter calls (all 15 setters) in 3 time zones: the 21,393 time-setter results are identical to the base. Of the 18,607 date-setter results, 616 to 619 differ from the base. The 7,813 UTC ones equal an exact BigInt MakeDay. The 12,927 inside V8's caps equal V8, except 2 setYear calls in America/Los_Angeles on a date 1 ms after the minimum time value, where every local setter of the base answers NaN too ([JSC] Date: convert a local time to UTC with the zone's own offset when the local time is outside the time value range #615 is about that).
• 400,000 more date-setter calls per time zone with day counts near and beyond 2^52, years that cancel them, and the edges of the range: the commit with the setters' own day sum gives what the commit before it gives on every call.
• run-jsc-stress-tests on the date and Temporal tests of JSTests/stress and LayoutTests/jsc-layout-tests.yaml, and test262 built-ins/Temporal and intl402, ran on the first commit of this branch, on the previous base: 2,781 stress runs pass (the 16 failing runs are temporal-invalid-calendar-identifier-huge-string.js, killed by the memory limit of the machine, on the base as well), 15,048 test262 runs pass with 0 new failures.
• On the merge with main, 66ac271a67, against 680ef47cec: the stress test fails on the base and passes in the 17 configurations and 8 time zones. The fuzz outputs of the base and of the change are byte for byte the ones of the previous base. test262 built-ins/Date, annexB/built-ins/Date, intl402/Date and staging/sm/Date (1,316 runs), and built-ins/Temporal, intl402/Temporal and intl402/DateTimeFormat (13,752 runs): 0 new failures, and every run has the same result with and without the change. run-jsc-stress-tests on the 199 date and Temporal tests of JSTests/stress: 3,221 of 3,237 runs pass. The 16 others are temporal-invalid-calendar-identifier-huge-string.js, killed by the memory limit of the machine, on the base as well.

Design

• The comparisons between variants in this section were measured on the previous base, 79618395ee.
• makeDay() keeps MakeDay's order for the day sum, Day(t) + dt - 1. Date.UTC can see it when its time arguments cancel a day count beyond 2^53 (the stress test has two such calls). The setters add the date to the month's days first, which does not wait for the year's day count. A setter cannot see the order: its time of day is below msPerDay, so a time value needs a day sum of at most 1e8, where every partial sum is exact.
• An int32 year or month argument is stored as it is. A month that went through a double, a range check and the 64-bit split made a carrying month argument 3.6% to 8% slower than the base on a chained setter, with fewer instructions. Without that path the figures above for a carrying month are -0.7% to +3.6%.
• dateSetterTimeValue() is ALWAYS_INLINE: out of line, setMonth(m) and setUTCMonth(m) in a chain cost 2.9% to 4.1% more cycles than the base under ThinLTO.
• The range check compares magnitudes only. With isSafeInteger(), which also truncates, Date.UTC was +3 instructions on the fjcvtzs flavour.
• The bound is 12 × 2^53. Up to it the floor of the real quotient month / 12 (MakeDay step 5 divides the mathematical value) is an exact Number, so the integer sum is the sum MakeDay specifies. A bound of 2^53 - 1 (the first version of this rework) answered NaN for 6,932 of the 300,000 fuzzed Date.UTC calls and made none of the wrong dates exact. With a bound of 2^62 the integer sum is exact arithmetic, but not MakeDay's, which adds the rounded quotient.
• A wider per-argument bound at the three guard sites (V8's 1e6 and 1e7, for example) fixes Date#setMonth returns Invalid Date when the result lies more than 8.64e15 ms from the date bun#46342 in about 6 lines. It leaves the setters' int split beside makeDay()'s double split, leaves Date.UTC with its wrong dates beyond 2^53, and a setter and Date.UTC still differ between the two bounds. That is the smaller change if the makeDay() contract of this fork must stay upstream's.
• The branch has main merged in at 680ef47cec, the commit bun pins since Upgrade WebKit to f59345e04a8d bun#46690. So the preview build of this PR differs from bun's WebKit by this change only. The earlier commits were based on 79618395ee. DatePrototype.cpp, DateConstructor.h, DateConstructor.cpp, JSDateMath.*, wtf/DateMath.* and the Temporal callers of makeDay() did not change between the two.

Self-review

• Addressed: the setters' own day sum. The carrying month argument, which was slower than the base and was reported from workloads that never carry. The delivery order on the bun side. The rarity of the trigger. The three upstream changes this replaces, and the request for a maintainer decision. The description of what stays out (below).
• In part: this is not filed at bugs.webkit.org, and makeDay() is not left as upstream has it. DatePrototype.cpp at the base is the file upstream has today.
• Not done, possible follow-ups: the differential fuzz with the BigInt reference as a checked-in stress test. A corpus run on the old and the new engine at each WebKit bump. test262 files for the year and month combination (0 of test262's Date files tell the base from this change). A preload workaround posted on the issue.

Not in this PR

• The 8 time setters have the same mechanism as the day fold removed here: they add their arguments into one milliseconds double in an order MakeTime does not use. One argument near 2^53 ms is 1 ms off: new Date(1875619980085361).setUTCMinutes(-150119987617) is -7131579276994640 and MakeTime gives -7131579276994639 (V8 too). Two arguments that cancel are up to a day off: new Date(82800000).setUTCSeconds(2 ** 70, -(2 ** 70) * 1000) is 82800000 where V8 and Date.UTC(1970, 0, 1, 23, 0, 2 ** 70, -(2 ** 70) * 1000) give 134217728. A form with all 15 setters on one path was built. It is not here because it moves those results, which is a decision of its own, and because it gives up the proof that the time setters compile to the same instructions. No issue tracks it yet.
• Other year/month normalisers stay for callers that pass an int month: WTF::dateToDaysFrom1970, WTF::daysFromYearMonth, v8::MakeDay behind Date.parse. TemporalCore::balanceISOYearMonth could share the int64 split.
• JSC accepts any normalised year that fits int32_t and lets timeClip() decide. V8 caps |year| at 1e6 and |month| at 1e7 first.

[robobun]

Reproduction (any time zone for the UTC lines):

// repro.js: TZ=UTC bun repro.js / node repro.js / jsc repro.js
const p = typeof print === "function" ? print : console.log;
p(new Date(0).setUTCFullYear(275761, -12, 1), Date.UTC(275761, -12, 1));      // bun 1.4.3: NaN 8639977881600000   this branch, node: 8639977881600000 twice
p(new Date(0).setFullYear(300000, -300000, 1));                               // bun 1.4.3: NaN   this branch, node: 8615994624000000 (TZ=UTC)
p(new Date(Date.UTC(-1, 0, 1)).setUTCMonth(12 * 275761));                     // bun 1.4.3: NaN   this branch, node: 8639977881600000
p(new Date(NaN).setUTCFullYear(-271821, 3600000));                            // bun 1.4.3: NaN   this branch, node: 827076182400000
const d = new Date(0);
d.setUTCFullYear(-187631, 8, 27);
p(d.setUTCMonth(3472364));                                                    // bun 1.4.3: NaN   this branch, node: 3148207948800000 (+101732-09-27, oven-sh/bun#46342)
p(new Date(0).setUTCFullYear(275760, -12, 1), new Date(0).setUTCFullYear(275761, 0, 1)); // controls, unchanged: 8639946345600000 NaN

How I verified, with jsc shells built from 680ef47cec (the tip of main, the commit bun pins) with and without this change, as a Release build and as a ThinLTO build with the flags of the shipped lane:

• JSTests/stress/date-setters-year-month-range.js fails without the change at its first setter assertion (expected 8639977881600000 but got NaN). With the change it passes in 8 time zones, with the JIT off, and in the 17 configurations of run-jsc-stress-tests.
• 40,000 fuzzed setter calls in 3 time zones: the 7,813 UTC date-setter calls equal an exact BigInt MakeDay, and the 12,927 date-setter calls inside V8's caps equal V8 (2 exceptions, on a date 1 ms after the minimum time value, are NaN without the change too). The 21,393 time-setter calls give what the base gives.
• test262 built-ins/Date, annexB/built-ins/Date, intl402/Date, staging/sm/Date (1,316 runs) and built-ins/Temporal, intl402/Temporal, intl402/DateTimeFormat (13,752 runs): 0 new failures, and every run has the same result with and without the change.
• 50 single-edit mutants of the change all fail the stress test under TZ=US/Pacific.
• The PR description has the measurements (instructions, cycles, size).

Bun side: oven-sh/bun#42193 pins the preview build of this PR and adds test/js/bun/jsc/date-setters-year-month-range.test.ts.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes spec-conformance numerics for all Date setters and tightens the shared makeDay() helper (also used by Date.UTC, the Date constructor, and Temporal), and Source/JavaScriptCore / JSTests are CODEOWNER-covered, a human look is still worthwhile.

What was reviewed:

• BrokenDownDate field widening to double and the new toTimeValue() path — recomposition now goes through makeDay/makeTime/makeDate → localTimeToMS → timeClip, matching the constructor path in DateConstructor.cpp.
• Per-argument std::isfinite checks in fillStructuresUsingTimeArgs/DateArgs — each argument is still individually gated, so Infinity, -Infinity combinations still yield NaN as the spec requires.
• The added monthInt32 < 0 || monthInt32 > 11 guard in makeDay() — for |month| < 2^53 the remainder is exact and in [0,11], so no regression for existing in-range callers; Temporal call sites pass a normalized month.
• The new stress test throws on failure, does no logging, and covers int32-wrap, 2^53 day-count precision, and non-finite arguments.

Extended reasoning...

Overview

This PR reworks how Date.prototype.set{FullYear,Month,Date,Hours,Minutes,Seconds,Milliseconds,Year} (and UTC variants) rebuild the time value in Source/JavaScriptCore/runtime/DatePrototype.cpp. BrokenDownDate fields become double, the two fillStructuresUsing*Args helpers now just store arguments and check finiteness, and a new BrokenDownDate::toTimeValue() runs the spec's MakeDate(MakeDay(...), MakeTime(...)) → localTimeToMS → timeClip sequence. DateConstructor.h's makeDay() gains a guard rejecting a month remainder outside [0, 11], which only occurs when |month| ≥ 2^53. A new JSTests/stress/date-setters-year-month-range.js exercises boundary years, huge months/days, int32-wrap cases, and non-finite arguments.

Security risks

None identified. This is pure numeric/spec-conformance work in Date runtime host functions. The int32 truncations that were removed were previously guarded by ad-hoc range checks; the replacement path keeps everything in double until makeDay()'s existing yearInt32 != ym check, and timeClip() still bounds the final result. No memory, auth, or injection surface is touched.

Level of scrutiny

Moderate-to-high. The change is small in line count and the recomposition now literally reuses the same helpers the Date constructor and Date.UTC already use, which is a strong correctness argument. However, it is a behavioral change to a widely used built-in, the makeDay() tweak is shared with Date.UTC/new Date()/Temporal callers, and floating-point edge-case reasoning (2^53 splits, day×msPerDay precision) is subtle enough that a JSC reviewer should sign off. .github/CODEOWNERS routes both Source/JavaScriptCore and JSTests to WebKit/jsc-reviewers.

Other factors

The PR description reports no new test262 failures, differential fuzzing against V8 across several time zones, and that the new stress test fails unpatched and passes patched under all JIT tiers in ~10 ms — consistent with JSTests conventions. DateCache::gregorianDateTimeToMS is intentionally left in place for the external Bun binding. Nothing in the timeline indicates outstanding third-party objections.

[coderabbitai]

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 8c48e165-3e57-4abc-97f5-f31f22f0001f

📥 Commits

Reviewing files that changed from the base of the PR and between 5e00ef5 and 189b3ff.

📒 Files selected for processing: 1

• JSTests/stress/date-setters-year-month-range.js

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

Walkthrough

Changes

Date setters now retain numeric fields through shared MakeDay and MakeTime normalization and time clipping. makeDay rejects converted months outside the valid range. Stress tests cover range handling, precision, overflow, non-finite inputs, and setter results.

Date setter range handling

Layer / File(s)	Summary
Shared date normalization Source/JavaScriptCore/runtime/DateConstructor.h, Source/JavaScriptCore/runtime/DatePrototype.cpp	Date fields retain double values and pass through shared normalization, local or UTC conversion, and time clipping. Parsing rejects non-finite inputs.
Setter conversion integration Source/JavaScriptCore/runtime/DatePrototype.cpp	Date setters and setYear use the shared conversion path and preserve finite numeric values.
Range behavior validation JSTests/stress/date-setters-year-month-range.js	Stress tests cover boundary normalization, constructor equivalence, precision, overflow, large values, non-finite arguments, and stored setter results.

Possibly related issues

• oven-sh/bun#46342: The issue reports Date#setMonth returning NaN for a normalized time beyond the stated range. This PR changes the same setter path’s handling of combined year and month values, but does not establish that it resolves every reported case.

Priority: Normal

Merge Risk: Minimal · up to 189b3

No actionable merge-blocking issue is established for this change; it is ready for normal checks.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check		The description provides a detailed problem statement, fix summary, test results, performance data, and scope notes. It does not follow the repository template because it lacks a WebKit Bugzilla title…	Add the associated WebKit Bugzilla title and URL, include the required review line, and add a template-formatted list of changed files and relevant functions. Ensure the pull request is linked from Bugzilla and uses the required repository …

Passed checks: 3 passed

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly identifies the main change: Date setters now use Date.UTC-style MakeDay normalization with an exact year/month split.

Full details: Description check

Explanation

The description provides a detailed problem statement, fix summary, test results, performance data, and scope notes. It does not follow the repository template because it lacks a WebKit Bugzilla title and link, the required “Reviewed by NOBODY (OOPS!).” line, and the template-style changed-file list. It references only an external Bun issue.

Resolution

Add the associated WebKit Bugzilla title and URL, include the required review line, and add a template-formatted list of changed files and relevant functions. Ensure the pull request is linked from Bugzilla and uses the required repository metadata.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Preview build of 66ac271: autobuild-preview-pr-616-66ac271a

[claude]

Beyond the inline findings, I also checked the int32 wraparound path the old per-argument guards protected against: with makeDay now forcing the month remainder into [0, 11], the only int arithmetic in dateToDaysFrom1970 (year += month / 12, --year) is a no-op and daysFrom1970ToYear is pure double math, so an in-int32 normalised year cannot wrap. localTimeToMS is guarded by canNarrowToInt64Milliseconds and passes NaN/huge values through to timeClip unchanged, and the ms default for setters called without a milliseconds argument is still seeded from this before fillStructuresUsingTimeArgs runs.

Extended reasoning...

The change reworks the Date.prototype setters in Source/JavaScriptCore/runtime/DatePrototype.cpp to route through makeDay/makeTime/makeDate/timeClip (BrokenDownDate fields become double, per-argument range guards removed), adds a month-remainder check to makeDay in Source/JavaScriptCore/runtime/DateConstructor.h, and adds a stress test. It touches no security-sensitive surface. Two confirmed findings (a timezone-dependent test failure and a non-NaN wrong result for |month| >= 2^53) are posted inline; this note only records the additional overflow and NaN-propagation paths examined and ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

[claude]

I re-reviewed the amended push and didn't find any bugs; the local-time case in the test now starts from new Date(2000, 0, 1), which addresses the US/Pacific concern from my earlier comment. Since this changes the arithmetic behind every Date setter and touches makeDay, which Date.UTC, the constructor and the Temporal code share, a human look from a JSC reviewer would still be worthwhile.

What was reviewed:

• In-range equivalence with the old path: makeDay(y, m, d) is dateToDaysFrom1970(y, m, 1) + d - 1 and makeTime matches timeToMS on exact integers, and localTimeToMS does the same conversion gregorianDateTimeToMS did, so previously-accepted inputs keep their results.
• fillStructuresUsingTimeArgs still keeps the sub-second part of this when no milliseconds argument is given (the old milliseconds += *ms else-branch), so setHours/setMinutes/setSeconds semantics are unchanged.
• The new [0, 11] guard in makeDay: all Temporal callers pass month - 1 in [0, 11] and the remainder is exact for |month| < 2^53, so Date.UTC, the constructor and Temporal see no change for normal inputs.
• Every expectation in the existing JSTests/stress/date-timeClip-large-values.js remains consistent with the new makeDay/makeTime/timeClip path.

Extended reasoning...

The change rewrites how the Date setters in Source/JavaScriptCore/runtime/DatePrototype.cpp build their result (BrokenDownDate fields become double, a new toTimeValue runs makeDay/makeTime/makeDate then localTimeToMS and timeClip), adds a remainder guard to makeDay in Source/JavaScriptCore/runtime/DateConstructor.h, and adds one stress test. It touches no security-sensitive surface (no auth, injection or data exposure), only Date arithmetic. The amended commit addressed the blocking test issue from the prior review, and the bug hunt ran dry with no findings. It is not approved outright because it alters core engine builtin semantics with subtle double arithmetic shared by Date.UTC and Temporal, the paths fall under the /Source/JavaScriptCore and /JSTests CODEOWNERS entries, and the test could not be executed in this environment.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[claude]

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.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[claude]

I reviewed the two new commits and found no bugs; both earlier inline findings (the local-time test cases that depended on the US/Pacific zone, and the inexact split for |month| >= 2^53) are addressed in the current code. Because this also changes the observable results of Date.UTC and the Date constructor (NaN for |year| or |month| above 2^53-1, where the spec's exact MakeDay would still yield a number) and the paths are under CODEOWNERS, a human look would still be worthwhile.

What was reviewed:

• makeDayForIntegers: the int64 floor-division split cannot overflow for |month| <= 2^53-1 (year + additionalYears stays far inside int64), the int32 identity check rejects wrap-around, and days + date - 1 keeps the previous makeDay order.
• fillStructuresUsingDateArgs / dateSetterTimeValue: argument conversion still runs in order on the NaN path, the +0 start for a NaN this in setFullYear/setYear is unchanged, and the setter now computes exactly what millisecondsFromComponents in DateConstructor.cpp computes (makeDate, makeTime, localTimeToMS, timeClip).
• Other makeDay callers (Temporal ISOArithmetic, TimeZoneICUBridge, CalendarICUBridge, DurationArithmetic) all pass integral values, so truncating instead of returning NaN for a fractional year/month does not reach them.
• The stress test's local-time assertions start from local-midnight constructor dates or compare against the same local path, so they hold under the TZ forced by run-jsc-stress-tests.

Extended reasoning...

The change touches Source/JavaScriptCore/runtime/DateConstructor.h (makeDay split into isYearOrMonthForMakeDay and makeDayForIntegers), Source/JavaScriptCore/runtime/DatePrototype.cpp (date setters and setYear now build their time value through makeDayForIntegers/makeDate/makeTime/localTimeToMS/timeClip instead of gregorianDateTimeToMS), and adds a 240-line stress test; it touches no security-sensitive surface (no auth, injection, or data exposure). The bug hunt ran dry with no findings, the two commits since the last review resolve both prior inline comments, and the code paths match the existing Date.UTC path. Defer rather than approve because the change alters observable Date.UTC / new Date results beyond 2^53 (a spec-vs-V8 design choice a maintainer should weigh), and /Source/JavaScriptCore and /JSTests are listed in .github/CODEOWNERS.

[claude]

I reviewed the three new commits and didn't find any bugs; both points raised in earlier reviews are now addressed in the code (the exact int64 month split with the 12 * 2^53 bound, and the local-time test cases starting from a local date instead of new Date(0)). Because this changes observable results of Date.UTC and the Date constructor for |year| or |month| beyond 2^53 (number to NaN) and the files are under the jsc-reviewers entry in CODEOWNERS, a human look would still be worthwhile.

What was reviewed:

• makeDayForIntegers: the truncating / and % with the negative-remainder fix-up give floor division for every month count; year + additionalYears cannot overflow int64 within the 12 * 2^53 bound; the int32 check on the combined year matches what the spec's Number addition yields whenever the result is a valid year.
• dateSetterTimeValue vs the old DateCache::gregorianDateTimeToMS path: same day * msPerDay + time, local offset and timeClip sequence; the NaN-start paths of setFullYear/setYear and the argument-conversion order are unchanged.
• The remaining makeDay callers (DateConstructor.cpp, Temporal core) pass integral values inside the new range, so only the >2^53 behaviour changes for them.

Extended reasoning...

The change touches Source/JavaScriptCore/runtime/DateConstructor.h (makeDay split into isYearOrMonthForMakeDay and makeDayForIntegers), Source/JavaScriptCore/runtime/DatePrototype.cpp (the seven date setters now build their time value through MakeDay instead of DateCache::gregorianDateTimeToMS), and a new 248-line stress test. It touches no security-sensitive surface: no auth, injection or data exposure, only Date arithmetic on already-converted doubles. The three commits since the last review resolve both earlier inline findings, and I found no new defect. Approval is withheld because the runtime files and JSTests are covered by the jsc-reviewers CODEOWNERS entry and the change alters observable Date.UTC/constructor results at the 2^53 edge, which a maintainer should weigh.

[claude]

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

Beyond the inline finding, I checked a few other things in this version and found them in order: the local-time assertions in JSTests/stress/date-setters-year-month-range.js now start from locally constructed dates (e.g. new Date(2000, 0, 1)) rather than new Date(0), so the US/Pacific time-of-day mismatch from an earlier review no longer applies; the int64 split in normalizeYearAndMonthForMakeDay makes the 2^56 month case exact, which closes the inexact-split concern raised on an earlier version; and makeDay's switch from NaN to truncation for fractional year/month does not affect any caller, since Date.UTC/new Date trunc their arguments first and the Temporal callers pass integers.

Extended reasoning...

The change rewrites the year/month range handling in makeDay (Source/JavaScriptCore/runtime/DateConstructor.h) and the date setters in Source/JavaScriptCore/runtime/DatePrototype.cpp, replacing per-argument int bounds with a 12 * 2^53 magnitude check and an exact int64 floor-division split, and adds a stress test. It touches no security-sensitive surface. One confirmed spec-divergence finding is posted inline; the note above only records the additional checks that were ruled out this run, including that feedback from earlier versions is reflected in the current code.

[claude]

I reviewed the latest revision (through 0141eac) and found no bugs; the points raised on earlier pushes are addressed in the code (the local-time cases in the stress test now start from new Date(2000, 0, 1), and the year/month split is exact in int64). Because this changes Date.UTC and new Date as well as the setters, and deliberately picks a fork-level limit (NaN above |month| = 12 * 2^53, replacing three upstream guards), a human maintainer should still weigh that choice.

What was reviewed:

• normalizeYearAndMonthForMakeDay / makeDay: with both inputs bounded by 12 * 2^53 the int64 sums cannot overflow, the int32 fit check precedes every narrowing, and dayInYear only ever sees a month in 0..11; the Temporal callers pass int32 years and months 0..11, so they are unaffected.
• dateSetterTimeValue: daysFrom1970ToYear is double arithmetic for any int32 year (at most ~7.8e11 days), so any in-range result needs |monthDay| well below 2^53 and the partial sums are exact as the comment states; a huge monthDay reaches timeClip and gives NaN.
• fillStructuresUsingDateArgs: the int32 fast path matches ToIntegerOrInfinity, -0 takes the double path to 0, and later arguments are still converted after an earlier one fails (pinned by the test's valueOf log).
• Ruled out: a JSTests/BUN-TEST-DIFFERENCES.md entry is not needed, since this is a new fork-only test rather than a changed upstream test.

Extended reasoning...

The change touches Source/JavaScriptCore/runtime/DateConstructor.h (makeDay, shared by Date.UTC, the Date constructor and Temporal) and DatePrototype.cpp (all seven date setters), replacing per-argument year/month bounds with an int64 MakeDay split and an int32 fit check, plus a 270-line stress test. It touches no security-sensitive surface (no auth, injection or data exposure), only numeric date semantics. Deciding against approve: the paths are covered by .github/CODEOWNERS (jsc-reviewers), the PR itself says a maintainer must choose between this design and a wider per-argument bound, and Date.UTC / new Date now return NaN above |month| = 12 * 2^53 where ECMA-262 defines a value, which is a deliberate fork-level limit rather than an obviously mechanical change. No third-party objections are open in the timeline metadata, and the bug hunt ran dry with no findings on this revision.