Validate Temporal.Duration field magnitudes#328
Merged
Conversation
- Enforce 2^32 calendar-unit limits and 2^53 normalized time limits - Add Temporal.Duration magnitude validation coverage
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds TC39 Temporal.Duration magnitude validation: constructor and Duration.from() now enforce calendar-unit absolute value < 2^32 and normalized time seconds absolute value < 2^53; includes new test coverage and two new error/suggestion resource strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
spec compliance
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 284 improved · 🔴 16 regressed · 64 unchanged · avg +6.9% arraybuffer.js — Interp: 🟢 9, 🔴 1, 4 unch. · avg +5.4% · Bytecode: 🔴 4, 10 unch. · avg -8.6%
arrays.js — Interp: 🟢 17, 2 unch. · avg +7.8% · Bytecode: 🟢 2, 🔴 5, 12 unch. · avg -3.0%
async-await.js — Interp: 🟢 3, 3 unch. · avg +5.8% · Bytecode: 🔴 4, 2 unch. · avg -14.1%
base64.js — Interp: 🟢 7, 3 unch. · avg +7.6% · Bytecode: 🟢 3, 🔴 2, 5 unch. · avg +1.2%
classes.js — Interp: 🟢 24, 7 unch. · avg +5.9% · Bytecode: 🟢 5, 🔴 2, 24 unch. · avg +0.4%
closures.js — Interp: 🟢 11 · avg +10.0% · Bytecode: 🟢 2, 9 unch. · avg +1.1%
collections.js — Interp: 🟢 11, 1 unch. · avg +10.1% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg -0.0%
destructuring.js — Interp: 🟢 21, 1 unch. · avg +7.6% · Bytecode: 🟢 4, 🔴 4, 14 unch. · avg -0.0%
fibonacci.js — Interp: 🟢 8 · avg +8.5% · Bytecode: 🔴 2, 6 unch. · avg -0.7%
float16array.js — Interp: 🟢 25, 🔴 4, 3 unch. · avg +4.2% · Bytecode: 🟢 14, 🔴 3, 15 unch. · avg +1.4%
for-of.js — Interp: 🟢 7 · avg +10.9% · Bytecode: 🟢 3, 4 unch. · avg +0.7%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 27, 15 unch. · avg +5.8% · Bytecode: 🟢 8, 🔴 4, 30 unch. · avg -0.1%
json.js — Interp: 🟢 20 · avg +9.0% · Bytecode: 🟢 4, 🔴 2, 14 unch. · avg +0.3%
jsx.jsx — Interp: 🟢 19, 2 unch. · avg +7.5% · Bytecode: 🟢 1, 🔴 14, 6 unch. · avg -2.8%
modules.js — Interp: 🟢 9 · avg +11.4% · Bytecode: 🔴 2, 7 unch. · avg -1.2%
numbers.js — Interp: 🟢 4, 7 unch. · avg +1.5% · Bytecode: 🔴 5, 6 unch. · avg -2.9%
objects.js — Interp: 🟢 7 · avg +14.2% · Bytecode: 🟢 6, 1 unch. · avg +4.7%
promises.js — Interp: 🟢 11, 1 unch. · avg +7.2% · Bytecode: 🟢 1, 🔴 2, 9 unch. · avg -0.6%
regexp.js — Interp: 🟢 11 · avg +6.4% · Bytecode: 🔴 4, 7 unch. · avg -2.1%
strings.js — Interp: 🟢 5, 🔴 9, 5 unch. · avg -15.0% · Bytecode: 🟢 6, 🔴 1, 12 unch. · avg +16.5%
typed-arrays.js — Interp: 🟢 18, 🔴 2, 2 unch. · avg +8.1% · Bytecode: 🟢 11, 🔴 6, 5 unch. · avg +0.7%
uint8array-encoding.js — Interp: 🟢 10, 8 unch. · avg +25.9% · Bytecode: 🟢 3, 🔴 9, 6 unch. · avg -14.7%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/built-ins/Temporal/Duration/magnitude-validation.js (1)
47-67: Consider adding a negative time overflow test.The calendar unit tests cover both positive and negative boundaries (lines 11-45), but the normalized time overflow tests only cover positive cases. For symmetry and completeness, consider adding a negative time overflow test.
🧪 Optional: Add negative time overflow test
test("moderate day count is valid", () => { // 1000 days = 86,400,000 normalized seconds, well under 2^53 const d = new Temporal.Duration(0, 0, 0, 1000); expect(d.days).toBe(1000); }); + + test("negative days producing normalized seconds <= -(2^53) throws RangeError", () => { + expect(() => new Temporal.Duration(0, 0, 0, -104251000000)).toThrow(RangeError); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Duration/magnitude-validation.js` around lines 47 - 67, Add a symmetric negative overflow test to the Temporal.Duration magnitude-validation suite: mirror one of the positive overflow cases (e.g., the "days producing normalized seconds >= 2^53 throws RangeError" test) by creating a Duration with a large negative field (for example, days: -104251000000) and assert it throws RangeError; reference the Temporal.Duration constructor and the existing positive tests in this file so the new test follows the same pattern and message style (you can also add analogous negative checks for hours and seconds using -2502000000000 and -9007199254740992 respectively if you want full parity).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/built-ins/Temporal/Duration/magnitude-validation.js`:
- Around line 47-67: Add a symmetric negative overflow test to the
Temporal.Duration magnitude-validation suite: mirror one of the positive
overflow cases (e.g., the "days producing normalized seconds >= 2^53 throws
RangeError" test) by creating a Duration with a large negative field (for
example, days: -104251000000) and assert it throws RangeError; reference the
Temporal.Duration constructor and the existing positive tests in this file so
the new test follows the same pattern and message style (you can also add
analogous negative checks for hours and seconds using -2502000000000 and
-9007199254740992 respectively if you want full parity).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6825ee7-e8ba-4376-8f39-0abf48e55fd6
📒 Files selected for processing (4)
tests/built-ins/Temporal/Duration/magnitude-validation.jsunits/Goccia.Error.Messages.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Values.TemporalDuration.pas
- Cover day and hour normalization paths - Assert RangeError at the negative 2^53 boundary
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Temporal.Durationmagnitude checks for calendar fields soyears,months, andweeksreject absolute values at or above2^32.2^53or more.Temporal.Duration.from()object-form validation, including boundary cases just below and at the limits.Fixes #326