Properly throw in case of numeric overflow in Calendar mathematics
#900
Conversation
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
We have applied multiple different strategies with regard to overflow during calendrical calculation, including capping input `Date` and clipping Julian day calculation with artificial bounds. Those are still insufficient as there are too many paths where numeric overflow may happen. This patch patches those places by `throw`ing whenever applicable. Also adds fuzzing tests to cover many more Calendar API. Resolves rdar://133558250
|
@swift-ci please test |
|
@swift-ci please test |
jmschonfeld
reviewed
Sep 6, 2024
| @@ -3257,10 +3257,10 @@ final class GregorianCalendarTests : XCTestCase { | |||
| XCTAssertEqual(cal.range(of: .dayOfYear, in: .year, for: leapYearDate), 1..<367) | |||
|
|
|||
| // Addition | |||
| let d1 = cal.add(.dayOfYear, to: date, amount: 1, inTimeZone: .gmt) | |||
| let d1 = try? cal.add(.dayOfYear, to: date, amount: 1, inTimeZone: .gmt) | |||
There was a problem hiding this comment.
If we don't expect this to throw, could we mark the test as throws and just do try here? That'd mean that if an error is thrown, the test would fail and the failure would indicate the error information rather than just a nil value for d1 which would probably help with debugging
| func test_dateComponentsFromDateOverflow() { | ||
| let calendar = Calendar(identifier: .gregorian) | ||
| let dc = calendar.dateComponents([.year], from: Date(timeIntervalSinceReferenceDate: Double(Int64.max))) | ||
|
|
There was a problem hiding this comment.
Just confirming - was there anything else that needed to be here or is this just testing that this doesn't crash?
iCharlesHu
approved these changes
Sep 6, 2024
|
@swift-ci please test |
jmschonfeld
approved these changes
Sep 16, 2024
cthielen
pushed a commit
to cthielen/swift-foundation
that referenced
this pull request
Nov 8, 2024
…wiftlang#900) * Properly `throw` in case of numeric overflow in Calendar mathematics We have applied multiple different strategies with regard to overflow during calendrical calculation, including capping input `Date` and clipping Julian day calculation with artificial bounds. Those are still insufficient as there are too many paths where numeric overflow may happen. This patch patches those places by `throw`ing whenever applicable. Also adds fuzzing tests to cover many more Calendar API. Resolves rdar://133558250 * Review feedback: Adopt typed throw and fix an oversight * Update tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We have applied multiple different strategies with regard to overflow during calendrical calculation, including capping input
Dateand clipping Julian day calculation with artificial bounds. Those are still insufficient as there are too many paths where numeric overflow may happen.This patch patches those places by
throwing whenever applicable. Also adds fuzzing tests to cover many more Calendar API.Resolves rdar://133558250