New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RegularYearMonthDayCalculator.AddMonths does not check for overflows. #1227

Closed
chtoucas opened this Issue Nov 30, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@chtoucas
Copy link

chtoucas commented Nov 30, 2018

Hello, I think that the method LocalDate.PlusMonths() can produce dates that are out of range. For instance,

new LocalDate(9999, 12, 31).PlusMonths(1)

does not overflow.

Also, I find it a bit disturbing that PlusYears, PlusMonths and PlusDays exhibit different behaviors:

  • PlusYears() throws an exception of type ArgumentOutOfRangeException
  • PlusMonths() throws an exception of type OverflowException or produces an invalid date
  • PlusDays() overflow (by C#) or throws an exception of type OverflowException

In details (I hope you don't mind if I write it in F#):

// Adding `Int32.MaxValue`.
let date1 = new LocalDate(2018, 11, 30)
// `YearsPeriodField.Add` throws an exception of type `ArgumentOutOfRangeException`.
date1.PlusYears(Int32.MaxValue) |> ignore
// `RegularYearMonthDayCalculator.AddMonths` throws an exception of type `OverflowException`.
date1.PlusMonths(Int32.MaxValue) |> ignore
// `FixedLengthDatePeriodField.Add` throws an exception of type `OverflowException`,
// but it is so because the following unchecked addition fails :
//   `int daysToAdd = value * unitDays`
date1.PlusDays(Int32.MaxValue) |> ignore

// Adding 1 (day, month or year) to 9999-12-31.
let date2 = new LocalDate(9999, 12, 31)
// `YearsPeriodField.Add` throws an exception to type `ArgumentOutOfRangeException`.
date2.PlusYears(1) |> ignore
// No exception raised, the result is simply *invalid*.
// `RegularYearMonthDayCalculator.AddMonths` does not check for overflows.
printfn "%O" <| date2.PlusMonths(1)
// `FixedLengthDatePeriodField.Add` throws an exception of type `OverflowException`,
// verification done by NodaTime in the case: `daysToAdd < 300 && daysToAdd > -300`.
date2.PlusDays(1) |> ignore
@jskeet

This comment has been minimized.

Copy link
Member

jskeet commented Nov 30, 2018

That PlusMonths doesn't throw an exception is definitely a bug, and I'll fix that (and do a patch release of 2.4).

The different exceptions thrown by different methods is "somewhat unfortunate but expected" as documented in https://nodatime.org/2.4.x/userguide/range - although I need to remove the first sentence of that.

@jskeet jskeet self-assigned this Nov 30, 2018

@jskeet jskeet added the bug label Nov 30, 2018

@chtoucas

This comment has been minimized.

Copy link

chtoucas commented Nov 30, 2018

Oh sorry, I didn't this that. Thanks for the info.

@jskeet

This comment has been minimized.

Copy link
Member

jskeet commented Dec 1, 2018

Fixed in master, and cherry-picked to 2.4.x branch, but I haven't done a 2.4.x release yet. As it's a pretty minor fix, I may wait until the next TZDB update and let it get rolled into that. If there aren't any TZDB updates for a while I'll do a manual release instead.

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