Error handling improvements

[calavera]

📬 Issue #, if available:

Fixes #901

✍️ Description of changes:

Change how we handle error types to be more ergonomic:

• Replace Cow with String: Diagnostics are serialized into JSON as soon as the function returns, which means that their value is copied right away. The performance improvement of using Cow is minimal in this case, but it has some ergonomic implications because we have to handle their lifetimes. By removing the explicit lifetimes, people can return Diagnostic values with static lifetimes which was not possible before.

• Add features to implement From<T> for Diagnostic for anyhow, eyre, and miette error types. This helps people that use those creates to transform their errors into Diagnostic without double boxing their errors.

🔏 By submitting this pull request

• I confirm that I've ran cargo +nightly fmt.
• I confirm that I've ran cargo clippy --fix.
• I confirm that I've made a best effort attempt to update all relevant documentation.
• I confirm that my contribution is made under the terms of the Apache 2.0 license.

[TethysSvensson]

I think this is a overall pretty good trade-off.

• I like the improved ergonomics by getting rid of the lifetime on Diagnostic. This also means that you can now directly return a diagnostic from your handlers.
• I like the improved docs
• I am a bit confused about why you would need to use both IntoDiagnostic and Into<Diagnostic>. Wouldn't it be possible to just use IntoDiagnostic for everything?

[calavera]

Thanks for the feedback @TethysSvensson

I am a bit confused about why you would need to use both IntoDiagnostic and Into. Wouldn't it be possible to just use IntoDiagnostic for everything?

It would be possible, but then we'd break even more people 🙈 I would recommend implementing Into<Diagnostic> over using IntoDiagnostic as a more gradual implementation change, and using IntoDiagnostic when you cannot implement Into<Diagnostic> like it's the case with external error libraries.

[TethysSvensson]

It would be possible, but then we'd break even more people 🙈 I would recommend implementing Into<Diagnostic> over using IntoDiagnostic as a more gradual implementation change, and using IntoDiagnostic when you cannot implement Into<Diagnostic> like it's the case with external error libraries.

Actually on re-visiting this, I am not even sure I understand the purpose of the IntoDiagnostic trait at all. It seems to do the exact same thing as Into<Diagnostic>, except it also has (feature-gated) impls for anyhow, eyre and mietre.

Wouldn't it be possible to get rid of IntoDiagnostic instead and instead do feature-gate a From<anyhow::Error> for Diagnostic?

[calavera]

Wouldn't it be possible to get rid of IntoDiagnostic instead and instead do feature-gate a From<anyhow::Error> for Diagnostic?

Yeah, you're right. For some reason I thought it was not possible, but it totally is. I made the update and things look much more simple now.

[tim3z]

Hi, thanks for improving this, looks a lot clearer and better to me. Docs helped me as a naive user a lot to understand the point of the breaking change.

One thing confuses me still though, the impl<T> From<Box<T>> for Diagnostic where T: std::error::Error blanket impl. Why should it be impossible to convert some T: std::error::Error but possible for Box<T> where T: std::error::Error? This was already my primary confusion when the breaking change came up. Returning an error didn't work anymore, but slapping a Box on it would fix it. But what has a Box to do with the error handling? This blanket impl makes it also impossible to implement impl From<Box<MyErrorType>> for Diagnostic (just tried with the thiserror example).

I think, it would make more sense to implement neither. And if the user really wants to use the simpel catchall conversion it is possible to go through Box<dyn std::error::Error>. That makes it explicit that you get to an impl by going through the trait object, not by slapping on a Box.

[calavera]

I think, it would make more sense to implement neither. And if the user really wants to use the simpel catchall conversion it is possible to go through Box<dyn std::error::Error>. That makes it explicit that you get to an impl by going through the trait object, not by slapping on a Box.

Thanks for the feedback @tim3z. I agree with you. I've removed the implementation.

[rimutaka]

Since there is no standard way of documenting features yet (rust-lang/rfcs#3485), shall we add # enables From<T> for Diagnostic for anyhow error types, see ReadMe for more info here?
Add the same for eyre, and miette or merge into a single comment.

The features are described in the ReadMe, but that's likely to be missed on the first read.

[calavera]

good idea. I've documented them all since I was there.