rust-lang-deprecated / failure

Error management
https://rust-lang-nursery.github.io/failure/
Apache License 2.0
1.42k stars 138 forks source link

Improving error matching consistency (at least for testing) #14

Open U007D opened 7 years ago

U007D commented 7 years ago

TL;DR

Can Failure introduce a consistent interface that implements PartialEq across all Fail types so errors can be compared for equality simply and literally? This will improve the ergonomics and productivity around unit testing, and possibly other scenarios as well.

Motivation

As a BDD developer, the first thing I do when developing new code is to write a unit_test. Rust's lack of consistent API around errors makes writing unit tests less ergonomic than perhaps they should be. Here's a simple example of what I would like to write (which works, but only for some error types(!)):

#[test]
fn passing_bad_param_results_in_not_found_error() {
    assert_eq!(some_func(bad_param).err().kind(), std::io::ErrorKind::NotFound);
}

I've run into what I consider to be major ergonomic issues with errors and unit testing. I haven't found a consistent way to work with errors that doesn't involve a lot of pattern-matching boilerplate. Here are some details adapted from a posting I made about a month ago:

No consistent basis with which to work with Errors

(inconsistencies or issues in bold)

using the Standard Library

using error-chain

using pattern matching

For example:

  fn string_validator_ref_handles_empty_input() {
    // GIVEN an empty string
    let input = String::new();
    let expected_result = ErrorKind::ValueNone;

    // WHEN some operation which generates an error occurs
    let result = (&input).validate_ref::<NonEmptyStringValidator>().err().unwrap()
  /* What I would *like* to be able to write */

  // THEN ensure the expected error was returned
    assert_eq!(*result, expected_result);
  }
  /* The reality */

  // THEN ensure the expected error was returned
    assert!(matches!(*result, expected_result); //Error: pattern unreachable (match doesn't work this way; unintuitive at this level)
    assert!(matches!(*result, val if val == expected_result)); //not ergonomic; Error without PartialEq (e.g. error-chain): binary operation `==` cannot be applied to type `error::ErrorKind`
    assert!(matches!(*result, ErrorKind::ValueNone); //works, but without variable support, important scenarios become unrepresentable:
        // suppose two methods must yield same result.  To ensure consistency through refactorings, a test is developed:
        // assert_eq!(matches(FuncA(badInput), FuncB(badInput)) //not possible, as per above; only constant variants w/PartialEq implemented can be used ergonomically

When considering Error equality, I believe it is out of scope for to consider the semantics of errors (e.g. "Are two instances of an error's Other variant equal?" Assuming the two instances of Other are value-equal then, yes. The fact that Other could be used by a library or application as a 'catch-all' to represent a wide variety of different error conditions in the real world is out of scope for this proposal. It is expected that the user of that error type will understand when a comparison of Other is and is not a meaningful operation).

Given that the Error type is freqently implemented using different types (marker structs, encapsulating structs, enums, and others) the only dependencies we should take are on its API. To that end, I would love to see a consistent .kind() method (or something else which serves the purpose of providing the error variant) which exists and is properly defined for every Failure.

withoutboats commented 7 years ago

I would like for error types to implement PartialEq, and for Error to, but there are problems. The core issue issue is that PartialEq is not object safe. This means we can't use it as a supertrait on Fail, and we can't really implement it for Error, which has a trait object inside it.

I have a vision of a solution, which requires specialization (an unstable feature). With specialization, we could support generating an (unsafe) function pointer for PartialEq if the type implements PartialEq, and not otherwise. Then, if the type ids match & there is a PartialEq implementation, we downcast both errors and dynamically call the PartialEq impl. (This conforms to PartialEq because PartialEq does not require reflexivity.)

In that sort of solution, we wouldn't require PartialEq, but Errors can be compared if their Fail type does implement it.

There's also nothing to stop users from deriving PartialEq for their Fail types now, whereas error-chain does not allow you to implement PartialEq for your error type at all.

U007D commented 7 years ago

Thanks for this, @withoutboats. I have a couple of thoughts, but want to spend a few days to ramp up on object safety first to know how viable my ideas might be before posting.

U007D commented 7 years ago

I've been delving into the concept of object safety, its reason for being, history, etc. I can't say I understand it all, and have many questions about why some limitations haven't been eliminated by compile-time trait object plumbing, but these questions are far outside the scope of this issue.

So for this issue, even "dreaded" (from a matching consistency perspective) std::io::Error's discriminants ultimately boil down to either an i32 (for the Os variant) or a std::io::ErrorKind (for the Simple and Custom variants), both of which implement PartialEq. Simpler error definitions such as std::fmt::Error and std::str::Utf8Error, are enums in their own right, which, AFAICT, all seem to directly derive PartialEq.

So my latest thinking is that without plugging holes in what types of traits can be considered object safe traits, whether Fail::Error could have a .PartialEqErrorValueUsableForConsistentComparison() method (the name is a work in progress ;)) which would return the error type's (PartialEq) discriminant so that the user could then at least compair Fail::Error's consistently (perhaps a la std::mem::Discriminant or possibly HKT could help? Note, std::mem::Discriminant is actually generic, so yay, more object safety concerns, but not sure how much of an issue that is for Fail::Error).

shepmaster commented 7 years ago

@U007D FWIW, PartialEq is the reason I use quick-error.

U007D commented 7 years ago

@shepmaster although better than the then-alternatives, I found quick-error still too verbose... Failure has been superb thus far.