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

libs
#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.

#21

@bascule oh, crap, sorry for taking so long to circle back to this. Yes, by good chance, this particular meeting will take place at the Mozilla SF office, at 1 PM, tomorrow, since it takes place during the Mozilla all-hands, and you are welcome to be there. It’ll also be over video conf. I’ll send you connection info in a bit.

#22

I would not plan to remove quick_main, for compatibility with older Rust; and I expect ? in main to take a long time to stabilize.

#23

I filled in the OP with some of my own commentary, including around the reasoning for the ‘links’ / ‘foreign_links’ split (links have special pattern-matching / backtrace propagation capabilities that foreign_links don’t); and the definition of local error types / traits (coherence violations otherwise).

I moved the renaming of quick_error to an issue - it is plainly an implementation detail and should not be clobbering the quick_error crate.

The internal quick_error macro was just a way to get the quick_error syntax without figuring out how to write it myself. It supports syntax that error-chain doesn’t even use, so would be better to pare it down to something error-chain specific.

1 Like
#24

Hi this crate still needs quality cookbook examples! There are some proposals no suggestion has been found particularly persuasive yet.

#25

The libs team discussed this at last week’s meeting. It was a pretty productive meeting, and a lot of remediations fell out of it. We’ll get either @aturon or myself to file the bugs soon.

Some of the big resolutions were:

  • lower the Error trait into probably the alloc crate so no_std error-chain makes sense
  • investigate a complete syntactic redesign to accommodate all the missing capabilities raised in the review. This will certainly require using some form of procedural macros. It may or may not be a 1.0 blocker

I’m uh, missing the etherpad link we used to take notes, so I hope somebody else has it.

#26

Ah, here’s the etherpad from the meeting:

https://public.etherpad-mozilla.org/p/rust-api-guidelines-error-chain

#27

FWIW, I am pretty happy not using error-chain.

I just read through this thread and then read its documentation on its rustdoc main page. Here are some things I noticed:

  1. The documentation spends a significant number of words on the rationale for why error-chain was created instead of deferring to previously existing crates. IMO, the middle of the main documentation page isn’t the place for this.

  2. It seems that it defines a Result<E> type, which I find confusing given the standard Result<R, E> type. I guess at one time it was considered good to do this. However, I’ve always had bad experiences with this because common coding patterns make the name Result ambiguous when it’s not qualified, and qualifying it can be a pain. I think we shouldn’t encourage this pattern in any way now, and in particular error-chain shouldn’t encourage it.

  3. The source code of a user of this crate consists of a bunch of macro invocations. It isn’t obvious what these macros actually produce. I think this is likely to be compounded if/when procedural macros are used, since presumably the mapping would only get more complex. At the very least, it would be very useful to see, in the documentation, exactly what code the macros generates. Then the reader could easily compare how much boilerplate is saved, and the reader could see what “magic” there is or isn’t. However, I think designs that result in less code generation from macros should be explored, if they haven’t already.

  4. I saw this in the example:

fn foo() -> Result<()> {
    Err("foo error!".into())
}

Apparently there is some way that strings are convertible to errors. Is this always the case, or is this something that is explicitly enabled? In particular, how does one go about preventing the automatic conversion of strings to the error type? I frequently do manual or (semi-)automated static analysis of code that relies on being able to quickly see which error codes are produced in which circumstances, and this kind of automated creation from strings seems like it would make that very difficult. I think think this kind of analysis is common and IMO the defaults and documentation examples should encourage coding patterns that facilitate it.

At the end of reading the documentation, I asked myself “Do I feel encouraged to use this in my code?” Honestly, I don’t think so, because it doesn’t seem that hard to just do manually what this crate automates, and I think that manually doing it would be clearer (to the reader of the code) than using the macros from this crate. I personally would like an alternative that seems much less magical and preferably one that uses ad-hoc polymorphism (macros) less (ideally not at all) and type-system-based polymorphism (traits) more. Also, to the extent that I want to chain the “cause”, I want to be able to do so without boxing so that the chaining works the same for std and no_std usage.

FWIW, I also disagree with the design of the std::error::Error trait itself, because I don’t think it makes sense for errors to have human-readable description() that cannot be localized, and I don’t think it’s important that they implement Display for the same reason. I see some value in them implementing Debug though (almost always by #[derive]). In the epochs discussion it was mentioned that std::error::Error can never be fixed even with the epochs mechanism. I think it would be great to find some way to define a lighter-weight replacement for std::error::Error with a better design and then build something like error-chain on top of the new design, so that no_std- and std- targetting crates can work the same.

7 Likes