@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.
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!
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?
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:
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:
The syntax not matching corresponding declaration form
The types declaration, with its hand-rolled system of defaults, doesn’t quite feel “rustic”
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.
@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.
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.
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.
I just read through this thread and then read its documentation on its rustdoc main page. Here are some things I noticed:
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.
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.
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.
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.