Add macro for boilerplate combination errors

[mcoffin]

Hey,

I've been working on some alacritty-related stuff just for fun, and found that adding anything to these Error enums was quite painful. This patch would provide macros (for internal use) that allow easy generation of the boilerplate for error-container enums.

It's just a simple refactor, but I'm hoping to gain some experience with the contribution process before I contribute my larger projects down the line (if they pan out).

Thanks in advance for the help.

[chrisduerr]

While this reduces the number of lines of error related code, I think this makes it significantly less readable and harder to maintain.

In general I'd advocate for as little magic as possible and I think the current implementation hits that pretty well. The macro code is realistically unreadable and I think the pattern 'write once, read many' applies here.

There definitely is a lot of duplication here though where it seems like just using std::error::Error is basically all it does, so I'm a bit torn on just closing this. So I don't really disagree with the idea that this isn't really very clean right now, but my main issue is with the way it has been resolved in this PR.

I'd have to look a little more into it to make a good recommendation what the best way to resolve this would be, but for me personally it's not a huge issue right now, there's just a bit of unpleasant glue between our errors and std::error::Error.

If you have any alternative proposals for resolving this, I'm open to discuss this. However if the idea was just to take what you've worked on for your private fork and upstream it, I don't think it'll make it into Alacritty in its current shape and closing this PR might be the better option.

[mcoffin]

I'm all down for looking at alternative solutions, but without some kind of macro, then you often get stuck with lots of boilerplate (as we had before), or just passing around Box<dyn ::std::error::Error>. Since hopefully these error-paths aren't too hot, the necessary heap allocation to just use trait objects might be worth it.

I chose this path originally because it rendered down to as similar code as was possible.

So, I see three paths forward if we don't like this implementation.

1. Stay 0-cost, but just find a way to make the macro code more readable.
2. Go to a slight-cost implementation, using Box<dyn ::std::error::Error>.
3. Keep it as-is and just deal with the boilerplate.

What about the macro code is inherently unreadable? I know it can be a little iffy to grok the matching patterns, so might some documentation / comments help improve that to a point where it would be workable?

EDIT: For a feeling of a little bit of the boilerplate pain, take a look at this commit. It's a messy diff, and was just for investigation and I've subsequently scrapped that approach to my idea, but you can get a sense for the fact that you have to edit 5 separate places each time you add a new variant to an error enum, and they're not even guaranteed to be in the same place in file.

• Add actual variant to enum definition
• Add variant-handling code to fn cause(&self)
• Add variant handling code for fn description(&self)
• Add variant handling code to std::fmt::Display instance,
• Add From<..> instance for the inner value you're wrapping.

EDIT2: You can actually see in that commit that I forgot an impl From<vulkano_win::CreationError> for Error { .. }

[nixpulvis]

Food for thought: https://github.com/rust-lang-nursery/failure

[mcoffin]

Food for thought: https://github.com/rust-lang-nursery/failure

I think once stabilized that would definitely be the route to go. I don't see that happening too quickly though, considering that they will only really have a stable std::error::Error trait once rust-lang/rust#53487 wraps up.

[chrisduerr]

I think once stabilized that would definitely be the route to go.

I strongly disagree with the idea of adding failure or error-chain. Over the course of Rust there have been a lot of different libraries for making error handling a little easier and none of them have provided a good long-term solution for clean error handling and APIs.

Especially now that Alacritty is more split into separate modules, it should be extremely carefully considered what kind of errors might be exposed in a public API. And I'm convinced there's no better solution for consumers than using Rust's native error handling.

It should also be mentioned that API consumers and readers should (imho) be valued over writing new code. Having to add a new error enum field is usually not that common and if any of the methods are not implemented a clear error will tell the author exactly what needs to be done. So I think the writing part is pretty much irrelevant. The bigger problem with the boilerplate is that it reduces the ratio of relevant and irrelevant code, making it harder to read the code.

[mcoffin]

It should also be mentioned that API consumers and readers should (imho) be valued over writing new code.

I definitely agree here. Perhaps the solution here would be to keep the boilerplate, but make sure it's all put together (i.e. struct Error {..}, impl std::error::Error { .. }, impl std::fmt::Display, and impl From<..> for Error should be together and towards the end of the module... or perhaps in it's own module?).

In fact, when I first started looking at the renderer API, I had to build the full rust docs just to start to grok what was public and important about the API because of all the other code mixed in to the module. Just src/renderer/mod.rs alone contains 15 top-level struct/enum items, and 18 top-level impl items, totaling 1600+ lines overall.

$ egrep '^(pub )?((struct)|(enum))' < src/renderer/mod.rs | wc -l
14
$ egrep '^impl' < src/renderer/mod.rs | wc -l
18

I'm still of the opinion that the macro increases readability for thinly wrapped Error enums since you don't have so many top-level items surrounding the error, but especially since I'm new to the project I get that it might not be the way to go.

Even just keeping error type definitions and implementations all together and out of the way of the meat of a module would go a long way towards readability in and of itself.

[nixpulvis]

@mcoffin, @chrisduerr just refactored the config along similar lines, see #2425. The goal generally is a lot like you said, "make sure it's all put together".

[mcoffin]

snip... just refactored the config along similar lines, see #2425. The goal generally is a lot like you said, "make sure it's all put together".

Oh, nice! I didn't even know that was a recent thing. I haven't had to touch the Config struct too much yet, but the one time I did I was surprised how easy it was to find/add things. Turns out that hasn't always been the case!

I see from his refactor that he did use a macro in one spot. What are the thoughts on trying to just make the Error macro more readable vs. chucking it entirely?

[chrisduerr]

I see from his refactor that he did use a macro in one spot. What are the thoughts on trying to just make the Error macro more readable vs. chucking it entirely?

That macro had nothing to do with the refactor. It existed before it and I just haven't changed it. So I wouldn't put any weight into that. It was not written for this change.

[mcoffin]

I see from his refactor that he did use a macro in one spot. What are the thoughts on trying to just make the Error macro more readable vs. chucking it entirely?

That macro had nothing to do with the refactor. It existed before it and I just haven't changed it. So I wouldn't put any weight into that. It was not written for this change.

Gotcha. I more scanned the current implementation rather than the whole diff. I guess what I really meant is that there is some existing macro_rules! usage, so what can we do to make this one more readable and usable?

[kchibisov]

I was following this PR, and I don't think, that implementing some "potential" public API with macros just because they reduce boilerplate is a good idea. Macros are great for reducing boilerplate in tests or inside some internal code, they also can be used in some kind of public API that can't be implemented without them( e.g println! since you need some kind of varargs) (imho). Also if you want to look at macro overuse, you should take a look at glibc ( tons of strange macro magics) and compare it e.g. with musl. So if you'd like just to add more macros everywhere, this can make it less pleasant to experiment with the current codebase.

[mcoffin]

I was following this PR, and I don't think, that implementing some "potential" public API with macros just because they reduce boilerplate is a good idea. Macros are great for reducing boilerplate in tests or inside some internal code, they also can be used in some kind of public API that can't be implemented without them( e.g println! since you need some kind of varargs) (imho). Also if you want to look at macro overuse, you should take a look at glibc ( tons of strange macro magics) and compare it e.g. with musl. So if you'd like just to add more macros everywhere, this can make it less pleasant to experiment with the current codebase.

Cool. Sounds like a bunch of the collaborators are in agreement against this one, so I'll close it. Thanks for the thoughts, everyone.