fix https://github.com/nim-lang/RFCs/issues/211: var a: DateTime compiles and is usable

[timotheecour]

• fix Making default(DateTime) useable RFCs#211

var a: DateTime now compiles and is usable, eg for delayed initialization (think serialization etc)

This is the simplest approach IMO until we finally implement #12378 to avoid having a broken type system, see my note nim-lang/RFCs#126 (comment)), taking into account backward compatibility and other factors.
var a: DateTime now simply works, and produces a DateTime with a "sentinel" value 0 for monthday and month. assertValidDate will check for it so that toTime will fail if uninitialized

notes

• I intentionally did not add a proc isValid(a: DateTime): bool because there are many ways a DateTime can be invalid, and it'd be expensive to compute if all user cares about is whether a was initialized.

• I intentionally did not add isInitialized, because we should instead use either a != default(DateTime) or a.notDefault using a new proc notDefault in some other module (see [RFC] add isDefault; more general than isNil, isEmpty etc #13526 for details)

• when Default fields for objects #12378 is implemented, we'll simply change this back to MonthdayRange* = range[1..31] and MonthdayRange.default would become 1 instead of 0

alternatives considered

There is little benefit in preventing those sentinel value at CT because there are other ways to produce invalid DateTime objects anyways (eg: think leap years, or April 31), but I still did consider those:

All these (especially A1, and except, to some extent, A4) are breaking changes of the worst kind, where things sometimes produce same results, sometimes don't, depending on how it's used (eg monthday <= 15):

• A1 changing to MonthdayRange* = range[0..30]

• A2 introducing a type MonthdayRange* = range[0.Monthday..30.Monthday] + type Monthday = distinct int + proc to1Based*(a: Monthday): int = a.ord + 1
  still a breaking change, eg for code like myDate.month.int, or for serialization/deserialization routines
  plus lots of work

• A3 introducing a type Monthday* = enum d1, d2, d3, ..., d30 + proc to1Based*(a: Monthday): int = a.ord + 1
  ditto

• A4 make month private and implement via getter setter
  breaking change (eg object ctor would break, taking monthday by reference would break, serialization complications, and many other unrelated cases)

[GULPF]

It would have been preferable if you had discussed this in the RFC instead of opening a PR, now the discussion will be split up in multiple places.

This might be the easiest way to to make var dt: DateTime compile, but it doesn't solve the other issues brought up in the RFC:

• The times module doesn't give any guarantees about how invalid dates are handled
• There's no way for the user to detect invalid dates

Even with this PR default(DateTime) is invalid according to the type system since DateTime.month will be 0, so it's surprising that it even works.

I intentionally did not add a proc isValid(a: DateTime): bool because there are many ways a DateTime can be invalid, and it'd be expensive to compute if all user cares about is whether a was initialized.

As I commented in the RFC, it's not expensive to check if a DateTime is valid as long as the fields are made private. Having a cheap and correct isValid(a: DateTime) is important if we're gonna be able to give any guarantees about how the times module behaves with invalid values. Having a type that can be invalid in many different ways and is incredible expensive to validate is not a good idea.

A4 make month private and implement via getter setter
breaking change (eg object ctor would break, taking monthday by reference would break, serialization complications, and many other unrelated cases)

This is what I proposed in the RFC (except I would make every field private) and I still believe it's the best option.

• It's not safe to use the object ctor for DateTime. DateTime is not just a collection of unrelated fields, it represents an unambiguous point in time + a timezone and it's not possible to enforce that when the fields are public.
• Taking monthday (or any other field) as reference is not safe, for the same reason as above.
• A serialization approach that requires every field of every type to be public doesn't sound very good. Also note that that DateTime.timezone already contains private closure fields, so fully serializing DateTime with that approach is already impossible.

I don't doubt that making the fields private would break some code, but I also believe it will prevent many bugs in the future. It will also make it significantly easier to further improve the DateTime type.

[Araq]

As a compromise I suggest both:

• We merge this PR because it's a "bug fix".
• Later we then change the API in some incompatible way to get the better design for version 1.4.

[timotheecour]

It would have been preferable if you had discussed this in the RFC instead of opening a PR, now the discussion will be split up in multiple places.

discussion is more concrete with a PR that users can see and try, especially wrt estimating amounts of breaking changes by looking at CI; a lot of details only become apparent with an implementation. You can open a competing PR if you think you have a better proposal, and the 2 implementations can be compared.

The times module doesn't give any guarantees about how invalid dates are handled

that's a separate issue and type system can't help (think: leap year and april 31th); there are many ways besides default(DateTime) a DateTime can be invalid.

There's no way for the user to detect invalid dates

that's a separate issue and can be solved by exposing existing times.assertValidDate and making it more correct

Even with this PR default(DateTime) is invalid according to the type system since DateTime.month will be 0, so it's surprising that it even works.

it's currently legal in Nim and doesn't give any warnings; if it becomes illegal or deprecated, this can be changed

As I commented in the RFC, it's not expensive to check if a DateTime is valid as long as the fields are made private
I don't doubt that making the fields private would break some code, but I also believe it will prevent many bugs in the future. It will also make it significantly easier to further improve the DateTime type.

no need to break any code, instead we'd create a new improved DateObj that is designed with lessons learned from DateTime, and share implementation with DateTime. But for now this PR fixes nim-lang/RFCs#211 without breaking code

note

the design for the new DateObj can take some inspiration from https://dlang.org/phobos/std_datetime_date.html#DateTime; it shall have no redundant fields (unlike current design), only private fields (for more flexibility in implementation), and can have a semantically valid default value if needed (but I don't think that's needed). In particular, it can consist of 2 fields like in phobos: a date and a tod. This will make implementation and API cleaner.

[GULPF]

As a compromise I suggest both:

• We merge this PR because it's a "bug fix".
• Later we then change the API in some incompatible way to get the better design for version 1.4.

Sounds good to me

discussion is more concrete with a PR that users can see and try, especially wrt estimating amounts of breaking changes by looking at CI; a lot of details only become apparent with an implementation. You can open a competing PR if you think you have a better proposal, and the 2 implementations can be compared.

Fair enough, but it often seems that opening a PR when there's no consensus derails the discussion to be about that particular implementation instead.

that's a separate issue and type system can't help (think: leap year and april 31th); there are many ways besides default(DateTime) a DateTime can be invalid.

I'm aware that there are many ways a DateTime can be invalid, but if the times module never returns such DateTimes and the user cannot create them with field assignments or obj ctor it wouldn't matter. The only invalid DateTime that could be constructed outside the times module would be default(DateTime).

that's a separate issue and can be solved by exposing existing times.assertValidDate and making it more correct

It cannot be fixed inside assertValidDate. Validating a date is not the same as validating a DateTime, which also contains a timezone. Example: 2020-03-29T02:30 is not a valid date + time for the Europe/Stockholm timezone, because DST causes the clock to move immediately from 02:00 to 03:00. This validation is to expensive to perform on every operation, hence why I want to prevent all invalid states in user code except default(DateTime).