Corefxlab: Consider making casts explicit from DateTime to Date

[mattjohnsonpint]

We currently have implicit casts between DateTime and Date in both directions and between TimeSpan and TimeOfDay in both directions.

While the reasons mentioned in the remarks section of each XML comments are valid, it is worth thinking about making the date ones explicit rather than implicit. Going from a DateTime to a Date truncates the time portion, which could be considered data loss if done unintentionally. Going the other direction introduces `00:00, which is probably acceptable as it doesn't lose any data from the original date.

The same concern is not present in the TimeSpan / TimeOfDay implicit casts. There we have no data loss in either direction.

[mattjohnsonpint]

/cc: @mconnew

[YohDeadfall]

The implicit cast from DateTime to Date is okay because it doesn't throw any exceptions and doesn't lead to overflows. Specifying Date as a parameter or a property type means that you don't care about the time part.

The cast from Date to DateTime should be explicit because Date doesn't have the time part. If someone asks date and time then they both should be provided, not only date.

The casts between TimeSpan and TimeOfDay should be explicit because the types represent different things. Probably these casts should be removed at all, but at the same time they could be useful for backward compatibility (old things use TimeSpan to represent time). Also going from a TimeSpan to a Time could cause overflow.

[benaadams]

The implicit cast from DateTime to Date is okay because it doesn't throw any exceptions and doesn't lead to overflows.

Specifying Date as a parameter or a property type means that you don't care about the time part.

Can't be implicit as it involves data loss; much like float -> int

The cast from Date to DateTime should be explicit because Date doesn't have the time part. If someone asks date and time then they both should be provided, not only date.

This is an int -> float type conversion with no data loss

[YohDeadfall]

At some point you're right :) But what's about the following scenario?

void DoSomething(Date date);

DateTime dt; // which you got somewhere

DoSomething((Date)dt);
DoSomething(dt);

The second involves data loss, but DoSomething doesn't need the time portion and doesn't care about it. In my opinion, we should think different about date and time because DateTime is a composite type even if it stores data as long. It consists of two parts, date and time. But I can be wrong :)

[YohDeadfall]

Imagine that you ask questions and get answers.

What date is it now?

|Answer |Is it accaptable?|Result |
|-------------------|-----------------|----------|
|2009-06-15T13:45:30|Yes |2009-06-15|
|2009-06-15 |Yes |2009-06-15|

What datetime is it now?

|Answer |Is it accaptable?|Result |
|-------------------|-----------------|---------------------------|
|2009-06-15T13:45:30|Yes |2009-06-15T13:45:30 |
|2009-06-15 |No |You know date, but not time|

[Drawaes]

Doesn't matter date and time aren't special here. In as much as datetime is a composite class so is a decimal or a float. There is the number and the number of decimal places.

I always thought the rule was pretty simple, loss of information means explicit cast.

[Drawaes]

From the MS guidelines

Do not provide an implicit conversion operator if the conversion is potentially lossy.
For example, there should not be an implicit conversion from Double to Single because Double has a higher precision than a Single. An explicit conversion operator can be provided for lossy conversions.

[YohDeadfall]

Yep, I know that and at some point agree with others as I said. But I had the idea and shared it because it's a discussion.

[Drawaes]

It's worth discussing and you may have a point. I just expect the bar to be pretty high on breaking a one rule that seems to have withstood the test of time.

[benaadams]

Maybe implicit doesn't makes sense either way and date <-> datetime should both be explicit.

With adding time to date being the suggested route to getting a datetime?

[YohDeadfall]

Yes, it was proposed in https://github.com/dotnet/corefxlab/issues/1740#issuecomment-339598603. Answer from https://github.com/dotnet/corefxlab/issues/1740#issuecomment-340064239:

So I don't think it's quite in alignment to have the + operator mean "addition" in one scenario and "combination" in another scenario, for types that are so closely related.

[benaadams]

Don't necessarily mean + operator e.g date.At(time) is fine

[YohDeadfall]

I though a bit more about my suggestion and found that it isn't good. If the cast from DateTime to Date is implicit then an implicit cast from DateTime to Time should exists too, but it looks like a long to int cast and it isn't intuitive. This is the first reason why the DateTime to Date cast shouldn't be implicit.

The second reason isn't only about implicitness, but also about casts at all. Because Date and Time will store the offset part (see #1901), the conversion from DateTime to Date or Time will use the current calendar if the kind isn't DateTimeKind.Utc.

So I think there are no casts should be and properties/method should be used instead. Something like that:

public struct DateTime
{
    public Date Date { get; }
    public Time Time { get; }
}
public struct DateTimeOffset
{
    public Date Date { get; }
    public Time Time { get; }
}
public struct Date
{
    // because Date and Time have the offset part
    public DateTimeOffset At(Time time) { }
    public DateTimeOffset AtMidnight() { }
}

var date = Date.Parse("2017-11-03+01:00");
var time = Time.Parse("12:00:00+01:00");
// Date and Time to DateTimeOffset
var dto1 = d.AtMidnight();
var dto2 = d.At(t);
// Date and Time to DateTime
var dtUnspecified = d.AtMidnight().DateTime;
var dtUtc = d.AtMidnight().UtcDateTime;
// DateTimeOffset to Date or Time
date = dto1.Date;
time = dto1.Time;
// DateTime to Date or Time
date = dtUtc.Date;
time = dtUtc.Time;

It's more oblivious and intuitive, but there is a blocking issue. DateTime and DateTimeOffset already have Date properties. Maybe the GetDate/ToDate and GetTime/ToTime methods should be implemented instead properties.

What do you think?

[mattjohnsonpint]

Yes, unfortunately the existing .Date property on DateTime and DateTimeOffset can't be changed to return type Date. That's been with us far too long, returning DateTime.

Also, the existing .TimeOfDay properties can't be removed, and I feel like putting a .Time property on the same structs would lead to confusion.

I did already expose .GetDate() and .GetTime() extension methods for both DateTime and DateTimeOffset that return the new types. I'll likely want to convert those to regular methods when merging this into corefx.

I was hoping that the casts would bridge the gaps between the old form and the new, but perhaps it would be clearer/safer if there were just none at all?

[benaadams]

I was hoping that the casts would bridge the gaps between the old form and the new, but perhaps it would be clearer/safer if there were just none at all?

Explicit casts + methods (that do same), no implicit casts?

[mconnew]

Just to throw some chaos into the mix 😀 what about constructor conversion? E.g.

Date myDate = new Date(DateTime.UtcNow);
Time myTime = new Time(DateTime.UtcNow);

[benaadams]

Wrong constructor 😉

DateTime now = DateTime.UtcNow;
Date myDate = (Date)now;
Time myTime = (Time)now;
DateTime myDateTime = new DateTime(myDate, myTime);

[mconnew]

So giving this more than 30 seconds of thought, in the general case of an explicit cast, you are saying "I know there's potentially some loss here, so I'm either okay with the loss, or I know something about the data and I know there won't be any loss (e.g. int32 will only ever have numbers < 1000)" and a cast shouldn't ever throw. Are there any scenarios where casting from a DateTime[Offset] to a Date would throw? What about in the other direction? E.g. what do you do if a Date has a timezone which isn't the local timezone or UTC and want to make a DateTime? Do you make it DateTimeKind.Unspecified? Do you throw? If the answer is ever throw, then you can't convert using a cast and need a method (constructor or otherwise).

[mattjohnsonpint]

If the answer is ever throw, then you can't convert using a cast and need a method

Ok, if that's a hard rule, then the cast from TimeSpan to Time is out because it can thrown an ArgumentOutOfRangeException if the timespan isn't representable as clock-time. For example, negative timespans, or timespans >= 24 hours.

[mattjohnsonpint]

We wouldn't have any casts between DateTimeOffset and Date in either direction. For Date to DateTime there is no data loss, but for DateTime to Date we'd be truncating both time and kind.

[mconnew]

That's my understanding, it's possible it's allowable by CLR spec but I can't remember a cast being allowed to throw other than the cast isn't compatible, e.g. trying to explicitly cast a string to an int. I'll try and see if I can find anything in the spec as I'm not completely comfortable with a "in my experience" justification.

[benaadams]

in the general case of an explicit cast, you are saying "I know there's potentially some loss here, so I'm either okay with the loss, or I know something about the data and I know there won't be any loss

(Date)DateTime -> Date

is like

(int)float -> int

float can have values way out of range of a int; like numbers larger than 2147483648

[mattjohnsonpint]

CA1065: Do not raise exceptions in unexpected locations says not to throw exceptions in implicit cast operators only.

[mconnew]

So I read the guidelines and it says this:

X DO NOT throw exceptions from implicit casts.

It doesn't say the same thing for explicit casts. It does say:

✓ DO throw System.InvalidCastException if a call to a cast operator results in a lossy conversion and the contract of the operator does not allow lossy conversions.

It could be argued that a TimeSpan that is > 24 hours being cast to Time would be lossy as the maximum value you can represent is 24 hours (ignoring leap seconds and other finer points) and you would need to either truncate the TimeSpan value to 24 hours or wrap around so throwing an InvalidCastException would be appropriate because this particular cast has the contract of no loss. So my non-authoritative read on this is any conversions which might throw can't be implicit, any conversion errors which can be interpreted as being unwanted data loss can be an explicit cast but should throw InvalidCastException. Any conversion which could result in an error which can't be interpreted as being caused by unwanted data loss can't be done by a cast operator.

[mconnew]

The guidelines also say:

Many languages do not support operator overloading. For this reason, it is recommended that types that overload operators include a secondary method with an appropriate domain-specific name that provides equivalent functionality.

It lists for cast operators to create the methods To<TypeName>/From<TypeName>. So at the very least all conversions should have methods with this naming convention.

[benaadams]

Out of range aside; TimeSpan -> Time is a bit weird? What does it mean?

Could see
Time.Add(TimeSpan) where it keeps wrapping. e.g. its 14:35 now; what's the time in 26 hrs => 16:35

But not sure how/why 3hrs should return 3:00am

Or why 3:00am would return 3hrs

On the other hand 6am.Difference(3am) => -3hrs seems ok (Method name aside)

[mconnew]

@benaadams, DateTime.TimeOfDay returns a TimeSpan. So we have an existing case of the time being represented as the length of time that has passed since midnight being represented as a TimeSpan. You might want to convert that to a Time. I'm not a big fan of needing to do things like create a Time object which represents midnight and adding the TimeSpan to it for the sake of semantic purity. I would rather see a method Time.FromTimeSpan which the docs specifically say creates a Time object which represents the time as time 0/midnight plus the duration represented by the TimeSpan. Maybe even mention it's intended purpose is to help convert from DateTime.TimeOfDay and that a TimeSpan is not a good type to use to represent Time.

[mattjohnsonpint]

Ok. let's just focus on one of these for now. TimeSpan to Time

If we didn't have to keep the old APIs intact, I'd just make DateTime.TimeOfDay return a Time type. But we do, and it returns a TimeSpan. That's really misleading since timespans are durations, and there could be more or less time elapsed since midnight than represented due to DST and other time zone transitions, if midnight exists at all. So which one or more of these approaches makes this easier on the developer?

Time t = dt.TimeOfDay;                     // implicit cast *
Time t = (Time) dt.TimeOfDay;              // explicit cast
Time t = Time.FromTimeSpan(dt.TimeOfDay);  // static factory method
Time t = new Time(dt.TimeOfDay);           // constructor overload
Time t = dt.GetTime();                     // instance/extension method on DateTime *

// * in current implementation

[mattjohnsonpint]

(I think we are agreed that the implicit cast should go away due to the necessary exception, and that the exception should change from ArgumentOutOfRangeException to InvalidCastException if we allow the explicit cast.)

[mattjohnsonpint]

Also, perhaps:

Time t = dt.TimeOfDay.AsTime();       // instance/extension method on TimeSpan

[mconnew]

You missed a few (not sure which variant the docs are suggesting should exist so providing both):

Time t = TimeSpan.ToTime(dt.TimeOfDay);
// or
Time t = dt.TimeOfDay.ToTime();

I like the simplicity of these options:

Time t = (Time) dt.TimeOfDay;
Time t = dt.TimeOfDay.ToTime();

The docs do say that any casts should have a To or From variant so I see these as being equivalent. Although a cast is simpler and I generally prefer them, I do get annoyed sometimes by the verbosity and number of parenthesis needed with casts. For example (making up the property as I haven't looked at the Time type):

int hour = ((Time)dt.TimeOfDay).Hour;
// vs
int hour = dt.TimeOfDay.ToTime().Hour;

This sort of construct can get unwieldy.

[benaadams]

DateTime.TimeOfDay returns a TimeSpan ...

Oh 😢

So which one or more of these approaches makes this easier on the developer?

I'd go for?

Time t = ts.FromMidnight();                // instance/extension method on TimeSpan
Time t = dt.TimeOfDay.FromMidnight();      // from above
Time t = new Time(TimeSpan sinceMidnight); // constructor overload
Time t = dt.GetTime();                     // instance/extension method on DateTime

[mconnew]

Carrying on with the theme that @benaadams was using:

Time t = Time.Midnight + dt.TimeOfDay;

[mattjohnsonpint]

I don't like emphasizing "time as time since midnight" concept, because:

• Midnight as 00:00 is not valid in every local day (ex. DST spring-forward in Brazil)
• Even when midnight is valid, the "time since" part is misleading, such as when 4:00 is actually 5 hours since midnight (DST fall-back in lots of places).

Also, Time.Midnight would be equivalent to Time.MinValue, which I feel is appropriate given MinValue/MaxValue on other similar types in this domain. I'm not opposed to Time.Midnight also, but then do we make Time.Noon and Time.ThreeOClock as well? 😜

[mconnew]

Although it wouldn't matter in this particular case, can't midnight also be 24:00:00?

[benaadams]

can't midnight also be 24:00:00?

Not in base 24

[benaadams]

Parse should probably parse it though

[mattjohnsonpint]

@benaadams - Yes, exactly. And it doesn't currently. I will create a bug.

[mattjohnsonpint]

https://github.com/dotnet/corefxlab/issues/1909

[mattjohnsonpint]

I've decided the simplest thing to do for now is to change the casts from DateTime to Date and from TimeSpan to Time to be explicit. The other direction is still implicit. Done as part of #1923.

[mattjohnsonpint]

Also made the casts in the other direction implicit. I thought I had done so, but I missed one.

Additionally, since TimeSpan -> Time throws InvalidCastException when the value is unrepresentable, I made the same choice for DateTime -> Date. That is, if there is any non-zero time value in the DateTime, trying to cast it to Date will throw an InvalidCastException.

Now everything is consistent.