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

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

Closed
mina86 opened this issue Dec 13, 2023 · 11 comments · Fixed by #322
Closed

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

mina86 opened this issue Dec 13, 2023 · 11 comments · Fixed by #322
Assignees
Milestone

Comments

@mina86
Copy link

mina86 commented Dec 13, 2023

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.

@mina86 mina86 changed the title FR: Add #[display(forward)] option for newtypes which delegates to wrapped type FR: Add #[display(delegate)] option for newtypes which delegates to wrapped type Dec 13, 2023
@JelteF
Copy link
Owner

JelteF commented Dec 13, 2023

Using no attribute at should already do what you want:

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

@mina86
Copy link
Author

mina86 commented Dec 13, 2023

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.

@mina86 mina86 closed this as completed Dec 13, 2023
@tyranron
Copy link
Collaborator

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

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

@mina86
Copy link
Author

mina86 commented Dec 13, 2023

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
Copy link
Collaborator

tyranron commented Dec 13, 2023

@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?

@tyranron tyranron reopened this Dec 13, 2023
@tyranron tyranron self-assigned this Dec 13, 2023
@mina86
Copy link
Author

mina86 commented Dec 13, 2023

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
Copy link
Owner

JelteF commented Dec 13, 2023

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
Copy link
Owner

JelteF commented Dec 13, 2023

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
Copy link
Author

mina86 commented Dec 13, 2023

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
Copy link
Collaborator

@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

@tyranron tyranron added this to the 1.0.0 milestone Dec 15, 2023
@JelteF
Copy link
Owner

JelteF commented Dec 15, 2023

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

tyranron added a commit that referenced this issue Dec 22, 2023
## Synopsis

See
#321 (comment):
> You’re discarding formatting flags provided by the user in format
string, e.g.:
> 
> ```rust
> #[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
> }
> ```






## Solution

See
#321 (comment):
> 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).
> 
> ```rust
> #[derive(derive_more::Display)]
> #[display("{_0:?}")] // <--- it's clear to be a trivial delegation
case
> struct Num(usize);
> ```
> 
> would expand to
> 
> ```rust
> 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
> 
> ```rust
> impl std::fmt::Display for Num {
> fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
>         let _0 = &self.0;
>         write!(fmtr, "{_0:?}")
>     }
> }
> ```
liveseed added a commit to liveseed/derive_more that referenced this issue Aug 20, 2024
## Synopsis

See
JelteF/derive_more#321 (comment):
> You’re discarding formatting flags provided by the user in format
string, e.g.:
> 
> ```rust
> #[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
> }
> ```






## Solution

See
JelteF/derive_more#321 (comment):
> 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).
> 
> ```rust
> #[derive(derive_more::Display)]
> #[display("{_0:?}")] // <--- it's clear to be a trivial delegation
case
> struct Num(usize);
> ```
> 
> would expand to
> 
> ```rust
> 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
> 
> ```rust
> impl std::fmt::Display for Num {
> fn fmt(&self, fmtr: &mut std::fmt::Formatter) -> std::fmt::Result {
>         let _0 = &self.0;
>         write!(fmtr, "{_0:?}")
>     }
> }
> ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants