Extend ResultExt to Option, allow chaining with boxed Errors #156
Conversation
src/error_chain.rs
Outdated
| $crate::State::new::<$error_name>(error, ), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm sorry, what is it for again?
There was a problem hiding this comment.
This is how I allow boxed errors to interact with error_chain.
It's not possible to define a separate ResultExt for results which contain boxed errors due to Rust's type inference--it won't allow for 2 traits with the same concrete types & type variables but with different type constraints (This rules out an approach as in the comment).
I'm open to other approaches to allow for boxed errors to be introduced in the error-chain, but I think the only better approach would be to create another trait (for example ResultExt2), and have it also have chain_err and implement it for boxed error Result types. I don't want to implement that, however, because it's a large change to the signature of the macro (a new 5th type name is needed), and I'm not sure it won't just have different type inference issues due to the chain_err name matching.
There was a problem hiding this comment.
Did you try to implement with_chain for E: Into<Box<Error>> or something like that?
There was a problem hiding this comment.
Yes, I tried that as well. Unfortunately, the problem we'd need to express is "if you have a boxed trait object, use the box as-is, and if you have an unboxed error object, box it". The problem is that because we have impl Error for Box<Error>, the compiler doesn't do the right thing since boxed errors look like unboxed errors.
There was a problem hiding this comment.
I spent a long time trying to find a solution to this. The only options that I've gotten to work involve introducing a new method on Error, or introducing a second ResultExt-alike. Apparently, supporting patterning-matching on chained Errors as well as boxed error types is quite challenging with Rust's type system.
There was a problem hiding this comment.
Could you move it to a new PR? It doesn't seem related to the Option impl.
There was a problem hiding this comment.
Hey Yamakaky,
I've been working with you on this PR for a long time. I'd really like to improve error_chain to make it more featureful, but I'm getting the impression that you're not planning to accept the with_boxed_chain method. Could you tell me where this is going? If this was 2 commits, would you merge it? I have production code depending on fixing these issues, and if you think we're going in different directions, I'd like to know that.
Thank you!
There was a problem hiding this comment.
Well, I guess that's me nitpicking about having two things in the same commit XD
Box being a strange builtin and specialization being... missing can both be a pain when dealing with generic implementation, I had this kind of problem in error-chain... I guess we can't do anything about it for now. Could you just rewrite with_chain so that it uses with_boxed_chain internally? Also please update the changelog.
There was a problem hiding this comment.
Ok, I rewrote with_chain to use with_boxed_chain, and I updated the changelog! I will be monitoring the situation with with_boxed_chain, specialization, etc, since I really want to improve the API for working with boxed trait errors!
I've tested these changes against another major internal codebase, and it all seems to work.
I think this is ready to merge. Let me know if I should add anything else; otherwise, hopefully you'll see a new PR in a few months from me handling the Error specialization correctly :)
src/error_chain.rs
Outdated
| }) | ||
| } | ||
| } | ||
| */ |
There was a problem hiding this comment.
You should remove this comment
|
Any update? |
|
Hey @Yamakaky -- sorry, I was focused on the project that required these changes. I also wanted to "burn them in", by converting & developing with these changes. I've used Do you think this is good to merge? Is there anything you'd else you'd like me to modify? |
Cargo.toml
Outdated
| @@ -1,7 +1,7 @@ | |||
| [package] | |||
|
|
|||
| name = "error-chain" | |||
| version = "0.10.1-pre" | |||
| version = "0.10.0" | |||
src/error_chain.rs
Outdated
| $crate::State::new::<$error_name>(error, ), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Did you try to implement with_chain for E: Into<Box<Error>> or something like that?
|
Hey @Yamakaky--What do you think of the changes now? Would you be willing to merge them for a new release? Also, I found a blog that I believe is related to removing |
|
I think it's good enough. Thanks! |
|
Thank you for merging this! I am looking forward to contributing more features :) |
|
;) |
No description provided.