Skip to content
This repository has been archived by the owner on Sep 12, 2024. It is now read-only.

Proposal: Reporter inversion #53

Open
seanmonstar opened this issue Aug 29, 2022 · 7 comments
Open

Proposal: Reporter inversion #53

seanmonstar opened this issue Aug 29, 2022 · 7 comments

Comments

@seanmonstar
Copy link

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:

  • Default behavior should be most useful for people who don't know it could be more informative.
  • Errors that don't expose their source don't know what context they are being formatted in.

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
Copy link
Member

yaahc commented Aug 30, 2022

Initial thoughts:

  • I'm worried that this will only increase the burden on authors of errors. Now not only do you have to specify your error message you have to implement full reporting logic and special cases for inverting the logic back to the current norm such that you can have a central reporter outputting errors in a consistent format. Presumably this would get abstracted into some helper crates but it seems to only increase the risk of error ecosystem inconsistencies.
  • In your example about opaque errors: Am I correct in understanding that you're talking about opaquely wrapping an error that itself has sources, and you're not exposing those sources at all through the fn source method so that people won't depend on the types via downcast? Or is it that you're wrapping a single error in an opaque type that acts as an otherwise transparent wrapper around that error, rather than having that error be the source of the opaque wrapper type, but you're still exposing that internal error's sources as your source as well? edit: I read the linked threads and saw that it is the former, you're interested in not exposing the source for downcast because users can then depend on it. edit: As an alternative suggestion, how about something like this? https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=91c003b4e5f7da2a77191a618d79cbe9 I'm not 100% sure it's sound but it at least passes miri.
  • RE: Errors that don't expose their source don't know what context they are being formatted in.
    • It sounds like the solution you're proposing here and the style of error you're talking about just fundamentally prevent application developers from controlling the format and output of their error reports. This feels like a regression in functionality that I would object to.
  • RE: Default behavior should be most useful for people who don't know it could be more informative.
    • I 100% agree this is a problem that we need to fix. I don't really have a good alternative solution other than rewinding time and making the Display impl for error types a blanket impl that does basic reporting and have the description method have the same signature as the Display trait. My second best idea is to introduce a new format specifier like {:!} to print an error via Error instead of Display, which could then hook into any customized reporting logic depending on where the print is being called, such as in a logging system or maybe some pop-up dialog. My main problem with this solution is I don't know if there's a generally applicable way we could lint for mistakenly printing Error types with Display. We could probably catch this in the println! and write! macros, but I'm not sure if we can catch it in tracing/log macros, let alone other more unique formatting interfaces.

@yaahc
Copy link
Member

yaahc commented Aug 30, 2022

@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
Copy link
Author

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
Copy link
Member

yaahc commented Sep 22, 2022

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
Copy link
Author

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
Copy link
Member

yaahc commented Sep 27, 2022

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
Copy link
Member

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 {}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants