affix is confusing with outer-level enum display formatting

[ErichDonGubler]

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. :)

[JelteF]

(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 Consider using non string expressions #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).

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

[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]

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

[tyranron]

@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).

[ModProg]

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

[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? 🤔

[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? 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),
}

[tyranron]

@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),
}

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)

[ModProg]

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™).

[ilslv]

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

[ModProg]

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.

[ilslv]

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

[ModProg]

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?

[ilslv]

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

[ModProg]

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

[JelteF]

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.

[ModProg]

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. (#142 (comment))

[ErichDonGubler]

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]

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 @ModProg¹ 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.¹ 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.

Footnotes

1. Actual identifiers associated with the tags proposed here are subject to bikeshedding. ↩

[ModProg]

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.

[ErichDonGubler]

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]

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

[ModProg]

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.

[erratic-pattern]

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.

[JelteF]

@tyranron

Not sure that I've understood you well. Could you demonstrate the idea with few examples?

Basically this would error:

#[derive(Display)]
#[display("default")]
enum Enum3 {
    Foo,
    #[display("Baz")]
    Bar, 
}

But this would be allowed:

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

And this would also be allowed:

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

This last one could then be combined with a non default call:

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

[JelteF]

I'm fine with _inner btw

[ilslv]

@tyranron @JelteF @ErichDonGubler @ModProg

I still feel like we are trying to stuff too much into a single macro. This functionality doesn't bring anything new to the table, so basically only a syntactic sugar to reduce boilerplate. I really like how non-sugary the Rust is and I think this is part of it's idiomatic approach.

Is affix functionality useful in terms of reducing a lot of boilerplate? I would argue that yes. Especially for error types:

// mod io
#[display("IO error: {}")]
enum Error {
    #[display("entity not found")]
    NotFound,
    #[display("permission denied")]
    PermissionDenied,
    // and 38 more variants ...
}

Is default or override useful in terms of reducing a lot of boilerplate? I would argue that no. I don't see any clear real-world use case for it where you wouldn't be better off with just manually specifying format for each variant. Also adding support for them isn't a breaking change, so we can reconsider this decision in the future.

---

About placeholder I'm still convinced that simple {} is the way to go.

{self} is no-go to me, because it can mean different things in different fmt derives:

#[derive(Display, Pointer)]
#[display("addr: {self}")]
#[pointer("{self}")]
enum Pointer<T> {
    #[display("nullptr")]
    Null,
    NonNull(*const T)
}

In the example above #[display("addr: {self}")] means affix formatting, while #[pointer("{self}")] is intended to simply use Display implementation. And imagine this example without #[display("addr: {self}")], very confusing to me.

All special names are frustrating friction point to me as I struggle to remember them, so _inner/__self/@variant are no-go for me.

The only argument against simple {} was about using it somewhere in between (very strange use-case imho), but format syntax covers it flawlessly:

#[derive(Display)]
#[display("{before} | {} | {after}", before = any_expr, after = any_expr)]
struct Affix {
    // ...
}

So I'm still arguing for solution proposed in #142 (comment)

[ilslv]

@ModProg I don't understand what you are trying to argue about

[ModProg]

@ModProg I don't understand what you are trying to argue about

sorry i think I just misunderstood your comment.

But yeah, I guess if we remove support for positional arguments we can fo it the way you proposed. Or we only make "{0}" the variant format. and allow additional positional arguments, though I'm a bit unsure that that it might not create confusion either way.

[JelteF]

@ilslv

About placeholder I'm still convinced that simple {} is the way to go.
All special names are frustrating friction point to me as I struggle to remember them, so _inner/__self/@variant are no-go for me.

While I agree that remembering the special names is a friction point when writing the derive. I think optimizing for readability is probably better in this case. With a special name it's instantly clear what it is that is being inserted there. It also gives you something to google for if you are not familiar with the feature

For instance someone not familiar with the feature might be confused by the this derive

#[derive(Display)]
#[display("My error: {}")]
enum Error {
    #[display("not found error: {source}")]
    NotFound{source: HTTPError}
    OtherHTTPError(HTTPError),
    OtherDNSError(DNSError),
}

I think that syntax can reasonably be interpreted as:

#[derive(Display)]
#[display(default("My error: {_0}"))]
enum Error {
    #[display("not found error: {source}")]
    NotFound{source: HTTPError}
    OtherHTTPError(HTTPError),
    OtherDNSError(DNSError),
}

Instead of its actual meaning:

#[derive(Display)]
#[display("My error: {_inner}")]
enum Error {
    #[display("not found error: {source}")]
    NotFound{source: HTTPError},
    OtherHTTPError(HTTPError),
    OtherDNSError(DNSError),
}

Having the _inner special name makes understanding the derive much easier IMHO. And I think it's probably worth the added cost when writing the derive.

I agree that removing support for positional args would solve the confusion of the syntax for the parser, but I don't think it would help this human confusion.

[JelteF]

@ilslv @tyranron what do you think? I feel like this is probably the final change (together with #248) that we want to do before creating 1.0.0

[ilslv]

@JelteF speaking of 1.0.0, I'm not really comfortable releasing so many changes to so many people without the ability to release breaking changes that easily. I would love to see 1.0 as the last breaking release for a substantial amount of time. And as release candidates mechanism isn't that useful in rust ecosystem, I would like to release 0.100 and maybe even couple of more breaking versions before committing to the 1.0.

[JelteF]

Regarding 1.0.0 vs 0.100.0 The main thing I would want to do is release 1.0.0 soon-ish, because I think derive_more is in a stable state. The first 0.99 release was 3 years ago. My plan at that time was to release 1.0.0 soon after, but I never did. Now we have a ton of breaking changes, which make the syntax more consistent and bring it in line with current Rust versions. After these changes I think we just shouldn't break things for a while again.

All this new code also has a lot of tests (a lot of those are thanks to you). So I'm pretty confident that more breaking changes are not necessary for a while. And if I'm wrong, I don't think it's a big deal to release 2.0 quickly after 1.0 if the amount of breakage is tiny and we break for a good reason (i.e. some crazy bug, not some cosmetic cleanup).

[ilslv]

For me releasing 2.0 soon after 1.0 for such popular and foundation library is a big deal. Ecosystem is still fragmented after the release of syn 2.0 and I think it will be like that for a long time.

[JelteF]

There's no difference in amount of split between doing 1.0 and then 2.0 or doing 0.100 and then doing 1.0. You get this split because there's two major versions. We want to release a 1.0, so by releasing 1.0 directly in the happy path there's only 1 major version, and in the bad case if there's some crazy bug then we have two major versions. But if we start with 0.100 and we still want to release 1.0, then even if we don't need to do any more breaking changes we need to release a new major version (or stay at a 0.x release again for 3 years). So I say, let's just bite the bullet and release 1.0 once we have no obvious breaking changes that we want to do.

[ilslv]

There's no difference in amount of split between doing 1.0 and then 2.0 or doing 0.100 and then doing 1.0

I don't agree with that. People have certain expectations about stability of 1.0. By releasing 0.100 I expect us getting bug reports/ideas from early adopters, that may be very helpful. And after 1.0 I expect slow migration of the rest of the ecosystem.

But if we start with 0.100 and we still want to release 1.0, then even if we don't need to do any more breaking changes we need to release a new major version

I don't see any problems with that.

[JelteF]

There's no difference in amount of split between doing 1.0 and then 2.0 or doing 0.100 and then doing 1.0

I don't agree with that. People have certain expectations about stability of 1.0. By releasing 0.100 I expect us getting bug reports/ideas from early adopters, that may be very helpful. And after 1.0 I expect slow migration of the rest of the ecosystem.

People already have certain expectations about derive_more. It's been stable for 3 years. There's 110k repos on github using derive_more. Suddenly starting to do multiple breaking releases in a row is something I think we should do. And thus I don't think we should plan our versioning to that. Our goal with the next release should be: This is a stable version that we don't plan on changing. It does have new features. But they are tested well. Any bugs that don't require breaking changes we will fix soon. Once that require breakage, will probably have to wait for another 3 years to be fixed.

And in the extremely unlikely case where something is completely broken, and requires a new major version to fix. Then we'll likely know within a few hours/days, which makes the ecosystem split problem pretty much a non-problem.

even if we don't need to do any more breaking changes we need to release a new major version

I don't see any problems with that.

Even if there's no actual breaking changes, cargo will still consider it incompatible versions. Thus doing that will split the ecosystem for no good reason.

[glandium]

You /could/ release a 0.100, wait a few weeks/months to see if there are breakages, and if there aren't any, then release a 1.0 and a 0.100.n that depends on 1.0 and reexports everything from it. Not saying you must, but that would be one way to deal with the situation without having to rush a 2.0 to fix problems that require a breaking change while avoiding an ecosystem split.

(I'll add that a 0.99.18 with syn2 changes only would be very welcome too)

[JelteF]

@ilslv

And as release candidates mechanism isn't that useful in rust ecosystem

Why did you say this. After thinking a bit more I agree that it would be good to have some more testing before actually releasing 1.0.0. But I think the nicest way to do that is to release a 1.0.0-beta.1 version.I think beta.1 signals even better that this is a release on which we want some feedback than a 0.100 would. It also shows up nicely in crates.io, with the version being yellow and defaulting to showing the readme for the last actual release (example: https://crates.io/crates/pgrx/versions)

And I think we'll get enough feedback from people since quite a few people seem to want a release for the syn 2.0 support. What do you think?

[JelteF]

@ilslv Also, what do you think of my previous argument for the _inner special name? Apart from the publishing situation, I think we should make a decision on the original issue from this thread:

While I agree that remembering the special names is a friction point when writing the derive. I think optimizing for readability is probably better in this case. With a special name it's instantly clear what it is that is being inserted there. It also gives you something to google for if you are not familiar with the feature

For instance someone not familiar with the feature might be confused by the this derive

#[derive(Display)]
#[display("My error: {}")]
enum Error {
    #[display("not found error: {source}")]
    NotFound{source: HTTPError}
    OtherHTTPError(HTTPError),
    OtherDNSError(DNSError),
}

I think that syntax can reasonably be interpreted as:

#[derive(Display)]
#[display(default("My error: {_0}"))]
enum Error {
    #[display("not found error: {source}")]
    NotFound{source: HTTPError}
    OtherHTTPError(HTTPError),
    OtherDNSError(DNSError),
}

Instead of its actual meaning:

#[derive(Display)]
#[display("My error: {_inner}")]
enum Error {
    #[display("not found error: {source}")]
    NotFound{source: HTTPError},
    OtherHTTPError(HTTPError),
    OtherDNSError(DNSError),
}

[ilslv]

@JelteF unfortunately very few people upgrade to alpha/beta versions. They aren't suggested on cargo update and aren't integrated with other tooling. People to upgrade are usually somehow involved and very invested in the project itself, and I think most people are just getting this crate as a transitive dependency and have no idea of its existence. The project also wasn't really active in a past few years, so most of the people brushed it alway as a "stable" version that isn't subject to any major upgrades.

This problem isn't really unique to this crate, I remember even tokio having very low percentage of beta consumers on 1.0 stabilization.

[JelteF]

@ilslv I think a feedback from a few involved/invested users is exactly what we want for such an intermediary release. And mostly for applications, not libraries. Otherwise we get this ecosystem split. So to me what you're describing sounds like a good thing, instead of a problem. I think even with ~20 people trying it out in their own projects we should have enough feedback to be relatively certain that nothing major is broken. And anyone searching for derive_more + syn2 will find that you can install a beta.

So unless you have major concerns, I think I'll publish a beta release later this week. IT seems pretty low risk, with possibly high reward. Worst case no-one uses it and we're at the same stage as where we are now. Best case we get valuable feedback. Intermediary case, people that really want syn2 support have a way out without pulling from git.

[ilslv]

@JelteF hm, I think that this is no-lose situation. In case we get valuable feedback — great. In case there is no feedback — we can still push breaking changes.

[JelteF]

I published v1.0.0-beta.1 now 🚀

[JelteF]

@ilslv @tyranron @MegaBluejay This is the last major item that's still blocking the 1.0.0 release. Let's make a decision here and implement it.

[tyranron]

@JelteF okay, let's discuss what's left...

After re-reading and rethinking the whole thread once again, it seems to me that the _inner solution is lesser evil here. So, I'd vote for the one. If somebody will come with a better one - we always can release 2.x with it.

@tyranron

Not sure that I've understood you well. Could you demonstrate the idea with few examples?

Basically this would error:

#[derive(Display)]
#[display("default")]
enum Enum3 {
    Foo,
    #[display("Baz")]
    Bar, 
}

But this would be allowed:

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

And this would also be allowed:

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

This last one could then be combined with a non default call:

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

Regarding this, I also feel fine.

If no major opposing would be expressed in next weeks, I'll implement this. At least, before really going 1.0.0, we could have it as another beta and give it a try.

[ModProg]

One addition concerning the edgecase of having a field called _inner.

Probably unlikely, but could result in possible confusion.

I think using default would be the workaround to allow having it, but it might be confusing to read.

This probably should error

#[derive(Display)]
#[display("{_inner}")]
enum Enum {
  Variant{ _inner: u8 }
}

and recommend

#[derive(Display)]
#[display(default ("{_inner}"))]
enum Enum {
  Variant{ _inner: u8 }
}

but the actual confusing to look at usage would be

#[derive(Display)]
#[display("{_inner}")]
#[display(default ("{_inner}"))]
enum Enum {
  Variant{ _inner: u8 }
}

though this will happen for every non keyword name and is therefore an unavoidable compromise.

and while it means that noone will be able to use a const named _inner in the display derive, this is a highly unconventional name and IMO acceptable.

const _inner: u8 = 5;
#[derive(Display)]
#[display("{_inner}")]
enum Enum {
  Variant
}

Will yield Variant instead of 5.

[JelteF]

To be clear: +1 on the _inner solution being the lesser evil. Let's get this implemented 🚀

This probably should error

#[derive(Display)]
#[display("{_inner}")]
enum Enum {
  Variant{ _inner: u8 }
}

and recommend

#[derive(Display)]
#[display(default ("{_inner}"))]
enum Enum {
  Variant{ _inner: u8 }
}

agreed

but the actual confusing to look at usage would be

#[derive(Display)]
#[display("{_inner}")]
#[display(default ("{_inner}"))]
enum Enum {
  Variant{ _inner: u8 }
}

though this will happen for every non keyword name and is therefore an unavoidable compromise.

yeah seems fine to me. Especially since underscore prefix usually means it's a non public field, so renaming it shouldn't be big deal normally.

and while it means that noone will be able to use a const named _inner in the display derive, this is a highly unconventional name and IMO acceptable.

const _inner: u8 = 5;
#[derive(Display)]
#[display("{_inner}")]
enum Enum {
  Variant
}

I think you should still be able to do the following (should have a test in the actual implementation):

const _inner: u8 = 5;
#[derive(Display)]
#[display(default("{_inner}"))]
enum Enum {
  Variant
}

[ModProg]

Or you could do

const _inner: u8 = 5;
#[derive(Display)]
#[display("{_inner}: {}", self::_inner)]
enum Enum {
  Variant
}
// -> "Variant: 5"

so yeah, the only thing against _inner at this point is the introduction of a custom "keyword" that might not always be intuitive, but I cannot think of a better solution at this point and I feel we explored alternatives sufficiently. (Though _variant) could be a valid naming as well.

[JelteF]

If no major opposing would be expressed in next weeks, I'll implement this. At least, before really going 1.0.0, we could have it as another beta and give it a try.

@tyranron do you still have bandwidth to work on this? I think we agree on the API design, now we "only" need to actually implement it.

[ModProg]

One thing I just came across is wanting to have the repr number in enum's display, but I can probably just do #[display("{}", self as i32)] no need for special handling.