Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't treat source as a magic field name #284

Open
TheLostLambda opened this issue Jan 25, 2024 · 0 comments
Open

Don't treat source as a magic field name #284

TheLostLambda opened this issue Jan 25, 2024 · 0 comments

Comments

@TheLostLambda
Copy link

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:
image

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:

image

When that error was being pretty-printed by miette, it ended up duplicating the error messages!

image

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant