You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Report<E> is not compatible with std::error::Error #2886
Hello! I ran into a problem, which is better to show with example:
structFoo;impl<V>FooforSomeTrait<V>whereV:TryInto<Vec<u8>>,
<VasTryInto<Vec<u8>>>::Error: error_stack::Context,{fndo_stuff(&self,value:V) -> error_stack::Result<(),MyError>{
...
let my_vec:Vec<u8> = value.try_into().into_report().change_context(MyError);
...}}structBar;implTryInto<Vec<u8>>forBar{typeError = error_stack::Report<TransformError>;fntry_into(self) -> Result<Vec<u8>,Self::Error>{/// Some serialization}}fnmain(){let foo = Foo;let bar = Bar;let baz = "baz".to_owned();
foo.do_stuff(baz);// Good
foo.do_stuff(bar);// Error. The trait `StdError` is not implemented for `error_stack::Report<TransformationError>`}
I have implementation of SomeTrait that requires TryInto<Vec<u8>>::Error implement error_stack::Context. But Report is not impl Context and is not impl Error.
Proposed Solution
Implement Error for Report:
impl<E> std:error::ErrorforReport<E>{}
It's simple and allows to write more abstract code. In other words Report will compatible with Errors
TL;DR: This is blocked by the specialization feature.
I'd love to implement Error for Report but currently that's sadly not possible for a few reasons:
(major) With an upcoming change we remove IntoReport, which implies impl<T, C: Context> ResultExt for Result<T, C>. Implementing Error for Report would imply a conflicting implementation. It's likely, that this can be solved with feature(specialization) enabled, but I have not tested this, yet. If this would be a breaking change this is fine as we want to adjust other parts of the API as soon as specialization is possible (e.g. merging attach and attach_printable).
(major) If Error is implemented on Report it's expected that Error::source is behaving correctly. As error-stack supports multiple source errors, the mapping does not work correctly. We're planning some overhaul for this "related-errors" feature in the near future and I will definitely will keep this issue in mind.
(probably the hardest issue to solve) It's easily possible to create Report<Report<E>>, which results in unexpected behavior, annoying errors, and hard-to-debug issues. It's simply not what people want to use. This cannot easily be solved without some kind of negative trait bounds which, AFAIK, is nothing that is immediately planned right now. Possibly it's possible to solve this via specialization, though.
(minor) There is a conflicting implementation between the From conversion of Report<E> and Box<dyn Error> when Error is implemented for Report<E>, but if anything like this is going to be added, we'd probably implement Context for Report<E: 'static> instead to be more generic. This is probably not a big issue but another annoyance when dealing with the Error trait. Ultimately we probably going to remove the Context trait when core::error::Error is stabilized but that's not fully decided yet as Context is potentially more generic than Error semantically.
One other possible route would also be to not remove IntoReport, yet, but rather try to implement Error on Report earlier (I don't expect specialization to be stabilized soon), but we still need to figure out the other issues first. However, with IntoReport and Report<E>: Error creating Report<Report<E>> is very easy by calling Result<T, Report<E>>::into_report(). We even have a dedicated test case to forbid this:
let _:Result<(),Report<RootError>> = result.into_report();
let result = Err::<(),_>(Report::new(RootError));
// Not allowed
let _ = result.into_report();
it's possible that one of the listed issues is not really an issue or that I forgot something but it's definitely not easily possible now - and I'm very sad about this! We'd be very happy to hear more thoughts about this as well!
Happy to hear some thoughts from @indietyp as well.
Activity
TimDiekmann commentedon Aug 11, 2023
Hi @Tohman21 and thanks for filing the issue.
TL;DR: This is blocked by the
specializationfeature.I'd love to implement
ErrorforReportbut currently that's sadly not possible for a few reasons:IntoReport, which impliesimpl<T, C: Context> ResultExt for Result<T, C>. ImplementingErrorforReportwould imply a conflicting implementation. It's likely, that this can be solved withfeature(specialization)enabled, but I have not tested this, yet. If this would be a breaking change this is fine as we want to adjust other parts of the API as soon asspecializationis possible (e.g. mergingattachandattach_printable).Erroris implemented onReportit's expected thatError::sourceis behaving correctly. Aserror-stacksupports multiple source errors, the mapping does not work correctly. We're planning some overhaul for this "related-errors" feature in the near future and I will definitely will keep this issue in mind.Report<Report<E>>, which results in unexpected behavior, annoying errors, and hard-to-debug issues. It's simply not what people want to use. This cannot easily be solved without some kind of negative trait bounds which, AFAIK, is nothing that is immediately planned right now. Possibly it's possible to solve this viaspecialization, though.Fromconversion ofReport<E>andBox<dyn Error>whenErroris implemented forReport<E>, but if anything like this is going to be added, we'd probably implementContextforReport<E: 'static>instead to be more generic. This is probably not a big issue but another annoyance when dealing with theErrortrait. Ultimately we probably going to remove theContexttrait whencore::error::Erroris stabilized but that's not fully decided yet asContextis potentially more generic thanErrorsemantically.One other possible route would also be to not remove
IntoReport, yet, but rather try to implementErroronReportearlier (I don't expectspecializationto be stabilized soon), but we still need to figure out the other issues first. However, withIntoReportandReport<E>: ErrorcreatingReport<Report<E>>is very easy by callingResult<T, Report<E>>::into_report(). We even have a dedicated test case to forbid this:hash/libs/error-stack/tests/ui/into_report.rs
Lines 19 to 24 in 12fecc4
it's possible that one of the listed issues is not really an issue or that I forgot something but it's definitely not easily possible now - and I'm very sad about this! We'd be very happy to hear more thoughts about this as well!
Happy to hear some thoughts from @indietyp as well.
[-]Report<E> is not compatible with std::error:Error[/-][+]`Report<E>` is not compatible with `std::error::Error`[/+]Tohman21 commentedon Aug 11, 2023
Thanks for quick answer. Yep, the problem is deeper than I thought