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.
Thanks for the detailed feedback @briansmith. I don’t have a whole lot to add to what you said, but thought it warranted a response.
This is an interesting point. I don’t personally see any issue with having motivations in the crate docs, but it seems worthwhile for docs-well-travelled to keep crate docs focussed and move motivations and tutorials to a dedicated site, like docs.rs/serde and serde.rs.
I think so too.
There is this example output module in the docs, but do you mean more details on the error_chain! macro itself? I think in general macros need more detailed docs than other items because the definition isn’t usually helpful.
This was a discussion point, was it talked about in the meeting? I can’t quite tell from the etherpad what the thinking was on it. I don’t see the need for it, personally but am interested in motivations.
The more I think about this the more important it seems. I think there’ve been a few attempts at localisation over the years but nothing has really stuck, right?
I think that brson is correct error-chain is great given the current limitations of Rust. It makes error handling substantially more ergonomic.
It might be worth reminding users of the performance implications though. In Rust one tends to get used to handling Result as control flow and thinking it’s just an enum and not particularly slow. However when error-chain packages up a backtrace it can cause a substantial performance hit. It took me a couple days to realise this was the case in my app. For many use cases it’s fine and the usability and debugging are worth it, but it would be good to attach some warnings I think.
[quote=“KodrAus, post:28, topic:5362”]
There is this example output module in the docs, but do you mean more details on the error_chain! macro itself?[/quote]
Yes, I’d like the document to explain how all the macros get expanded out in a typical usage.
If you’re looking to avoid the alloc / backtrace performance hit and just want the automatic Error and From impls, you can use derive-error. I like that error-chain gives extra information when I need it, but it definitely is a tradeoff.
Also, the current docs say that backtraces are only collected if RUST_BACKTRACE is enabled, so there should be no significant performance hit under normal operation.
If this is referring to the MsgErrorKind variant, that lets you write things like chain_err(|| "something bad happened"), this is one of the best features of error-chain, because you don’t have to create a new variant for every possible error when you don’t care about that ceremony. It’s discussed a few places in the docs and in this blog post. There is some concern that that variant causes bloat.