[RFC] Updating the fields of a DateTime is unsafe

[GULPF]

The problem

It's current possible to mutate the fields of a DateTime object, but it's very unsafe since it's easy to end up with an invalid state. Examples of invalid states include:

• Dates that are impossible in the gregorian calendar, e.g February 30.
• Date & time that are impossible in the timezone being used, e.g 2018-03-25T02:30 in the Europe/Stockholm timezone (at 02:00 the clock is moved forward one hour).
• Mutating anything without also updating derived fields like isDst and utcOffset.

The API should be modified to ensure that DateTime can only be used in safe ways.

Proposal

Both options below requires all fields of DateTime to be made private to avoid manual construction and direct write access. This is a breaking change of course.

• Option 1: add assignment operators for the fields that are allowed to be mutated. These procs can update the derived fields and prevent invalid states. This is how it's done in D. It's a breaking change for code that writes to the fields.

• Option 2: embrace immutability and add procs for updating fields without mutation, e.g dt = dt.withYear(2015). This is how it's done in Java. Deprecated overloads for field assignments can still be added to prevent breaking old code, which means that this option only breaks code that uses the DateTime(...) constructor.

Note that it only makes sense to update some fields. For example utcOffset and isDst should never be updated directly, so they can be made private without any way to update them. The fields weekday and yearday should likely be removed and replaced with procs that calculate the values on the fly. The only fields that should be allowed to be updated are the date & time fields, e.g year/month/day and so on.

[andreaferretti]

Why should Option 2 break code that uses the DateTime constructor?

[GULPF]

Because the fields of DateTime would be private, so DateTime(year: 2015) would give a compiler error. The same is true for Option 1. The "real" constructor initDateTime will still work.

[andreaferretti]

Ah ok, you're right

[cooldome]

You will be surprised but I will be in the opposite camp here. I have experience working with fancy dates rolling calculations and must say ability to set date 2018-Feb-41 is actually very useful. The date 2018-Feb-41 is not ambiguous, it is actually equal to 2018-Mar-13. The date 2018-Feb-32 can easily appear as a result of some date calculation and what is needed from the library is just normalize(date) function that will turn this user input into more readable form. Library of course should return only normalised dates on its own, but user can go fancy we just need to document how dates are interpreted

edit: negative months and month days are not a problem either 2018-0-(-2) is equal to 2017-11-29

[disruptek]

Why not just allow the construction of 2018-Feb-41 to be internally equivalent to 2018-Mar-13? It seems to me that this is the best of both worlds. I don’t fancy having to normalize a date myself; this is why we have computers...

[andreaferretti]

@cooldome Hopefully, rolling dates should be a task handled by the library, not by manually setting non-existent days and normalizing them. This is why we can do now() + 3.days and so on. Actually, this is most of the point of having a date library

By the way, this would be ambguous, because - say - rolling days then hours can be different from rolling hours then days. Setting both a day and an hour past the natural bounds would result in something which cannot be uniquely normalized

[Araq]

@cooldome The problem with this is that we need to ensure that these strange dates keep being supported by the times library. "It works now" is not good enough, it needs to be documented then and tests ensure it keeps working. And I think some special cases will hurt us and break it.

[krux02]

I am ok with it to add a check proc isValid(arg: DateTime): bool. But to have temporary invalid state of the DateTime object is a feature, not a bug.

[krux02]

I am closing this issue here as rejected. I came to the conclusion that these changes would cause more problems than they would solve.

[krux02]

Ok, I reopened it as I found out, you are the author of this module. But my point stays the same.

• If people want invalid dates, let them have invalid dates.
• Replacing members with getters/setters is not the same. Setting the values via the reflection API will then be illegal.
• as already mentioned, you would break object constructors.
• If you don't want illegal state it should be by construction, for example by using a date object that stores the date in a single number and every number is a valid date, for example the time in seconds since 1970.01.01

[GULPF]

I am closing this issue here as rejected. I came to the conclusion that these changes would cause more problems than they would solve.

Please don't close RFC's this way. RFC's do take some time and effort to write, and seeing them closed because of problems that aren't even specified is disheartening.

If people want invalid dates, let them have invalid dates.

What's the benefit of allowing invalid dates? A good API should prevent wrong usage.

Replacing members with getters/setters is not the same. Setting the values via the reflection API will then be illegal.

It's not the same, but it's the only way to offer a safe API.

as already mentioned, you would break object constructors.

The object constructor for DateTime is not safe, so breaking it is unavoidable if we want a safe API.

If you don't want illegal state it should be by construction, for example by using a date object that stores the date in a single number and every number is a valid date, for example the time in seconds since 1970.01.01

Why? Not being able to design a safe API unless every state of every datatype is safe is a strange limitation.

It should be noted that almost every programming language has a safe API for their DateTime type. Nim is the only exception I can think of...

@cooldome

I don't think that behavior would fit in with how the times module works currently. The times module uses range types heavily (HourRange, MonthdayRange, etc) which forbids that type of date arithmetics.

[narimiran]

Please don't close RFC's this way. RFC's do take some time and effort to write, and seeing them closed because of problems that aren't even specified is disheartening.

+1. And the same goes for easily dismissing PRs.

[cooldome]

I will tell you a real story that did happen. A public organisation (could be any company, millions of legal entities worldwide like this) was providing services to its customers and required passport details to apply. One day a new client comes with a valid passport with birth date 1938-02-30.
IT system rejected his application as birth date was invalid and company told him that he should change his passport as there is mistake in it.

It turned out that It is legally impossible to change your birth date, you can change your name or address but you can't change your date of birth. Government refused him to give a new passport with a different birth date.

The client then sue the company in court claiming he has unlawfully refused in service and he won the case.

I don't think nim stdlib will be usable in a company/public organisations that provides services to human beings if it will be too strict. It is library responsibility to help dealing with all sorts of problems.
The correct implementation should tell that the date is invalid but still allow to proceed if it was necessary.

[GULPF]

@cooldome But can't this be handled during parsing? Allowing 1938-02-30 to be parsed as 1938-03-02 is a lot easier than allowing 1938-02-30 to exist as a DateTime. Just because the user input might be in an invalid form doesn't mean that the data type needs to be able to represent it.

[andreaferretti]

Also, this would not just a problem in representing this date as a DateTime object. It would create problems when stored in the database, when computing age, when sending to external systems....

[github-actions]

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.

[konsumlamm]

See nim-lang/Nim#14197.