Skip to content

Perfect-derive for miette Diagnostic derive (adding required bounds automatically to the impl based on the generics usage) #421

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

Open
wants to merge 9 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?

@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 :)

@JustusFluegel
Copy link
Author

JustusFluegel commented Feb 3, 2025

ok tests are now there as well (thought I would do it later but I noticed
i had some extra time on hand so I did it now), 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)

@zkat zkat added the breaking A semver-major breaking change label Mar 2, 2025
@zkat
Copy link
Owner

zkat commented Mar 2, 2025

Putting it behind a feature flag seems wise, yes.

@zkat zkat removed the breaking A semver-major breaking change label Mar 2, 2025
@JustusFluegel
Copy link
Author

JustusFluegel commented Mar 3, 2025

Hi, as I am in the process of moving right now I will probably only get back to this the next weekend or so, but my current plan for this is:

  • Add the suggested feature flag ( I would probably name it perfect_derive which aligns with other work on the same idea across the ecosystem )
    • Since we don't add bounds to the impl at all right now this should be a purely additive feature, which is what all feature flags should be
  • Write a higher level summary on the changes of this pr
    • explicitly list all the codegen changes inside the derive macro ( to make it easier to spot accidental breaking changes in review, even though I didn't mean to introduce any (Edit: There might be a change that could be considered breaking, you decide on your semver policy, it's outlined below :)) )
    • summarize how this feature actually works
  • Add docs to document the feature flag in more detail on both docs.rs as well crates.io ( if applicable )
  • fix the ci breakages ( I kindly request a ci re-run :) )
  • and, since I thought a bit more about the changes I did here I think I found a few ways to simplify things that I am then also going to do while I am at it :)

And finally, after all this is done I should probably prepare a change log entry, so I am going to do that as well.

If that sounds like a reasonable plan feel free to let me know what you think (or any other things I should do) otherwise I would just go ahead with that plan once I've got some more time to work on this ( as I've mentioned probably around next weekend )

@JustusFluegel
Copy link
Author

( accidentally pressed the close button, sorry for the noise )

@JustusFluegel JustusFluegel force-pushed the add-required-bounds-while-deriving branch 5 times, most recently from e35e69e to ea48010 Compare March 10, 2025 21:14
@JustusFluegel JustusFluegel marked this pull request as draft March 11, 2025 11:25
@JustusFluegel
Copy link
Author

Hey just a quick update that I am still working on this, just life kinda got in the way and I didn't have as much time as I wished to work on this. Will try to get this to a review-able state soon tho :)

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 Flügel <[email protected]>
Signed-off-by: Justus Fluegel <[email protected]>
Signed-off-by: Justus Flügel <[email protected]>
@JustusFluegel JustusFluegel force-pushed the add-required-bounds-while-deriving branch 9 times, most recently from 5308723 to 629ae49 Compare April 28, 2025 10:43
@JustusFluegel JustusFluegel force-pushed the add-required-bounds-while-deriving branch 2 times, most recently from 4851090 to 5017778 Compare April 28, 2025 10:49
@JustusFluegel
Copy link
Author

JustusFluegel commented Apr 28, 2025

Codegen changes

Summary of generated (derived) code changes from this pr, excluding additional while bounds included with the perfect-derive feature:

#[labels] gen_struct

miette::macro_helpers::OptionalWrapper::<#ty>::new()
     .to_option(&self.#span)
+     .as_ref()
-     .map(|#var| #ctor(
-          #display,
-          #var.clone(),
-     ))
+     .map(|#var| {
+         use ::std::borrow::ToOwned;
+
+         #ctor(
+             #display,
+             (*#var).to_owned(),
+         )
+     })

This should be non-breaking since we explicitly add the ref before we remove it again (ends up with the same amout of references before calling .to_owned(), but this should fix #377 ), and ToOwned is strictly larger than Clone. (The impl<T: Clone> ToOwned for T { ... } impl in the std library guarantees this, since we can't have overlapping impls and as such all ToOwned impls for T: Clone must use their Clone implementation, at least without specialization)

.chain({
    let display = #display;
-   self.#span.iter().map(move |span| {
+   ::std::iter::IntoIterator::into_iter(&self.#span).map(move |span| {
        use miette::macro_helpers::{ToLabelSpanWrapper,ToLabeledSpan};
+       use ::std::borrow::ToOwned;


-       let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(span.clone());
+       let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(span.to_owned());
        if display.is_some() && labeled_span.label().is_none() {
            labeled_span.set_label(display.clone())
        }

        Some(labeled_span)
    })
})

This might strictly be considered breaking because of the change from x.iter() to (&x).into_iter() but clippy already warns at implementations of the iter() fn on types T without a corresponding IntoIterator implementation for &T, as well as the other way around. Another argument for this being strictly breaking, is that we didn't specify a fully qualified path for .iter() which in practice means that implementations that are in scope are considered as well.

I would like to argue though that this is not really that big of an issue as it may seam at first, since in practice using the derive macro with generic types was almost impossible or very hard and using custom, not well-behaving collection types (I don't know of any in the ecosystem), meaning ones that have a .iter() but not a IntoIterator for &T impl, should be super rare.

But I guess depending on how strict you are with your semver this would mean a breaking change.

As for the .to_owned() change, it's the same argument as above.

std::option::Option::Some(
    Box::new(
        labels_iter
-            .filter(Option::is_some)
-            .map(Option::unwrap)
+            .filter_map(|x| x) 
    )
)

This should be a very straightforward simplification.

#[labels] gen_enum

miette::macro_helpers::OptionalWrapper::<#ty>::new()
     .to_option(#field)
+     .as_ref()
-     .map(|#var| #ctor(
-          #display,
-          #var.clone(),
-     ))
+     .map(|#var| {
+         use ::std::borrow::ToOwned;
+
+         #ctor(
+             #display,
+             (*#var).to_owned(),
+         )
+     })

same as with gen_struct above.

std::option::Option::Some(
    Box::new(
        labels_iter
-            .filter(Option::is_some)
-            .map(Option::unwrap)
+            .filter_map(|x| x) 
    )
)

same as with gen_struct above.

.chain({
    let display = #display;
-     #field.iter().map(move |span| {
+    ::std::iter::IntoIterator::into_iter(#field).map(move |span| {
        use miette::macro_helpers::{ToLabelSpanWrapper,ToLabeledSpan};
+       use ::std::borrow::ToOwned;
        let mut labeled_span = ToLabelSpanWrapper::to_labeled_span(
+            span.to_owned()
-            span.clone()
        );
        if display.is_some() && labeled_span.label().is_none() {
            labeled_span.set_label(display.clone());
        }
        Some(labeled_span)
    })
})

again, same as with gen_struct above.

Apart from formatting changes (I would recommend running rustfmt with the --config error_on_line_overflow=true option which is the reason for these changes, the lines were too long)
this should be every single change in the macro output apart from the additional where bounds added when the perfect-derive feature is enabled, so without enabling any additional feature flag.

Some context for why I made these changes: using IntoIterator and ToOwned provided Traits I can actually use as bounds in the where clause.
The iterator simplification is in there just cause I spotted an unwrap and wanted to get rid of it :)

@JustusFluegel JustusFluegel force-pushed the add-required-bounds-while-deriving branch from 5017778 to 0451f59 Compare April 28, 2025 12:01
@JustusFluegel
Copy link
Author

JustusFluegel commented Apr 28, 2025

Functional summary

So a functional summary of the changes introduced in this pr:

The general idea of perfect-derive is to keep track of the types of fields of structs and enums and look at the context what they are used for inside the derive macro and then add trait bounds (additional where clause items) to the types automatically.

To facilitate this, this pr introduces a TypeParamBoundStore which is basically a map from types (of fields) to trait bounds, which will then be added to the where clause at the end when generating the trait impl.
Additionally, the TypeParamBoundStore implements some simple heuristics to determine if trait bounds added to it via it's methods are actually generic, e.g. have the generics used inside of them.
This prevents adding trivial type bounds like String: Display to the where clause, which while not breaking (the compiler accepts such trivial bounds) are unnecessary. It also ensures no duplicate bounds are added to each type.

At the point of implementation, where the struct fields are then used, we always add the where bounds to the store as if the type is generic, meaning for example miette::Diagnostic for the type of the first field of the enum variant annotated with #[diagnostic(transparent)].

When the perfect_derive feature is disabled, the TypeParamBoundStore is then just replaced with a empty mock implementation where it is a ZST, which should be completely optimized away by the compiler.

Using the TypeParamBoundStore at the place of use required passing a &mut reference to it around, which is why there are quite a few changes related to that in this pr.

@JustusFluegel
Copy link
Author

JustusFluegel commented Apr 28, 2025

I hope the last 2 comments help a bit with reviewing this pr, I know it is not super small (~+750 lines overall) but I appreciate if you could take the time @zkat :)

Thank you in advance
Justus Flügel

@JustusFluegel JustusFluegel marked this pull request as ready for review April 28, 2025 12:19
@JustusFluegel JustusFluegel changed the title Add required bounds to derived impl Perfect-derive for miette Diagnostic derive (adding required bounds automatically to the impl based on the generics usage) Apr 29, 2025
@JustusFluegel
Copy link
Author

JustusFluegel commented May 2, 2025

One Idea I had to make this strictly non-breaking in the one small place where it theoretically could be, is to continue to emit the old declarations in the one place where it could be seen as breaking if the feature is not enabled, and then just disable that on the next breaking release. - if that is desired just let me know and I'll quickly patch that in as well :)

@JustusFluegel JustusFluegel force-pushed the add-required-bounds-while-deriving branch from 0451f59 to dc74a05 Compare May 2, 2025 16:51
@JustusFluegel JustusFluegel force-pushed the add-required-bounds-while-deriving branch from dc74a05 to 0e95e81 Compare May 3, 2025 11:59
@JustusFluegel
Copy link
Author

Also: May I kindly request a ci re-run? :)

@JustusFluegel
Copy link
Author

JustusFluegel commented Jun 2, 2025

Quick ping @zkat just if you have seen this? No worries if you currently don't have the time/will/... to look at this.
A quick comment that you aware of this would be awesome tho :)

@zkat
Copy link
Owner

zkat commented Jun 3, 2025

I've seen it. I'm just a bit busy to look at it right this second, in the context of potentially moving miette to facet

@JustusFluegel
Copy link
Author

JustusFluegel commented Jun 3, 2025

Thanks for taking the time to respond :) No worries if you are busy, take care of your more important things first, this (and I) can wait.

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.

2 participants