Reaffirm TryWriteable design

[sffc]

From #5493

We previously made two design decisions about TryWriteable that might be worth revisiting before 2.0:

1. TryWriteable and Writeable are not implemented at the same time
2. TryWriteable functions return doubly nested Results

My initial position is that I stand by both of these choices, though I could be convinced that we should implement a lossy Display on TryWriteable types.

Anyone want to weigh in?

@zbraniecki @robertbastian @Manishearth

[sffc]

Note: if we implement Display, then we should also plan to implement a concrete to_string.

[robertbastian]

Doubly nested Results are required in case the fmt::Write is infallible, such as String. The outer Result can then be unwrapped and the error case cleanly disappears.

[Manishearth]

A lossy Display seems nice to have. I think the original design of having these be mutually exclusive is fine.

[zbraniecki]

Doubly nested Results are required in case the fmt::Write is infallible, such as String

I'm not sure I agree. If I have a type that is fallible, and I want to display it in println I think lossy Display by default is more expected than having to discover the LossyWrapper.

[robertbastian]

That's not what I'm trying to get at. TryWriteable::write_to_string returns Result<String, Self::Error>, and Self::Error does not need to contain a core::fmt::Error variant (it shouldn't because that's unreachable).

If try_write_to returned an unnested result, the error type would have to be able to represent core::fmt::Error, and then write_to_string would look more fallible than it is (consider the case of Self::Error = Infallible). Unless you want to have two error types for each TryWriteable.

[zbraniecki]

Ah, I see! Thanks for explaining that to me. I haven't consider Infallible. I'm convinced.

[sffc]

What did people think about implementing Display on everything that implements TryWriteable like we do on everything that implements Writeable?

[robertbastian]

Not much, errors should be handled or explicitly ignored.

[sffc]

To continue this conversation:

FormattedNeoDateTime implements TryWriteable. TryWriteable, as noted above, has a funny API signature, and developers are presented with decisions about error handling that they are not in the best position to reason about.

In "normal" call sites, the TryWriteable won't fail. I have an example in the docs where it is possible to make it fail via manual data loading in the lower-level power user types DateTimeNames and DateTimePattern. Getting it to fail is even harder if you come in via the high-level icu::datetime::Formatter APIs; it is possible, but you would need a custom data provider and/or a custom input type. (The custom input type failure case might go away depending on the resolution to #5269)

Experience has sometimes shown that using the lossy behavior is superior to throwing an error. Citation: there was a Google app that had a panic when a unit formatting call failed due to missing data, and it just basically showed up in logs and it took engineers months to get around to investigating it, but meanwhile users in the affected languages were unable to use the app. When the engineers finally tried to reproduce the error, it was unclear what the impact was, because they were unable to see which string in the UI was at fault. If the error had been lossy instead, the bug would have been in the form of screenshots that didn't look right, and users would have still been able to use the app. I know we usually say "loud errors are better", but i18n is all about best-effort.

For the reasons above, I would like to see us move forward with a "lossy" Display impl on TryWriteable types. This also means a concrete to_string function (#4590).

I'd also be happy with a narrower proposal to impl Display specifically on FormattedNeoDateTime but not in general on TryWriteables.

[robertbastian]

If our recommendation is for FormattedNeoDateTime users to just ignore the TryWriteable error, it doesn't need to be a TryWriteable, we can just be lossy. If you give users a to_string method, nobody will ever find the try_to_string and handle the error. I'm fine with doing that, but then we don't need TryWriteable.

[Manishearth]

• @sffc (reiterates comments in issue)
• @robertbastian If the only way to trigger these errors is bad data, do we have to signal the errors? In most of ICU4X we do GIGO without signaling that that happened, why do we put the signal on the API in this case?
• @hsivonen The error situation... is the only way for the error to happen is to do dynamic ICU4X data loading, and you have unvalidated UTF-8 in the data?
• @sffc No, the case is, for example, a Formatter<MonthDay> but you override the pattern data to include a year field
• @hsivonen We've had the principle that we don't panic at runtime but that you do GIGO if you give garbage ICU4X data. What principle makes the datetime exception different than in Collation where you say "look over there but there's nothing over there"?
• @sffc A difference is that we explicitly produce fallback substitution strings. We also tag them with an "error part", so we could just use TryWriteable internally but expose a Writeable API
• @sffc A compromise is that the high-level FormattedNeoDateTime could implement Writeable instead of TryWriteable, and the lower-level FormattedDateTimePattern could continue to implement TryWriteable.
• @robertbastian I'd be okay with that

Proposal:

• If the failure mode happens only with bogus data or incorrect trait impls, prefer Writeable. If the failure mode can happen with default data and valid inputs, prefer TryWriteable.
• As a result, the type currently called FormattedNeoDateTime should implement Writeable instead of TryWriteable, and the type currently called FormattedDateTimePattern should continue to implement TryWriteable.

LGTM: @sffc @robertbastian @Manishearth @hsivonen @nekevss (@echeran)