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

Add DateTime.UnixEpoch and DateTimeOffset.UnixEpoch fields #23747

Closed
TylerBrinkley opened this issue Oct 5, 2017 · 37 comments
Closed

Add DateTime.UnixEpoch and DateTimeOffset.UnixEpoch fields #23747

TylerBrinkley opened this issue Oct 5, 2017 · 37 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@TylerBrinkley
Copy link
Contributor

Occasionally I come across a point in my code where it'd be useful to have a DateTime.UnixEpoch field as opposed to specifying my own with new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc). Usually this is when I'm calculating expiration dates for web tokens where the expiration property is the number of seconds since the Unix epoch (see https://en.wikipedia.org/wiki/Unix_time for definition).

Proposed API

 namespace System {
     public struct DateTime {
+        public static readonly DateTime UnixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
     }
     public struct DateTimeOffset {
+        public static readonly DateTimeOffset UnixEpoch = new DateTimeOffset(1970, 1, 1, 0, 0, 0, TimeSpan.Zero);
     }
 }

Example Usage

public class TokenIntrospectionResponse
{
    public int ExpirationInSeconds
    {
        get => (int)((ExpirationUtc - DateTime.UnixEpoch).TotalSeconds);
        set => ExpirationUtc = DateTime.UnixEpoch.AddSeconds(value);
    }

    public DateTime ExpirationUtc { get; set; }
}
@TylerBrinkley TylerBrinkley changed the title Add DateTime.UnixEpoch Field Add DateTime.UnixEpoch and DateTimeOffset.UnixEpoch fields Oct 5, 2017
@Clockwork-Muse
Copy link
Contributor

:winces:
I get where you're coming from... but I feel like this is almost certainly going to cause programmers who don't know any better to reinforce the assumptions that DateTime represents an absolute point, or that doing the subtraction is going to work in all cases. That is, for the same reason people are surprised that:

var u = DateTime.UtcNow;
var l = DateTime.Now;
// Prints "07:00:00" for me, is the current offset from UTC
Console.WriteLine(u - l);

@tarekgh
Copy link
Member

tarekgh commented Oct 5, 2017

@Clockwork-Muse I think we already have this problem with DateTime and that is why we are encouraging people to be careful when using it or just use DateTimeOffset. I am not seeing the proposal will make the situation worse. whoever going to use UnixEpoch are the people who really are doing new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc) today.

Do you think having UTC in the name can help a little with the issue you are mentioned? or renaming the property to something more descriptive can help?

@jnm2
Copy link
Contributor

jnm2 commented Oct 5, 2017

Do you think having UTC in the name

It would be redundant because there's no such thing as a local time Unix epoch. That would fit better in intellisense docs, wouldn't it?

@tarekgh
Copy link
Member

tarekgh commented Oct 5, 2017

@jnm2 I agree with you. I am thinking how we can mitigate @Clockwork-Muse concern or at least help people not to run into such problem.

@Clockwork-Muse
Copy link
Contributor

@jnm2 - I agree with you, the Unix epoch is an absolute stamp, so adding "Utc" would be redundant.
The problem is that DateTime is a "local" value, even with DateTimeKind.Utc; it's a local stamp in the UTC zone. So the property would be returning something claiming to be absolute, but the rest of the behavior of the type acts like a local/relative value (for example, consider how comparisons work).

@tarekgh - my preferred solution would be getting an all-up new date/time library, as has been previously discussed. Barring that, I'd put it on DateTimeOffset, and let people wanting a local stamp version do the extra work themselves.

@tarekgh
Copy link
Member

tarekgh commented Oct 5, 2017

The problem is that DateTime is a "local" value, even with DateTimeKind.Utc

This is not true. the ticks inside the DateTime still corresponds to UTC. The problem you raised is only happening if you have an operation that mixes different DateTimeKind.

I'd put it on DateTimeOffset, and let people wanting a local stamp version do the extra work themselves.

I am not seeing hurting adding the property to DateTime too. people who want to use that can still run to the exact same problem you mentioned. my preference here is to have the property on DateTime too.
The docs and the intellisense text can clarify this is UTC date and not absolute value.

@Clockwork-Muse
Copy link
Contributor

This is not true. the ticks inside the DateTime still corresponds to UTC. The problem you raised is only happening if you have an operation that mixes different DateTimeKind.

Sorry, when I said "local", I didn't mean DateTimeKind.Local. I meant "the value is a local timestamp to an assumed zone". And that's why there're problems that result when mixing kinds;

var u = new DateTime(2017, 10, 5, 0, 0, 0, DateTimeKind.Utc);
var l = new DateTime(2017, 10, 5, 0, 0, 0, DateTimeKind.Local);
// prints "636427584000000000|636427584000000000"
Console.WriteLine(u.Ticks + "|" + l.Ticks);
// prints "True"
Console.WriteLine(u == l);

....because the ticks in DateTime are offsets from the same calendar point in the local zone, not the same global/universal instant, as they would be normally.

And there's another thing: Ticks have nothing to do with UTC. Ticks represent an offset from an epoch (what is unimportant), and represent an absolute stamp... or are supposed to. The common credo is that they're offsets from UTC, but that doesn't actually matter: If I chose 1969-12-31 16:00:00 PST as my epoch start, so long as we talked in ticks it doesn't matter that we used different (seeming) reference points. For that matter, you could use tick offsets to converse with aliens having no concept of our calendar or timezones, or us theirs (assuming we could agree on tick length, which is an interesting physics problem, but w/e)

@tarekgh
Copy link
Member

tarekgh commented Oct 6, 2017

Thanks @Clockwork-Muse for the clarification.

@jnm2
Copy link
Contributor

jnm2 commented Oct 6, 2017

For that matter, you could use tick offsets to converse with aliens

Tell them a tick is 919.263177 periods of the radiation corresponding to the transition between the two hyperfine levels of the ground state of the cesium 133 atom. Though presumably the aliens would be so far away that time dilation comes into play and tracking time would become more complex yet. (@jskeet better get Noda Time ready for intergalactic calendars? 😃)

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Oct 7, 2017

I am perfectly happy with this proposal.

@jskeet
Copy link

jskeet commented Oct 7, 2017

Me too. I dread to think about how many places I've written that code myself :(

@karelz
Copy link
Member

karelz commented Oct 10, 2017

Maybe naive question, but why is it called UnixEpoch?
Is there some RFC specifying that? Industry standard? Or is it Unix OS specific?

@stephentoub
Copy link
Member

@karelz
Copy link
Member

karelz commented Oct 10, 2017

Thanks! Updated top post with the link.

@terrajobst
Copy link
Member

terrajobst commented Oct 17, 2017

Video

Looks good as proposed. We may want to add the ToUnixXxx and FromUnixXxx to DateTime as well though.

@karelz
Copy link
Member

karelz commented Oct 17, 2017

FYI: The API review discussion was recorded - see https://youtu.be/BEBq3__WfDc?t=2343 (13 min duration)

@adriangodong
Copy link
Contributor

I can take this and work 10/21-10/22. I'll yield to any first time contributor.

@TylerBrinkley
Copy link
Contributor Author

@terrajobst the addition of the ToUnixXxx and FromUnixXxx methods to DateTime were discussed in #17213 but the issue was closed as it was deemed that adding them would encourage users to continue using DateTime instead of DateTimeOffset. Additionally there were some questions about how ToUnixXxx would work with a DateTime with a DateTimeKind.Unspecified, i.e. should it throw an exception, or assume local or Utc.

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2017

@adriangodong thanks for willing doing this. Let me know if there is anything I can help you with.

@karelz
Copy link
Member

karelz commented Oct 19, 2017

I spoke with @tarekgh and the problem of To/FromUnixXxx is more about consistency with all other DateTime behavior and therefore expectations the API would set.
Adding these 2 methods would be one way or another inconsistent with some other behaviors of DateTime: Addition/substraction always results in DateTimeKind.Unspecified, while TimeZoneInfo.ConvertTimeToUtc always treats the source as DateTimeKind.Local. Moreover DateTime itself never converts timezones (this would be the first one). We would have even harder time explaining why it behaves the way we implement it (incl. that people should not use DateTime because of these exact reasons).

From that reason I agree that leaving the substraction as manual operation is probably best. We should not add the To/FromUnixXxx APIs.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Oct 19, 2017

@karelz

Addition/subtraction always results in DateTimeKind.Unspecified

That is incorrect.

  • Addition or subtraction in the form of DateTime ± TimeSpan = DateTime has the same Kind as the input DateTime operand.
  • Subtraction in the form of DateTime - DateTime = TimeSpan ignores the Kind of both operands.

TimeZoneInfo.ConvertTimeToUtc always treats the source as DateTimeKind.Local

That is also incorrect. The documentation describes this correctly, which is as follows:

For ConvertTimeToUtc(DateTime):

  • Input having DateTimeKind.Local converts from local time to UTC
  • Input having DateTimeKind.Unspecified also converts from local time to UTC
  • Input having DateTimeKind.Utc is passed through without any conversion

For ConvertTimeToUtc(DateTime, TimeZoneInfo)

  • Input having DateTimeKind.Local converts from local time to UTC only if the time zone is TimeZoneInfo.Local, otherwise it throws an exception.
  • Input having DateTimeKind.Utc is passed through without conversion only if the time zone is TimeZoneInfo.Utc, otherwise it throws an exception.
  • Input having DateTimeKind.Unspecified is converted from the time zone given to UTC.

Moreover DateTime itself never converts timezones (this would be the first one)

Not exactly true. DateTime.Now converts from UTC to local time, as does DateTime.ToUniversalTime, and DateTime.ToLocalTime converts from local time to UTC. Since the proposed APIs do not deal with other time zones, I see no difference between them and these.

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2017

Some clarifications as it looks there were some confusions:

  • When said add/subtract result Unspecified kind, was trying to point to any operation that involves 2 DateTime objects tends to ignore the Kind property. e.g comparing 2 DateTime objects.

  • For TimeZoneInfo.ConvertTimeToUtc, was trying to say if the Unspecified kind DateTime was treated as Local.

  • for DateTime itself never converts timezones (this would be the first one), meant that when doing any operation on the DateTime (e.g. adding days, ticks...etc.) the Time zone is not considered in such calculations. in other words, DateTime internal calculations mostly ignoring the time zone.

To summarize, what Karel listed still hold as reasons we need to avoid exposing ToUnix/FromUnix functionality on DateTime.

@mattjohnsonpint
Copy link
Contributor

How is making a built-in of new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc) making those things any worse? At least this is better in many ways than new DateTime(1970, 1, 1) - which is how it is often done in the wild. Consider:

DateTime dt = unixepoch.AddSeconds(myUnixTimestamp);
DateTime eastern = TimeZoneInfo.ConvertTimeBySystemTimeZoneId(dt, "Eastern Standard Time");

This works as expected when unixepoch has DateTimeKind.Utc. But if it's implemented without the kind, then it has DateTimeKind.Unspecified, and is treated as local time by the conversion function, resulting in the wrong answer.

Having a built-in DateTime.UnixEpoch can't hurt anything, it can only help.

As I pointed out in #17213, the behavior of DateTime is already problematic in the ways you described. But people create DateTime values from UTC-based sources all the time. In this case, it's from a Unix timestamp, but the behaviors are not different than if I just parsed an ISO8601 string with a Z at the end, or if I just constructed a new DateTime directly with UTC-based values.

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2017

How is making a built-in of new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc) making those things any worse?

Nobody said that. everyone agree it is useful to have it. This is why it is approved by the design committee.

Karel and myself's comments were regarding the request exposing ToUnixXxx and FromUnixXxx on DateTime.

Anyway, I think we all agree on:

  • it is good to have unixEpoch
  • it is better to avoid ToUnixXxx and FromUnixXxx exposed from DateTime

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Oct 19, 2017

Ok. Then lets move the rest of the discussion over to #17213, or open a new one if you think it's appropriate. Thanks for clarification.

@adriangodong
Copy link
Contributor

I've done an initial scan, so changes needed are:

Are these changes correct?

@karelz
Copy link
Member

karelz commented Oct 21, 2017

And tests. Otherwise it sounds reasonable.

@tarekgh
Copy link
Member

tarekgh commented Oct 23, 2017

Note that after you get the change to coreclr repo, it should get merged to corert repo too https://github.com/dotnet/corert/tree/master/src/System.Private.CoreLib/shared/System. so the changes need to be done in corefx repo should wait for the packages of coreclr and corert be built and merged to corefx first.

@TylerBrinkley
Copy link
Contributor Author

If it's alright with @adriangodong I'd like to take this on, though I'm unsure of how to get the coreclr changes merged to the corert repo as @tarekgh has mentioned.

@tarekgh
Copy link
Member

tarekgh commented Nov 6, 2017

@TylerBrinkley when you change anything in coreclr repo under the shared folder (e.g. https://github.com/dotnet/coreclr/tree/master/src/mscorlib/shared/System) it'll get automatically open a PR in corert repo to mirror the changes. so basically no other work need to be done more than ensuring the changes are merged and the packages (of coreclr and corert) are built and referenced in corefx. we can help you with that as needed.

@tarekgh
Copy link
Member

tarekgh commented Nov 6, 2017

CC @safern who monitor the change monitoring between coreclr and corert

@TylerBrinkley
Copy link
Contributor Author

You can unassign me as @adriangodong already submitted a PR for the coreclr changes, he just never linked it back to this proposal.

@tarekgh
Copy link
Member

tarekgh commented Nov 13, 2017

dotnet/corert#4905

@tarekgh
Copy link
Member

tarekgh commented Nov 16, 2017

@adriangodong did you have a chance to enable the added APIs in corefx repo?

@adriangodong
Copy link
Contributor

@tarekgh Yep, dotnet/corefx#25235

@TylerBrinkley
Copy link
Contributor Author

Can we add the netfx-port-consider label to this issue?

@tarekgh
Copy link
Member

tarekgh commented Nov 16, 2017

Can we add the netfx-port-consider label to this issue?

The added APIs should be included in the next wave of creating the new netsatndard version and support it on the full framework.

CC @weshaggard @AlexGhiondea

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 20, 2020
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 help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests