Neo DateTime FFI #5940
Comments
AnyCalendar pulls in a lot of data statically, no? We could add I like your first set of six types.
I'd prefer to avoid doing mutation/moving over FFI if possible, there are safety concerns there. Eventually I'd like diplomat to start enforcing these things which would make me a bit more relaxed about it but right now our enforcement basically is "we have very few mutable types and we know how they get used". In particular, I see hazards of mixing this with parts formatting since that involves persisting borrows. Not opposed, but disprefer it.
Quite interested. (Yes, builder APIs also involve mutation, but this is a bit more relaxed: it's abnormal to ever borrow from a builder, builder types exist to be mutated and then consumed, and there are no other operations people typically add)
Requiring additional allocation for each DTF construction doesn't seem great. I do like option 1 and think that validation won't be too hard since Rust exhaustive matching will force us to handle everything. Might be messy, I'm not sure, if I recall correctly the "valid" fieldset combinations are actually pretty easily described.
I think this would be a fine API to have for 2.0. People probably will be wanting this anyway. It's not going to be efficient, but it's a nice API to have and gives us breathing room to design something good if we want. Edit: Shane mentioned that semantic skeleton string formats are not specified in UTS 35, so this probably needs more work and is not an easy way out. May have more thoughts later. |
|
I put up a new builder API in #5944. We now have several ways of specifying skeletons: Comparing and contrasting:
What I think we should do, after a brief conversation with @Manishearth:
Thoughts? @zbraniecki |
Only if the AnyCalendar constructor is used. I propose using the Gregorian constructor and then casting the resulting FixedCalendarDateTimeFormatter into a DateTimeFormatter via into_formatter. I really want a way to allow for time zone selection over FFI without including all time zone data, but I guess for 2.0 we can just say, here's the builder API, and if you want less data, upvote this issue and write a custom FFI fn in the interim. |
|
A few more thoughts on ZonedDateTimeFormatter FFI: I'm only considering options that allow for DCE-based data slicing. Here are some options with C++ pseudocode: // Option 1: build a DTF first and move out of it to get a ZDTF
auto formatter = DateTimeFormatter::create_mdet(locale).with_zone_specific_short();
// Option 1a: don't automatically retain the locale
auto formatter = DateTimeFormatter::create_mdet(locale).with_zone_specific_short(locale);
// Option 2: builder type specifically for ZDTF
auto builder = ZonedDateTimeFormatterBuilder::new(locale);
builder.load_mdet();
builder.load_zone_specific_short();
auto formatter = builder.build();
// Option 2a: similar API to option 1
auto builder = ZonedDateTimeFormatterBuilder::create_mdet(locale);
auto formatter = builder.build_with_zone_specific_short();
// Option 3: builder type for all datetime formatters, replacing type-specific constructors
auto builder = DateTimeFormatterBuilder::new(locale);
builder.load_mdet();
builder.load_zone_specific_short();
auto formatter = builder.build_zoned_datetime();
// Option 3a: a more type-safe version
DateTimeFormatterBuilder::new(locale)
.with_mdet()
.with_zone_specific_short()
.build();A bit more of a description of each:
For any of these options, there are a few mechanical ways to implement the "build" function:
I am currently thinking of exploring Option 3a with RefCell/Mutex gated on a |
|
@Manishearth The It also means we could remove the |
Also I think I said this in person but I am happy with this plan, and do not have strong opinions on the choice of sync mitigation strategy though your latest idea seems liek the best. |
Which clones here will be expensive? The options are all |
Each step of the builder loads data and returns DataErrors (or DateTimeLoadErrors). It's the only way to make data slicing work. |
|
How would that work? You'd have to poke a lot of holes in to the datetime crate. What are the intermediate builder types? Cloning |
I'd probably build it in Rust first: "Could also be motivated as a Rust abstraction, keeping better alignment between Rust and FFI"
In Rust, I wouldn't want to rely on this, since people can make payloads that own their data, but maybe it's okay in FFI? And we tell people that if they are making custom payloads, then don't use this function and make your own FFI wrapper? I'm maybe okay with that. If we do this, then I'd prefer to implement option 1 or 1a. @Manishearth? |
|
From a brief discussion, @Manishearth is okay with this path, and @robertbastian prefers the ergonomics of option 1. I also think option 1 has the best ergonomics, though I am a bit sad to be adding a DataLocale to the heap size of DateTimeFormatter even if you don't normally need it. |
|
Been thinking a bunch about how to implement this in a way that avoids expanding the Rust API in undesirable ways. Here's an idea I had: impl<C, FSet> TypedDateTimeNames<C, FSet> {
pub fn from_formatter(
self,
preferences: DateTimeFormatterPreferences,
formatter: FixedCalendarDateTimeFormatter<C, FSet>
) -> Self {
// pull out the inner field of the formatter; throw away the pattern
}
}
impl<C, FSet> TypedDateTimeNames<C, FSet> {
pub fn try_into_formatter_unstable<P>(
self,
provider: &P,
field_set: FSet,
) -> Result<FixedCalendarDateTimeFormatter<C, FSet>>
/* where P contains FSet's Pattern-related data but not any names*/
{
// load the pattern and check that it matches
}
}This way, we can convert a formatter into a TypedDateTimeNames, load the time zone data, and then convert it back with a new pattern. Note that this is a low-level operation, so I put the APIs on With |
Toward #5940 This allows a client such as FFI to add arbitrary data to a DateTimeFormatter instance. I put the conversion functions and docs on the DateTimeNames types because this is a power-user feature.
Some thoughts on the structure of DateTime FFI.
Types: we might not need very many
The Rust type
DateTimeFormatter<CompositeFieldSet>does everything. We can have many constructors that load different subsets of data. The reason to have multiple types is to reduce stack size (memory usage in FFI) and make the Drop impl a bit smaller, which I think is valuable.It probably makes sense to start with these types:
TimeFormatterTimeFormatter<TimeFieldSet>DateFormatterDateTimeFormatter<DateFieldSet>DateTimeFormatterDateTimeFormatter<CompositeDateTimeFieldSet>ZonedDateTimeFormatterDateTimeFormatter<CompositeFieldSet>GregorianDateTimeFormatterFixedCalendarDateTimeFormatter<Gregorian, CompositeDateTimeFieldSet>GregorianZonedDateTimeFormatterFixedCalendarDateTimeFormatter<Gregorian, CompositeFieldSet>Maybe we don't need the Gregorian types. The only difference is the presence of the
AnyCalendarfield and its associated destructor, which seems to be fairly small in the grand scheme. We can still of course have the Gregorian constructors, but they can resolve to these types.I think we can also have just
DateTimeFormatterand notDateFormatter.Basically: time zone data is way bigger than date data which is way bigger than time data. So if you already have date data, removing time data is mostly in the noise, and if you already have zone data, removing date data is mostly in the noise.
Static Field Set Constructors
For dates, times, and datetimes, I think we can just have all the constructors. There are 20ish.
I think we should flatten the field set options into positional arguments, like we do in some other types. This avoids having a bunch of tiny field set structs like we do in Rust.
For time zones, I see a few options…
DateTimeFormatter, transforming it into aZonedDateTimeFormatter. This would require a bit of work on the Rust side, but I think it's doable.Dynamic Field Set Constructors
The Rust approach is infeasible over FFI, since it is built on the idea of a bunch of little structs that compose into bigger structs and eventually into an all-encompassing dataful enum.
I think we instead want to introduce a builder API. I think we should start in Rust and then port it to FFI. I've had in mind for a while that the builder API would work similarly to the Serde impl.
There are a few approaches for specifying the field sets:
I lean toward option 2 because it is a smaller API surface, but I can be easily persuaded. If option 1 can be implemented more efficiently since it is structs and enums instead of a vector, that would convince me.
Alternatively, we could say that we don't export dynamic field sets over FFI. Clients in each language can build their own delegation mechanism that sits on top of the static constructors. This would be annoying for clients, but it is totally feasible. And if CLDR adds a string semantic skeleton syntax, we can add a single narrow constructor for that.
@Manishearth
The text was updated successfully, but these errors were encountered: