Refactor error types

[killercup]

Currently, we have an enum with a bunch of error variants, most of which are actually just Box<Error> (see 0.13 docs). I think we can do this in a nicer way.

Requirements

• Future-proof: Adding new errors should not be a breaking change
• Little overhead: The error type should be as small as possible
• Give good error messages in release mode
• Enable nice debugging in debug mode

[killercup]

Musings on how to implement this

We could use quick-error, a macro crate that will reduce much of the overhead our error code currently has. This will also make it feasible to add more specialized error variants to parts of diesel.

But why stop with syntactic? The error_chain crate is quick-error on steroids and has become sort of a standard for many. It offers a lot of what we want, incl. nice macros for generating error types and early returns, converting strings to errors ad-hoc (nice for development), and backtraces (when RUST_BACKTRACE is set). It exposes the ErrorKind enum as a public field, though, which allows chaining errors, but also makes it a breaking change to add variants if the error type is public.

A way around that might be to write an opaque error type ourselves that implements Error but little else, and use error-chain internally. I'm not sure how much overhead that introduces when dealing with an error, but the opaque struct could be pointer-sized (struct Error(Box<InternalError>)). We might need to add a method to construct diesel errors from external crates, but I guess that is no big deal.

[killercup]

This might also be a good chance to refactor our code style to return errors: error-chain has two nice macros, bail!("foo") (return error) and ensure!(x < 42, "foo") (assert! that returns error instead of panicking) that we might want to make use of (or copy).

[weiznich]

Maybe someone should rise concerns here about the none breaking extensibility of the ErrorKind field?

[seanmonstar]

I recently just had the exercise in the reqwest crate. I also considered error_chain, but only briefly. I considering a public facing enum to be the wrong design, because it's impossible to add new variants, and it exposes the exact representation of how you store the errors. For reference, here's the reqwest error: https://github.com/seanmonstar/reqwest/blob/master/src/error.rs

[sgrif]

We do add a __Nonexhaustive variant to make sure we can add new ones in the future. https://github.com/diesel-rs/diesel/blob/master/diesel/src/result.rs#L24

[seanmonstar]

Yea, and while shame on anyone actually matching on such a variant, it is possible, and it would break crates or apps that did it when you add a new variant.

However, enum fields are all public, so if you were to, say, decide to stop storing some errors as boxes and instead of the normal values, you can't do so with a public enum. You could if it were in a private struct field.

[Eijebong]

In the future, there might be rust-lang/rfcs#2008 which would fix this problem

[jimmycuadra]

#[non_exhaustive] would address the issue of adding new variants, but it doesn't address the ability to change internal representations of enum variants like Sean mentioned.

[killercup]

Update: Seems like people are actively working on #[non_exhaustive]: rust-lang/rust#44109

[sgrif]

I've spent most of the day working on this, and after coming at it from 3 different angles -- I don't think this is a good change.

Basically the benefits of using error-chain here are:

• Our variants that take Box<Error + Send> go into a more general "chain"
• Backtrace tracking

That comes at several costs though:

• Significantly more boilerplate
  • Contrary to the goals of this PR, I found that the amount of code I had to write to use error-chain was more than the boilerplate it seeks to remove.
  • I think we there is a fundamental mismatch between what our errors look like, and what error-chain is designed for. It seems to be best suited for places where you are just trying to glob errors from a bunch of different libraries into 1 enum. We are instead trying to classify our errors based on where they came from. We cannot be more specific than Box<Error> for most variants, since we need to be extensible.
• Errors become painful to construct.
  • Most things from chain_error are unusable if an error is already boxed. I don't want FromSql to care that it's getting wrapped in a DeserializationError or ToSql to care that it's getting wrapped in a SerializationError.
    • We could work around that by adding type Error to those traits, but I don't think that's a net win.
    • We could also work around it by creating a SerializationError type separately, but that type would basically have 1 variant, and now we're just making it hard to access the "real" error.
• Pattern matching is way more painful.
  • If nothing else, the fact that it requires importing a type called Error into scope is a problem.
• More weight is given to our least common errors
  • The variants that were most improved by this were InvalidCString, DeserializationError, SerializationError, and QueryBuilder error. None of those are error types that users will commonly encounter, and if they do they're likely unrecoverable. By far the most common are NotFound, and DatabaseError. For those two types it is very important that we retain ergonomic pattern matching.

Ultimately I don't think either of the benefits get us very much. As I mentioned, the places where the "chain" is useful are places that are uncommon, and we could achieve the same thing by implementing std::Error::cause (which we should do and I will follow up with a PR for that).

I think we're also over-estimating the value of the backtrace. Ultimately for our users, the only valuable piece of the backtrace will be where the query was executed. They can easily get that by using error-chain for their application error, which I expect many people do. Having a backtrace that points to where in our deserialization code an error occurred is only useful for the developers of Diesel. This is why Rails, for example, removes all lines that point to library code from exception backtraces by default.

So given all of that, I'm going to close this in favor of just implementing Error::cause.