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

Redesign Display derive macro #225

Merged
merged 44 commits into from
Jan 24, 2023
Merged

Conversation

ilslv
Copy link
Contributor

@ilslv ilslv commented Nov 18, 2022

Resolves #217

Synopsis

Solution

  1. 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
Copy link
Contributor Author

ilslv commented Nov 18, 2022

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

ilslv commented Nov 18, 2022

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

@tyranron
Copy link
Collaborator

tyranron commented Nov 18, 2022

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

ilslv commented Nov 18, 2022

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

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

ilslv commented Nov 18, 2022

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

@ilslv ilslv marked this pull request as ready for review November 22, 2022 11:23
@tyranron
Copy link
Collaborator

tyranron commented Dec 12, 2022

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

  1. 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.

  2. 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
Copy link
Contributor

ModProg commented Dec 12, 2022

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

@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
Copy link
Contributor

ModProg commented Dec 12, 2022

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

@ilslv
Copy link
Contributor Author

ilslv commented Dec 13, 2022

@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
Copy link
Contributor

ModProg commented Dec 13, 2022

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

JelteF commented Dec 19, 2022

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

JelteF commented Dec 19, 2022

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

@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
tyranron previously approved these changes Jan 23, 2023
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

The CI issue is fixed in #231.

@JelteF
Copy link
Owner

JelteF commented Jan 23, 2023

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 Outdated Show resolved Hide resolved
@tyranron tyranron merged commit ae8eeb9 into JelteF:master Jan 24, 2023
JelteF pushed a commit that referenced this pull request Aug 9, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using non string expressions
4 participants