rust-lang / project-error-handling

Error handling project group
Apache License 2.0
268 stars 18 forks source link

default error formatting in `fn main() -> Result<(),Error>` using Debug instead of Display #39

Open ghost opened 3 years ago

ghost commented 3 years ago

The default error formatting when you use the ? operator in main() leaves much to be desired. From the output, it appears to use Debug, which is often automatically derived and will often print very unfriendly walls of characters, especially in combination with the new experimental backtrace api:

Error: ParseError { kind: InsufficientBytes { required: 4, received: 3 }, backtrace: Backtrace [{ fn: "err::parse", file: "./main.rs", line: 35 }, { fn: "err::main", file: "./main.rs", line: 57 }, { fn: "core::ops::function::FnOnce::call_once", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/core/src/ops/function.rs", line: 227 }, { fn: "std::sys_common::backtrace::__rust_begin_short_backtrace", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/sys_common/backtrace.rs", line: 125 }, { fn: "std::rt::lang_start::{{closure}}", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/rt.rs", line: 66 }, { fn: "core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/core/src/ops/function.rs", line: 259 }, { fn: "std::panicking::try::do_call", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/panicking.rs", line: 379 }, { fn: "std::panicking::try", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/panicking.rs", line: 343 }, { fn: "std::panic::catch_unwind", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/panic.rs", line: 431 }, { fn: "std::rt::lang_start_internal", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/rt.rs", line: 51 }, { fn: "std::rt::lang_start", file: "/rustc/9d9c2c92b834c430f102ea96f65119e37320776e/library/std/src/rt.rs", line: 65 }, { fn: "main" }, { fn: "__libc_start_main" }, { fn: "_start" }] }

whereas the Display impl looks pretty good, and very similar to panic![] and the formatting I would expect from other languages or gdb:

required 4 bytes while parsing but received 3
   0: err::parse
             at ./main.rs:35:18
   1: err::main
             at ./main.rs:46:19
   2: core::ops::function::FnOnce::call_once
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/core/src/ops/function.rs:227:5
   3: std::sys_common::backtrace::__rust_begin_short_backtrace
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/sys_common/backtrace.rs:125:18
   4: std::rt::lang_start::{{closure}}
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/rt.rs:49:18
   5: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/core/src/ops/function.rs:259:13
      std::panicking::try::do_call
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/panicking.rs:379:40
      std::panicking::try
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/panicking.rs:343:19
      std::panic::catch_unwind
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/panic.rs:431:14
      std::rt::lang_start_internal
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/rt.rs:34:21
   6: std::rt::lang_start
             at /rustc/5c029265465301fe9cb3960ce2a5da6c99b8dcf2/library/std/src/rt.rs:48:5
   7: main
   8: __libc_start_main
   9: _start

It seems like Display is almost always what you want to see in this fn main() context (with some very light wrapping to feature-detect for backtraces) if you haven't set up a custom error reporter of your own. After reading through every issue in this repo and many error-related issues in the rust repo, I still have some questions:

  1. why does main() use Debug to print errors instead of Display?
  2. is it realistic/possible to change this behavior or are we pretty much stuck with it?

I think it would be really unfortunate if one of the first things you needed to do when picking up rust is to learn about error reporters so that you can debug your program effectively. Somehow failure prints errors in the second format, perhaps by formatting Display-like output in its Debug implementation. Is that also a viable option if we're stuck with Debug in fn main() forever? It's very hard to find information about what is going on with rust errors right now.

I have been writing some libraries and I was removing failure since it has been deprecated, but now I am completely at a loss to figure out what I'm supposed to do to provide a nice end-user error experience, as the current core rust behavior seems like a regression over what failure provides.

Here is the program I wrote to reproduce this behavior:

#![feature(backtrace)]
type Error = Box<dyn std::error::Error+Send+Sync>;
use std::backtrace::{BacktraceStatus,Backtrace};

#[derive(Debug)]
pub struct ParseError {
  kind: ParseErrorKind,
  backtrace: Backtrace,
}

#[derive(Debug)]
pub enum ParseErrorKind {
  InsufficientBytes { required: usize, received: usize }
}

impl std::error::Error for ParseError {
  fn backtrace<'a>(&'a self) -> Option<&'a Backtrace> {
    Some(&self.backtrace)
  }
}

impl std::fmt::Display for ParseError {
  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
    match &self.kind {
      ParseErrorKind::InsufficientBytes { required, received } => {
        write!(f, "required {} bytes while parsing but received {}", required, received)
      }
    }
  }
}

fn parse(src: &[u8]) -> Result<u32,ParseError> {
  if src.len() < 4 {
    Err(ParseError {
      backtrace: Backtrace::capture(),
      kind: ParseErrorKind::InsufficientBytes { required: 4, received: src.len() }
    })
  } else {
    Ok(u32::from_be_bytes([src[0],src[1],src[2],src[3]]))
  }
}

fn main() -> Result<(),Error> {
  let buf = vec![1,2,3,4,5,6,7,8];
  println!["# desired main() error formatting (via Display + backtrace):\n"];
  if let Err(e) = parse(&buf[0..3]) {
    match std::error::Error::backtrace(&e) {
      Some(bt) => match bt.status() {
        BacktraceStatus::Captured => eprint!["{}\n{}", e, bt],
        _ => eprint!["{}", e],
      },
      None => eprint!["{}", e],
    }
  }
  println!["\n"];
  println!["# default main() error formatting (via Debug):\n"];
  parse(&buf[0..3])?;
  Ok(())
}
yaahc commented 3 years ago

Hey @substack, thank you for opening this issue. We've discussed this issue in the context of stabilizing the Termination trait in the past. We cannot change the bound on the Termination impl for Result from Debug to Display because this API has already been stabilized, which would make this a breaking change.

That said, we do already have a plan for fixing this. We're going to specialize the Termination impl for E: Error, which we can do because Error is a subtrait of Debug, so it is strictly more specific. Where as Display is an independent trait with no relationship to Debug in the type system, which would make it ambiguous if we were to try to specialize on Debug vs Display.

The only reason we haven't done this already is because there is a soundness issue when specializing trait implementations that could be conditional based on lifetimes. This will require niko's proposed work around for always applicable trait impls being implemented. Once we have this in place though we hope to update the Termination impl on Result to correctly leverage the Error trait when possible so that you don't get the garbled and lossy output that is often presented via Debug.

BurntSushi commented 3 years ago
1. why does main() use Debug to print errors instead of Display?

This is where at least some discussion of that happened that led to deciding on Debug: https://github.com/rust-lang/rust/issues/43301#issuecomment-357829548

I just re-read it, and from what I can tell, I think the thinking was that ?-in-main was going to be for quick throw-away stuff or examples, and that "production" CLI tools would want to do their own thing anyway.

is it realistic/possible to change this behavior or are we pretty much stuck with it?

I had sadly not realized when this was getting stabilized back then, otherwise I like to think I would have spoken up. Almost right after it stabilized, I asked about reversing course, but it's quite difficult. IIRC, @yaahc has a plan for this, but I can't recall where I read those plans. I can't find them in this repo.

Currently, the anyhow crate is personally what I use in CLI tools. It prints the "display" version of an error via its Debug impl. If you compile with Rust nightly, you get backtraces. There is also eyre and snafu. Thankfully, the ecosystem doesn't need to pick one of these. They should all be largely inter-compatible.

programmerjake commented 3 years ago

Hmm, could this be changed to work better over an edition change? I would think so since main is not generally public API of whatever crate is writing fn main

yaahc commented 3 years ago

Hmm, could this be changed to work better over an edition change? I would think so since main is not generally public API of whatever crate is writing fn main

Possibly. It might be possible to change how the edition handles the return value of main to use a different version of the Termination trait, we cannot do so backwards compatibly via the current Termination trait, since std is shared between editions.

programmerjake commented 3 years ago

Hmm, could this be changed to work better over an edition change? I would think so since main is not generally public API of whatever crate is writing fn main

Possibly. It might be possible to change how the edition handles the return value of main to use a different version of the Termination trait, we cannot do so backwards compatibly via the current Termination trait, since std is shared between editions.

Switching to a new Termination trait sounds like a good idea! We'd probably want a annotation of some sort on fn main to allow using the old Termination trait from a new edition to allow automatic edition upgrades to retain behavior. Maybe:

#[termination(edition=2018)]
fn main() -> Result<(), impl Debug> {
    todo!()
}

I think writing an RFC of some sort soon would be good so that we could possibly get it into edition 2021. @yaahc would you be interested in doing that?

yaahc commented 3 years ago

I think writing an RFC of some sort soon would be good so that we could possibly get it into edition 2021. @yaahc would you be interested in doing that?

I don't think I can commit to writing another RFC right now, I feel like I'm already at the limit of how much work I can handle and writing has been particularly difficult for me recently. Also, I feel like it's too close to the 2021 edition to be able to really bake a solution to this problem, so I'd prefer to take it slow and aim for the 2024 edition instead, assuming we even want to make this change. I'm hoping that the specialization approach will be sufficient and obviate the need for this proposal.

programmerjake commented 3 years ago

I don't think I can commit to writing another RFC right now, I feel like I'm already at the limit of how much work I can handle and writing has been particularly difficult for me recently.

Hope it all goes well for you!

Also, I feel like it's too close to the 2021 edition to be able to really bake a solution to this problem, so I'd prefer to take it slow and aim for the 2024 edition instead, assuming we even want to make this change. I'm hoping that the specialization approach will be sufficient and obviate the need for this proposal.

Ok, sounds good enough to me!

kshelekhin commented 7 months ago

It would also be nice to be able to replace Error with something. For example, it's a common pattern to print program name:

$ ./foobar
foobar: cannot remove "/": Permission denied