DelayedFormat error handling is not as optimal

[lifthrasiir]

Currently an invalid formatting string gives Item::Error in the iterator, and it invokes std::fmt::Error on the actual formatting. I initially assumed that, while this does not immediately fail, a vast majority of .format() call will be followed by .to_string() or formatting macros, and they will fail loudly so the user can be easily notified during the initial test.

Actually, my assumption turned out to be wrong: std::fmt::Error is rarely used (std has no use of it AFAIK) and the failure mode for it is not well-tested nor well-explored. For the striking example, ToString::to_string will abruptly stop at the first such error and happily return the string built up to that point. This is very surprising, and I'm yet to find the rationale for this (rust-lang/rfcs#380 and rust-lang/rfcs#504 should be relevant but they are almost silent on this). The root commit causing this traces back to 2014 and I doubt this is a consistent decision.

Given this "relevation", I plan to change the error handling of DelayedFormat. On the initial construction, it will run a copy of the item iterator and panic on Item::Error (actually, StrftimeItems may have a method for checking this). DelayedFormat already requires the iterator to be Cloneable so this won't cause a direct API change, but this will cause a rescanning of the format string. I guess that is not a big deal, but if it is a big deal we may have a unsafe DelayedFormat constructor. The plan is to make this change to 0.3.

[lifthrasiir]

Alex Crichton has confirmed that ToString::to_string implementations are indeed supposed not to trigger such situation. This also means that Chrono's current DelayedFormat implementation is seriously flawed, prompting the subsequent change.

[dashed]

Any progress on this?

I have a usecase where format specifiers are provided by the user, and I had thought that . to_string() would panic on invalid format specifiers; but it doesn't.

[lifthrasiir]

@dashed Not yet. Recently I was busy in the daily job so I was unable to think more about this. (I do the occasional Rust hacking, but new versions of Chrono---and probably also Encoding---are blocked on several non-trivial decisions which I cannot coherently make without a lot of spare time.) I'm glad to take a PR or opinion about this.

[quodlibetor]

I that the best way to handle parsing user format strings is something like this:

    let res = StrftimeItems::new("%Y-%m-%dT%H:%M:%S%.6fZ")
        .map(|item| match item {
            Item::Error => Err(item),
            _ => Ok(item),
        })
        .collect::<Result<Vec<_>, _>>();

We should probably remove the Item::Error variant and make all parse/fallible operations return Result, which would make this more convenient.

[Keats]

I just ran into that, it causes a panic that cannot be avoided when using the idiomatic approach :/
+1 for format to return a Result.

[Nemo157]

There is explicit documentation in std now that returning a self-constructed std::fmt::Error is an "incorrect implementation": https://doc.rust-lang.org/stable/std/macro.format.html#panics

[pitdicker]

Closing in favor of #1127.