Add initial neo datetime FFI

[sffc]

#5940

This is the DateTime version. I plan to do basically the exact same design for:

• Time
• Date
• GregorianDate
• GregorianDateTime
• ZonedDateTime
• GregorianZonedDateTime

See datetime.cpp for usage examples.

This supports both a builder API and a comprehensive static field set API by using the with_fset function I added.

It is all in the FFI crate, which means this is less likely to conflict with some of the other open PRs in the datetime crate (CC @robertbastian).

[robertbastian]

Issue: please replace the existing API instead of adding an additional API. Any Neo name that you introduce now will just need to be cleaned up next week, and any code you don't delete today will have to be maintained until someone gets around to deleting it (we were in this state in the datetime crate for too long, it sucked).

Also, I would like to review a diff, not a brand new API that I manually have to compare to the old one. This applies to both the Diplomat definitions, as well as the C++ and Dart tests.

[Manishearth]

I think the point here is to migrate one API, and then if that goes well migrate the rest with the same pattern, but the APIs are interconnected so it's not possible to do that without "Neo". I guess an alternate route would be to have this PR as a draft to show the new API, get it reviewed but not landed, and then do the whole migration.

[sffc]

Like in Rust, the new FFI API is a different mental model than the old FFI API, and it is not based on the old one. Consider it a new addition to icu_capi. I plan to delete (rather than migrate) the old one, which I am happy to do in the same PR. I am asking you to review the shape of the new API to catch any architectural issues before I spend time copying it into all the rest of the types.

[sffc]

I marked it as draft to indicate that I don't plan to land this in its current form, but I would still like a review of it.

[sffc]

Old C++:

    std::unique_ptr<DateTimeFormatter> any_dtf = DateTimeFormatter::create_with_length(
        *locale.get(),
        DateTimeLength::Medium
    ).ok().value();
    out = any_dtf->format(*any_date.get(), *any_time.get()).ok().value();
    std::cout << "Formatted value is " << out << std::endl;
    if (out != "Oct 5, 2 Reiwa, 1:33\u202fPM") {
        std::cout << "Output does not match expected output" << std::endl;
        return 1;
    }

New C++, static field set:

    std::unique_ptr<NeoDateTimeFormatter> fmt_ymdt = NeoDateTimeFormatter::create_ymdt(
        *locale.get(),
        NeoDateTimeLength::Medium,
        TimePrecision::Minute,
        DateTimeAlignment::Auto,
        YearStyle::Auto
    ).ok().value();
    out = fmt_ymdt->format_iso(*date.get(), *time.get());
    std::cout << "Fieldset YMDT: " << out;
    if (out != "11 jul 2022, 13:06") {
        std::cout << " (unexpected!)";
        saw_unexpected_output = true;
    }
    std::cout << std::endl;

New C++, dynamic field set:

    DateTimeFieldSetBuilder builder;
    builder.length = std::optional<NeoDateTimeLength>(NeoDateTimeLength::Long);
    builder.date_fields = std::optional<DateFields>(DateFields::YM);
    std::unique_ptr<NeoDateTimeFormatter> fmt_ym_bld = NeoDateTimeFormatter::create_from_builder(
        *locale.get(),
        builder
    ).ok().value();
    out = fmt_ym_bld->format_iso(*date.get(), *time.get());
    std::cout << "Fieldset YM in DateTimeFormatter via builder: " << out;
    if (out != "julio de 2022") {
        std::cout << " (unexpected!)";
        saw_unexpected_output = true;
    }
    std::cout << std::endl;

(these examples are not doing the exact same thing but they are all end-to-end formatting call sites)

[sffc]

Some specific things I want comments on:

Field sets are modeled as constructors with positional arguments. For example, in Rust there is a struct with three fields:

https://unicode-org.github.io/icu4x/rustdoc/icu/datetime/fieldsets/struct.MDT.html

In C++, I modeled it as a constructor with the same three positional arguments.

I do not currently have a discriminator in the constructor name as we do in other places. Should I add one? For example, if we add a new semantic option in the future (not unlikely), it would need to be added as a positional argument. We have the _mv1 as an escape hatch, so I don't consider this a hard requirement.

I could also use a struct. I would name the struct based on what it contains, not what it does, so that the same struct can be shared across multiple field set constructors.

Or, I could use the builder struct in the field set constructors, and make the constructor fail at runtime if the builder struct is not compatible with the requested field set.

For example:

// Currently in PR:
// Infallible except for data-related errors:
NeoDateTimeFormatter::create_ymdt(
    *locale.get(),
    NeoDateTimeLength::Medium,
    TimePrecision::Minute,
    DateTimeAlignment::Auto,
    YearStyle::Auto
)

// Alternative A:
// Infallible except for data-related errors:
NeoDateTimeFormatter::create_ymdt_with_length_time_precision_alignment_and_year_style(
    *locale.get(),
    NeoDateTimeLength::Medium,
    TimePrecision::Minute,
    DateTimeAlignment::Auto,
    YearStyle::Auto
)

// Alternative B:
LengthTimePrecisionAlignmentYearStyleOptions options;
options.length = std::optional<NeoDateTimeLength>(NeoDateTimeLength::Medium);
options.time_precision = std::optional<TimePrecision>(TimePrecision::Minute);
options.alignment = std::optional<DateTimeAlignment>(DateTimeAlignment::Auto);
options.year_style = std::optional<YearStyle>(YearStyle::Auto);
// Infallible except for data-related errors:
NeoDateTimeFormatter::create_ymdt_with_options_v1(
    *locale.get(),
    options
)

// Alternative C:
DateTimeFieldSetBuilder builder;
builder.length = std::optional<NeoDateTimeLength>(NeoDateTimeLength::Medium);
builder.time_precision = std::optional<TimePrecision>(TimePrecision::Minute);
builder.alignment = std::optional<DateTimeAlignment>(DateTimeAlignment::Auto);
builder.year_style = std::optional<YearStyle>(YearStyle::Auto);
// Fails if the builder contains options incompatible with the field set, as well as data-related errors:
NeoDateTimeFormatter::create_ymdt(*locale.get(), options)

[sffc]

I will also take feedback on the format functions.

I intend to add the following format functions:

1. format takes the same type as Rust format (matching calendar in GregorianDateTimeFormatter, any calendar in DateTimeFormatter)
2. format_same_calendar has the same behavior as Rust format_same_calendar
3. format_iso exists only in DateTimeFormatter, ZonedDateTimeFormatter, etc (not in the Gregorian version)

We don't have format_iso in Rust but I put it in C++ since we don't have overloading or traits.

Also, please notice that the format functions takes two positional arguments, the date and the time. I did not name the function format_iso_datetime. Instead, the required arguments come from the type. DateTimeFormatter's format functions take a date and a time as positional arguments.

[Manishearth]

Seems like the right approach!

[Manishearth]

I do not currently have a discriminator in the constructor name as we do in other places. Should I add one? For example, if we add a new semantic option in the future (not unlikely), it would need to be added as a positional argument. We have the _mv1 as an escape hatch, so I don't consider this a hard requirement.

I don't want to complicate the ctor name unless we strongly expect us to want to change this in the future, given that the escape hatch is there.

I find alternative A to be more confusing than the current thing, length_time_precision_alignment_and_year_style is a lot of things, and it's not even clear if it's time, precision or time_precision.

I could also use a struct. I would name the struct based on what it contains, not what it does, so that the same struct can be shared across multiple field set constructors.

Mild preference to keeping the API surface here smaller in terms of types created. But not opposed.

Or, I could use the builder struct in the field set constructors, and make the constructor fail at runtime if the builder struct is not compatible with the requested field set.

Not opposed, but I like what you have now.

[sffc]

Action on me:

1. Change the Dart tests to use the new FFI so that Robert can review what it looks like in Dart.
2. (ideally after Rob leaves feedback above, but before then if necessary) Remove the "old" FFI for DateTimeFormatter and migrate the tests to the new FFI

[sffc]

I changed it to optional positional arguments, and I removed the FFI for the field set builder. I will add it back when I add time zone formatting. aa85e53

[robertbastian]

Happy with this API, please go ahead and add the others

[sffc]

Happy with this API, please go ahead and add the others

I'd like to land this PR as-is and follow up with at least two more PRs:

1. Migrate the rest of the non-zoned things (not controversial, just tedious)
2. Add the zoned formatter (needs design)

[robertbastian]

Can you do GregorianDateTimeFormatter now?

[sffc]

Can you do GregorianDateTimeFormatter now?

ok. Done.

[sffc]

Disclaimer: I have only implemented the format_iso function as of now. I may try pushing the other format signatures.