Redesign `Display` derive macro #225

ilslv · 2022-11-18T10:29:45Z

Resolves #217

Synopsis

Solution

Use expressions instead of string literals for fmt arguments:
#[display("{}", "crab.rave()")] -> #[display("{}", crab.rave())]

Checklist

Documentation is updated (if required)
Tests are added/updated (if required)
CHANGELOG entry is added (if required)

ilslv · 2022-11-18T11:44:32Z

@tyranron @JelteF so we are on the same page, let me go over design decisions, that are already set in stone, before final decision on #142 is made.

This should cover every possible use-case, everything else should result in compile-time error. For example:

#[derive(Display)]
#[display("Field: {}")] // error: 1 positional argument in format string, but no arguments were given
struct Single(i32);

Structs

Unit

#[derive(Display)]
struct Unit; // "Unit"

#[derive(Display)]
struct Tuple (); // "UnitTuple"

#[derive(Display)]
struct Struct {} // "UnitStruct"

Single-field

#[derive(Display)]
struct Tuple(i32); // "1"

#[derive(Display)]
struct Struct { field: i32 } // "1"

#[derive(Display)]
#[display("Pre: {_0}")]
struct PreTuple(i32); // "Pre: 1"

#[derive(Display)]
#[display("Pre: {field}")]
struct PreStruct { field: i32 } // "Pre: 1"

Multi-field

#[derive(Display)]
#[display("{_0} {ident} {_1} {} {}", _1, any_expr(_0, _1), ident = 123, _1 = _0)]
struct Tuple(i32, i32) // "1 123 1 2 3"

#[derive(Display)]
#[display("{field1} {ident} {field2} {} {}", field2, any_expr(_0, _1), ident = 123, field2 = field1)]
struct Struct { field1: i32, field2: i32 } // "1 123 1 2 3"

Enums

Without variants (Infallible-like)

#[derive(Display)]
enum Void {}

const fn has<T: Display>() {}
const _ = has::<Void>();

Unit variant

#[derive(Display)]
enum Enum {
    First, // "First"
    FirstTuple(), // "FirstTuple"
    FirstStruct {}, // "FirstStruct"
    #[display("SECOND")]
    Second, // "SECOND"
    #[display("SECOND_TUPLE")]
    SecondTuple(), // "SECOND_TUPLE"
    #[display("SECOND_STRUCT")]
    SecondStruct {}, // "SECOND_STRUCT"
}

Single-field variant

#[derive(Display)]
enum Enum {
    First(i32), // "1"
    FirstNamed  { field: i32 }, // "1"
    #[display("Second: {_0}")]
    Second(i32), // "Second: 1"
    #[display("Second: {field}")]
    SecondNamed { field: i32 }, // "Second: 1"
}

Multi-field variant

#[derive(Display)]
enum Enum {
    #[display("{_0} {ident} {_1} {} {}", _1, any_expr(_0, _1), ident = 123, _1 = _0)]
    Tuple(i32, i32), // "1 123 1 2 3"
    #[display("{field1} {ident} {field2} {} {}", field2, any_expr(_0, _1), ident = 123, field2 = field1)]
    Struct { field1: i32, field2: i32 }, // "1 123 1 2 3"
}

Interactions with top-level attribute

TODO: Fill out, once #142 is resolved

Unions

#[derive(Display)]
#[display("No field accesses are allowed, but {} is ok", "interpolation")]
union Union { // No field accesses are allowed, but interpolation is ok
    field: i32,
}

ilslv · 2022-11-18T12:18:22Z

Discussed with @tyranron: Merge this PR without support for top-level enum attribute and do it as a separate PR.

tyranron · 2022-11-18T12:44:23Z

@ilslv @JelteF one thing I still dislike is semantics for unit types:

#[derive(Display)]
struct Unit; // "Unit"

#[derive(Display)]
struct Tuple (); // "UnitTuple"

#[derive(Display)]
struct Struct {} // "UnitStruct"

#[derive(Display)]
enum Enum {
    First, // "First"
    FirstTuple(), // "FirstTuple"
    FirstStruct {}, // "FirstStruct"
}

This is too subtle and implicit. And may be too unexpected if I write derive(Octal) or similar.

I think it's better to return error in such cases, pushing a user to explicitly specify what he wants. Giving a convenient top-level attribute semantics, there would be almost no ergonomics impact if he needs exactly this behaviour:

#[derive(Debug, Display)]
#[display("{self:?}")]
struct Unit; // "Unit"

#[derive(Debug, Display)]
#[display("{self:?}")]
struct Tuple (); // "Tuple"

#[derive(Debug, Display)]
#[display("{self:?}")]
struct Struct {} // "Struct"

#[derive(Debug, Display)]
#[display("{self:?}")]
enum Enum {
    First, // "Enum::First"
    FirstTuple(), // "Enum::FirstTuple"
    FirstStruct {}, // "Enum::FirstStruct"
}

But I would argue that most of the time, a user doesn't expect something Debug-like on Displaying unit types.

ilslv · 2022-11-18T13:03:48Z

@tyranron

This is too subtle and implicit. And may be too unexpected if I write derive(Octal) or similar.

Valid point, if something like this exists, it should be only for Display.

I want to explain, why I've added this behaviour in a first place.
From my personal experience, it's very unpleasant to write something like this by hand:

// #[derive(Display)]
enum Enum {
    A, 
    B,
    C,
}

impl fmt::Display for Enum {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            Self::A => "A".fmt(f),
            Self::B => "B".fmt(f),
            Self::C => "B".fmt(f),
        }
    }
}

This is a lot of tedious code to write and copy-pasting may lead to errors. Have you noticed, that code block above contains an error?

I consider this feature desired, understandable and easily discoverable just by writing code without looking into the docs. Emphasis on discoverable, because this is one of the most delightful features of Rust to me. Discoverable intuitive design with compile-time checks is a match made in heaven. I don't think you can misinterpret it and your suggested approach doesn't cover this use-case.
For unit structs this feature is less desirable, but I would add it just for symmetry. #[display("{self:?}")] looks really scary and something like only maintainer with a deep knowledge of this crate will use 🙃

tyranron · 2022-11-18T14:05:03Z

@ilslv

#[display("{self:?}")] looks really scary and something like only maintainer with a deep knowledge of this crate will use 🙃

Isn't this the first thing a rustacean would try (or write manually) when he thinks about "Display as Debug" semantics?

I don't think you can misinterpret it

I do think such a feature implicitly pushes a library user to mix Display/Debug semantics, which is usually not a good idea. Given that most use-cases of this macro, in practice, is defining custom error types, I would prefer a compilation error about forbidden error description, rather than implicitly making it Debug-like.

and your suggested approach doesn't cover this use-case.

Ah, you're talking more about strum::Display semantics, rather than "Display as Debug" semantics, so we can replace strum::Display with derive_more::Display naturally. Now, what you're talking about makes much more sense. 🤔

Yeah, restricting it to Display only, has sense. But I would like to hear other opinions as well.
ack @JelteF @ModProg

ilslv · 2022-11-18T14:12:27Z

@tyranron

Isn't this the first thing a rustacean would try (or write manually) when he thinks about "Display as Debug" semantics?

To be fair I didn't think about this possibility, when implementing this functionality. And even if I just wasn't thinking enough, this assumption applies only to experienced rustaceans, but this library isn't only for them with ~50k daily downloads.

Given that most use-cases of this macro, in practice, is defining custom error types

I wouldn't just jump to this assumption, especially when there is thiserror. General-purpose Displays should be at the same level of priority.

so we can replace strum::Display with derive_more::Display naturally

Exactly!

…ay` macro

tyranron · 2022-12-12T18:11:02Z

@JelteF @ilslv @ModProg gathering all the discussions regarding this PR here. Sorry for late reply, was quite overwhelmed with stuff last weeks.

To be clear, I haven't yet looked at the code. I think the Unit enum feature sounds useful, but shouldn't be part of this PR. Also introducing it doesn't break the Display derive API, so I don't think it should be part of the 1.0.0 milestone. I think we should move discussion about that feature to #216

As @ilslv mentioned in #216, it's already present in master and 0.99 releases. I don't think that throwing it would be a nice way to go.

A good question raised in the thread in #225 would be what should be the default? My summary of that discussion is that there are two different usages for which a different default makes sense:

Error types: you probably want a custom error message. By requiring to set this attribute you would not be able to forget to set a custom message for one of your variants.

Any other enum type: You probably want nice display by default

I'd say go for strict by default for now, i.e. error by default. Then if that is deemed too cumbersome we can always relax the default. However, unrelaxing the default would be a breaking change.

I gave this quite a though and came to quite a fresh idea:
As we cannot unite these use cases meaningfully, what about splitting them in the most natural way? Since the Error trait requires Display anyway, let's derive it directly when expanding Error macro, and have all the validation we require for such use case. As the bonus(?), this way we will be semantically closer to thiserror::Error.

#[derive(Display)]
enum AnotherEnum { // just a regular general-purpose enum here, not an error type
    A,          // prints "A"
    B(String),  // prints the inner string
}

#[derive(Error)] // look, ma! we don't need to specify obvious `Display` here
enum AnotherError {
    A,          // expansion error, as we forgot to specify error description here
    B(String),  // prints the inner string
}

#[derive(Error)] 
enum YetAnotherError {
    #[error("Errored because of A")]
    A,          // prints "Errored because of A"      
    #[error("Because of {_0}")]
    B(String),  // prints "Because of <inner-string>"
}

#[derive(Display, Error)]
#[error(skip(Display))]
enum StrangeError { // we intentionally do want exactly this behavior here
    A,          // prints "A"
    B(String),  // prints the inner string
}

This way, we:

doesn't tight user hands in expressing corner-cases, while do provide reasonable and meaningful defaults
improve ergonomics for deriving error types

ModProg · 2022-12-12T18:58:54Z

@tyranron generally sound good, only thing it cannot unite is my "I want an error when I forget to add a format to an enum Variant".

But that would've been a breaking change anyway.

tyranron · 2022-12-12T19:28:58Z

@ModProg

only thing it cannot unite is my "I want an error when I forget to add a format to an enum Variant".

Why? Isn't this case about this?

#[derive(Error)] // look, ma! we don't need to specify obvious `Display` here
enum AnotherError {
    A,          // expansion error, as we forgot to specify error description here
    B(String),  // prints the inner string
}

Or you've meant any enum variant, not just a unit-like?

ModProg · 2022-12-12T20:52:14Z

@tyranron yes, and my usecase wasn't an error. It was generating commands.

ilslv · 2022-12-13T12:49:37Z

@tyranron

#[derive(Error)] 
enum YetAnotherError {
    #[error("Errored because of A")]
    A,          // prints "Errored because of A"      
    #[error("Because of {_0}")]
    B(String),  // prints "Because of <inner-string>"
}

I don't really like approach of Error serving 2 functions, because everything else doesn't. I really like seeing explicit #[derive(Display, Error)] vs #[derive(Error)] in case of a thiserror crate.

#[derive(Error)] // look, ma! we don't need to specify obvious `Display` here
enum AnotherError {
    A,          // expansion error, as we forgot to specify error description here
    B(String),  // prints the inner string
}

What if there is manual impl Display for AnotherError? I don't know whether we can error on compile time, based on existence of Display impl, especially when generics will get involved.

Moreover I don't really think "forgetting to specify formatting" is a big deal. In this case error message may confuse you for a second, but code itself should help pin down the cause for an error.
I do think that "forgetting to update error message on refactoring" is a bigger issue, because you may see familiar error message and spent a lot of time hunting error cause in a wrong place. It may be very frustrating, so I would vote for leaving unit and single-field structs with smarter formatting.

ModProg · 2022-12-13T13:35:50Z

Moreover I don't really think "forgetting to specify formatting" is a big deal. In this case error message may confuse you for a second, but code itself should help pin down the cause for an error.

Well if it is an error that is, in different usecases this might be a bigger issue.

But I do understand that for most users this is more convenient than requiring them always.

Maybe it could be a default-feature that can be disabled? But in the end, I'm fine with anything.

JelteF · 2022-12-19T00:23:24Z

@tyranron While I agree that Error deriving display is more ergonomic, it's not in line with how derives normally work afaik. At least in the stdlib it's not how it works. Eq alse requires deriving PartialEq. Also, I think such a proposed subtle difference between how derive(Error) derives Display and how derive(Display) would cause confusion itself. Especially since (in your example at least), one uses error(...) while the other would use display(...) as the attribute.

After all this extra information and discussion, I'm now siding with @ilslv and I agree we should derive units by default.

JelteF · 2022-12-19T00:24:51Z

Maybe it could be a default-feature that can be disabled? But in the end, I'm fine with anything.

That seems like a non-breaking change (since it's opt-in), so I think we can postpone that decision until after 1.0

tyranron · 2022-12-20T09:05:15Z

@JelteF thank you for the feedback. You and @ilslv convinced me that the current behaviour would be a lesser evil.

I'll review this PR shortly, so we can move towards releasing 1.0.

tyranron

I'll review this PR shortly.

shortly

_shortly_™... 😅

@ilslv what an amazing work. Thanks!

I've adjusted docs a little and some local variables naming. Everything else seems good.

tyranron · 2023-01-23T14:40:22Z

@JelteF would you like to take a look on this? Or we can merge it?

The CI issue is fixed in #231.

JelteF · 2023-01-23T16:11:29Z

I merged #231 if this is rebased I guess CI should pass now. Feel free to merge it once that happens.

impl/doc/display.md

Related to #225 ## Synopsis Diagnostic introduced in #225 regarding old syntax usage, displays incorrect message in case formatting arguments are specified as identifiers (not string literals): ```rust #[derive(derive_more::Display)] #[display(fmt = "Stuff({}): {}", _0)] pub struct Bar(String); ``` ``` error: unknown attribute, expected `bound(...)` --> tests/compile_fail/display/legacy_fmt_syntax.rs:8:11 | 8 | #[display(fmt = "Stuff({}): {}", _0)] | ^^^ ``` In my experience, this situation happens very often in the wild. ## Solution Fix the diagnostic to consider ident formatting arguments properly.

ilslv added 14 commits November 16, 2022 15:48

WIP

984c81b

WIP

10effda

WIP

d080672

WIP

42e55a4

WIP

8adf9d5

Support unions

10d629f

Support unit structs with attribute

b280b8a

Support unit structs

6a290e1

Support Infallible-like enums with 0 variants

79fd08e

Support Infallible-like enums with 0 variants

306cd66

Remove old commented out code

a02c0c2

Merge branch 'master' into 217-display-expr

96e6430

Revert docs, as they need more in-depth rewrite

c5492c8

Add test for struct UnitTupleStruct();

2305e04

Add better structured tests for unit structs

1ba69ce

ilslv added 8 commits November 18, 2022 14:04

Single-field structs tests

88bd94b

Multi-field structs tests

caf56ca

No-variants enum tests

86a95e0

Unit-variants enum tests

8f323d0

Single-field variants enum tests

7e8f429

Single-field variants enum tests

68c0390

Multi-field variants enum tests

4677c40

Single-field struct Octal and Binary tests

ccedff6

Docs and minor refactorings

194023a

ilslv added 6 commits November 22, 2022 09:03

Add tests for generic structs

f430cd5

Add todo!() for implicit display on unit enum variant

7338351

Add todo!() for implicit display on unit enum variant

eee1131

Add error with suggestion for legacy fmt syntax

5a70ab7

Return support for implicit formatting of unit enum variant on `Displ…

1fa9236

…ay` macro

CHANGELOG

82893fe

ilslv marked this pull request as ready for review November 22, 2022 11:23

Minor corrections

f4113c5

tyranron previously approved these changes Jan 23, 2023

View reviewed changes

ModProg suggested changes Jan 23, 2023

View reviewed changes

impl/doc/display.md Outdated Show resolved Hide resolved

tyranron added 2 commits January 24, 2023 14:22

Merge branch 'master' into 217-display-expr

d864deb

Fix doc

7d312fd

tyranron dismissed their stale review via 7d312fd January 24, 2023 12:24

tyranron approved these changes Jan 24, 2023

View reviewed changes

tyranron merged commit ae8eeb9 into JelteF:master Jan 24, 2023

tyranron mentioned this pull request Feb 28, 2023

Showcase some possible bugs in Display and DebugCustom #232

Closed

ilslv mentioned this pull request Mar 2, 2023

Error on mutlifield struct or enum variant without formatting attribute in Display-like macros #243

Merged

3 tasks

tyranron mentioned this pull request Aug 9, 2023

Fix legacy diagnostic for Display-like macros #288

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign `Display` derive macro #225

Redesign `Display` derive macro #225

ilslv commented Nov 18, 2022 •

edited

Loading

ilslv commented Nov 18, 2022 •

edited

Loading

ilslv commented Nov 18, 2022

tyranron commented Nov 18, 2022 •

edited

Loading

ilslv commented Nov 18, 2022

tyranron commented Nov 18, 2022

ilslv commented Nov 18, 2022

tyranron commented Dec 12, 2022 •

edited

Loading

ModProg commented Dec 12, 2022

tyranron commented Dec 12, 2022

ModProg commented Dec 12, 2022

ilslv commented Dec 13, 2022

ModProg commented Dec 13, 2022

JelteF commented Dec 19, 2022

JelteF commented Dec 19, 2022

tyranron commented Dec 20, 2022

tyranron left a comment

tyranron commented Jan 23, 2023

JelteF commented Jan 23, 2023 •

edited

Loading

Redesign Display derive macro #225

Redesign Display derive macro #225

Conversation

ilslv commented Nov 18, 2022 • edited Loading

Synopsis

Solution

Checklist

ilslv commented Nov 18, 2022 • edited Loading

Structs

Unit

Single-field

Multi-field

Enums

Without variants (Infallible-like)

Unit variant

Single-field variant

Multi-field variant

Interactions with top-level attribute

Unions

ilslv commented Nov 18, 2022

tyranron commented Nov 18, 2022 • edited Loading

ilslv commented Nov 18, 2022

tyranron commented Nov 18, 2022

ilslv commented Nov 18, 2022

tyranron commented Dec 12, 2022 • edited Loading

ModProg commented Dec 12, 2022

tyranron commented Dec 12, 2022

ModProg commented Dec 12, 2022

ilslv commented Dec 13, 2022

ModProg commented Dec 13, 2022

JelteF commented Dec 19, 2022

JelteF commented Dec 19, 2022

tyranron commented Dec 20, 2022

tyranron left a comment

Choose a reason for hiding this comment

tyranron commented Jan 23, 2023

JelteF commented Jan 23, 2023 • edited Loading

Redesign `Display` derive macro #225

Redesign `Display` derive macro #225

ilslv commented Nov 18, 2022 •

edited

Loading

ilslv commented Nov 18, 2022 •

edited

Loading

tyranron commented Nov 18, 2022 •

edited

Loading

tyranron commented Dec 12, 2022 •

edited

Loading

JelteF commented Jan 23, 2023 •

edited

Loading