JelteF / derive_more

Some more derive(Trait) options
MIT License
1.68k stars 117 forks source link

FR: Add `#[display(delegate)]` option for newtypes which delegates to wrapped type #321

Closed mina86 closed 8 months ago

mina86 commented 9 months ago

Add option to allow delegating Display implementation to wrapped field or to implementation of another formatting trait.


Currently, using something akin to #[display(fmt = "{}", _0)] for newtypes suffers from the issues of all user-provided flags being reset. For example:

#[derive(derive_more::Display)]
#[display(fmt = "{}", _0)]
struct Num(usize);

struct Expected(usize);

impl std::fmt::Display for Expected {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        self.0.fmt(fmtr)
    }
}

fn main() {
    println!("got={:03} want={:03}", Num(7), Expected(7));
        // → got=7 want=007
}

The idea is to support #[dispaly(delegate)] annotation which would delegate call rather than use write! macro. For example:

#[derive(derive_more::Display)]
#[display(delegate)]
struct Num(usize);

would generate:

impl std::fmt::Display for Num {
    #[inline]
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        self.0.fmt(fmtr)
    }
}

This could be further extended to full form:

#[derive(derive_more::Display)]
#[display(delegate, trait=Debug, field=_0.display())]
struct Path(std::path::Path, ());

which would end up as:

impl std::fmt::Display for Expected {
    #[inline]
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        std::fmt::Debug::fmt(&self.0.display(), fmtr)
    }
}

Those are further ideas though. Even just a simple #[display(delegate)] which would work on structs with a single field would be neat.

JelteF commented 9 months ago

Using no attribute at should already do what you want:

#[derive(derive_more::Display)]
struct Num(usize);
mina86 commented 9 months ago

So it does, thanks. For some reason I was convinced that #[display(...)] is required. Still would be cool if delegating to other traits was possible, but the main point of this FR is indeed already present.

tyranron commented 9 months ago

@mina86 what is the problem to just use other traits directly?

#[derive(derive_more::Display)]
#[display("{:?}", _0.display())]
struct Path(std::path::Path, ());
mina86 commented 9 months ago

You’re discarding formatting flags provided by the user in format string, e.g.:

#[derive(derive_more::Display)]
#[display(fmt = "{:?}", _0)]
struct Num(usize);

impl std::fmt::Debug for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        self.0.fmt(fmtr)
    }
}

fn main() {
    let num = Num(7);
    println!("{num:03?}");  // prints ‘007’ as expected
    println!("{num:03}");   // prints ‘7’ instead
}
tyranron commented 9 months ago

@mina86 hmmm... good catch!

Theoretically, we can support this with the current syntax, because we can detect so-called trivial cases and transform them into delegation (we do parse formatting string literal anyway).

#[derive(derive_more::Display)]
#[display("{_0:?}")] // <--- it's clear to be a trivial delegation case
struct Num(usize);

would expand to

impl std::fmt::Display for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        let _0 = &self.0;
        std::fmt::Debug::fmt(_0, fmtr)
    }
}

rather than

impl std::fmt::Display for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        let _0 = &self.0;
        write!(fmtr, "{_0:?}")
    }
}

Do you see any pitfalls with this?

mina86 commented 9 months ago

That would work for me and I don’t see any issues. Just two points to clarify:

#[derive(derive_more::Display)]
#[display("{_0:x}")]            // delegates to LowerHex?
struct Num(usize);

#[derive(derive_more::Display)]
#[display("{}", _0.display())]  // does _0.display().fmt(fmtr)?
struct Path(std::path::PathBuf);
JelteF commented 9 months ago

I think that's quite interesting. The only downside I can see (apart from the extra code) that it if you try to strip them intentionally, there's no way to do that. But that doesn't seem a huge problem. That does make it a breaking change though, so we'd want to do this for the 1.0 release...

JelteF commented 9 months ago

I actually think we should support it even with a prefix/suffix. So if you have this:

#[derive(derive_more::Display)]
#[display("prefix-{_0:?}-suffix")] // <--- it's clear to be a trivial delegation case
struct Num(usize);

it generates:

impl std::fmt::Display for Num {
    fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
        let _0 = &self.0;
        fmt.write_str("prefix-")
        std::fmt::Debug::fmt(_0, fmtr)
        fmt.write_str("-suffix")
    }
}
mina86 commented 9 months ago

I’m not sure about the latter point. Consider in your example, it’s likely that the intention was that the resulting formatted string contains no separators or padding. For example, in IBC there are identifiers in format prefix-<num> and the identifier is invalid if <num> has leading zero digits.

tyranron commented 9 months ago

@JelteF

The only downside I can see (apart from the extra code) that it if you try to strip them intentionally, there's no way to do that.

There is always a way to do that like this:

#[derive(Display)]
#[display("{}", format_args!("{_0:?}"))]
struct Num(usize);

I actually think we should support it even with a prefix/suffix.

I don't think so. It's clearly not a delegation case anymore, and following fmt semantics would be rather confusing here.

I think we should add this only if somebody will ask for it for real. Otherwise, seems to be unnecessary complication.

That does make it a breaking change though, so we'd want to do this for the 1.0 release...

meme_cat_honor

JelteF commented 9 months ago

alright I'm convinced. Let's only handle the trivial delegation cases.