Crate evaluation for 2017-06-27: error-chain

libs
1

Crate evaluation for 2017-06-27: error-chain

For additional contribution opportunities, see the main libz blitz thread.

This post is a wiki. Feel free to edit it.

Links

Needs your help!

Anything that is not checked off still needs your help! There is no need to sign up or ask for permission - follow any of these links and leave your thoughts:

Guidelines checklist

Legend

  • [y] = guideline is adhered to, no work needed.
  • [n] = guideline may need work, see comments nearby
  • [/] = guideline not applicable to this crate

Checklist

Guidelines checklist

This document is collaboratively editable. Pick a few of the guidelines, compare the error-chain crate against them, and fill in the checklist with [y] if the crate conforms to the guideline, [n] if the crate does not conform, and [/] if the guideline does not apply to this crate. Each guideline is explained in more detail here. If [n], please add a brief note on the following line with an explanation. For example:

  - [n] Crate name is a palindrome (C-PALINDROME)
        - error-chain backwards is niahc-rorre which is not the same as error-chain

Cookbook use case statements

Cookbook example ideas

Come up with ideas for nice introductory examples of using error-chain, possibly in combination with other crates, that would be good to show in the Rust Cookbook. Please leave a comment in that issue with your ideas! You donā€™t necessarily have to write the example code yourself but PRs are always welcome.

API guideline updates

API guideline ideas

What lessons can we learn from error-chain that will be broadly applicable to other crates? Please leave a comment in that issue with your ideas!

  • What do guidelines say about hidden macro conventions?

Discussion topics

  • How to support no_std.

  • Suitability for libraries

    • error-chain is definitely oriented towards getting started quickly writing apps, but it isnā€™t obviously wrong for libs to use error-chain. Should it be explicitly recommended either way?

  • Design compromises around links / foreign links

  • When and how to enable backtraces

    • This has been iterated on a lot and has performance implications

  • Structural typing between ā€˜linksā€™ in from conversions

  • ChainedError issues

    • What is the purpose of ChainedError being a trait?
    • Does extract_backtrace make sense as a method on the ChainedError trait? It doesnā€™t use the trait in any way
    • Why the display method on ChainedError?
    • Can ChainedError be hidden or made private?

  • There are several issues around the macro syntax:

  • Thereā€™s not currently a good story for boxing up chainable errors

  • The names ā€œlinksā€ and ā€œforeign linksā€ are not fully evocative of the crucial distinction: whether chaining occurs

    • Itā€™d be good to dive more deeply into the rationale for this precise split.
    • (brson) links allow for deep matching on chained error types and backtrace-propagation because they follow the error-chain ā€˜protocolā€™. Their From conversions pass the backtrace between each other, while building up a nested enum for the ErrorKind.
    • (brson) foreign_links do not follow the error-chain protocol and can do neither of these things

  • What are the pros/cons of locally defining Error and ResultExt?

    • It seems like links could be simplified if Errors were just type aliases
    • (brson) I doubt it is possible to formulate it in such a way because of coherence issues. I did try. The ResultExt trait that defines chain_err is defined in terms of the local ErrorKind. The inability to define a single ChainedError type and need to define so many local types is pretty much the reason the error-chain crate is one massive macro.

  • Error derefs to ErrorKind, how do we feel about that?

    • (brson) seems like an abuse of Deref

  • Msg(String) always being a variant, with conversions ā€“ worth talking about

    • This may be one aspect that separates libs from apps

  • error_chain, error_chain_processed and quick_error ā€“ can these be hidden?

    • (brson) not error_chain surely
    • (brson) istm everything but error_chain and quick_main should be hidden and follow some convention for hidden macros

  • Do we want to allow conversions with chain_err

  • Sync bounds

    • (brson) I think it should be required, and that non-Send/Sync Errors are non-interoperable special cases, need to think hard about their choices.

Crate issues

How are we tracking?

Pre-review checklist

Do these before the libs team review.

  • [x] Create evaluation thread based on this template
  • [x] Work with author and issue tracker to identify known blockers
  • [x] Compare crate to guidelines by filling in checklist
  • [x] Record other questions and notes about crate
  • [ ] Draft several use case statements to serve as cookbook examples
  • [ ] Record recommendations for updated guidelines

Post-review checklist

Do these after the libs team review.

  • [ ] Publish libs team meeting video
  • [ ] Create new issues and tracking issue on crateā€™s issue tracker
  • [ ] Solicit use cases for cookbook examples related to the crate
  • [ ] File issues to implement cookbook examples
  • [ ] File issues to update guidelines
  • [ ] Post all approachable issues to TWiR call for participation thread
  • [ ] Update links in coordination thread
3 Likes
2

Hi, Iā€™ve updated the checklist somewhat.

2 Likes
3

Fantastic, thank you!

4

Thanks for getting this started @aturon and @budziq.

5

Possible discussion piece: I noticed that when using quick_run!, and it printed out the cause trace for an error that was returned, it can show duplicated messages depending on the implementation of the errors. This happens with reqwest, but see this small example:

enum Error {
    Io(io::Error),
    SomeCrateError,
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Error::Io(ref e) => fmt::Display::fmt(e),
            // ...
        }
    }
}

impl ::std::error::Error for Error {
    fn cause(&self) -> Option<&::std::error::Error> {
        match *self {
            Error::Io(ref e) -> Some(e),
            _ => None,
        }
    }
}

Then, in some function somewhere:

fn do_a_thing() -> Result<(), Error> {
    Err(io::Error::new(Other, "oh no"))?;
    Ok(())
}

Youā€™ll see this:

oh no
Caused by: oh no

This isnā€™t really error-chains fault, itā€™s just following the chain of causes. But itā€™s related, and also related to error design in general. If the crates Display were updated to not include the output from the wrapped io::Error, then it the display becomes much less useful when used without error-chain. For instance, if println!("{}", e) printed io error.

6

One thing Iā€™d really appreciate is if this crate functioned, even in a degraded manner (i.e. no backtraces or std::error support) in no_std contexts, as it seems to becoming the de facto way to write Rust error boilerplate.

I opened an issue here:

https://github.com/brson/error-chain/issues/138

I am still investigating the feasibility.

3 Likes
7

Perhaps this comment is looking a bit beyond the libs blitz, but it seems like an apropos time to bring it up.

I think error chain is very good (significantly better than the status quo before error chain), but I feel slightly concerned about how its trending toward becoming the de facto solution. I suspect error chain could be even better if the problem space were examined in a lang+libs framework, instead of just a libs framework (that is, considering potential extensions to the language as well).

For example, the existence of both links and foreign_links seems inelegant to me; is there a solution based on specialization that could combine them?

Even more abstractly, if error chain is going to become ā€œthe wayto define an error in Rust,ā€ Iā€™d like to see a more thorough discussion of what other options were considered and discarded, what the fundamental trade offs are, etc. Maybe that discussion exists and I just havenā€™t read it.

16 Likes
8

@withoutboats FWIW Iā€™ve always considered error-chain to be something of hack and wish it didnā€™t have to exist, and I expect other solutions to appear. Both the existence of error-chain and its popularity in my mind speak to a weakness of the language and std. I hope though that even as error-chain is adopted it doesnā€™t prevent use of other error handling techniques, that it interoperates well.

It would be a fine result for me if error-chain got bumped to 1.0 basically as-is, with the understanding that error handling will continue to evolve, that there may be an error-chain 2.0, that other solutions might deprecate it completely.

8 Likes
9

Hey folks, Iā€™m not sure if this is the right place and time, but maybe Iā€™m lucky and you can help me out! If not, consider this a request for a ā€œbest practicesā€ document that should be part of the error-chain docs! :smiley:

Iā€™m currently evaluating switching Diesel to use error-chain internally. One of our main concerns is presenting an interface to the user of the query builder that is forward-compatible, i.e., one we can add new errors to without breaking user code, while at the same time give return nice error that are easy to debug. My current idea is using error-chain (because itā€™s pretty nice to use; weā€™d probably have one root error type, plus maybe more specific error types in modules) and only exposing a struct DieselError { inner: LeRootErrorChainError } to the user. Iā€™ve written a bit more here.

From what Iā€™ve seen of error-chain so far, it seem geared toward application developers, not library authors. Would you recommend us using it at all? Is there anything we should definitely keep in mind?

10

To echo what @withoutboats said, I think that it would be big for ergonomics and usability to get something like error chain into the language or std.

A standardized convention for getting error traces and error nesting across all libraries would be huge for debugging and usability.

11

Well, I got error-chain to work with no_std, but the result is not particularly pretty:

I think it speaks to some changes which it would be nice to have happen upstream in Rust. Most notably this one:

2 Likes
12

Thanks for the efforts @bascule.

13

Iā€™ve updated the op with discussion points raised in this thread, and some of my own concerns.

Thereā€™s still quite a bit to do here and and a little over a week to do it:

  • checklist needs to be completed and potential issues documented
  • need to review outstanding issues with @Yamakaky for potential 1.0 blockers

@withoutboats, @bascule, are either of you interested in attending the libs meeting for this on the 27th, 1PM, to discuss?

Anybody up for tackling any of the remaining checklist sections? The "macros" section seems especially important.

14

@brson I should be able to come then. Itā€™s at Mozilla SF?

15

I felt like your response was a good resolution & Iā€™m not sure my comment is near-term actionable.

16

Is the plan to remove the quick_main macro once this RFC lands? Looks like itā€™s proposed to merge, but thereā€™s still some unresolved issues.

The implementation isnā€™t too far off what quick_main does anyways.

17

It also seems a bit unfortunate to have an almost identical public copy of quick_error in the crate. If someone uses both quick_error and error_chain, the order the crates are imported will determine what version of the quick_error macro gets used:

#[macro_use]
extern crate quick_error;
#[macro_use]
extern crate error_chain;

// quick_error from error_chain
quick_error!(
    pub enum MyError {

    }
);

#[macro_use]
extern crate error_chain;
#[macro_use]
extern crate quick_error;

// quick_error from quick_error
// This doesn't actually compile, because quick_error requires you `#[derive(Debug)]
quick_error!(
    pub enum MyError {

    }
);
  • Could the error_chain version be given a different name, like error_chain_quick_error and made #[doc(hidden)]?
  • Is this use-case of re-exporting quick_error covered by one of the current macros RFCs?
18
  • Iā€™ve filled out some more detail in the evaluation and left some thoughts on the macro syntax and public implementation details
  • Iā€™ve added some discussion points to the bottom of the list in the OP (@aturon you might want to review those and see if theyā€™re worthwhile)
19

Thanks, folks, for advancing this while I was away on vacation last week!

Today I did a careful review of the crate myself, and added a bunch of discussion items, including pulling the most important guidelines issues into scope. You can see these at the top, but for reference:

  • There are several issues around the macro syntax:

  • Thereā€™s not currently a good story for boxing up chainable errors

  • The names ā€œlinksā€ and ā€œforeign linksā€ are not fully evocative of the crucial distinction: whether chaining occurs

    • Itā€™d be good to dive more deeply into the rationale for this precise split.

  • What are the pros/cons of locally defining Error and ResultExt?

    • It seems like links could be simplified if Errors were just type aliases

  • Error derefs to ErrorKind, how do we feel about that?

  • Msg(String) always being a variant, with conversions ā€“ worth talking about

    • This may be one aspect that separates libs from apps

  • error_chain, error_chain_processed and quick_error ā€“ can these be hidden?

  • Why the display method on ChainedError?

  • Can ChainedError be hidden or made private?

  • Do we want to allow conversions with chain_err

3 Likes
20

I definitely agree, and I think such a discussion would ideally be part of the crate evaluation. I have similar reservations to yours about the links/foreign links split and its rationale, for example.

I'm not sure we'll be able to complete a deep dive into the tradeoffs during the official libs team meeting, but I think it's something we can and should do out of band. Even if we don't end up changing the library much for a 1.0, it'd be great to document the design rationale and note shortcomings that might be addressed with new language features.