Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deconstruct for DateTime, DateTimeOffset, DateOnly, and TimeOnly #24601

Closed
Tracked by #78518
jkotas opened this issue Jan 7, 2018 · 60 comments · Fixed by #79499
Closed
Tracked by #78518

Deconstruct for DateTime, DateTimeOffset, DateOnly, and TimeOnly #24601

jkotas opened this issue Jan 7, 2018 · 60 comments · Fixed by #79499
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Jan 7, 2018

Proposal

Allow retrieving the group of date and time parts in one call. This will help with performance and code writing/reading clarity.

API Proposal

namespace System
{
    public readonly partial struct DateTime     
    {
        public void Deconstruct(out int year, out int month, out int day) { }
        public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second) {  }
        public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second, out int millisecond, out int microsecond) { }
    }

    public readonly partial struct DateTimeOffset
    {
        public void Deconstruct(out int year, out int month, out int day, out TimeSpan offset) { }
        public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second, out TimeSpan offset) {  }
        public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second, out int millisecond, out int microsecond, out TimeSpan offset) { }
    }

    public readonly partial struct DateOnly
    {
        public void Deconstruct(out int year, out int month, out int day) { }
    }
    
    public readonly partial struct TimeOnly
    {
        public void Deconstruct(out int hour, out int minute, out int second) { }
        public void Deconstruct(out int hour, out int minute, out int second, out int millisecond, out int microsecond) { }
    }
}

Code Sample

    var (year, month, day) = DateTime.Now;

Old description

From @panost on January 7, 2018 13:42

For example method https://github.com/dotnet/coreclr/blob/0efe34efa69dea7f9b94ddc8251810e0a264671c/src/mscorlib/shared/System/DateTime.cs#L847

could be made public and renamed to Deconstruct, so we can write

var (year,month,day) = DateTime.Now;

The same pattern can be applied to DateTimeOffset and TimeSpan

Copied from original issue: dotnet/coreclr#15776

@aobatact
Copy link

aobatact commented Jan 13, 2018

I would be happy to get a year , month , day at the same time but won't it make confusion between (hour, minute, second) ? Isn't enough to set GetDatePart public?

@panost
Copy link

panost commented Jan 13, 2018

@aobatact You can have Deconstuct overloads for example you can add

Deconstuct(out int year, out int month, out int day, out int hour, out int minute, out int second, out int milliSecond);
Deconstuct(out int year, out int month, out int day, out int hour, out int minute, out second);

but if you only care for the TimeOfDay parts then an additional method is needed such as

public static void GetTimeOfDayParts( this DateTime dt, out int hour, out int minute, out int second, out int milliSecond ) {
    const int MilliSecondsInSecond = 1000;
    const int MilliSecondsInMinute = 60 * MilliSecondsInSecond;
    const int MilliSecondsInHour = 60 * MilliSecondsInMinute;

    milliSecond = (int)( ( dt.Ticks % TimeSpan.TicksPerDay ) / TimeSpan.TicksPerMillisecond );
    hour = milliSecond / MilliSecondsInHour;
    milliSecond -= hour * MilliSecondsInHour;
    minute = milliSecond / MilliSecondsInMinute;
    milliSecond -= minute * MilliSecondsInMinute;
    second = milliSecond / MilliSecondsInSecond;
    milliSecond -= second * MilliSecondsInSecond;
}

// tuple overload
public static (int hour, int minute, int second, int milliSecond) GetTimeOfDayParts( this DateTime dt ) {
    (int hour, int minute, int second, int milliSecond) tuple;

    GetTimeOfDayParts( dt, out tuple.hour, out tuple.minute, out tuple.second, out tuple.milliSecond );
    return tuple;
}

and use it as

var (hour, minute, second, _) = DateTime.Now.GetTimeOfDayParts();

@aobatact
Copy link

aobatact commented Jan 13, 2018

I know that I can have ovreloads to it. My question is how the user know what values are deconstructed. As for other types that can Deconstruct like ValueTuple, Tuple, KeyValuePair, etc... , it is obvious which and what order they are deconsructed (they are deconstructing all the fields), but why can we say that deconstruct with three values means deconstruct to year, month, day ? Why not hour, minute, second ?

@aobatact
Copy link

4
If the description of what values are deconstructed are shown in IDE it might be OK but currently I can't find a way for users to know.

@panost
Copy link

panost commented Jan 13, 2018

Because you do know that DateTime doesn't have a constructor with 3 arguments that are hour,minute,seconds but only one that is year,month,day. The Deconstructor overloads should follow the signature of a public constructor if that's possible

@aobatact
Copy link

Well , that's right. I wonder why I didn't come up with a constructor.

@damageboy
Copy link
Contributor

I just did this on my own in my project, I'd like to point out that this also helps with perf:

BenchmarkDotNet=v0.10.12, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.192)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical cores and 4 physical cores
Frequency=2742186 Hz, Resolution=364.6726 ns, Timer=TSC
.NET Core SDK=2.1.4
[Host] : .NET Core 2.0.5 (Framework 4.6.26020.03), 64bit RyuJIT
DefaultJob : .NET Core 2.0.5 (Framework 4.6.26020.03), 64bit RyuJIT

Method Mean Error StdDev Scaled ScaledSD
BaseLine 29.104 ns 0.2917 ns 0.2728 ns 1.00 0.00
Deconstructed 9.834 ns 0.2300 ns 0.5978 ns 0.34 0.02

I was actually about to open an issue about adding deconstruct in this form when I saw that it's already here...

Also, in my personal view, the DateTime deconstructor should be limited to (y, m d), where users wanting to deconstruct the time elements can use .TimeOfDay and deconstruct that in a separate call.

There would be very little performance impact for users only needing the (h, m, s) etc. elements in doing that, as it should be apparent from looking into the code...

Naturally DateTime can also provide the 6+ element deconstructor, but IMO it just looks unfriendly...

@jkotas
Copy link
Member Author

jkotas commented Jan 18, 2018

Could you please share code for your microbenchmark?

@panost
Copy link

panost commented Jan 18, 2018

The results of the benchmark are expected, since the BaseLine calls the GetDatePart
three times (once for each Year,Month and Date). So BaseLine time =~ 3 * Deconstruct time

@JonHanna
Copy link
Contributor

Also, in my personal view, the DateTime deconstructor should be limited to (y, m d), where users wanting to deconstruct the time elements can use .TimeOfDay and deconstruct that in a separate call.

If we were limited to only one I'd say the opposite, that we should have the fuller deconstruction since callers can choose to ignore what they don't need and it's the more flexible.

Luckily, we're not limited to only one.

@damageboy
Copy link
Contributor

@jkotas: https://github.com/damageboy/datetime-deconstruct

@panost right...

I also have a #if INSANE for doing magic number multiplication which for some reason works faster than what the JIT does in a consistent yet irrelevant way...

Dotnet-GitSync-Bot referenced this issue in Dotnet-GitSync-Bot/corefx Aug 22, 2019
* Mark methods specially handled in JitInterface Intrinsic

For CPAOT compiler's sake.

Signed-off-by: dotnet-bot <[email protected]>
jkotas referenced this issue in dotnet/corefx Aug 22, 2019
* Mark methods specially handled in JitInterface Intrinsic

For CPAOT compiler's sake.

Signed-off-by: dotnet-bot <[email protected]>
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ericstj
Copy link
Member

ericstj commented Jul 29, 2020

Moving this to 6.0 as it's not going to meet the deadline for 5.0. @panost are you still interested in this issue?

@ericstj ericstj modified the milestones: 5.0.0, 6.0.0 Jul 29, 2020
@jkotas

This comment has been minimized.

@KoalaBear84
Copy link

Be sure to also check for DateOnly/TimeOnly: #49036

@panost
Copy link

panost commented Jun 7, 2021

@KoalaBear84 This issue (DateOnly/TimeOnly) is closed, I can't comment there

@ericstj Sorry I didn't read your question a year ago. Yes it will be nice to be added. The code is in this method . Also, it will be nice to be shared with DateOnly (through a common static method accepting dayNumber?)

@ericstj
Copy link
Member

ericstj commented Jun 7, 2021

@tarekgh @mattjohnsonpint: thoughts on this issue?

@ericstj
Copy link
Member

ericstj commented Jun 7, 2021

Related: #19397 which added Deconstruct for DictionaryEntry and KeyValuePair<TKey,TValue>

@tarekgh
Copy link
Member

tarekgh commented Jun 7, 2021

The ask is reasonable here mainly for writing less code getting the various parts of the date and time. And as pointed before it will be more optimized compared to requesting every part separately. We need to look more carefully at the proposal to ensure right signatures. I would keep this for the next release though.

@tarekgh tarekgh modified the milestones: 6.0.0, Future Jun 7, 2021
@mattjohnsonpint
Copy link
Contributor

I agree with Tarek. We should think more carefully about the method signatures.

FWIW, This was also dropped as feedback in comments on my blog post about DateOnly.

@IanMarteens
Copy link

The main reason for having a Deconstruct for DateTime/DateOnly is that calling Year, Month and Day in three different calls is very inefficient. There's a lot of repeated work. I agree that a deconstructor for DateTime could be confusing, but that's not the case with DateOnly.

@panost
Copy link

panost commented Jun 9, 2021

@IanMarteens

a deconstructor for DateTime could be confusing

DateTime already has a constructor with the exact same arguments

@tarekgh
Copy link
Member

tarekgh commented Jun 9, 2021

DateTime already has a constructor with the exact same arguments

It will be not obvious the deconstructor will always match the constructor parameters. I can see users can easily try to get the time parts only and can run into such problem. Imagine someone calling DateTime.Now.TimeOfDay and then trying to deconstruct that.
Deconstructor of DateOnly and TimeOnly can be reasonable to have though.

@tarekgh
Copy link
Member

tarekgh commented Nov 16, 2022

@panost good point. Why return offsetMinutes and not TimeSpan? constructors take TimeSpan and not offset minutes.

@Clockwork-Muse
Copy link
Contributor

... especially since, historically, there have been sub-minute offsets (usually when a timezone transitioned off a local-solar-noon zone).

Although we don't have a way to get the seconds from TimeZoneInfo...

@tarekgh
Copy link
Member

tarekgh commented Nov 16, 2022

@Clockwork-Muse in DateTimeOffset we don't allow sub-minutes offsets. https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs,956.

My question is more about is it useful for users to get the value as TimeSpan or as minutes count. Also, having the constructors are taking TimeSpan is another factor I prefer Having TimeSpan.

@tarekgh
Copy link
Member

tarekgh commented Nov 16, 2022

Thinking more, I am seeing it is better to deconstruct using TimeSpan. This will make it consistent with the constructor. Also, we have another issue #52081 which will expose TotalOffsetMinutes from DateTimeOffset if anyone wants to get this specific value.

@panost let me know what you think about that.

@panost
Copy link

panost commented Nov 17, 2022

I vote for deconstructors that take the most optimal path. I always imagine a use case of serializing (deconstructing) millions of DateTimeOffset fields. So offsetInMinutes is just a copy of the private field. If you want a TimeSpan Offset you can always use the already available Offset property

But this is not a very strong opinion. If you leave the decosntructors as is (without time offset), it is also Ok with me, if #52081 is available. Adding TimeSpan you add one (or three?) unnecessary multiplications for each call

@tarekgh
Copy link
Member

tarekgh commented Nov 17, 2022

Think about it more from the scenario point of view. When users really want to get the value as TimeSpan and when they need to get it as minutes count. In mainstream cases TimeSpan would be more useful. That is why I prefer to keep the TimeSpan.

@panost
Copy link

panost commented Nov 17, 2022

Yes I agree with that view, orthogonal design is a strong point, I mean keeping the arguments (and overloads) that the constructors have.

@tarekgh tarekgh added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 17, 2022
@tarekgh
Copy link
Member

tarekgh commented Nov 17, 2022

I marked the issue ready for the design review.

@tarekgh tarekgh added the blocking Marks issues that we want to fast track in order to unblock other important work label Nov 17, 2022
@terrajobst
Copy link
Member

terrajobst commented Nov 29, 2022

Video

  • We should consider having "simple" deconstructors that return the logical components
    • DateTime -> (DateOnly, TimeOnly)
    • DateTimeOffset -> (DateOnly, TimeOnly, TimeSpan)
  • Conversely, we should consider adding them as constructors
    • DateTime(DateOnly, TimeOnly)
    • DateTimeOffset(DateOnly, TimeOnly, TimeSpan)
  • We should give milliseconds without microseconds
namespace System;

public readonly partial struct DateTime
{
    public DateTime(DateOnly date, TimeOnly time);
    public DateTime(DateOnly date, TimeOnly time, DateTimeKind kind);
    public void Deconstruct(out DateOnly date, out TimeOnly time);
    public void Deconstruct(out int year, out int month, out int day);
    //public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second);
    //public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second, out int millisecond, out int microsecond);
}

public readonly partial struct DateTimeOffset
{
    public DateTimeOffset(DateOnly date, TimeOnly time, TimeSpan offset);
    public void Deconstruct(out DateOnly date, out TimeOnly time, out TimeSpan offset);
    // public void Deconstruct(out int year, out int month, out int day, out TimeSpan offset);
    // public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second, out TimeSpan offset);
    // public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second, out int millisecond, out int microsecond, out TimeSpan offset);
}

public readonly partial struct DateOnly
{
    public void Deconstruct(out int year, out int month, out int day);
}

public readonly partial struct TimeOnly
{
    public void Deconstruct(out int hour, out int minute);
    public void Deconstruct(out int hour, out int minute, out int second);
    public void Deconstruct(out int hour, out int minute, out int second, out int millisecond);
    public void Deconstruct(out int hour, out int minute, out int second, out int millisecond, out int microsecond);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 29, 2022
@deeprobin
Copy link
Contributor

@tarekgh

Since I have already addressed some changes in the classes DateTime, DateOnly & TimeOnly so far, I would very much like to face this API.

If that goes okay, I would be happy if someone of you assigns me.

@tarekgh tarekgh removed the blocking Marks issues that we want to fast track in order to unblock other important work label Nov 29, 2022
@tarekgh
Copy link
Member

tarekgh commented Nov 29, 2022

Thanks @deeprobin for willing to help with the implementation.

@svick
Copy link
Contributor

svick commented Nov 30, 2022

    public void Deconstruct(out DateOnly date, out TimeOnly time);
    public void Deconstruct(out int year, out int month, out int day);

When var (year, month, day) = dateTime; works, is it a problem that var (year, month) = dateTime; does the wrong thing? Especially since cutting off the smaller parts works for deconstructing TimeOnly.

Though it should be fairly obvious what is wrong because of the types involved. And I don't have a better suggestion.

@panost
Copy link

panost commented Dec 1, 2022

@terrajobst I can't imagine a use case for

 public void Deconstruct(out DateOnly date, out TimeOnly time);

I mean, where you will need both of them (date and time) other than in the two use cases I already gave for

public void Deconstruct(out int year, out int month, out int day, out int hour, out int minute, out int second);

also the ctors of DateTime from DateOnly, TimeOnly should include another overload with Calendar

public DateTime(DateOnly date, TimeOnly time, Calendar calendar, DateTimeKind kind)

@tarekgh
Copy link
Member

tarekgh commented Dec 1, 2022

I mean, where you will need both of them (date and time) other than in #24601 (comment) I already gave for

You already gave the example for a scenario need the date and time parts. I can imagine other scenarios like scheduling calendars which will need to extract the date and time. We have avoided having a deconstruct which returns many outputs in this proposal. Also, talking logically, DateTime is really a Date and Time. deconstructing it to these parts makes a lot of sense.

also the ctors of DateTime from DateOnly, TimeOnly should include another overload with Calendar

Note, we have added the new constructors only because we added the deconstructors. If we find any scenario in the future that needs this constructor, we can consider adding it.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 10, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 15, 2023
tarekgh pushed a commit that referenced this issue Jan 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime
Projects
None yet