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

Add required bounds to derived impl #421

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JustusFluegel
Copy link

@JustusFluegel JustusFluegel commented Feb 2, 2025

This is an attempt to make the derive macro for Diagnostic work better with generics, allowing to write code like for example

#[derive(Debug, thiserror::Error, miette::Diagnostic)
enum MyError<T> {
     #[error(transparent)]
     #[diagnostic(transparent)]
     Other(#[from] T).
     #[error("bar")]
     #[diagnostic(help = "baz")]
     Foo
}

This should for example make #162 just work, as well as things like

#[derive(Debug, thiserror::Error, miette::Diagnostic)
struct MyOtherError<T> {
     #[label]
     label: T
}

and the other supported attributes.

AFAIK the only gotcha with this implementation right now is that #[related] only works with concrete collection types and not with generic collections, but the type inside the collection can be generic. (meaning: Vec<T> works but not any arbitrary C where C: IntoIter<Item: Diagnostic>), and I think that is a current limitation of the rust compiler / the current Diagnostic trait design.

Questions: Do we want to gate this behind a feature flag since it pulls in extra-traits from syn and performs a bunch of extra work?

This is implemented using a new TypeBoundsStore struct which tracks
usage of types during parsing and stores required bounds for generics,
trying to use some heuristics to remove unneeded bounds.

Signed-off-by: Justus Fluegel <[email protected]>
Signed-off-by: Justus Fluegel <[email protected]>
@JustusFluegel JustusFluegel marked this pull request as ready for review February 2, 2025 22:20
@JustusFluegel
Copy link
Author

This should be mostly good, I'll add some tests but I'd appreciate a comment-type review in the meantime if possible :)

…ementation strictly more flexible using to_owned over clone

Signed-off-by: Justus Fluegel <[email protected]>
@JustusFluegel
Copy link
Author

JustusFluegel commented Feb 3, 2025

ok tests are now there as well (thought I would do it later but oh well), I am sure there are some things I should change since this is a pretty big pr and I am not that familiar with this project but from my perspective this should be finished apart from that.

@JustusFluegel
Copy link
Author

JustusFluegel commented Feb 4, 2025

Oh and I just noticed since I added an explicit to_owned instead of just relying on an implicit copy for the label (I think, should test, maybe add a unit test for that?) #377 should work just fine as well. Since Copy ⊂ Clone ⊂ ToOwned this shouldn't actually be breaking (?), and make the derive strictly more flexible (although might be worth to check if that is actually the case)

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

Successfully merging this pull request may close these issues.

1 participant