derive_more icon indicating copy to clipboard operation
derive_more copied to clipboard

`affix` is confusing with outer-level `enum` display formatting

Open ErichDonGubler opened this issue 4 years ago • 60 comments

Currently, when one uses a top-level display attribute for the Display derive on an enum, it switches to an affix mode that defines the enum's overall format and expects, at most, a single placeholder where inner variant display logic can print into:

#[derive(Display)]
#[display(fmt = "Prefix {} Suffix")] // at most 1 placeholder allowed here
enum Enum {
    #[display(fmt ="value")] // prints "Prefix value Suffix"
    Variant1,
    // ...
}

However, there's a few problems with this current behavior:

  • It's REALLY surprising to discover that fmt works rather differently than anywhere else here. It's not obvious why formatting should be different here, and there's no "obvious" signal that behavior is different. I feel qualified to say this since I implemented this behavior myself a year ago, and still was confused after reading diagnostics until I remembered what I had implemented.
  • Affix formatting defined this way is currently all-or-nothing. Once the design of the Display impl requires using a specific format some of the time,

I think that we can do better here. There are enough surprises with the current behavior that I think exploring alternatives is warranted. Here's what we stand to gain:

  • We can develop an API that operates as similarly to variant-level formatting as possible, which makes the design more coherent.
  • Because derive_more inhabits the same ecosystem as other crates that do Display deriving with rather different approaches, we could possibly try to find a top-level enum formatting option that is also consistent with those approaches (and play a bit better with other crates conceptually). In a second, I'll propose something similar to what thiserror, an error crate with facilities for deriving Display, currently does.

So, here's my proposal:

  • The current affix-oriented behavior should be moved to an affix attribute key, and (this) new behavior will be defined for fmt instead.

  • Re-define the fmt spec at the top level of an enum to be the default printing logic for an enum's variants that can be overridden by a more specific fmt rule. An overridable default has basis in some prior art; thiserror::Error treats top-level enum format specifiers as the "default" message (motivating issue, current relevant upstream master code at time of writing):

    use thiserror::Error;
    #[derive(Debug, Error)]
    #[error("error")]
    enum Enum {
        A, // displays "error"
        #[error("b")]
        B, // displays "b"
    }
    
  • Disallow affix and fmt to be used together at the same time for the time being, since that's a lot of open design space that doesn't need to be handled right this second.

To concretely translate my above suggestions into code:

use derive_more::Display;

// Using affixes at the top level of an `enum` is the same as before, except we
// use an `affix` key in the `display` attribute instead of `fmt`.
#[derive(Display)]
#[display(affix = "<{}>")]
enum UsesAffix {
    #[display(fmt = "a")]
    A, // prints "<a>"
    #[display(fmt = "b")]
    B, // prints "<b>"

	// This makes no sense and is still disallowed.
	// #[display(affix = ...)]
	// C,

    // ...
}

// A top-level `display(fmt = ..., ...)` now is:
//
// * Just like variant-level `display` attributes in that it uses formatting
//     args as usual. Only `self` is bound in arguments' context, of course.
// * Overridable by a variant-level `display` attribute.
//
// One motivating example for this is to just use `Debug` printing
// by default unless we happen to have something better.
#[derive(Display)]
#[format(fmt = "{:?}", self)]
enum DisplayDefaultsToDebug {
	A, // prints "A"
	#[display(fmt = "Display B")]
	B, // prints "Display B"
}

I'm working on a draft PR for this here. I'm speculatively doing work without getting validation in this issue first, but I don't mind. :)

ErichDonGubler avatar Sep 24 '20 22:09 ErichDonGubler

(sorry for not responding earlier to this)

@ilslv @tyranron @ModProg Given we're working towards 1.0 and we're planning to change some of the Display API in breaking ways with #217 anyway, I feel like we should consider and fix this weird edge case too. My notes on the proposal from @ErichDonGubler above are as follows:

  1. I don't really like the word affix, and I think if we do it this way we should also error very clearly if there's more than one {} part in the string. One way to solve that would be to add support for #[display(suffix("something ")) and #[display(prefix(" something more"))] instead of a shared "affix" string.
  2. The default fmt string makes sense, but given the discussion in #217 it shouldn't use fmt anymore but should be the plain string.
  3. Does the default fmt string handle interpolation at all? I would think it would be best to disallow it (at least for the first version of this behaviour).

JelteF avatar Nov 16 '22 18:11 JelteF

We could also make the specific variant format availble via some variable name so you can use it via {variant_fmt} that would make the behavior clear and also support e.g. using it twice.

ModProg avatar Nov 16 '22 18:11 ModProg

I would prefere using an affix style fmt as it is more flexible than just prefix suffix and also more readable conserning white space etc., but I'd be open on the naming.

ModProg avatar Nov 16 '22 18:11 ModProg

Would you be able to access the enum variants values via e.g. _0 - _1? Could be useful for enums very every variant has the same format

ModProg avatar Nov 16 '22 19:11 ModProg

@JelteF @ilslv

My first reaction to this was to remove affix functionality completely, just because it's so rare feature, that I've realized it exists just now, despite the fact I was improving Display macro implementation earlier 😄

However, given it a thought, I do see a lot of usefulness having it, and even see places in my codebases where it may simplify things or allow removing manual impl Display. So, I do prefer to keep it and give it a solid design.

I also do think, that derive(thiserror::Error) did a poor design choice by making top-level enum attribute acting as a default for variants:

use thiserror::Error;
#[derive(Debug, Error)]
#[error("error")]
enum Enum {
    A, // displays "error"
    #[error("b")]
    B, // displays "b"
}

The initial design choice of derive_more::Display where the top-level enum attribute provides top-level formatting seems to be more obvious and consistent with struct case:

use derive_more::Display;
#[derive(Display)]
#[display(fmt = "<{}>")]
enum UsesAffix {
    #[display(fmt = "a")]
    A, // prints "<a>"
    #[display(fmt = "b")]
    B, // prints "<b>"
}
#[derive(Display)]
#[display(fmt = "<{_0}>")]
struct Foo(u8);

While I do agree with this claim:

  • It's REALLY surprising to discover that fmt works rather differently than anywhere else here. It's not obvious why formatting should be different here, and there's no "obvious" signal that behavior is different. I feel qualified to say this since I implemented this behavior myself a year ago, and still was confused after reading diagnostics until I remembered what I had implemented.

That the form #[display(fmt = "<{}>")] doesn't look like valid Rust, so shouldn't exist.

Returning to derive(thiserror::Error) I think that a better design option would be something like this:

use derive_more::Display;
#[derive(Display)]
#[display(default("error"))]
enum Foo {
    A, // prints "error"
    #[display("b")]
    B, // prints "b"
}

Another point here, that I do see defaulting and top-level affixing being quite orthogonal. It seems fully OK (and even desired) to have both at the same time:

use derive_more::Display;
#[derive(Display)]
// Look, ma! No weird syntax anymore!
#[display("Foorror: {self}! And {}", "arbitrary_expression".to_owned())]
#[display(default("error"))]
enum Foo {
    A, // prints "Foorror: error! And arbitrary_expression"
    #[display("b")]
    B, // prints "Foorror: b! And arbitrary_expression"
}

The question is: do we want to be semantically compatible with derive(thiserror::Error)?

If yes, then for compatibility, we only need to invert attribute markers. If we don't like affix naming (I don't like it too), we may use simpler wrap, for example:

use derive_more::Display;
#[derive(Display)]
#[display(wrap("Foorror: {self}! And {}", "arbitrary_expression".to_owned()))] 
#[display("error")]
enum Foo {
    A, // prints "Foorror: error! And arbitrary_expression"
    #[display("b")]
    B, // prints "Foorror: b! And arbitrary_expression"
}

As for me, I'd like to use default despite being incompatible with derive(thiserror::Error), because it seems to me less subtle and more clever/consistent.


@ModProg

We could also make the specific variant format availble via some variable name so you can use it via {variant_fmt} that would make the behavior clear and also support e.g. using it twice.

Just using self should be totally enough and obvious for rustaceans.

I would prefere using an affix style fmt as it is more flexible than just prefix suffix and also more readable conserning white space etc., but I'd be open on the naming.

I do agree here. Prefix/suffix is just not that powerful, because we may want to repeat the inner twice, for example.

Would you be able to access the enum variants values via e.g. _0 - _1? Could be useful for enums very every variant has the same format

Yup, I'd propose it to work in the most obvious and clever way. So _0/_1 or named fields are allowed in both default and wrapping top-level attributes. If some variant doesn't have it - we always may throw an error (and even if we don't, the compiler will do that for us).

tyranron avatar Nov 17 '22 11:11 tyranron

@tyranron I agree with everything you said here.

I'd also prefer default.

One helpful error could be when you specify a non default fmt string and neither specify self nor access self in any of the format arguments. Like telling you to use default to specify the default formatting instead.

ModProg avatar Nov 17 '22 14:11 ModProg

@ModProg

One helpful error could be when you specify a non default fmt string and neither specify self nor access self in any of the format arguments. Like telling you to use default to specify the default formatting instead.

I think this would be too limiting, disallowing to express patterns like this:

#[derive(Display)]
#[display("My app errored: {_0}")]
enum MyError {
    Io(io::Error, PathBuf),
    Serde(serde_json::Error, String),
}

While I understand the possible caveats like this:

#[derive(Display)]
#[display("My app errored: {_0}")]
enum MyError {
    Io(io::Error, PathBuf),
    #[display("serde failed on input `{_1}`: {_0}")]    // won't be displayed
    Serde(serde_json::Error, String),
}

Maybe it makes sense to throw such error when both self is not used and at least one variant has custom formatting? 🤔

tyranron avatar Nov 17 '22 15:11 tyranron

@ModProg

One helpful error could be when you specify a non default fmt string and neither specify self nor access self in any of the format arguments. Like telling you to use default to specify the default formatting instead.

I think this would be too limiting, disallowing to express patterns like this:

#[derive(Display)]
#[display("My app errored: {_0}")]
enum MyError {
    Io(io::Error, PathBuf),
    Serde(serde_json::Error, String),
}

While I understand the possible caveats like this:

#[derive(Display)]
#[display("My app errored: {_0}")]
enum MyError {
    Io(io::Error, PathBuf),
    #[display("serde failed on input `{_1}`: {_0}")]    // won't be displayed
    Serde(serde_json::Error, String),
}

Maybe it makes sense to throw such error when both self is not used and at least one variant has custom formatting? thinking

But why wouldn't you use default in that case?

#[derive(Display)]
#[display(default("My app errored: {_0}"))]
enum MyError {
    Io(io::Error, PathBuf),
    Serde(serde_json::Error, String),
}

ModProg avatar Nov 17 '22 16:11 ModProg

@ModProg

But why wouldn't you use default in that case?

#[derive(Display)]
#[display(default("My app errored: {_0}"))]
enum MyError {
    Io(io::Error, PathBuf),
    Serde(serde_json::Error, String),
}

meme_firefly_wait

I cannot give here any meaningful reason, except "because I don't care and only want to specify formatting for the whole thing?". For the inexperienced user, it seems meaningful to provide default only when exceptions appear, and when there are none he wouldn't even think about default?

Is this a good motivator to prefer inverted wrap marker and derive(thiserror::Error) compatibility? 🤔

(I still do tend to use default + error on non-self + variant)

tyranron avatar Nov 17 '22 17:11 tyranron

For the inexperienced user, it seems meaningful to provide default only when exceptions appear, and when there are none he wouldn't even think about default?

That makes sense. And there is really no difference to a wrap without a reference to self and a default (as long as there are no variant specific formats).

Technically, we could also make a format the default, as long as it does not reference self. And then it becomes wrapping... and you need to specify the default for the variants not specified as default.

i.e.

// Is the default
#[derive(Display)]
#[display("My app errored: {_0}")] //  item level attribute (default)
enum MyError {
    Io(io::Error, PathBuf),
    #[display("serde failed on input `{_1}`: {_0}")]    // will be displayed instead of item level attribute
    Serde(serde_json::Error, String),
}

// References self therefor is nolonger the default
#[derive(Display)]
#[display("My app errored: {self}")] //  item level attribute (wrap)
#[display(default("{_0}"))]  // will be displayed inside of item level attribute
enum MyError {
    Io(io::Error, PathBuf),
    #[display("serde failed on input `{_1}`: {_0}")]    // will be displayed inside of item level attribute
    Serde(serde_json::Error, String),
}

Not 100% sure on that though as it feels a bit magically, but as we need to detect and replace self anyway there is no additional risk associated with this (I think™).

ModProg avatar Nov 17 '22 18:11 ModProg

@tyranron @JelteF I think my argument about discoverability applies here as well. If we support following:

// derive_more::From fits nicely into this pattern as well
#[derive(Debug, Display, Error, From)]
enum CompoundError {
    Simple,
    WithSource {
        source: Simple,
    },
    #[from(ignore)]
    WithBacktraceFromSource {
        #[error(backtrace)]
        source: Simple,
    },
    #[display(fmt = "{source}")]
    WithDifferentBacktrace {
        source: Simple,
        backtrace: Backtrace,
    },
    WithExplicitSource {
        #[error(source)]
        explicit_source: WithSource,
    },
    #[from(ignore)]
    WithBacktraceFromExplicitSource {
        #[error(backtrace, source)]
        explicit_source: WithSource,
    },
    Tuple(WithExplicitSource),
    WithoutSource(#[error(not(source))] Tuple),
}

I don't see why we shouldn't support this:

// derive_more::From fits nicely into this pattern as well
#[derive(Debug, Display, Error, From)]
#[display("Compound error: {}")]
enum CompoundError {
    Simple,
    WithSource {
        source: Simple,
    },
    #[from(ignore)]
    WithBacktraceFromSource {
        #[error(backtrace)]
        source: Simple,
    },
    #[display(fmt = "{source}")]
    WithDifferentBacktrace {
        source: Simple,
        backtrace: Backtrace,
    },
    WithExplicitSource {
        #[error(source)]
        explicit_source: WithSource,
    },
    #[from(ignore)]
    WithBacktraceFromExplicitSource {
        #[error(backtrace, source)]
        explicit_source: WithSource,
    },
    Tuple(WithExplicitSource),
    WithoutSource(#[error(not(source))] Tuple),
}

And I think that single empty placeholder {} is clearer than {self}, because it would break our "exactly the same formatting, as std::fmt" assumption.

ilslv avatar Nov 22 '22 11:11 ilslv

And I think that single empty placeholder {} is clearer than {self}, because it would break our "exactly the same formatting, as std::fmt" assumption.

But I'd argue that #[display("Compound error: {}, {self}", some_expression())] should at least be supported on top.

And is {} really "exactly the same as std::fmt", as in normal fmt {} always requires an argument to be present.

I wouldn't be opposed to have the behavior of: "If there are no additional arguments passed to display handle {} as {self}", but I'm not convinced it really matches the std::fmt behavior better.

ModProg avatar Nov 22 '22 12:11 ModProg

@ModProg

But I'd argue that #[display("Compound error: {}, {self}", some_expression())] should at least be supported on top.

Isn't #[display("{self}")] a recursive call?

ilslv avatar Nov 22 '22 13:11 ilslv

Isn't #[display("{self}")] a recursive call?

True. So maybe variant? But that would collide with a field called variant.

Is there any use for a recursive Display? Atleast in the usecase of this macro?

ModProg avatar Nov 22 '22 14:11 ModProg

@ModProg

Is there any use for a recursive Display?

No, I don't think so.

True. So maybe variant?

As I mentioned before I'm against "magical" names.

ilslv avatar Nov 23 '22 11:11 ilslv

@ilslv So I'd say self is our best choice then. As, the recursive usage doesn't make any sense.

I am not opposed to have {} work as well. But replacing self with the display of the variant is a feature allows a bit more flexibility here.

ModProg avatar Nov 23 '22 12:11 ModProg

I think there's been enough time to think on this. What is everyone's opinion at this time? I definitely think we should implement this for the 1.0 release, since changing it later would be a breaking change.

JelteF avatar Jan 26 '23 20:01 JelteF

My position hasn't really changed. I still think we should allow {self} to reference the variants formatting. I am not opposed to supporting {} for that as well, as long as we support #[display("Compound error: {}, {self}", some_expression())].

We should have a way of specifying both a default and a wrap, but I'd be fine with making a format that doesn't reference the variants formatting (either through {} or {self}) the default. (https://github.com/JelteF/derive_more/issues/142#issuecomment-1319024057)

ModProg avatar Jan 26 '23 23:01 ModProg

After some reflection, some real-world use, and considering @ModProg's comment, I'm concerned that all prior proposals actually don't go far enough in breaking from current and OP-proposed syntax, which has problems. Even the already implemented design space is too complex to rely on a single fmt tag. Some reactions to my OP and others' commentary here, two years later:

  1. I agree with @ModProg's comment: an implicit {} group at the item level (fmt = "...{}...") that switches between providing a default or providing an affix based on the lack of a positional arg is confusing. [std::fmt] would ordinarily fail to compile with {} and no args. We should take an opportunity to move away from it, if we have it.
  2. All previous placeholder suggestions ({}, {variant}, {self}) are unfortunately confusable with a "real" [std::fmt] argument.
  3. Experience report: anyhow`'s prior art has generally been confusing both to myself and to teammates on projects, even over time after having written the code ourselves. Examining the current affix proposal, its behavior changes pretty dramatically based on the content of the formatting args. In other words, I'm swayed by @tyranron's arguments.
  4. Using derive_more::Display as a dependency tends to initially be a personal choice, I've noticed. When derive_more feels approachable, it's a lot easier to adopt as an extra dep. Optimizing the syntax for clarity in reading seems like the best way to avoid ecosystem friction overall.

Will post another comment here with details about a solution I think will solve the problems we're debating about here. 🙂

ErichDonGubler avatar Jan 27 '23 19:01 ErichDonGubler

Designing from scratch

Let's enumerate the behaviors we have expressed that we want:

  1. Item level: at most one of each:
    1. Default variant formatting (currently exposed via fmt = ...)
    2. Affix formatting (currently exposed as {})
  2. Variant level: at most one of any:
    1. Non-overriding (currently exposed as fmt = ...)
    2. Overriding (not exposed anywhere currently, but desirable)

Display's implementation from before I added affix functionality only had (1i) and (2ii). There was only a single behavior per level of fmt = ... prior to that PR. The addition of item-level affix backwards-compatibly (originally because @JelteF wanted backwards compatibility) has caused some confusion. Two years later, I think it's safe to say that our current design is holding us back more than what backwards compatibility might gain us.

I think the best way to offer the above behaviors going forward would involve new syntax that fulfills these requirements:

  1. Use an affix placeholder that:
    1. Clearly does not compile with Rust's vanilla fmt syntax, signaling that it's an affordance of this macro, and not anything from fmt (this resolving concerns from @ilslv about "magical" names)
    2. Is obviously not associated with any positional formatting arg (resolving concerns from @ModProg1 and myself
    3. Communicates that it will delegate formatting to variants
  2. Make it clear which item-level behavior is being used
  3. Make it clear which variant-level behavior is being used
  4. Avoid problems in a migration from old syntax (i.e., everybody uses fmt = … right now, let's not risk changing its meaning and let old code compile)
  5. Allow variant-level overriding of item-level affix, per my OP.
  6. Makes it easy to copy/paste from existing {write,format,println,…}!(…) by using parentheses around format specs, instead of the equals syntax we've historically used. This one is optional but I thought it'd be a nice-to-have.

Each of the current proposals fails to fulfill these in one way or another. Let me prove it to you with a table (and, *ahem*, some new contenders):

Proposals\Requirements 1i: affix placeholder clearly not part of std::fmt syntax 1ii: affix placeholder clearly not a positional arg 1iii: affix placeholder clearly delegates to variants 2: item-level behavior is clear 3: variant-level behavior is clear 4: avoids migration problems 5: easy to adjust copy/paste vanilla std::fmt macro usage
@erichdongubler's OP proposal:
  • Item:
    • Default: fmt = "…", … (no unqualified positional, a la `anyhow`)
    • Affix: affix = "…{}…", … (single unqualified positional)
  • Variant:
    • Overriding: does not exist
    • Non-overriding: fmt = …
@ModProg's proposal:
  • Item:
    • Default: fmt = "…", …
    • Affix: fmt = "…{self}…", …
  • Variant:
    • Overriding: does not exist
    • Non-overriding: fmt = "…", …
⚠️ (self's clarity is up for debate)
@erichdongubler's new proposal, Equals Edition™ (spoiler alert!):
  • Item:
    • Default: default = "…", …
    • Affix: item = "…{@variant}…", …
  • Variant:
    • Overriding: override = "…", …
    • Non-overriding: variant = "…", …
@erichdongubler's new proposal, Parens Edition™ (spoiler alert!):
  • Item:
    • Default: default("…", …)
    • Affix: item("…{@variant}…", …)
  • Variant:
    • Overriding: override("…", …)
    • Non-overriding: variant("…", …)

You may have noticed that I snuck in a couple of new solutions at the end! Yes, I believe that we can fulfill all of the above requirements. 🤪 I propose we move to have distinct tags for each of the numerous flavors of format spec that make behavior unambiguous in all cases, and therefore easier to use overall.[^1] Let me give an applied example, rewriting last example in the original OP in the Parens Edition™ syntax:

#[derive(Display)]
#[display(item("<{@variant}>"))] // 1ii: affix formatting
#[display(default("unknown"))] // 1i: default variant formatting
enum UsesAffix {
    #[display(variant("a"))] // 2i: non-overriding formatting
    A, // prints "<a>"
    B, // prints "<unknown>"
	#[display(override("{c}"))] // 2ii: overriding formatting
	c, // prints "{C}"
}

...or, with the Equals Edition™:

#[derive(Display)]
#[display(item = "<{@variant}>")] // 1ii: affix formatting
#[display(default = "unknown")] // 1i: default variant formatting
enum UsesAffix {
    #[display(variant = "a")] // 2i: non-overriding formatting
    A, // prints "<a>"
    B, // prints "<unknown>"
	#[display(override = "{c}")] // 2ii: overriding formatting
	c, // prints "{C}"
}

Cases needing errors or warnings become much more clear, too; using Parens Edition™ notation:

#[derive(Display)]
#[display(item("\\{@variant}/"))]
enum NoDefaultProvided {
    #[display(variant("1"))]
    One, // prints `\1/`
	Two, // error: no `default` provided by item level, no `variant` specified here
}
#[derive(Display)]
enum NoDefault {
    #[display(variant("Left"))]
    Left, // prints `Left`
	Right, // error: no `default` provided by item level, no `variant` specified here
}
#[derive(Display)]
#[display(item("oops, I meant to write a default"))] // maybe throw an error: no `@variant` formatting arg was used
enum ShouldaDoneDefault {
    #[display(variant("Red"))] // otherwise, warning: this is ignored because `item` has no `@variant` placeholder, which probably wasn't intended! Maybe suggest switching `item` to `default`, or using the placeholder.
    Red, // prints `oops, I meant to write a default`
}
#[derive(Display)]
enum PersonKind {
	#[display(variant("dog"))]
    Dog, // prints `dog`
    #[display(override("cat"))] // warning: trying to `override` an `item` that doesn't exist
    Cat,
}

EDIT: Added the PersonKind/override warning diagnostic case.

[^1]: Actual identifiers associated with the tags proposed here are subject to bikeshedding.

ErichDonGubler avatar Jan 27 '23 20:01 ErichDonGubler

I'd prefer the paren syntax. I agree that something like @variant cannot be confused with meaning something else (we could also use something like just @ not sure how much sense that makes).

Some proposed changes to paren:

  1. @ instead of @variant, (as long as it is the only @ reference, I know I break this my self with 2.)
  2. add @name for default, variant and override (I don't think it makes much sense for item, and it might be ambiguous for default and item)
  3. Make remove inner "call" for variant and item
#[derive(Display)]
#[display("affix: {@}")] // errors if it doesnt contain an `@`
#[display(default("the {@name} {_0}"))] // could error if every variant has a formatting
enum Enum {
  Default(usize),
  #[display("{@name} formatting")]
  Custom.
  #[display(override("Likely only very seldom used")] // could error if no "affix" exists
  Override
}

We also need to decide how this all plays together with the default display for Unit and Newtype and with the proposed rename_all #216.

ModProg avatar Jan 27 '23 23:01 ModProg

Reexamining my proposal(s) above, I don't think I've given @tyranron enough credit; their solution was, at least, a partial inspiration for the Parens Edition™ version of my above proposal. Kudos!

ErichDonGubler avatar Jan 27 '23 23:01 ErichDonGubler

@ModProg: RE: your suggestions:

  • RE: (1) and (3): I would still prefer @variant and variant(...), because it's more clear that they're related (which I consider necessary for requirement 2). Removing the explicit tags optimizes for writing, but causes clarity to suffer, IMO. Can you explain why you think those forms might be better?
  • (2) seems like a separate feature request, which I think would best be served by being a separate issue (CC @JelteF). It's interesting that this sort of design space opens up with a sigil like @, though!

ErichDonGubler avatar Jan 27 '23 23:01 ErichDonGubler

Can you explain why you think those forms might be better?

Just personal taste, as I felt like those are the ones that you most likely want. Similar to how {} is just a shorthand for {idx} in rust's fmt.

  • (2) seems like a separate feature request, which I think would best be served by being a separate issue (CC @JelteF). It's interesting that this sort of design space opens up with a sigil like @, though!

yeah, I just wanted to mention it, so we can build the rest in a way that doesn't prohibit such features.

ModProg avatar Jan 28 '23 09:01 ModProg

Just wanted to add my use case for consideration since it currently doesn't seem to be supported.

I have a lot of simple enums that I serialize to JSON strings. I would like to derive Display implementations directly from the Serialize implementation so that the seride(rename_all = ...) attribute that I use is preserved when writing with Display. I was hoping to be able to write something like this, usingserde_plain::to_string to generate plaintext names:

#[derive(Deserialize, Serialize, Eq, PartialEq, PartialOrd, Ord, Clone, Copy, Debug, Display)]
#[serde(rename_all = "lowercase")]
#[display(fmt = "{}", "serde_plain::to_string(&self).unwrap()")]
pub enum TestOs {
    Alpine,
    RockyLinux,
    Debian,
    Ubuntu,
    Macos,
    Windows,
}

However the display(fmt = "...", args...) form is not supported at the top-level of an enum with variants. Currently I see no way to use derive_more for this and so I'm using the derive_display_from_serialize! function macro from serde_plain. I would prefer a more idiomatic derive macro for this purpose rather than a function macro, but haven't found any good alternatives so far.

erratic-pattern avatar Feb 23 '23 20:02 erratic-pattern

I agree with @ModProg that I think we should not require (but maybe allow) the variant(...) "call". The variant call seems quite verbose for what seems to be the most common usecase. Especially since I would not want to require a variant or override call when there's no item call at the top.

The item call I feel much less strong about removing. But I do think it's important to throw an error if there's no @variant in the item call, and probably hint to the existince of the default call. If that's what we'll do I don't see a strong need for having an item call, as the pressence of @variant would already show that it's not the default. But like I said I'm also fine with having an item call if others disagree.

JelteF avatar Feb 27 '23 21:02 JelteF

Also I like the @ sigil for this. But I think we should always have a name after it for clarity, so no bare @ but @variant.

@name seems like a separate feature indeed. And I would like to keep it's discussion out of this issue, because this is one of the issues that's currently blocking 1.0. And adding extra sigils wouldn't be breaking changes.

JelteF avatar Feb 27 '23 21:02 JelteF

@JelteF @ilslv @ErichDonGubler @ModProg

First of all, I do like an idea of having override very much, but see this more an addition, rather than a requirement, and so, can be implemented post 1.0, as is not breaking in any way.

3. Make remove inner "call" for variant and item

Removing the explicit tags optimizes for writing, but causes clarity to suffer, IMO. Can you explain why you think those forms might be better?

I think we should not require (but maybe allow) the variant(...) "call". The variant call seems quite verbose for what seems to be the most common usecase. Especially since I would not want to require a variant or override call when there's no item call at the top.

I agree with removing them too. Having too much of specifiers makes discoverability considerably worse (point often protected by @ilslv). It's hard to remember them, and it's unlikely to write something meaningful without advising with documentation. It should be easy to write stuff out-of-the-box in the intuitive way.

Like I want to format a variant or a struct, I just write #[display("my format {field}")] like I do in a plain code format!("my format {field}"), without thinking about affixes, overriding, defaulting as long as I don't need it. Once I need some decoration on top of enum variants, it's natural for me to try top-level attribute for that #[display("wrapped <{self}>")]. Then, realizing that I don't want to have this for some variant, it would be quite obvious to specify #[display(override("no decoration"))]. All this follows natural thinking patterns and may be reasonably discovered without diving into docs, and so, also is quite easy to be remembered.

Also I like the @ sigil for this. But I think we should always have a name after it for clarity, so no bare @ but @variant.

This is something I want to debate quite much about.

I don't really like the idea to introduce something new to std::fmt syntax, because:

  • We don't have control over it. It easily may be changed in future in the way conflicting with our additions semantically, causing us not only to do redesign, but also causing existing library users' code to become semantically incompatible.
  • It's poor regarding discoverability. There is no way a library user can intuitively come up to use {@variant}. It's too artificial.
  • Regarding @name and other possible @ sigil extensions, I do think it's quite out of our scope to invent new formatting syntax and semantics in this crate.

I still do think that the self is the way to go here. Let's have a closer look at it:

  • It's the most natural and intuitive, when it comes to decoration. That's the first thing one could think of to try when he wants decoration, without reading docs.
  • Using pure self in attributes at the moment introduces a recursive call, thus not really used anywhere, and won't be. Using {@variant} still won't free us and won't allow using {self}, because a recursive call won't go anywhere. Regarding this, it would be okay to say that {self} is not reserved, due to being impractical if handled naively. So, working with it more clever, only opens our hands more, not ties. So having something like #[display("decorate <{self}>")] top-lever attribute either works as expected, or doesn't work at all, and there are no uncertain in-between situations.

But I do think it's important to throw an error if there's no @variant in the item call, and probably hint to the existince of the default call.

I don't think that's worth it. From the user's perspective, I don't see much motivation why I should write something like this (referring to @kallisti-dev example):

#[serde(rename_all = "lowercase")]
#[display(default("{}", serde_plain::to_string(&self).unwrap()))]
pub enum TestOs {

rather than just this:

#[serde(rename_all = "lowercase")]
#[display("{}", serde_plain::to_string(&self).unwrap())]
pub enum TestOs {

It's quite unlikely I would think in this situation about default, and introducing it seems strange for a reader.

Even more, potentially we can support not only pure {self}, but self in arbitrary expressions like #[display("<{}>", MySuperFormat(self))], without introducing a recursive call just by doing something like this in macro expansion:

fn fmt(&self, f: fmt::Writer<'_>) -> fmt::Result {
    let __derive_more_self = format_args!(/* non-decorated formatted stuff */);
    write!(f, "<{}>", MySuperFormat(__derive_more_self))
}

The only real issue here with self I see, that when it comes to arbitrary expressions, there is no way to distinguish between self as "pure &self itself" and self as "not-yet-decorated formatted stuff", because substituting self in an expression like this #[display("{}", serde_plain::to_string(&self).unwrap())] would give not what we expect. This returns us back to the @ sigil question, proving that @variant and self semantics are not quite the same in edge cases.

Fine then... Rather than introducing a new @ sigil formatting syntax, we better reserve a concrete identifier for that, like __self (or __variant, or __preformatted, or __formatted, or __fmt, or __output, etc). This preserves us from extending fmt grammar (and consequences of doing so), while allows the desired semantics separation with self. However, still obscures the feature, making it undiscoverable.

Let's sketch the final design I propose in examples:

#[derive(Display)]
#[display("what {self}?"]
struct AnyStruct1;  // complile error: recursive call

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

#[derive(Debug, Display)]
#[display(default("what {self:?}?")]
struct AnyStruct3;  // compile error: `default` is not allowed here, remove it

#[derive(Debug, Display)]
#[display(override("what {self:?}?")]
struct AnyStruct4;  // compile error: `override` is not allowed here, remove it

#[derive(Display)]
#[display("what {__self}?"]
struct AnyStruct5;  // compile error: reserved identifier `__self` is not allowed here

#[derive(Display)]
enum Enum1 {
    Foo,  // prints: Foo
    #[display("Baz")]
    Bar,  // prints: Baz
}

#[derive(Display)]
#[display(default("default"))]
enum Enum2 {
    Foo,  // prints: default
    #[display("Baz")]
    Bar,  // prints: Baz
}

#[derive(Display)]
#[display("decorate <{__self}>")]
#[display(default("default"))]
enum Enum3 {
    Foo,  // prints: decorate<default>
    #[display("Baz")]
    Bar,  // prints: decorate<Baz>
}

#[derive(Display)]
#[display("decorate <{__self}>")]
enum Enum4 {
    Foo,  // prints: decorate<Foo>
    #[display(override("Baz"))]
    Bar,  // prints: Baz
}

#[derive(Display)]
#[display("whatever")]
enum Enum5 {
    Foo,  // prints: whatever
    Bar,  // prints: whatever
}

#[derive(Display)]
#[display("whatever {__self}")]
enum Enum6 {
    Foo,  // prints: whatever Foo
    Bar,  // prints: whatever Bar
}

#[derive(Display)]
#[display("whatever {}", append_bang(__self))]
enum Enum6 {
    Foo,  // prints: whatever Foo!
    Bar,  // prints: whatever Bar!
}

#[derive(Display)]
#[display("whatever {self}")]
enum Enum7 {  // complile error: recursive call
    Foo,
    Bar,
}

#[derive(Debug, Display)]
#[display("whatever {self:?}")]
enum Enum8 {
    Foo,  // prints: whatever Enum8::Foo
    Bar,  // prints: whatever Enum8::Bar
}

#[derive(Display, Serialize)]
#[display("whatever {}", serde_json::to_string(self).unwrap())]
#[serde(rename_all = "lowercase")]
enum Enum9 {
    Foo,  // prints: whatever "foo"
    Bar,  // prints: whatever "bar"
}

#[derive(Display)]
#[display("whatever {}", serde_json::to_string(&__self).unwrap())]
enum Enum10 {
    Foo,  // prints: whatever "Foo"
    #[display("Baz")]
    Bar,  // prints: whatever "Baz"
}

This feels quite intuitive and natural for me, except the __self naming of the reserved identifier. I don't have better naming at the moment, but it should be much more intuitive and clearer, and even if it can collide with some existing identifiers, we always may disambiguate it like this:

#[derive(Display)]
#[display("field: {}, full: {__self}", self.__self)]
enum Enum10 {
    #[display("some foo")]
    Foo { __self: u8 },  // prints: field: <u8>, full: some foo
    #[display("bar {__self}")]
    Bar { __self: String },  // prints: field: <String>, full: bar <String>
}

tyranron avatar Feb 28 '23 14:02 tyranron

This feels quite intuitive and natural for me, except the __self naming of the reserved identifier.

I agree, that doesn't really feel amazing, but it would be inline with e.g _0 for tuple structs, (though I think we can reduce to a single underscore).

and even if it can collide with some existing identifiers, we always may disambiguate it like this:

Another example for a (for whatever reason named constant)

#[display("{}", ::module::__self]

I agree that we should keep the plausible scope of this derive macro in mind. If the expression gets too complex, it is probably a good idea to implement Display manually.

Therefor, I would also be fine with saying {self} inside the format string is the variant's Display, and self inside the arguments is the actual self. Though, that either removes the possibility to access the variant's format or would create both _self and self which is probably undesirable.

I don't really like the idea to introduce something new to std::fmt syntax, because:

* We don't have control over it. It easily may be changed in future in the way conflicting with our additions semantically, causing us not only to do redesign, but also causing existing library users' code to become semantically incompatible.

I find that highly unlikely, (though it wouldn't be a breaking change for std, as they currently only allow identifiers, so it makes sense to be rather safe than sorry here).

ModProg avatar Feb 28 '23 15:02 ModProg

I agree here with many of the points that @tyranron makes.

The default(...) specifier feels odd in the case where you're only specifying a top-level attribute. I think in cases where there are per-variant attributes, it makes more sense, such as in:

#[derive(Display)]
#[display("decorate <{__self}>")]
#[display(default("default"))]
enum Enum3 {
    Foo,  // prints: decorate<default>
    #[display("Baz")]
    Bar,  // prints: decorate<Baz>
}

Also +1 to keeping the syntax as close as possible to existing formatting conventions, rather than introducing new symbols.

As far as default vs variant naming, I'm not sure which is more readable, as I can see arguments for each.

@ModProg :

I agree that we should keep the plausible scope of this derive macro in mind. If the expression gets too complex, it is probably a good idea to implement Display manually.

I admit my particular use case is not much different from a manual Display implementation, only saving a few lines of code really, and so may not be worth consideration here if it complicates the formatting syntax.

The main motivation in my case for wanting to derive Display in this way is that I have many of these enums which follow the same pattern, and I'd like to reduce the boilerplate as much as possible to a simple line of code that says "invoke this expression on my enum to get the Display representation".

I think in code that works with HTTP APIs this is probably a common scenario, where you already have serde attributes that describe a textual representation and you'd like to have a Display that's consistent with that representation. I would say with the ubiquity of serde this is probably an integration worth considering in itself (behind a feature gate).

if there's a unifying and elegant design here, I say go for it, but otherwise I don't think it's worth it if it introduces inconsistencies or confusing semantics that clash with existing usage patterns. The examples proposed by @tyranron seem to be the most consistent/clear so far, with the exception of the _self problem.

It might be worth considering having a completely separate form for this type of derive. In fact, I really don't need a format string at all to express what I'm after here. Something like this would work just as well:

#[display(from(serde_plain::to_string(&self).unwrap()))]

This would have the benefit of not requiring special handling of _self in argument contexts, as I think it would very confusing for the self identifier to have different meanings in different contexts.

erratic-pattern avatar Feb 28 '23 18:02 erratic-pattern