New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward Display implementation for foreign errors #13

Merged
merged 1 commit into from Aug 2, 2016

Conversation

Projects
None yet
3 participants
@cramertj
Contributor

cramertj commented Jul 24, 2016

Fix #2.

One caveat to this is that it adds the type of the foreign error to the signature of the error-chain foreign_link_variant. As a result, the visibility of the foreign error type must be the same as the visibility of the error-chain type (public).

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 25, 2016

Contributor

Oh thanks!

Gosh, I didn't fully realize the implications of this issue. This looks to me like foreign errors must be Copy, since this is storing it both in the ErrorKind variant as well as the boxed 'cause'. Requiring Copy is something we can't do. Clone might be better, but I think we might be able to be more clever.

Instead of adding a field to the ErrorKind variant we can probably modify the Display impl to reach into the 'cause' box for the display string. The Display impl is implemented by the macro here:

        impl ::std::fmt::Display for $error_name {
            fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
                ::std::fmt::Display::fmt(&self.0, f)
            }
        }

You could imagine this function containing a match statement on self.0, matching all the foreign link variants and in each case calling fmt on the cause box instead (which is self.1.0). The bounds of that box will have to change to Error + Send + Display, but I think requiring all errors to implement Display is pretty reasonable; it's certainly expected.

It may also make sense to store foreign errors in the ErrorKind variant but not in the 'cause' box (I recall it being a difficult design choice), but that's pretty deep surgery.

Let me know what you think.

Contributor

brson commented Jul 25, 2016

Oh thanks!

Gosh, I didn't fully realize the implications of this issue. This looks to me like foreign errors must be Copy, since this is storing it both in the ErrorKind variant as well as the boxed 'cause'. Requiring Copy is something we can't do. Clone might be better, but I think we might be able to be more clever.

Instead of adding a field to the ErrorKind variant we can probably modify the Display impl to reach into the 'cause' box for the display string. The Display impl is implemented by the macro here:

        impl ::std::fmt::Display for $error_name {
            fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
                ::std::fmt::Display::fmt(&self.0, f)
            }
        }

You could imagine this function containing a match statement on self.0, matching all the foreign link variants and in each case calling fmt on the cause box instead (which is self.1.0). The bounds of that box will have to change to Error + Send + Display, but I think requiring all errors to implement Display is pretty reasonable; it's certainly expected.

It may also make sense to store foreign errors in the ErrorKind variant but not in the 'cause' box (I recall it being a difficult design choice), but that's pretty deep surgery.

Let me know what you think.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jul 26, 2016

Contributor

Yeah, this PR as it stands requires the errors are copy-- I suppose that was more than a little presumptuous of me.

I'll take a look at the solutions you suggested and let you know what I find out.

Contributor

cramertj commented Jul 26, 2016

Yeah, this PR as it stands requires the errors are copy-- I suppose that was more than a little presumptuous of me.

I'll take a look at the solutions you suggested and let you know what I find out.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jul 27, 2016

Contributor

I'm now including the Display of all errors with a cause. The output format is ": " if it has an error, and "" otherwise. Do you think that's okay behavior, or would you prefer we specifically tag foreign error variants and only print the cause for those types?

Contributor

cramertj commented Jul 27, 2016

I'm now including the Display of all errors with a cause. The output format is ": " if it has an error, and "" otherwise. Do you think that's okay behavior, or would you prefer we specifically tag foreign error variants and only print the cause for those types?

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 27, 2016

Contributor

@cramertj Foreign errors are a bit of a special case: whereas error-chain ErrorKinds represent an 'actual' error, foreign error ErrorKinds are more of a stand-in for the boxed foreign error (the ErrorKind description is more-or-less useless - as demonstrated by this bug). So I think I prefer for display to delegate to the cause only when its a foreign error. But that raises another question: if we are printing the boxed cause as the error, then what happens when you iterate over the entire chain of causes to print them each during error reporting? We're going to see duplicates. One might consider having the iter() method skip the first cause if we're going to be displaying it as the primary error text, but that feels pretty convoluted - if we start going down that path it starts bringing the entire design into question.

Another approach we could possibly take is to do what you were originally doing - storing the foreign error in the ErrorKind, but storing None in the cause box. That would eliminate the duplication of error descriptions. But I know there was a strong reason I didn't go that route originally. I think my reasoning was that putting an error type (any error type) into the ErrorKind, means that the cause-chain for those errors is handled differently than for native error-chain errors. That is - if you want to traverse the cause chain you have to call cause on the error value stored in the ErrorKind for those errors, and you have to maintain the invariant that for those errors the cause box is None.

Tradeoffs abound. Do those considerations make sense? Not sure how I feel. What are your thoughts?

Contributor

brson commented Jul 27, 2016

@cramertj Foreign errors are a bit of a special case: whereas error-chain ErrorKinds represent an 'actual' error, foreign error ErrorKinds are more of a stand-in for the boxed foreign error (the ErrorKind description is more-or-less useless - as demonstrated by this bug). So I think I prefer for display to delegate to the cause only when its a foreign error. But that raises another question: if we are printing the boxed cause as the error, then what happens when you iterate over the entire chain of causes to print them each during error reporting? We're going to see duplicates. One might consider having the iter() method skip the first cause if we're going to be displaying it as the primary error text, but that feels pretty convoluted - if we start going down that path it starts bringing the entire design into question.

Another approach we could possibly take is to do what you were originally doing - storing the foreign error in the ErrorKind, but storing None in the cause box. That would eliminate the duplication of error descriptions. But I know there was a strong reason I didn't go that route originally. I think my reasoning was that putting an error type (any error type) into the ErrorKind, means that the cause-chain for those errors is handled differently than for native error-chain errors. That is - if you want to traverse the cause chain you have to call cause on the error value stored in the ErrorKind for those errors, and you have to maintain the invariant that for those errors the cause box is None.

Tradeoffs abound. Do those considerations make sense? Not sure how I feel. What are your thoughts?

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jul 27, 2016

Contributor

Out of those two options, I'd prefer the latter as it seems more consistent with the design of the overall library. The former seems like it's trying to bypass the error<->cause relationship.

On a more intuitive level, the second representation (with the error stored in the ErrorKind with None in the cause) makes more sense to me: the foreign error ErrorKind variant isn't really caused by the foreign error, it's a wrapped representation of the foreign error. Because it's just a wrapper, it makes sense for the foreign error ErrorKind variant to not have its own cause, but instead forward on to the cause of the type it wraps.

Contributor

cramertj commented Jul 27, 2016

Out of those two options, I'd prefer the latter as it seems more consistent with the design of the overall library. The former seems like it's trying to bypass the error<->cause relationship.

On a more intuitive level, the second representation (with the error stored in the ErrorKind with None in the cause) makes more sense to me: the foreign error ErrorKind variant isn't really caused by the foreign error, it's a wrapped representation of the foreign error. Because it's just a wrapper, it makes sense for the foreign error ErrorKind variant to not have its own cause, but instead forward on to the cause of the type it wraps.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 27, 2016

Contributor

@cramertj If we go that route we'll need to modify the iter and cause methods to do something special for foreign errors. Which is fine.

I'm happy to try this out. Want to modify the PR to see if it works?

Contributor

brson commented Jul 27, 2016

@cramertj If we go that route we'll need to modify the iter and cause methods to do something special for foreign errors. Which is fine.

I'm happy to try this out. Want to modify the PR to see if it works?

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jul 27, 2016

Contributor

Sure! I'm at work at the moment, so it'll have to wait until later this evening. Thanks for your guidance!

Contributor

cramertj commented Jul 27, 2016

Sure! I'm at work at the moment, so it'll have to wait until later this evening. Thanks for your guidance!

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Jul 28, 2016

Contributor

Alright, I gave it a shot and added some tests. Let me know what you think.

Contributor

cramertj commented Jul 28, 2016

Alright, I gave it a shot and added some tests. Let me know what you think.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 28, 2016

Contributor

Thanks @cramertj. I've got some fires to attend to today. May not get back to this until tomorrow.

Contributor

brson commented Jul 28, 2016

Thanks @cramertj. I've got some fires to attend to today. May not get back to this until tomorrow.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 2, 2016

Contributor

Oof, sorry that 'tomorrow' turned into 5 tomorrows. This look great @cramertj! I'll get a new build out today.

Contributor

brson commented Aug 2, 2016

Oof, sorry that 'tomorrow' turned into 5 tomorrows. This look great @cramertj! I'll get a new build out today.

@brson brson merged commit c511a36 into rust-lang-nursery:master Aug 2, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 2, 2016

Member

Does this mean that the Display implementation will show the full chain of "native" error-chain errors, and stop at the first "foreign" error? Or will it continue through "foreign" errors as long as the .cause chain continues?

Member

joshtriplett commented Aug 2, 2016

Does this mean that the Display implementation will show the full chain of "native" error-chain errors, and stop at the first "foreign" error? Or will it continue through "foreign" errors as long as the .cause chain continues?

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 2, 2016

Contributor

@joshtriplett The Display implementation only shows one 'level' of errors (before and after this patch), but the cause implementation delegates to the foreign error, and the iter method uses the cause method. So when iterating over the error chain it will show the complete chain, native and foreign.

Contributor

brson commented Aug 2, 2016

@joshtriplett The Display implementation only shows one 'level' of errors (before and after this patch), but the cause implementation delegates to the foreign error, and the iter method uses the cause method. So when iterating over the error chain it will show the complete chain, native and foreign.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 2, 2016

Member

@brson One level counting the top, or not? (In the example in #2, would this still just print "git error", or would it print the git error too?

I'll file a separate issue about having a function (or trivial adaptor like Path::display()) to print the whole chain.

Member

joshtriplett commented Aug 2, 2016

@brson One level counting the top, or not? (In the example in #2, would this still just print "git error", or would it print the git error too?

I'll file a separate issue about having a function (or trivial adaptor like Path::display()) to print the whole chain.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 2, 2016

Contributor

@joshtriplett In that example display would print the actual underlying git error and not print "git error" at all.

Contributor

brson commented Aug 2, 2016

@joshtriplett In that example display would print the actual underlying git error and not print "git error" at all.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 2, 2016

Contributor

In other words, compared to the previous implementation the "git error" 'level' doesn't exist at all. This implementation removes a (useless) link in the chain.

Contributor

brson commented Aug 2, 2016

In other words, compared to the previous implementation the "git error" 'level' doesn't exist at all. This implementation removes a (useless) link in the chain.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 2, 2016

Contributor

Although, you might (and I kinda did) consider that extra level useful in that it is in some sense a delimiter between the native and foreign errors. If that matters, we might consider adding a method to e.g. indicate whether an error is a foreign error or not.

Contributor

brson commented Aug 2, 2016

Although, you might (and I kinda did) consider that extra level useful in that it is in some sense a delimiter between the native and foreign errors. If that matters, we might consider adding a method to e.g. indicate whether an error is a foreign error or not.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Aug 2, 2016

Contributor

@brson Actually, depending on how much you care about backwards-compatibility, it might be a good idea to remove the "description" option from the foreign link variant and make the description forward to the description of the underlying error. Right now, I think the underlying error description is unaccessible.

Contributor

cramertj commented Aug 2, 2016

@brson Actually, depending on how much you care about backwards-compatibility, it might be a good idea to remove the "description" option from the foreign link variant and make the description forward to the description of the underlying error. Right now, I think the underlying error description is unaccessible.

@cramertj cramertj deleted the cramertj:foreign_error_display branch Aug 2, 2016

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Aug 2, 2016

Contributor

@cramertj Yeah, I was thinking about that right as I was tagging it. It's not very useful to have. #15

Contributor

brson commented Aug 2, 2016

@cramertj Yeah, I was thinking about that right as I was tagging it. It's not very useful to have. #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment