Redundant bound for Debug on skipped field/make display(bound) do the same as debug(bound)

[ModProg]

struct NoDebug;
                                      
#[derive(derive_more::Debug)]
struct HasDebug<T>(#[debug(skip)] T);
                                      
dbg!(HasDebug(NoDebug));

I'd be fine with needing to manually specify something like #[debug(bounds())].

[ModProg]

I wonder if (this would need to happen before 1.0) we could have:

1. no #[<trait>(bounds(...))] => default behavior i.e. bound on all generics.
2. #[<trait>(add_bound(T, T::Associated, U: CustomTrait))] => keep default behavior, but additionally add trait bounds: T: <Trait>, T::Associated: <Trait>, U: CustomTrait
3. #[<trait>(bounds(T, T::Associated, U: CustomTrait))] => no default bounds, only add trait bounds: T: <Trait>, T::Associated: <Trait>, U: CustomTrait. This means, that #[<trait>(bounds())] results in no bounds.

Syntax was inspired by derive_where: https://docs.rs/derive-where/latest/derive_where/#generic-type-bounds

[ModProg]

The 1. behavior could be extended with some heuristics checking for usages of the generic, i.e. remove the bound if it is only used in skipped fields. But this is more complex and I don't know if we can do this reliably.

[JelteF]

Adding this to 1.0.0 milestone because removing bounds is technically a breaking change.

@ilslv @tyranron I thought for Display we automatically added bounds when necessary. Are we missing this completely for Debug? Or do we only not take into account the skip attribute when adding the bounds.

[ilslv]

@JelteF Debug should also add only required bounds, this is definitely a bug

[ModProg]

Maybe bounds should be handled somewhat centrally, i.e. the same for all traits.

I think we could, have a struct, that takes:

1. The generics of the derive input
2. The bounds and add_bound attribute
3. in the "body generation" we tell this bound handler about every used field's type and it marks the generics used in this type as "used", i.e. adds a bound except when bounds was specified.

Afterwards, we call some "get impl generics" on this bound handler that returns the generics we can use in the impl trait signature.

[tyranron]

@ModProg I'm not a fan on bound/add_bound separation. Seems like having bound is enough. The situations where we may want add_bound over bound are too rare.

[ilslv]

agreed, the idea is to impose only bounds, that are 100% required and then add missing ones with bounds(...)

[tyranron]

@ilslv @JelteF @ModProg

the idea is to impose only bounds, that are 100% required and then add missing ones with bounds(...)

From what I see in code docs, the current behavior is:

• For Display-like macro to replace auto-inferred bounds with what is provided in bound(..).
• For Debug macro to enrich the provided bound(..)s with auto-inferred ones.

We cannot actually change the way it works for Display, because this way we leave no choice how to get rid of undesired auto-inferred bounds. Or can we?

[ModProg]

@ModProg I'm not a fan on bound/add_bound separation. Seems like having bound is enough. The situations where we may want add_bound over bound are too rare.

What do we do on generics of field types? i.e. Vec<T> we should add T: Debug but for PhantomData<T> there is a point in not wanting that bound.

We can hard code them, but that is problematic, as anyone can add their own type that does or does not have a bound on their generics.

I'd like the default behavior to be matching the std, i.e. add bounds on generics that are in fields that are used, so you don't need to add a bounds(T: Debug) when you have a Vec<T>. But we should have some way, other than straight up skipping a field, to ignore a used generic from the trait bounds.

But I'd be fine with having bounds be my add_bounds and have a rm_bounds or replace_bounds instead. We could also have the ? syntax that the compiler uses for, e.g. Sized. I.e. to remove a bound you'd write bounds(T: ?Debug). But that would maybe be hard to find for a newer user.

[ModProg]

From what I see in code docs, the current behavior is:

• For Display-like macro to replace auto-inferred bounds with what is provided in bound(..).

• For Debug macro to enrich the provided bound(..)s with auto-inferred ones.

That sounds bad in my opinion, the behavior should be identical over all traits.

[ModProg]

We could also have a no_infered_bounds flag? But I think there is no way to not enforce some unneeded bounds like PhantomData<T> without missing some convenient bounds like the Vec<T>.

[tyranron]

@ModProg

What do we do on generics of field types?

We do not bound type parameters blindly as std does, we bound concrete field types instead, depending on how they're used. This is described in docs:

E.g., for a structure Foo defined like this:

use derive_more::Debug;

#[derive(Debug)]
struct Foo<'a, T1, T2: Trait, T3, T4> {
    #[debug("{a}")]
    a: T1,
    #[debug("{b}")]
    b: <T2 as Trait>::Type,
    #[debug("{c:?}")]
    c: Vec<T3>,
    #[debug("{d:p}")]
    d: &'a T1,
    #[debug(skip)]
    e: T4,
}

trait Trait { type Type; }

The following where clauses would be generated:

• T1: Display
• <T2 as Trait>::Type: Display
• Vec<T3>: Debug
• &'a T1: Pointer

---

That sounds bad in my opinion, the behavior should be identical over all traits.

I don't see any problems with that. Debug and Display-like derives have different semantics, they work differently, they generate code differently, the use-cases and expectations are different. What really matter here for me is final ergonomics for the majority of cases. And the way they are now is OK with that.

Making #[debug(bound(..))] replacing existing trait bounds would worsen ergonomics definitely.

The interesting question here is whether making #[display(bound(..))] adding to existing trait bounds wouldn't be wrong? Is there any case where we would like to replace auto-inferred bound with our custom one? At first glance, seems that no, because we need a custom bound in the situations where auto-inferring doesn't work. But is this always true? Can we come up with the situation where we need to replace an auto-inferred bound with a custom one?

[ModProg]

@tyranron, yeah, sorry I was under the false impression derive_more was doing the same as std.

Can we come up with the situation where we need to replace an auto-inferred bound with a custom one?

I don't think so, my case presented earlier only applies to the std strategy not ours, so I cannot really think of one.

I don't see any problems with that. Debug and Display-like derives have different semantics, they work differently, they generate code differently, the use-cases and expectations are different.

They do, but they have exactly the same syntax on bound so it should be identical in my opinion, but maybe we can make both behave like debug, and then it would be fine.

I think the main issue from my side was the false impression that derive_more uses the same flawed bounds std does.

[tyranron]

@JelteF @ilslv

The interesting question here is whether making #[display(bound(..))] adding to existing trait bounds wouldn't be wrong? Is there any case where we would like to replace auto-inferred bound with our custom one? At first glance, seems that no, because we need a custom bound in the situations where auto-inferring doesn't work. But is this always true? Can we come up with the situation where we need to replace an auto-inferred bound with a custom one?

What do you think on that? It really seems that we can change the "replacing" strategy to "adding" in #[display(bound(..))], and so, having more API consistency and better ergonomics.

[tyranron]

Keeping it open until we decide on the question above.

[ModProg]

Keeping it open until we decide on the question above.

Maybe should rename the issue?

[tyranron]

ping @JelteF

[JelteF]

Okay read the thread. I agree let's make this consistent and only have bound add to the auto-inferred bounds. If we're automatically adding bounds that are incorrect, then I would consider that a bug on our end. (just like personally I consider the strategy used by the std Debug derive to be a bug, but that seems impossible to change for them now due to backwards compatibility)

[tyranron]

cc @MegaBluejay please, do after finishing all the stuff with AsRef/AsMut.