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

question: defining a Vec<DateTime> with mixed timezones #822

Open
WhyNotHugo opened this issue Sep 18, 2022 · 8 comments
Open

question: defining a Vec<DateTime> with mixed timezones #822

WhyNotHugo opened this issue Sep 18, 2022 · 8 comments

Comments

@WhyNotHugo
Copy link

I need to define a field for one of my types with a collection/list of datetimes which have different timezone types. E.g.: I need something like:

struct MyStuff {
    dates: Vec<DateTime<_>>,
}

And I need to be able to insert DateTime<Utc>, DateTime<Local>, and other chrono::offset::TimeZone implementations.

So far, I've tried using Box<dyn ...>, but this won't compile since the size is not known at compile time.

Is this even possible with the current API? I'm finding the fact that DateTime instances are parametrised with their TimeZone a bit tricky to work with. I've seen an approach of using an enum that implements TimeZone and each enum variant delegates to is associated type -- but this requires my code to implement an enum variant for any potential TimeZone implementation that might be used. This produces an artificial limitation in library code that applications can't use their own TimeZone.

Fox context: I'm parsing icalendar objects, and some fields (like recurrence dates of events) can have multiple date-times in various timezones. This includes Utc, Local (a.k.a. "floating") and timezones encoded into the file which I'll need to decode as a custom TimeZone impementation.

Related: fmeringdal/rust-rrule#85

@esheppa
Copy link
Collaborator

esheppa commented Sep 18, 2022

Interesting question @WhyNotHugo - it may take a bit of back and forth to settle on a good design for this, however that may be useful to inform a potential redesign/simplification of the TimeZone and Offset traits.

From my current understanding of this, I think using an enum is by far the best option and as an added bonus it avoids the generic parameter. You can probably avoid a case for Utc and just use FixedOffset for that which means you only need three cases.

From what I can understand, it would be useful if we made the following changes (I'm not sure whether all are feasible, but they can be investigated):

  • TimeZone was object safe, and we had a blanket impl of TimeZone for Box<dyn TimeZone> and potentially other smart pointers
  • we used FixedOffset everywhere as to avoid the associated type

@WhyNotHugo
Copy link
Author

I'll use an enum for now, but a tricky aspect for this scenario is that icalendar RRULEs depend on VTIMEZONE definitions and vice-versa, so it forces both things to be implemented in the same crate as far as I can tell.

TimeZone was object safe, and we had a blanket impl of TimeZone for Box and potentially other smart pointers

I think making it object safe would be very useful indeed. At a glance, it seems doable, if the next item can be done too.

we used FixedOffset everywhere as to avoid the associated type

I think this is doable. TimeZone::from_offset would have to be dropped, and I'm not sure what would be a suitable replacement since I'm not entirely sure what this method is actually used for.

chrono_tz uses its own custom Offset implementation to carry additional data. Notably, the name and abbreviation of the offset. It would be necessary to consider how this functionality would be implemented if the associated type is always FixedOffset.

@esheppa
Copy link
Collaborator

esheppa commented Oct 1, 2022

I've done some exploration around possible changes to TimeZone to make it object safe, and also looking at using FixedOffset everywhere to avoid the Offset trait in: #830

@djc
Copy link
Contributor

djc commented Oct 3, 2022

Yeah, it seems like using FixedOffset would be a decent option here.

@WhyNotHugo
Copy link
Author

WhyNotHugo commented Jul 21, 2023 via email

@pitdicker
Copy link
Collaborator

@WhyNotHugo Sorry, I did not read closely enough, and thought this was solved.

Fox context: I'm parsing icalendar objects, and some fields (like recurrence dates of events) can have multiple date-times in various timezones. This includes Utc, Local (a.k.a. "floating") and timezones encoded into the file which I'll need to decode as a custom TimeZone impementation.

For most uses converting all values to DateTime<FixedOffset> would be the best solution, but not in your case.


I think changing DateTime to no longer take a generic TimeZone not is something we can do anytime soon, if ever. It would be quite a fundamental change. And it would come with a bunch of different trade-offs on how to ship/load a timezone database with chrono.

For now it comes down to an enum or trait object. The enum option is not acceptable in your case?
Maybe an enum outside the DateTime is easier to use?

enum dates {
    utc: DateTime<Utc>,
    local: DateTime<Local>,
    custom: DateTime<MyTimeZone>,
}

You already found #432.
I personally don't think trait objects are going to be great for DateTime's.
It would need a small allocation for every value, and inflate the base type by 2 pointer-sized values.

@pitdicker pitdicker reopened this Jul 21, 2023
@pitdicker
Copy link
Collaborator

Had a number of months to think about this, and closing again.

As written in the previous post we have basically two options:

  1. Make the TimeZone or Offset traits object-safe.
  2. Create an offset type that is a superset of all possible offset implementations.

1. Make the TimeZone or Offset traits object-safe.

The simplest alternative to our TimeZone trait is something like:

pub struct DateTime<Off: Offset> { /* ... */ }

pub trait Offset: Clone + Debug {
    /// Recreate the offset given a `NaiveDateTime` in UTC.
    fn from_utc_datetime() -> Result<Self, Error>;

    /// Recreate the offset given a `NaiveDateTime` in local time.
    fn from_local_datetime() -> LocalResult<Self>;

    /// Convert this offset to a FixedOffset.
    fn fix(&self) -> FixedOffset;
}

One of the three fundamental methods can't be made object-safe: a method to lookup an offset when given a local datetime. If the local time is ambiguous we want to return two values of the type Self (wrapped in a LocalResult). However an object-safe trait can only ever return a single Self type, by giving it as an input with &mut self.

Also using DateTime with trait objects is just wasteful in terms of size and allocations.

2. Create an offset type that is a superset of all possible offset implementations.

What information would such a type carry? At least

  • Offset to UTC in seconds (FixedOffset).
  • Difference between the offset in DST and the standard offset for the time zone.
  • String or fixed-length string slice with an abbrevation of the time zone.
  • String with the name of the time zone.
  • You may still want to encode where the offset came from. For example does it represent a Local time, and so should it update when the time zone of Local changes?

Both options are not really realistic. You are better off creating an enum wrapping either the DateTimes or the TimeZones that you want to combine into the same Vec.

@pitdicker pitdicker closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2024
@pitdicker
Copy link
Collaborator

pitdicker commented Mar 6, 2024

On second thought:

We could potentially let some Offset::update_for_local_datetime method return a LocalResult<()>, and you would have to call a second method to get the second variant in case the local time is ambiguous. That would only lead to a performance loss in the rare cases where a local time falls within a time zone transition.

pub struct DateTime<Off: Offset> { /* ... */ }

pub trait Offset: Clone + Debug {
    /// Recreate the offset given a `NaiveDateTime` in UTC.
    fn update_for_utc_datetime(&mut self) -> Result<(), Error>;

    /// Create the offset given a `NaiveDateTime` in local time.
    /// Not available when used as a trait object.
    fn from_local_datetime() -> LocalResult<Self> where Self: Sized;

    /// Recreate the offset given a `NaiveDateTime` in local time.
    /// Returns the first result if the local time is ambiguous.
    fn update_for_local_datetime_first(&mut self) -> LocalResult<()>;

    /// Recreate the offset given a `NaiveDateTime` in local time.
    /// Returns the last result if the local time is ambiguous.
    fn update_for_local_datetime_last() -> Result<Self>;

    /// Convert this offset to a FixedOffset.
    fn fix(&self) -> FixedOffset;
}

@pitdicker pitdicker reopened this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants