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
Hi all! This is mostly just to open a discussion and share an opinion, but I'm happy to try implementing the change as well if people are onboard.
My current gripe is this:
Specifically the "optional if field name is source" part! I think this goes against the general expectation for most Rust code to explicitly (as opposed to implicitly) specify behavior, and has recently caused me a problem in the wild when dealing with the knuffel crate — specifically, this bit of code was causing me grief:
When that error was being pretty-printed by miette, it ended up duplicating the error messages!
You can see the error is printed once on the Error: line, then again on the line below. The reason for this is that miette, when printing errors, will also attempt to print any underlying source errors in a sort of stack-trace fashion beneath the main error. In this case, however, the #[error("{}", source)] indicates that author's original intention was likely to just make this DecodeError::Conversion variant a transparent wrapper of the underlying error source.
Not being acutely aware of this implicit thiserror behavior makes it easy to fall into traps where your public error type exports a .source() implementation that's unexpected, and even if you are aware of this behavior, you must rename all of your source fields that you don't want automatic derivations for, or abandon the #[derive(Error)] entirely for a manual impl.
Unfortunately, in the case of knuffel, this implicit behavior will force a breaking change (tailhook/knuffel#33) since the source field is public and has been renamed to error to resolve the double-printing issue. As far as I'm aware, there is no way to unimplement .source() for any field named source within a #[derive(Error)]?
Overall, I think that this special treatment of fields named source sets a lot of traps for crate users, and can directly affect their public API — potentially requiring breaking API changes just to disable. Personally, I'd strongly prefer that this implicit behavior is removed and that only fields explicitly marked as #[source] derive .source() implementations. I think that makes this crate more predictable to use and brings us closer in-line with the Rust's more explicit programming style.
Ironically, removing this generator of breaking changes will, in itself, be a breaking change, but I believe this to be worth it in the long run.
Let me know what you think!
The text was updated successfully, but these errors were encountered:
Hi all! This is mostly just to open a discussion and share an opinion, but I'm happy to try implementing the change as well if people are onboard.
My current gripe is this:
Specifically the "optional if field name is source" part! I think this goes against the general expectation for most Rust code to explicitly (as opposed to implicitly) specify behavior, and has recently caused me a problem in the wild when dealing with the
knuffel
crate — specifically, this bit of code was causing me grief:When that error was being pretty-printed by
miette
, it ended up duplicating the error messages!You can see the error is printed once on the
Error:
line, then again on the line below. The reason for this is thatmiette
, when printing errors, will also attempt to print any underlying source errors in a sort of stack-trace fashion beneath the main error. In this case, however, the#[error("{}", source)]
indicates that author's original intention was likely to just make thisDecodeError::Conversion
variant a transparent wrapper of the underlying errorsource
.Not being acutely aware of this implicit
thiserror
behavior makes it easy to fall into traps where your public error type exports a.source()
implementation that's unexpected, and even if you are aware of this behavior, you must rename all of yoursource
fields that you don't want automatic derivations for, or abandon the#[derive(Error)]
entirely for a manualimpl
.Unfortunately, in the case of
knuffel
, this implicit behavior will force a breaking change (tailhook/knuffel#33) since thesource
field is public and has been renamed toerror
to resolve the double-printing issue. As far as I'm aware, there is no way to unimplement.source()
for any field namedsource
within a#[derive(Error)]
?Overall, I think that this special treatment of fields named
source
sets a lot of traps for crate users, and can directly affect their public API — potentially requiring breaking API changes just to disable. Personally, I'd strongly prefer that this implicit behavior is removed and that only fields explicitly marked as#[source]
derive.source()
implementations. I think that makes this crate more predictable to use and brings us closer in-line with the Rust's more explicit programming style.Ironically, removing this generator of breaking changes will, in itself, be a breaking change, but I believe this to be worth it in the long run.
Let me know what you think!
The text was updated successfully, but these errors were encountered: