Returning fmt::Error in fmt trait impls

1

Somebody posted on the Discord about this unhelpful panic message in chrono:

use chrono::Local;

fn time() -> String {
    Local::now().date().format("%I:%M %p").to_string()
}

fn main() {
    println!("{}", time())
}

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.86s
     Running `target/debug/playground`
thread 'main' panicked at 'a Display implementation returned an error unexpectedly: Error', /rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/alloc/src/string.rs:2213:14

This appears to be because chrono returns Err(fmt::Error) on its own in some circumstances:

I seem to recall seeing discussions in the past about this sort of usage of fmt::Error, and my general understanding of why this unhelpful message appears is that fmt::Error was only ever intended to be generated by fmt::Write impls, not by e.g. fmt::Display.impls.

I was going to file an issue on chrono about this, but when I went to go check the documentation of fmt::Error, I was surprised to see there is no mention of this!

  • Is my understanding correct that this usage of fmt::Error in chrono is discouraged?
  • What alternative can be recommended for such code? (is it just panicking, considering that fmt::Error already causes a panic? (update: see edit below))
  • What documentation or similar resource can be linked to that says something to this effect?

Edit: Hmm, looks like returning fmt::Error is NOT necessarily guaranteed to panic. The panic seen here is specifically in ToString::to_string, and there's also one in format!, because these both return String. However, if one were to instead use write! on an implementation of std::io::Write, then it becomes io::ErrorKind::Custom { kind: Other, error: "formatter error" }. Thus, panicking may not be a good alternative.

2

If this is meant to be called from an object's Display impl using a format string stored in that type, the type should probably assert the format string is valid in its constructor (e.g. the call to .format("...")) instead of arbitrarily later on when the invalid value is used (Display::fmt() via to_string()).

For an example where not doing the assertion when an invalid value is created causes issues for the developer, consider pointers. Often, where the place you crash due to dereferencing a null pointer may be very far away from when it was created. This makes it a massive pain to figure out which line of code introduced the original error.

I'd also argue that methods which say they handle errors (e.g. by returning a Result) should try really hard to avoid panicking. Instead they should return an error or try to soldier on by using some sort of fallback.

As an example, imagine writing a panic hook which logs an error before unwinding... I wouldn't care if a call to std::fmt::Display returned std::fmt::Error when I try to write!() the message to a buffer because I can just drop the message and say "oh well, at least I tried to record the panic".

fn my_panic_hook(info: &std::panic::PanicInfo) {
  let mut buffer = [0_u8; 1024];
  let now = Local::now();

  match write!(buffer, "{}: {}", now, info) {
    Ok(bytes_written) => log_the_error_message(&buffer[..bytes_written]),
    Err(_) => {
      // Oh well, we tried 🤷
    }
  }
}

However if Display panics inside my panic handler I'm going to trigger a double-panic and immediately crash the entire application, instead of just one thread or the stack frames up to the next std::panic::catch_unwind().

3

.format("%")? would be ideal. If the API can't change, then panicking in .format() would at least help catching the problem earlier, with a nicer message.

Alternatively, Chrono could include error in the string it produces like:

assert_eq!("foo (bad format string)", date.format("foo %").to_string());
4

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.