Update the `TimeZoneProvider` trait

[nekevss]

This issue is meant more for a general discussion for what the trait should look like.

Current

pub trait TimeZoneProvider {
    fn check_identifier(&self, identifier: &str) -> bool;

    fn get_named_tz_epoch_nanoseconds(
        &self,
        identifier: &str,
        local_datetime: IsoDateTime,
    ) -> TemporalResult<Vec<EpochNanoseconds>>;

    fn get_named_tz_offset_nanoseconds(
        &self,
        identifier: &str,
        epoch_nanoseconds: i128,
    ) -> TemporalResult<TimeZoneTransitionInfo>;

    // TODO: implement and stabalize
    fn get_named_tz_transition(
        &self,
        identifier: &str,
        epoch_nanoseconds: i128,
        direction: TransitionDirection,
    ) -> TemporalResult<Option<EpochNanoseconds>>;
}

There are a couple questions here.

1. This doesn't allow for normalizing and fetching a time zone index, which would allow TimeZone to be Copy.
2. It's not clear that a request would require epoch_nanoseconds as all time zone database calculations happen in seconds.

Below is some quick general thoughts on changes that could be made to address the first problem.

pub trait TimeZoneProvider {
    type Index: Clone + Copy + Debug;

    fn is_available_name(s: &str) -> Option<Self::Index>;

    fn get_identifier(index: Self::Index) -> &str;

    // other methods
}

The idea here would be to allow the index to be flexible for others usages. Although, I'd also be fine with it being a more straightforward struct like IdentifierIndex(usize) for example.

As for the second, I would propose moving the TimeZoneProvider trait away from nanoseconds in favor of seconds represented by an i64.

[Manishearth]

Note that if it is a trait object, TimeZone may need to become generic since ideally we store the id in the timezone instead of a string.

Unless we wish to decide to always store iana id in TimeZone and require the trait to convert

[jedel1043]

Wild idea (shamelessly stolen from ICU4X). What if we did something like this instead?

pub trait TimeZoneProvider {
    fn for_identifier(&self, identifier: &str) -> Rc<TimeZoneData>;
}

Where TimeZoneData is an enum of all our supported data formats (tzdb, zoneinfo64), and we use that to implement all the algorithms.

This would remove a LOT of churn, because there would be only TimeZone::new_with_tz_provider and the rest of the *_with_tz_provider methods could be removed.

[nekevss]

That approach wouldn't allow time zone to be Copy though, correct?

I'm not entirely convinced returning the data and passing around an Rc around is ideal.

[jedel1043]

That approach wouldn't allow time zone to be Copy though, correct?

Why do we want it to be Copy again?

I'm not entirely convinced returning the data and passing around an Rc around is ideal.

It works fairly fine for the data loading capabilities of ICU4X, so I think using the same API here shouldn't be that bad. What downsides do you see about carrying an Rc (or Arc if you think it would be better to support multiple threads)?

[Manishearth]

Why do we want it to be Copy again?

It's preferable (cheaper) for FFI. But it's not required.

[nekevss]

I'm not entirely sure how TimeZoneData would work. Primarily because I'm not entirely sure we can reliably represent the data in an enum, nor am I entirely sure we should be representing that data in an enum.

tzdb data could technically be in a wide variety of formats, fat, slim, epoch only, or arbitrarily ranged. Most of the current approach has been specifically not to choose any format but allow whatever format implements the TimeZoneProvider trait, and implement the most common providers.

[jedel1043]

I'm not entirely sure we can reliably represent the data in an enum

How so? We just need to create an enum like the following:

enum TimeZoneData<'a> {
    Tzif(ZeroTzif<'a>),
    ZoneInfo64(ZeroZoneInfo64<'a>),
    // More formats...
}

Then add the same methods as the current TimeZoneProvider to the enum instead.

tzdb data could technically be in a wide variety of formats, fat, slim, epoch only, or arbitrarily ranged

Yeah, but all of those share the common ZeroTzif struct for deserialization, right?

Most of the current approach has been specifically not to choose any format but allow whatever format implements the TimeZoneProvider trait

I don't think we NEED to support all possible timezone providers, just the most commonly used ones. And even if someone has some unsupported timezone format, they can just convert their data to one of our supported formats while we add support for it.

[nekevss]

ZeroTzif is not all the data we need. We'll still need the id normalizer data. Not sure how zoneinfo64 wants to handle that.

Also, this could make the logic needed to handle all cases of zerotzif really complex ... I'm not even sure, we'd have to look into it.

[jedel1043]

We'll still need the id normalizer data.

Can we just bundle this in the binary? We can set the requirement that if a timezone is supported by a provider, it must support fetching it by its normalized id, then we normalize as we require using the bundled data. Wouldn't work per the discussion on #415 (comment), but if we can make it so that the provider also has to resolve the identifier before returning the data, it would work nicely:

fn get_timezone_data(id: &str) -> Rc<TimezoneData> {}

struct TimezoneData {
    normalized_id: String,
    typ: TimezoneDataType
}

enum TimezoneDataType {
    Tzif(ZeroTzif),
    ZoneInfo(ZeroZoneInfo)
}

[Manishearth]

Generally I feel like if we can make things work with Copy identifiers, we should, instead of using an Rc. Rc incurs an allocation cost, and then we need to deal with the thread-safe/non-thread-safe choice.

Rc is convenient for us to work with internally, but the thread safety thing makes it more complex for the user. In general I'm in favor of taking on internal complexity to simplify things for external users.

[jedel1043]

Rc is convenient for us to work with internally, but the thread safety thing makes it more complex for the user. In general I'm in favor of taking on internal complexity to simplify things for external users.

I see it the complete opposite way though. If we make a ref counted pointer our data, its really convenient for users because they need to pass their provider in a single place instead of multiple ones, while we would be complicating the internals of the library, because the onus of implementing the correct algorithms for resolving the correct timezone offset is completely ours.

[jedel1043]

Basically, with our current design the users need to care about both loading the correct data, then processing that data to return the correct offset for a timestamp (assuming they have a custom timezone data format), but with reference counted pointers they would just have to care about converting their data into one of our supported formats, and after that they can delegate the offset resolution logic to the library.

[Manishearth]

Oh, if the goal is to get rid of all of the _with_provider calls, that would be great. I'd be in favor of that.

[nekevss]

I'm really not sure we should make a change like that. I'm aware the with_provider calls are a bit much, but those are really meant to be power user APIs with the wrappers around it being for native use.

A few more points I'd be concerned about:

1. Making this change would hard block temporal_rs from being no_alloc, which I was hoping we'd be able to do this and was slowly working towards that even though it might've meant a change to Calendar.
2. I am really not convinced about the idea of ZeroTzif or the ZoneInfo64 being an variants and we just declare data formats temporal_rs supports and which one's we don't. For one, I think the zoneinfo64 data requires access to the zoneinfo rules, which I guess may be covered with a different type, and ZeroTzif is still very unstable and probably has more iteration to be completed.
3. I'd really prefer to try and remove the time zone logic from temporal_rs wherever possible so that it is reusable outside of temporal_rs.

If someone wants to make a draft PR, I guess we can take a look at it.