JelteF / derive_more

Some more derive(Trait) options
MIT License
1.73k stars 123 forks source link

Only put Debug-like bounds on type variables #371

Closed sornas closed 3 months ago

sornas commented 4 months ago

Resolves #363

Well, at least it's a suggestion for a resolution :p

Synopsis

The problem, as reported in the issue, is that code like the following

#[derive(derive_more::Debug)]
struct Item {
    next: Option<Box<Item>>,
}

expands into something like

impl std::fmt::Debug for Item where Item: Debug { /* ... */ }

which does not compile. This PR changes the Debug derive so it does not emit those bounds.

Solution

My understanding of the current code is that we iterate over all fields of the struct/enum and add either a specific format bound (e.g. : fmt::Binary), a default : fmt::Debug bound or skip it if either it is marked as #[debug(skip)] or the entire container has a format attribute.

The suggested solution in the issue (if I understood it correctly) was to only add bounds if the type is a type variable, since rustc already knows if a concrete type is, say, : fmt::Debug. So, instead of adding the bound for every type, we first check that the type contains one of the container's type variables. Since types can be nested, it is an unfortunately long recursive function handling the different types of types. This part of Rust syntax is probably not going to change, so perhaps it is feasible to shorten some of the branches into _ => false.

One drawback of this implementation is that we iterate over the list of type variables every time we find a "leaf type". I chose Vec over HashSet because in my experience there are only a handful of type variables per container.

Status

I should add more tests, probably? I've at least verified the case mentioned above (for tuple structs, data structs and enums). I must admit I haven't really looked in the documentation, but I should probably edit that too.

Checklist

JelteF commented 4 months ago

@sornas I have some time to work on derive_more the next ~2 weeks. Since this is one of the last items that's blocking 1.0, I was wondering if you'll have time to work on this during that time. Otherwise I think I might pick it up to get it over the finish line, if I'm able to get the other 1.0 blockers done.

sornas commented 4 months ago

unfortunately i don't think i'll have time, so feel free to take over. but i'll let you know here if it changes.

link2xt commented 3 months ago

This change resulted in a regression, after updating from 1.0.0-beta.6 to 1.0.0-beta.7 compilation fails in https://github.com/deltachat/deltachat-core-rust/pull/5850

JelteF commented 3 months ago

Hmm, that's not great. Can you make a new issue for this that shows some code that now fails compilation?

link2xt commented 3 months ago

Update: seems to be fixed (or worked around?) in https://github.com/n0-computer/iroh/pull/2578

link2xt commented 3 months ago

I opened an issue https://github.com/JelteF/derive_more/issues/392