rust-lang / project-error-handling

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

Proposal: Reporter inversion #53

Open seanmonstar opened 1 year ago

seanmonstar commented 1 year ago

I would like to propose the idea of inverting the way error reporters generally work in Rust. Instead of a reporter iterating all causes of an error to format them, pass in format rules to the top-level error.

The problems I'm trying to solve are:

Problems

Default to most common case

I posit that the most common case that users print errors is for log purposes. I'll use an example of code I've seen over and over:

task::spawn(async move {
    if let Err(e) = do_it_better().await {
        warn!("did it worse: {}", e);
    }
})

The current best practices hopes to educate users who have done this to either change the error type they've been returning at this top-ish level tasks, or to remember to wrap the error in a reporter right before printing it.

I suggest that, instead of relying on education, the best practice simply assumes most users won't know about reporters. This change makes it so the common case simply prints all of the error chain to their logs. Specifically, separated by a :, not with newlines.

Once a user is educated about reporters, they can opt-in to customizing further, such as choosing newlines over colons, or how many down the stack to go, or whatever. When opted into a reporter, the reporter can then tell the Display of the error "I only want the top error plz".

As prior art, Go's simple way to wrap an error includes displaying it together on the same line, while also being available from errors.Unwrap.

Opaque errors don't know their context

Some libraries may wish to wrap errors in an opaque way, such that they don't expose the source so that users don't depend on a specific error. This is common in other languages, when it's preferred to not couple the internal details to the error, while still wanting to be to share the error messages to help debugging. hyper is one library with that desire.

Since these opaque errors don't include the wrapped error in source, the current reporters cannot access them. But since no context is provided to Display, the opaque error cannot try to mimic the output of the rest of the stack.

Solution

The format traits already include a way to inject some context: the std::fmt::Formatter. That's how a type can check f.alternate() or f.fill(). Some of those are unused by structs, which we could overload. Or, we could define new ones.

I outlined overloading some existing format flags a few years ago. My opinion about some details has changed since then, mostly that I believe the default should be to print everything in a single-line friendly mode, since as I described above, that's what I think the most common case is.

I appreciate that the proposal allows specifying how deep down the stack to print. It also uses f.alternate() to determine if it should fit on one line, or use newlines and a stack trace. If we created new flags, we could inject a mode or even "separator" characters, similar to f.fill(). However, the exact details are a bit of a bikeshed, and what I want most is simply to have the idea of inverting the reporters to be considered.

yaahc commented 1 year ago

Initial thoughts:

yaahc commented 1 year ago

@mystor pointed out that if we implemented the lint on format_args it would end up covering the tracing and log macros as well.

seanmonstar commented 1 year ago

I'm worried that this will only increase the burden on authors of errors.

That's fair! After reading your write-up, and pondering about it over lunch, I thought: what if it's as easy as the f.debug_struct()/etc builders that are in the standard library? We can of course start with builders somewhere else (like errors::fmt etc etc), and once solid, those just become f.display_error(msg, source) or something.

you're interested in not exposing the source for downcast because users can then depend on it.

Yes, that's right. In the opaque case, I'd be fine with giving out the source if the trait didn't allow downcasting. But since it does, it can create unintended coupling. (To be clear, I think downcasting is useful, but I want to choose what internals I expose can be downcasted).

fundamentally prevent application developers from controlling the format

That's not my intention. I want application developers to be able to express the format similar to how you would do with formatting most other types, with format flags. There could eventually be a way to provide custom innards of Formatter that could do more, even.

But the stated problem does currently exist. If a crate author follows the advice of not putting the value in Error::source if it wants to print it, they don't get to know how it should be printed.

a new format specifier like {:!} to print an error

That's certainly an option, and feels like an extension of what I've proposed so far.

yaahc commented 1 year ago

Yes, that's right. In the opaque case, I'd be fine with giving out the source if the trait didn't allow downcasting. But since it does, it can create unintended coupling. (To be clear, I think downcasting is useful, but I want to choose what internals I expose can be downcasted).

Did you see the snippet I posted about how to potentially prevent downcasting without otherwise altering the chain of sources? I feel like that has the potential to completely solve the second problem you listed as trying to solve and lets us focus the conversation on solving the first problem which I agree 100% is a severe issue and is one of the elements of error reporting I'm most interested in improving. The only problem with the snippet is it would probably need to be a language feature because we couldn't add such an opaque error wrapper type to the standard library without then making it possible for people to downcast to it.

seanmonstar commented 1 year ago

I tried playing around with your snippet, trying to come up with some generic Opaque that could continuously wrap the source to hide the type, and could a way to make it all work with the possibility of the type being Box<dyn Error>, or fitting a &dyn Error that could be transmuted and returned from source().

yaahc commented 1 year ago

Hmm, I think I see what you're getting at. I'm not able to think of any way to make an opaque type that can reapply itself recursively to it's sources, so I went ahead and opened a thread in #t-lang to try to nerd snipe some other clever people into brainstorming alternative solutions: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/disabling.20downcast.20on.20Error.20types

programmerjake commented 2 months ago

what if there was a f.in_reporter() flag on formatters and a f.report_error(|f| write!(f, "this type's msg")) for use in Display::fmt where report_error will just run the closure if f.in_reporter() otherwise it will run a default reporter algorithm that follows sources. this way custom reporters can just set in_reporter and then visit the source chain manually for custom formatting, and naive users can still get decent error reporting by default with just {}