dtolnay
Don't treat `source` as a magic field name
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 (https://github.com/tailhook/knuffel/pull/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!
