Making default(DateTime) useable

[GULPF]

The problem

The invalid default value of DateTime has caused lots of trouble over the years. This RFC is a proposal for making default(DateTime) useable while at the same time avoiding breaking existing code when possible.

default(DateTime) is an invalid Nim value

Because Nim always uses binary 0 as the default value, default(DateTime) is not a valid value according to the type system. DateTime contains two fields that are not valid when initialized to binary 0: DateTime.monthday (int range 1-31) and DateTime.month (enum starting at 1).

default(DateTime) is an invalid DateTime

Even if the above issue is ignored, the default value of DateTime is logically still not a valid value. There are several reasons for this that I won't go into here, but for example the weekday, utcOffset, isDst, and timezones fields might not be logically valid when initialized to their default values.

This is not a problem in itself, but these invalid DateTime values are handled badly in the times module (read: not really handled at all). If default(DateTime) becomes a valid Nim value, then this value should have a well defined behavior when sent to a proc in the times module.

The solution

default(DateTime) is an invalid Nim value

Without nim-lang/Nim#12378 there's no way set the fields to valid values, so the only way to make this work is to make 0 a valid value. I propose to change the type of the problematic fields to int, which would make 0 a valid value again.

To reduce the amount of breakage this change would cause, I propose that #75 is also implemented. This would mean that

• All fields of DateTime are made private.
• Getters are added for each field. The getters for month and monthday would convert the field to the correct type before returning it.
• Deprecated setters are added for each field. They should be deprecated for the reasons described in [RFC] Updating the fields of a DateTime is unsafe #75. The documentation for the times module already has a warning that the fields should not be assigned to.

With these changes, I think the only significant breakage would be that the DateTime(...) built in constructor will no longer work. In my opinion this is just a benefit, since this constructor has never been safe to use anyway.

default(DateTime) is an invalid DateTime

With the above changes, we can easily add a isInitialized or isValid proc that tells if a DateTime is valid. When receiving an invalid DateTime, the procs in the times module should raise a Defect. The exception might be $ since it's used in debug contexts, so it's probably more useful to just return "Uninitialized DateTime". The setters should also be an exception since they would be deprecated anyway.

This would also be a breaking change, but hopefully it wouldn't break much code in practice. It would break any code that constructs an invalid DateTime value and then tries to do something with it without first assigning the fields. This should be rare, at least I cannot imagine a reason to do that.

[Varriount]

While I do see this as a viable solution, What benefits does this strategy have over adding an "isInitialized" field that all the datetime procedures check for?

[andreaferretti]

Deprecated setters are added for each field

the only significant breakage would be that the DateTime(...) built in constructor will no longer work

If setters are deprecated and the constructor does not work, how one is supposed to create a valid DateTime?

[GULPF]

@Varriount Nim is becoming more strict in rejecting invalid values. var dt: DateTime already results in a compile time error, and default(DateTime) gives a warning. IMO there's no point in supporting default(DateTime) in the times module if it's not supported by Nim.

@andreaferretti I'm referring to the built in object constructor, e.g DateTime(year: 2020). The initDateTime constructor will continue to work.

[Araq]

IMO The fix should be:

• DateTime.month make it start at 0. mJan can easily be mapped to 0 without much harm.
• DateTime.monthday make it start at 0.

Thus I can keep DateTime in hash tables etc. The object constructor allowing invalid values doesn't concern me much, there is all sort of ways to construct non-existing dates, an isValid proc would help.

[GULPF]

@Araq

DateTime.month make it start at 0. mJan can easily be mapped to 0 without much harm.

It would break currently valid code without even causing a compile time error. Besides, month numbering logically starts at 1.

The object constructor allowing invalid values doesn't concern me much, there is all sort of ways to construct non-existing dates, an isValid proc would help.

The only ways to construct an invalid DateTime is by using the object constructor, manual assignment to the fields, or the default value. This RFC removes the first option and deprecates the second, only leaving the default value as a special invalid state. By doing this isValid can be implemented by simply checking if DateTime.monthday == 0, since that's the only invalid state that can exist. If field assignments and the object constructor are allowed, then like you say there can be lots of invalid states. A correct isValid would then require timezone information and would therefor be much slower.

Another benefit of making the fields private is that some of the fields can be removed in the future. For example I doubt that most people ever use the yearday field, but it's calculated on every call to times.now() anyway. If would be much more sensible to just make it a proc that calculates the value on the fly. Making the fields private with deprecated setters is a step towards that.

[andreaferretti]

Nim is becoming more strict in rejecting invalid values. var dt: DateTime already results in a compile time error

I just noticed this. In fact, this breaks csvtools since the macro in there constructs an object and then starts populating it from a serialized value. That is, the generated code looks like

var t: T
t.field1 = deserialize(field1)
t.fieldn = deserialize(fieldn)

When the object has a field of type DateTime this does not work anymore. I assume similar issues will appear with other deserializers.

Is there a suggested solution to this?

[Araq]

There is a workaround:

{.push fieldChecks: off.}
t.field1 = deserialize(field1)
t.fieldn = deserialize(fieldn)
{.pop.}

[andreaferretti]

Well, but already var t: T does not compile if T has a field of type DateTime

[Araq]

Does var t = default(T) work?

[andreaferretti]

Yes, thank you, this fixes it!