Decide types of DateFields::era and DateFields::month_code

[sffc]

As mentioned in #7010, we should consider switching DateFields to use &str for the month code.

But, as mentioned in #7099 (comment), perhaps it should be a &[u8]? But then debug impls and potentially serde derives and such won't work as nicely.

Should we add a dependency on potential_utf and use a type from that crate?

@robertbastian @Manishearth

[Manishearth]

I think &str for short strings is fine, and convenient. I think PotentialUtf is rather heavyweight, and &[u8] is ... okayish but still extra steps. If we're using something typed it should be fully typed (MonthCode, either validated or invalidated), or it should be something easily specified as a literal.

I don't think we have time to create a new public/stable ValidMonthCode type. If that's something we seriously want to try doing we can make from_fields unstable, but i'd rather not.

[sffc]

Even if we get ValidMonthCode, I don't plan to use it in DateFields, since DateFields is very much a pile of unvalidated things. For example, we stopped using NonZeroU8.

I don't necesarilly think we should create DateFields and DateFieldsUtf8 but we could theoretically do such a thing, and the function can take impl Into<DateFieldsUtf8> or something.

We could accept a bare TinyStr? Then we get rid of the lifetime parameter, which seems nice. Doesn't help with FFI, though.

How much worse is writing &b"M01" instead of "M01" really?

potential_utf is a small crate. Why not use it?

Should we create our own little DateFields-specific type to avoid the extra crate import? Maybe one that can eventually be auto-generated with the ICU4X shared macro file that we keep punting?

[Manishearth]

We could accept a bare TinyStr? Then we get rid of the lifetime parameter, which seems nice. Doesn't help with FFI, though.

A bare tinystr is fine: we already have a bunch of APIs that accept TinyStr and then use DiplomatStr over FFI.

How much worse is writing &b"M01" instead of "M01" really?

It's not too bad. I'm not strongly opposed to using bytes here.

potential_utf is a small crate. Why not use it?

Because it's an additional step. I like using it in data, it serves that purpose well. For input types it's an additional step for people to convert: if we're doing that then we should just have a more specific type that is at least self documenting. And we already have an unvalidated MonthCode type.

Basically, either it should be convenient or if we are losing convenience by having a special type, the special type should pull its weight by being specific.

It's less about he additional dependency, more that I don't think PotentialUtf pulls its weight as an input type.

[sffc]

A bare tinystr is fine: we already have a bunch of APIs that accept TinyStr and then use DiplomatStr over FFI.

Citation?

I see only one hit for TinyAsciiStr in ffi/capi and it is MonthCode.

[sffc]

With potential_utf, I think the call site can look like

let fields = DateFields {
    era: Some("ad".into()),
    month_code: Some(b"M01".into()),
}

which seems fine? It's better than TinyStr because it is infallible.

[sffc]

To quantify the impact of using PotentialUtf8 at the call site, I spent 15 minutes making a PR demonstrating it: #7109

[sffc]

Note: potential_utf is a dependency of icu_locale, which is a default but optional dependency of icu_calendar (pulled in by compiled_data)

[sffc]

That code doesn't work (unfortunately) if I use &[u8] instead of &PotentialUtf8:

error[E0277]: the trait bound `&[u8]: From<&str>` is not satisfied

[sffc]

ok let's give the options numbers:

1. &str
2. &[u8]
3. &PotentialUtf8
4. TinyAsciiStr
5. MonthCode for the month code, something else (not sure what yet) for the era code
6. ValidMonthCode for the month code, something else (not sure what yet) for the era code
7. Two structs, one with &str, one with &[u8], and a generic function
8. One struct with a generic type parameter
9. A type like &PotentialUtf8 that is defined in-crate

I'll propose some relative orderings and see if we can all agree on at least these, so that we can eliminate some options:

• 4 > 5 because we skip the step of wrapping the TinyStr.
• 3 > 9 because we already have the dependency in tree.
• 3 > 4 because the conversions are infallible. (others might not agree with me on this)
• 7 > 8 because type inference is wonky on structs with generic type parameters (playground)

So that gets us down to 5 options with distinct pros and cons:

• 1: &str
  • Pro: Ergonomic in Rust
  • Con: Requires extra validation step in FFI
• 2: &[u8]
  • Pro: Friendly to FFI; most general
  • Con: Less ergonomic in Rust; wonky Debug impl
• 3: &PotentialUtf8
  • Pro: Friendly to FFI; somewhat ergonomic in Rust
  • Con: Extra type people need to learn
• 6: ValidMonthCode
  • Pro: Reduces error cases in Date::try_from_fields
  • Con: Maybe less ergonomic in Rust; requires even more mapping code in FFI
• 7: Two structs
  • Pro: Rust gets optimal ergonomics
  • Con: Extra types to learn about and maintain

Thoughts?

[Manishearth]

3 > 9 because we already have the dependency in tree.

agree.

7 > 8 because type inference is wonky on structs with generic type parameters (playground)

agree, but I really don't like 7 either.

Will think more

4 > 5 because we skip the step of wrapping the TinyStr.

I guess, I see MonthCode and ValidMonthCode as the same option with ValidMonthCode being a potential 3.0 thing if we take that route.

3 > 4 because the conversions are infallible. (others might not agree with me on this)

Don't really agree with that one, but not a big deal.

(i'll think about the options later)