Reconsider conversion operators `DateOnly` <--> `DateTime`

olmobrutall commented 3 years ago

Background and motivation

As part of adapting Signum Framework to .Net 6 I had some friction with the current API for DateOnly / TimeOnly, specially in the LINQ provider. I think similar inconveniences will arise in Entity Framework.

During the design and implementation that @tarekgh was championing, at some point (https://github.com/dotnet/runtime/issues/49036#issuecomment-802237375 / https://github.com/dotnet/runtime/issues/49036#issuecomment-803058176) was decided to remove any implicit or explicit conversion operator between DateTime and DateOnly, and also TimeSpan and TimeOnly. There was some discussion about whether this operators should be implicit or explicit, but not any discussion why they were removed.

I think this is a bad decision for a few reasons:

Conversion operators are more discoverable: Why there is date.ToDateTime(...), but no dateTime.ToDateOnly() and you have to use DateOnly.FromDateTime instead? There is a "DateOnly was invented later" argument that is not evident for a new developer, making DateOnly a second class citizen. For conversion operators, as a consumer, you don't need to worry whether they are implemented in the source or target type.
Is inconsistent with other similar APIs, like public static implicit operator DateTimeOffset(DateTime dateTime).
When casting from DateOnly to DateTime, I see there is a real ambiguity in what DateTimeKind should be used, but can be safely assume that the time is expected to be 00:00:00. The current API however always forces a TimeOnly but is more forgiving on the DateTimeKind part. Why?

But more specific, it makes working in SQL / LINQ scenarios much harder:

Conversion operators can be lifted https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#lifted-operators,

This means if there is a conversion from DateOnly to DateTime, C# compiler creates a conversion from DateOnly? to DateTime? automatically for you (Yeah!):

db.Invoces.Select(inv = new { DateTime = (DateOnly?)inv.InvoiceDate })  //With automatically lifted operator
db.Invoces.Select(inv = new { DateTime = inv.InvoiceDate == null ? null : inv.InvoiceDate.ToDateTime(TimeOnly.MinValue) }) //Current API

db.Invoces.Select(inv = new { Date = (DateOnly?)inv.InvoiceDateTime }) //With automatically lifted operator
db.Invoces.Select(inv = new { Date = inv.InvoiceDateTime == null ? null : DateOnly.FromDateTime(inv.InvoiceDateTime) }) //Current API

You could get some relive by implementing some extension methods:

    public static DateTime ToDateTime(this DateOnly date)
    {
            return date.ToDateTime(new TimeOnly());
    }

    public static DateOnly ToDateOnly(this DateTime dateTime)
    {
           return DateOnly.FromDateTime(dateTime);
    }

     public static TimeOnly ToTimeOnly(this TimeSpan time)
     {
            return TimeOnly.FromTimeSpan(time);
     }

db.Invoces.Select(inv = new { DateTime = (DateOnly?)inv.InvoiceDate })  //With automatically lifted operator
db.Invoces.Select(inv = new { DateTime = inv.InvoiceDate == null ? null : inv.InvoiceDate.ToDateTime() }) //With custom extension methods
db.Invoces.Select(inv = new { DateTime = inv.InvoiceDate == null ? null : inv.InvoiceDate.ToDateTime(TimeOnly.MinValue) }) //Current API

db.Invoces.Select(inv = new { Date = (DateOnly?)inv.InvoiceDateTime }) //With automatically lifted operator
db.Invoces.Select(inv = new { Date = inv.InvoiceDateTime == null ? null : inv.InvoiceDateTime.ToDateOnly() }) //With custom extension methods
db.Invoces.Select(inv = new { Date = inv.InvoiceDateTime == null ? null : DateOnly.FromDateTime(inv.InvoiceDateTime) }) //Current API

And in theory you could simulate lifted operators using ?., but nowadays you can't and 2 (Buuuh!!!), so you need to add a few overloads more to make it usable with nullable types:

        public static DateTime? ToDateTime(this DateOnly? date)
        {
            return date == null ? null: date.Value.ToDateTime(new TimeOnly());
        }

        public static DateTime? ToDateTime(this DateOnly? date, DateTimeKind kind)
        {
            return date == null ? null : date.Value.ToDateTime(new TimeOnly(), kind);
        }

        public static DateOnly? ToDateOnly(this DateTime? dateTime)
        {
            return dateTime == null ? null : DateOnly.FromDateTime(dateTime.Value);
        }

        public static TimeSpan? ToTimeSpan(this TimeOnly? time)
        {
            return time == null ? null : time.Value.ToTimeSpan();
        }

        public static TimeOnly? ToTimeOnly(this TimeSpan? time)
        {
            return time == null ? null : TimeOnly.FromTimeSpan(time.Value);
        }

db.Invoces.Select(inv = new { DateTime = (DateOnly?)inv.InvoiceDate })  //With automatically lifted operator
db.Invoces.Select(inv = new { DateTime = inv.InvoiceDate.ToDateTime() }) //With custom extension methods for nullables
db.Invoces.Select(inv = new { DateTime = inv.InvoiceDate == null ? null : inv.InvoiceDate.ToDateTime() }) //With custom extension methods
db.Invoces.Select(inv = new { DateTime = inv.InvoiceDate == null ? null : inv.InvoiceDate.ToDateTime(TimeOnly.MinValue) }) //Current API

db.Invoces.Select(inv = new { Date = (Date?)inv.InvoiceDateTime }) //With automatically lifted operator
db.Invoces.Select(inv = new { Date = inv.InvoiceDateTime.ToDateOnly() }) //With custom extension methods for nullables
db.Invoces.Select(inv = new { Date = inv.InvoiceDateTime == null ? null : inv.InvoiceDateTime.ToDateOnly() }) //With custom extension methods
db.Invoces.Select(inv = new { Date = inv.InvoiceDateTime == null ? null : DateOnly.FromDateTime(inv.InvoiceDateTime) }) //Current API

This is a total of 8 extension methods required that, together with the current API, has to be implemented in the LINQ provider to convert to and from the new types. Something that SqlServer or Postgrees do without getting on the way (database interoperability was mentioned in the initial presentation of the feature).

What was the argument for removing the implicit/explicit operators @terrajobst?

Finally, if this operators are reconsidered, I would also restore public static DateTime operator +(DateOnly d, TimeOfDay t) for similar arguments: Discoverability, working with nulls, consistency with DateTimeOffset and similarity to SQL.

API Proposal

Restore:

  public static explicit operator DateOnly(DateTime dateTime) { throw null; } 
  public static implicit operator DateTime(DateOnly date) { throw null; }
  public static DateTime operator +(DateOnly d, TimeOfDay t) { throw null; }

  public static explicit operator TimeOfDay(TimeSpan timeSpan) { throw null; }
  public static explicit operator TimeOfDay(DateTime dateTime) { throw null; }

API Usage

db.Invoces.Select(inv = new { DateTime = (DateOnly?)inv.InvoiceDate })  //With automatically lifted operator
db.Invoces.Select(inv = new { Date = (Date?)inv.InvoiceDateTime }) //With automatically lifted operator

Risks

I don't see any additional risk if explicit operators are used "if the conversion is potentially lossy". https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/operator-overloads#conversion-operators

The conversion from DateOnly to DateTime assumes that:

Time will be 00:00:00: completely reasonable, just like 5 (int) converts to 5.0 (double).
Kind will be DateTimeKind.Unspecified: already assumed by the simplest overload of ToDateTime.

All the other conversions should be uncontroversial.

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

tannergooding commented 2 years ago

@tarekgh, could you provide some additional input here. I think some of this has already shipped so we can't change it.

tarekgh commented 2 years ago

@tannergooding I think the ask here is to add operators to the DateOnly and TimeOnly. These were in the original proposal of the APIs, and it was decided to remove them. Unfortunately, the review on March 18, 2021, somehow not recorded which captures the details of the discussion.

@bartonjs do you recall the reasons deciding to remove the operators on these types? I recall you were the one asking for that :-)

bartonjs commented 2 years ago

The general reasoning would be

Operators (especially conversions) should have named equivalents, because operators are not discoverable.
You have a named version, do you really need an operator?
Implicit operators are good at creating ambiguity in overload resolution... are you confident that adding them won't cause problems?

olmobrutall commented 2 years ago

The general reasoning would be

Operators (especially conversions) should have named equivalents, because operators are not discoverable.

Yes, I don't suggest to remove them.

You have a named version, do you really need an operator?

I think the world needs it more than the effort it is to build it. The alternative is up to 8 extension methods on each application.

Implicit operators are good at creating ambiguity in overload resolution... are you confident that adding them won't cause problems?

Empirically, previously to this commit I know about 10 applications that were working OK with the operators defined in Date.cs.

Currently DateTime has no conversion operations and there is no way to add them (extension operators), so I can not foresee any overloading problem. What I am missing?

bartonjs commented 2 years ago

Currently DateTime has no conversion operations and there is no way to add them (extension operators), so I can not foresee any overloading problem. What I am missing?

I don't see a particular problem right now, but let's just imagine that there's a method group somewhere which overloads both DateTime and DateTimeOffset, and that DateOnly gained implicit conversion operators to both of those:

public partial class SomeType
{
    public void SomeMethod(DateTime start, ...);
    public void SomeMethod(DateTimeOffset start, ...);
}

calling that method group and passing a DateOnly would have two candidates of equal weight (both require an implicit conversion via a user-defined (not base class, interface, or special math relationship) conversion operator). So if there already was at least one implicit conversion from a DateOnly to something else then adding another is risky (I don't think that applies here, but I was just giving general "questions we ask in API review that makes people conclude to not have conversion operators").

There's one more question/reason that does apply here, to me, that I didn't say before, and that's "does it do what someone would expect?". DateOnly represents a date. If it becomes a DateTime then it gains an arbitrary midnight... one of three possible midnights (Local, UTC, Unknown). At any given callsite where you passed what looks like a "pure date", but it's getting converted to a DateTime, are you going to get the output you desire? The date portion could flow forwards or backwards a day depending on your local vs UTC offset, and what DateTimeKind the operator picks.

I believe this last question/reason is actually why the operators got removed: we got rid of anything that assumed midnight (the conversion methods require you to specify a TimeOnly and an optional DateTimeKind). While "assume midnight" might work for particular libraries/applications, it's not something that necessarily generalizes well to the ecosystem.

Clockwork-Muse commented 2 years ago

While "assume midnight" might work for particular libraries/applications, it's not something that necessarily generalizes well to the ecosystem.

Also - Not all days have midnight. There are multiple timezones where DST skips midnight, and going to some other time (usually 01:00).

(Strictly speaking this is only a problem because of the existence of DateTimeKind. If that was removed the transformation to DateTime would be safe, but the resolution into a particular timezone might not be, and would require adjustment)

olmobrutall commented 2 years ago

The operator DateTime -> DateOnly: it is explicit because of lossy precision. This means there is no overloads ambiguity and also no midnight problem. Right?
The operator DateOnly -> DateTime: could we make it explicit at least? It will also avoid the overload resolution problem and will make the midnight assumption more aware.

About the problem of not every day having midnight, this is a problem that the named method is already solving when calling it with date.Value.ToDateTime(new TimeOnly());, and a problem that SQL Server or other DBMS are already solving.

Clockwork-Muse commented 2 years ago

About the problem of not every day having midnight, this is a problem that the named method is already solving when calling it with date.Value.ToDateTime(new TimeOnly());, and a problem that SQL Server or other DBMS are already solving.

This is false, in these ways:

date.Value.ToDateTime(new TimeOnly()); leaves DateTime.Kind as Unspecified, so it has no idea whether there's even a timezone involved, or if so, which one.
date.Value.ToDateTime(new TimeOnly(), DateTimeKind.Local); just uses raw ticks, which doesn't validate that the resulting time is valid for the local time at all.

DateTime performs no validation as to whether the inputs you give constitute a valid time in the local timezone, nor whether any date/time math should adjust for DST (DateTimeOffset doesn't either, but doesn't need to because it's only offset-based, there's no implicit timezone involved). You will get an invalid time with these methods. Alas, .NET F/Core doesn't have a built-in way to perform the necessary resolve to a valid time for you, you'd have to do it yourself. The only time DateTime references timezone information is during DateTime.Now (or conversion to/from UTC); after that you're on your own.

Handling of timezones in SQL Server or any other DBMS is usually poor, especially if you want to deal with actual timezones and/or future dates, not just offsets. It also requires you be using one, and that apparently you want to route all date/time values through the DB, which is not great for performance. Too, this assumes that the timezone definitions are the same on both the DB and the application server, which isn't guaranteed (preferably rely on just one, usually the application server). Using a DB is not a magic bullet here, and may not help - in server applications your timezone had better be UTC for safety reasons, which means you can't rely on any Local shenanigans anyways.

If you're doing any serious date/time work, especially involving future dates and multi-timezone scheduling, you're going to have a much easier time using NodaTime.

MisinformedDNA commented 2 years ago

The general reasoning would be

Operators (especially conversions) should have named equivalents, because operators are not discoverable.

I disagree on discoverability. I think there are plenty of devs who would try an implicit or explicit conversion before looking for a named conversion.

You have a named version, do you really need an operator?

Yes, that's why explicit operators exist. The explicit operator would handle null conversions whereas the named conversion does not. I'd also agree with OP that an explicit operator would be more discoverable than DateOnly.FromDateTime()

Implicit operators are good at creating ambiguity in overload resolution... are you confident that adding them won't cause problems?

I don't think implicit operators should be added, just explicit ones.

At the very least, I think these two named conversions should also have explicit operators:

DateOnly.FromDateTime() - Explicit cast from DateTime to DateOnly
TimeOnly.FromDateTime() - Explicit cast from DateTime to TimeOnly

StijnOttenVMT commented 1 year ago

As this hasn't been closed and there hasn't been activity for almost a year. What is the reason these few lines of code ended up not being implemented?

Is it the explicit implicit debate?

Why not implement explicit now and later debate on changing it to implicit? Since changing from explicit to implicit isn't a breaking change while the other way around is.

I get needing to be sure before making changes to dotnet but come on DateOnly and TimeOnly have really been treated as second class since they were introduced.

tannergooding commented 1 year ago

What is the reason these few lines of code ended up not being implemented?

It being a few lines of code does not necessarily mean it is simple to do.

Why not implement explicit now and later debate on changing it to implicit? Since changing from explicit to implicit isn't a breaking change while the other way around is.

It is a binary breaking change to change explicit conversions to implicit conversions. It is only non binary-breaking if you expose implicit in addition to any existing explicit.

However, it is often source breaking to expose new implicit conversions regardless, because it can introduce new ambiguities as part of overload resolution. For example, if you have a type C with implicit conversions to both A and B and a method group that takes A and B, but not C; then you will get an error since C could be implicitly converted to either A or B and there is then no "best match".

I get needing to be sure before making changes to dotnet but come on DateOnly and TimeOnly have really been treated as second class since they were introduced.

They are new types. It is only natural for there to be some growing pains as the ecosystem moves to adopt them and discover the best patterns and approaches to support them.

This can be particularly painful when you have a net new type compared to a type that's existed for the full 20 year lifetime of the ecosystem, and thus have a large amount of existing code to consider compatibility and interoperability around.

Before anything can move forward, the API proposal should be updated to be "proper" and declare the exact types the APIs will be on. The pattern we've established for which type exposes conversions is that T exposes conversions from itself to another type (e.g. Int128 exposes explicit operator UInt128(Int128 value); while UInt128 exposes explicit operator Int128(UInt128 value);).

The API proposal is currently:

  public static explicit operator DateOnly(DateTime dateTime) { throw null; } 
  public static implicit operator DateTime(DateOnly date) { throw null; }
  public static DateTime operator +(DateOnly d, TimeOfDay t) { throw null; }

  public static explicit operator TimeOfDay(TimeSpan timeSpan) { throw null; }
  public static explicit operator TimeOfDay(DateTime dateTime) { throw null; }

The proposal should be something more like the following:

namespace System;

public partial struct DateOnly
{
    // Lossless conversion, can be implicit
    public static implicit operator DateTime(DateOnly date);

    public static DateTime operator +(DateOnly d, TimeOnly t);
}

public partial struct DateTime
{
    // Lossy conversion (loses the time part), must be explicit
    public static explicit operator DateOnly(DateTime dateTime);

    // Lossy conversion (loses the date part), must be explicit    
    public static explicit operator TimeOnly(DateTime dateTime);
}

public partial struct TimeSpan
{
    // Domain conversion, must be explicit
    public static explicit operator TimeOnly(TimeSpan timeSpan);
}

The implicit conversion for DateOnly->DateTime might warrant some minimal discussion just because new implicit conversions can be tricky. There should be no ambiguities introduced, however, given its the only implicit operator on either type.

The explicit conversion for TimeSpan->TimeOnly might warrant discussion as well. This isn't necessarily a "lossy conversion" but rather more like a "domain conversion" where the concepts do not necessarily line up, but can abstractly be thought to carry similar information. That is, it is very similar conceptually to a Point vs Vector; where the Point (which would be akin to TimeOnly) represents an exact position; where-as a Vector (which would be akin to TimeSpan) represents a directional offset from a given point. Thus, a point is functionally a vector from 0, 0, 0; much like a TimeOnly is functionally a TimeSpan from 00:00.00000. However, these aren't necessarily something you would want to expose as conversions and only using named APIs may make more sense.

The addition operator DateTime = DateOnly + TimeOnly is also something that's "interesting" and potentially worth discussion. Not only are other operators potentially missing, but it breaks from the normal T in, T out pattern. This pattern is broken in a few places for the various Date/Time types, so it might be acceptable. But it'll be a discussion point regardless. Notably potentially missing are:

public partial struct TimeOnly
{
    // Should time/date addition be commutative
    public static DateTime operator +(TimeOnly t, DateOnly d);

    // TO - TO = TS, so logically TO + TS should produce a TO
    public static TimeOnly operator +(TimeOnly t, TimeSpan s);
}

One could also argue that there should be an explicit conversion from TimeOnly->DateTime or from TimeSpan->TimeOnly to ensure parity between the conversions. One directional conversions often do not make sense and may represent a gap in functionality.

MisinformedDNA commented 1 year ago

I don't see the point of DateTime = DateOnly + TimeOnly, new DateTime(DateOnly, TimeOnly) makes more sense to me. Also, what is the use case for conversions between TimeSpan and TimeOnly?

StijnOttenVMT commented 1 year ago

Also, what is the use case for conversions between TimeSpan and TimeOnly?

Personally I can see some use there because TimeOnly doesn't implement AddTicks() but that's more a problem with the constant refusal of the dotnet Devs to add any quality of life improvements to TimeOnly and DateOnly.

(On that topic why does it take years to integrate it into EF Core and ASP.NET)

Aside from that point TimeOnly and TimeSpan do both keep track of time but since they represent inherently different things they're indeed very much like the given example of Point and Vector but even worse since TimeSpan can contain more information than TimeOnly can

tannergooding commented 1 year ago

but that's more a problem with the constant refusal of the dotnet Devs to add any quality of life improvements to TimeOnly and DateOnly.

There's no refusal to add APIs. There's two API suggestions that I can see (this one and https://github.com/dotnet/runtime/issues/62413) and both have had some engagement with feedback from the area owner leaving some open questions and feedback. There was then little to no follow up engagement from the original proposer/community.

I don't see the point of DateTime = DateOnly + TimeOnly, new DateTime(DateOnly, TimeOnly) makes more sense to me.

Different things make sense to different people. Some people prefer named methods and other casting operators, for example ;). That's why I explicitly called out the ones I did as potentially needing more discussion, if not here then in API review.

Ultimately, this moving forward is dependent on the @dotnet/area-system-datetime owners marking it ready-for-review when they believe its in the correct shape with the necessary questions answered. But helping drive the design can largely be done by anyone with enough interest, provided there is consistent engagement from both sides.

MisinformedDNA commented 1 year ago

very much like the given example of Point and Vector

But there is no explicit or implicit cast operator for Point and Vector, is there?

MisinformedDNA commented 1 year ago

public static TimeOnly operator +(TimeOnly t, TimeSpan s); would have to define overflow scenarios.

Corey-M commented 1 year ago

I don't see the point of DateTime = DateOnly + TimeOnly, new DateTime(DateOnly, TimeOnly) makes more sense to me.

I don't see the point of having TimeSpan.Add(TimeSpan), TimeSpan operator +(TimeSpan, TimeSpan) makes more sense to me.

We all have different ways that we approach problems. No one way is the right way.

Also, what is the use case for conversions between TimeSpan and TimeOnly?

TimeSpan has been used to represent time of day since the very first draft of C#, and we've spent the last 20+ years treating it as such. Mountains of legacy code exist in which TimeSpan is used that way, and currently we have to do all sorts of things to convert to/from TimeOnly to use it with that legacy code.

And then there's the fact that TimeOnly current has extremely limited functionality. It has internal resolution in the ticks range (100ns) but unlike TimeSpan there are no granular Add methods - apparently we never need to add seconds or milliseconds to a TimeOnly, but we can add fractional hours or minutes. TimeSpan has a much richer interface.

Going forward we're going to see a lot more instances of TimeOnly in APIs, and I'm really not looking forward to all the hoops you apparently want me to jump through in order to marry those APIs to legacy APIs that don't support TimeOnly.

public static TimeOnly operator +(TimeOnly t, TimeSpan s); would have to define overflow scenarios.

In all cases where arithmetic operations exist as named methods (Add in this case) the corresponding operator should act exactly the same as the method. Since we already have a TimeOnly.Add(TimeSpan) method, there is no further discussion needed: TimeOnly.Add(TimeSpan) handles overflow by truncation, so operator +(TimeOnly, TimeSpan) should do the same.

StijnOttenVMT commented 12 months ago

TimeSpan has been used to represent time of day since the very first draft of C#, and we've spent the last 20+ years treating it as such. Mountains of legacy code exist in which TimeSpan is used that way, and currently we have to do all sorts of things to convert to/from TimeOnly to use it with that legacy code.

Yeah it is kinda absurd that for 20 years TimeSpan has been seen as the default way to represent the time of day and now that we have an actual time of day struct TimeOnly there are no explicit and implicit conversions between them to easily support legacy APIs. And honestly there are still enough modern APIs and libraries, some of which even being offical microsoft libraries, that haven't updated to TimeOnly and DateOnly

TimeOnly.Add(TimeSpan) handles overflow by truncation, so operator +(TimeOnly, TimeSpan)

Would it make sense to then also implement an operator checked +(TimeOnly, TimeSpan) that throws an OverflowException? And if so would it then make sense to instead return a child of OverflowException that contains an int overFlowAmount?

mkorsukov commented 4 months ago

DateOnly type is so useful in daily use cases, so I think about an addition to DateTime and DateTimeOffset types in the form of ToDateOnly() instance method.

Here is code snippets:

DateTime.cs

public DateOnly ToDateOnly() =>
    DateOnly.FromDateTime(this);

DateTimeOffset.cs

public DateOnly ToDateOnly() =>
    DateOnly.FromDateTime(_dateTime);

Can't create a PR, so unit tests included.

DateTimeTests.cs

[Fact]
public static void ToDateOnly()
{
    DateTime dateTime = new DateTime(2024, 06, 13, 11, 55, 45, 0, DateTimeKind.Utc);
    Assert.Equal(new DateOnly(2024, 06, 13), dateTime.ToDateOnly());
}

DateTimeOffsetTests.cs

[Fact]
public static void ToDateOnly()
{
    DateTimeOffset dateTimeOffset = new DateTimeOffset(new DateTime(2024, 06, 13, 11, 55, 45, 0, DateTimeKind.Utc));
    Assert.Equal(new DateOnly(2024, 06, 13), dateTimeOffset.ToDateOnly());
}

dotnet / runtime