derive_more
derive_more copied to clipboard
`affix` is confusing with outer-level `enum` display formatting
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 doDisplay
deriving with rather different approaches, we could possibly try to find a top-levelenum
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 whatthiserror
, an error crate with facilities for derivingDisplay
, 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 forfmt
instead. -
Re-define the
fmt
spec at the top level of anenum
to be the default printing logic for anenum
's variants that can be overridden by a more specificfmt
rule. An overridable default has basis in some prior art;thiserror::Error
treats top-levelenum
format specifiers as the "default" message (motivating issue, current relevant upstreammaster
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
andfmt
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. :)
(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:
- 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. - The default
fmt
string makes sense, but given the discussion in #217 it shouldn't usefmt
anymore but should be the plain string. - 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).
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.
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.
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
@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 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
One helpful error could be when you specify a non default fmt string and neither specify
self
nor accessself
in any of the format arguments. Like telling you to usedefault
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
One helpful error could be when you specify a non default fmt string and neither specify
self
nor accessself
in any of the format arguments. Like telling you to usedefault
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
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)
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™).
@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.
And I think that single empty placeholder
{}
is clearer than{self}
, because it would break our "exactly the same formatting, asstd::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
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?
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
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
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.
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.
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)
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:
- 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. - All previous placeholder suggestions (
{}
,{variant}
,{self}
) are unfortunately confusable with a "real" [std::fmt
] argument. -
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. - Using
derive_more::Display
as a dependency tends to initially be a personal choice, I've noticed. Whenderive_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. 🙂
Designing from scratch
Let's enumerate the behaviors we have expressed that we want:
- Item level: at most one of each:
- Default variant formatting (currently exposed via
fmt = ...
) - Affix formatting (currently exposed as
{}
)
- Default variant formatting (currently exposed via
- Variant level: at most one of any:
- Non-overriding (currently exposed as
fmt = ...
) - Overriding (not exposed anywhere currently, but desirable)
- Non-overriding (currently exposed as
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:
- Use an affix placeholder that:
- Clearly does not compile with Rust's vanilla
fmt
syntax, signaling that it's an affordance of this macro, and not anything fromfmt
(this resolving concerns from @ilslv about "magical" names) - Is obviously not associated with any positional formatting arg (resolving concerns from @ModProg1 and myself
- Communicates that it will delegate formatting to variants
- Clearly does not compile with Rust's vanilla
- Make it clear which item-level behavior is being used
- Make it clear which variant-level behavior is being used
- 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) - Allow variant-level overriding of item-level affix, per my OP.
- 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:
|
❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ |
@ModProg's proposal:
|
❌ | ✅ | ⚠️ (self 's clarity is up for debate) |
❌ | ❌ | ❌ | ❌ |
@erichdongubler's new proposal, Equals Edition™ (spoiler alert!):
|
✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ❌ |
@erichdongubler's new proposal, Parens Edition™ (spoiler alert!):
|
✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ |
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.
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:
-
@
instead of@variant
, (as long as it is the only@
reference, I know I break this my self with 2.) - add
@name
fordefault
,variant
andoverride
(I don't think it makes much sense foritem
, and it might be ambiguous fordefault
anditem
) - Make remove inner "call" for
variant
anditem
#[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.
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!
@ModProg: RE: your suggestions:
- RE: (1) and (3): I would still prefer
@variant
andvariant(...)
, 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!
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.
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.
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.
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 @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
anditem
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". Thevariant
call seems quite verbose for what seems to be the most common usecase. Especially since I would not want to require avariant
oroverride
call when there's noitem
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 theitem
call, and probably hint to the existince of thedefault
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>
}
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).
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.