add displaydoc crate to derive fmt::Display for SignalProtocolError
#296
Conversation
|
https://crates.io/crates/displaydoc might come handy as well. ;) |
|
Right, we've previously had a PR to switch to |
|
Got it! I'll try out displaydoc! Might steal from #287! |
cosmicexplorer
force-pushed
the
thiserror-attempt
branch
from
c2c1067 to
9df218d
Compare
|
Ok, redone to use |
cosmicexplorer
force-pushed
the
thiserror-attempt
branch
from
9df218d to
e7c573f
Compare
cosmicexplorer
changed the title
thiserror crate to derive fmt::Display for SignalProtocolErrordisplaydoc crate to derive fmt::Display for SignalProtocolError
rust/protocol/src/error.rs
Outdated
| use std::fmt; | ||
| use std::panic::UnwindSafe; | ||
|
|
||
| pub type Result<T> = std::result::Result<T, SignalProtocolError>; | ||
|
|
||
| /// Wraps a boxed error struct and delegates the [std::error::Error] trait to it. | ||
| /// | ||
| /// A [Box] wrapping an error apparently does not implement [std::error::Error] itself, which breaks |
There was a problem hiding this comment.
Hm, that implementation is in the stdlib. Something else must be going on.
There was a problem hiding this comment.
Unfortunately, there is a distinction between Box<E: Error> and Box<dyn Error>. I have added this explanatory link to the docstring: https://stackoverflow.com/a/62452516/2518889.
There was a problem hiding this comment.
I believe we could work around this by adding a type parameter E: Error to the definition of SignalProtocolError, but that seems like it would make instantiating SignalProtocolError even more annoying than wrapping in CallbackErrorWrapper.
cosmicexplorer
force-pushed
the
thiserror-attempt
branch
4 times, most recently
from
f7a8ba8 to
1fd0010
Compare
rust/protocol/src/error.rs
Outdated
| /// Wraps a boxed error struct and delegates the [std::error::Error] trait to it. | ||
| /// | ||
| /// Unlike a `Box<E: Error>` wrapping a concrete error type, a `Box<dyn Error>` wrapping | ||
| /// a type-erased trait object does *not* implement [std::error::Error] itself (see [this | ||
| /// stackoverflow answer] for details). This breaks the ability to nicely use the `#[source]` | ||
| /// annotation from the [thiserror::Error] derive macro. In order to avoid manually impling | ||
| /// [std::error::Error::source] for each case of [SignalProtocolError], we create this wrapper | ||
| /// struct to delegate to the underlying `dyn Error` in order to nicely derive [thiserror::Error] | ||
| /// for [SignalProtocolError::ApplicationCallbackError]. | ||
| /// | ||
| /// [this stackoverflow answer]: https://stackoverflow.com/a/62452516/2518889 |
There was a problem hiding this comment.
I don't think the resulting code is clearer than just implementing source manually (and therefore Error). In the choice between making one place a lot more complicated and many places a little more complicated, I'd usually err on the side of containing the complexity. (If only we didn't need UnwindSafe here.)
There was a problem hiding this comment.
Thank you so much for investigating where that might be added to thiserror! I have removed the thiserror dependency for now along with CallbackErrorWrapper until we can avoid making many places a little more complicated as described.
cosmicexplorer
force-pushed
the
thiserror-attempt
branch
2 times, most recently
from
3980aab to
0a76d24
Compare
- remove thiserror for now until we can derive UnwindSafe
cosmicexplorer
force-pushed
the
thiserror-attempt
branch
from
0a76d24 to
497ded2
Compare
|
Okay, the Java failure is our fault for me coincidentally picking a bad nightly, rust-lang/cargo#9919, so I'll merge this anyway. |
Problem
I looked into @jrose-signal's comment #287 (comment) comparing the use of
thiserrorvs adding real docstrings as in #287. I believe they are totally orthogonal.With that in mind, it looks like
displaydocmight be able to just save some of our boilerplate inerror.rs.Solution
displaydoc::Displayand add docstrings to every case ofSignalProtocolErrorto auto-derivefmt::Display.Result
Our
fmt::Displayimplementation forSignalProtocolErrorrequires less code because we can rely on a macro to derive it! We also end up adding a first pass of documentation to all of our error cases!