add UnwindSafe impl for AsDynError #155
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello!! I love this crate :))
Problem
thiserror::Error
is not able to convert aBox<dyn Error + Send + Sync + UnwindSafe + 'static>
into adyn Error
with the#[source]
annotation, which caused it to be unsuitable for the purposes of the Signal client: see signalapp/libsignal#296 (comment). Signal usesUnwindSafe
(I just learned about this trait) to convert errors into a format suitable for its ffi/jni/node.js bindings.Solution
@jrose-signal pointed me to #41, which enabled wrapping a
Box<dyn Error + Send + 'static>
with#[source]
, and I was able to figure out how to extend it for the purposes of boxedUnwindSafe
errors as well.Result
Using this change allowed me to remove the boilerplate error manipulation code in the signal client, and this commit (cosmicexplorer/libsignal@9d82971) passed tests locally.
Please let me know if there are test cases I should add -- it looks like the
test_boxed_source()
method is already intended to apply to all the types of boxed errorsthiserror
supports, and I tried to create a test wrapping e.g.ParseIntError
which isUnwindSafe
usingstd::panic::catch_unwind
, but was having difficulty and was thinking it might be better to leave the tests simple as they are. Let me know if I should investigate more on that.