Skip to content

Add validity checks on parse#517

Merged
Manishearth merged 3 commits intoboa-dev:mainfrom
Manishearth:validity-parse
Aug 26, 2025
Merged

Add validity checks on parse#517
Manishearth merged 3 commits intoboa-dev:mainfrom
Manishearth:validity-parse

Conversation

@Manishearth
Copy link
Copy Markdown
Contributor

@Manishearth Manishearth commented Aug 26, 2025

https://tc39.es/proposal-temporal/#sec-temporal-iso8601grammar-static-semantics-early-errors requires us to check date/time validity on parse. This does so and adds a test for it.

This also makes ISO limits checks clearer, and adds a separate check_validity() to be used elsewhere. As a further defense in depth mechanism we add some assertions around ISO datetime construction.

I have not attempted to clean up IsoFoo::new_unchecked vs IsoFoo::new_with_overflow(reject) in these functions.

@Manishearth Manishearth requested a review from nekevss August 26, 2025 16:00
@Manishearth Manishearth force-pushed the validity-parse branch 2 times, most recently from 166f1f7 to 7843230 Compare August 26, 2025 16:05
@Manishearth Manishearth merged commit b3c4beb into boa-dev:main Aug 26, 2025
8 checks passed
@Manishearth Manishearth deleted the validity-parse branch August 26, 2025 18:16
@linusg
Copy link
Copy Markdown
Contributor

linusg commented Aug 26, 2025

Was this validated against test262? Updating temporal_rs to 0.0.13 breaks a bunch of leap second tests for me, this change seems to be the cause.

$ git diff --unified=0 tools/test262/results.json | grep '+ ' | grep FAIL
+  "test/built-ins/Temporal/Duration/compare/relativeto-propertybag-timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Instant/compare/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Instant/from/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Instant/prototype/equals/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Instant/prototype/toString/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Instant/prototype/toZonedDateTimeISO/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Now/plainDateISO/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Now/plainDateTimeISO/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Now/plainTimeISO/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/Now/zonedDateTimeISO/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDate/compare/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDate/from/argument-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDate/prototype/equals/argument-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDate/prototype/toPlainDateTime/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDate/prototype/toZonedDateTime/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDate/prototype/toZonedDateTime/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDateTime/compare/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDateTime/from/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDateTime/prototype/equals/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDateTime/prototype/toZonedDateTime/timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainDateTime/prototype/withPlainTime/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainMonthDay/from/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainMonthDay/prototype/equals/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainTime/compare/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainTime/from/argument-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainTime/from/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainTime/prototype/equals/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainYearMonth/compare/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainYearMonth/from/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/PlainYearMonth/prototype/equals/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/compare/argument-propertybag-timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/compare/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/from/argument-propertybag-timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/from/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/prototype/equals/argument-propertybag-timezone-string-leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/prototype/equals/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime/leap-second.js": "FAIL",
+  "test/built-ins/Temporal/ZonedDateTime/prototype/withTimeZone/timezone-string-leap-second.js": "FAIL",

@Manishearth
Copy link
Copy Markdown
Contributor Author

@linusg I validated most of the changes in temporal_rs 0.0.13 against test262, but not this one. I'm working on a fix.

I believe the bug is that we should be checking IsValidDate but not IsValidTime.

@Manishearth
Copy link
Copy Markdown
Contributor Author

#523

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants