Support _variant in outer level enum formatting for Display #377

JelteF · 2024-07-02T10:35:00Z

Resolves #142
Resolves #239

Synopsis

This adds back support for top-level format strings of the Display derive. It
now includes the display of the variant whenever the _variant placeholder
apears be found. It also supports using the field

Solution

This does not include the same support for Debug, since it is considered much less
useful there and the differences between the Debug implementation and Display
implementation make it a non-trivial port.

Only named arguments are supported in the format string, no positional ones.
This made the implementation easier, maybe in a future PR positional support
can be added.

This bumps MSRV to 1.70.0, on earlier versions the derived code throws the following error:

error: there is no argument named `_0`
    --> tests/display.rs:1373:26
     |
1373 |                 #[derive(Display)]
     |                          ^^^^^^^
     |
     = note: did you intend to capture a variable `_0` from the surrounding scope?
     = note: to avoid ambiguity, `format_args!` cannot capture variables when the format string is expanded from a macro
     = note: this error originates in the derive macro `Display` (in Nightly builds, run with -Z macro-backtrace for more info)

Checklist

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

JelteF · 2024-07-02T11:36:27Z

Still needs some cleanup, error-ing on edge cases, and updated docs, but the general design seems to work.

JelteF · 2024-07-03T12:43:00Z

Still needs some cleanup, error-ing on edge cases, and updated docs, but the general design seems to work.

Okay all done now, this is ready for review.

clippy.toml

tests/display.rs

tests/compile_fail/display/shared_format_positional_placeholders.stderr

Resolves #363 Related to #371 Requires #377 This makes sure we don't add unnecessary bounds for Display-like derives. It does so in the same way as #371 did for the Debug derive: By only adding bounds when the type contains a type variable.

tyranron

Needs desugaring re-implementation as was discussed.

tyranron · 2024-07-24T18:23:49Z

@JelteF I did the re-implementation, now it seems fine. This restriction is not necessary, actually, and by doing some deeper inspection of the shared formatting attribute we may lift it in the future. This especially may be useful for the use-cases where we want to add some special modifiers (for octals, etc) for all the variants. I haven't done it to omit complicating this PR and to not slowing the 1.0 release, as it requires more time and very rich test suite.

The only thing I want to add before merging this PR is to extend tests suite a little bit more. Will do it shortly.

impl/doc/display.md

JelteF

Overall soooo much better than my original implementation. Left a few small notes.

impl/src/fmt/display.rs

Resolves #363 Related to #371 Requires #377 This makes sure we don't add unnecessary bounds for Display-like derives. It does so in the same way as #371 did for the Debug derive: By only adding bounds when the type contains a type variable.

Co-authored-by: Jelte Fennema-Nio <[email protected]>

…o outer-level-enum-formatting

tyranron · 2024-07-25T09:25:50Z

@JelteF if you don't have any more suggestions on this, I'm ready to merge this one.

tests/display.rs

Resolves #363 Requires #377 Follows #371 ## Synopsis The problem is that the `Display` derive adds bounds for all types that are used in the format string. But this is not necessary for types that don't contain a type variable. And adding those bounds can result in errors like for the following code: ```rust #[derive(Display, Debug)] #[display("{inner:?}")] #[display(bounds(T: Display))] struct OptionalBox<T> { inner: Option<Box<T>>, } #[derive(Display, Debug)] #[display("{next}")] struct ItemStruct { next: OptionalBox<ItemStruct>, } ``` That code would generate the following error: ```text error[E0275]: overflow evaluating the requirement `ItemStruct: derive_more::Display` ``` ## Solution This makes sure we don't add unnecessary bounds for Display-like derives. It does so in the same way as #371 did for the Debug derive: By only adding bounds when the type contains a type variable. Co-authored-by: Kai Ren <[email protected]>

Resolves #328 Requires #377 Requires #380 ## Synopsis `Debug` and `Display` derives allow referring fields via short syntax (`_0` for unnamed fields and `name` for named fields): ```rust #[derive(Display)] #[display("{_0:o}")] struct OctalInt(i32); ``` The way this works is by introducing a local binding in the macro expansion: ```rust let _0 = &self.0; ``` This, however, introduces double pointer indirection. For most of the `fmt` traits, this is totally OK. However, the `fmt::Pointer` is sensitive to that: ```rust #[derive(Display)] #[display("--> {_0:p}")] struct Int(&'static i32); // expands to impl fmt::Display for Int { fn fmt(&self, f: fmt::Formatter<'_>) -> fmt::Result { let _0 = &self.0; // has `&&i32` type, not `&i32` write!(f, "--> {_0:p}") // so, prints address of the `_0` local binding, // not the one of the `self.0` field as we would expect } } ``` ## Solution Pass all local bindings also as named parameters and dereference them there. This allows `"{_0:p}"` to work as expected. Positional arguments and expressions still have the previous behaviour. This seems okay IMHO, as we can explain that in expressions these local bindings are references and that you need to dereference when needed, such as for `Pointer`. A downside of the current implementation is that users cannot use the names of our named parameters as names for their own named parameters, because we already use them. With some additional code this is fixable, but it doesn't seem important enough to fix. People can simply use a different name when creating their own named parameters, which is a good idea anyway because it will be less confusing to any reader of the code. If it turns out to be important to support this after all, we can still start to support it in a backwards compatible way (because now it causes a compilation failure). Co-authored-by: Kai Ren <[email protected]>

JelteF added this to the 1.0.0 milestone Jul 2, 2024

JelteF requested a review from tyranron July 2, 2024 10:44

JelteF self-assigned this Jul 2, 2024

JelteF mentioned this pull request Jul 2, 2024

Non-local impl definition warning #366

Open

JelteF force-pushed the outer-level-enum-formatting branch 3 times, most recently from da56134 to 734ed21 Compare July 2, 2024 11:35

JelteF force-pushed the outer-level-enum-formatting branch 8 times, most recently from 0573f07 to cf2c6bf Compare July 3, 2024 12:39

Support _variant in outer level enum formatting for Display

b877940

JelteF force-pushed the outer-level-enum-formatting branch from cf2c6bf to b877940 Compare July 3, 2024 12:44

JelteF mentioned this pull request Jul 4, 2024

Fix incorrect fmt::Pointer implementations attempt two #381

Merged

3 tasks

tyranron added 2 commits July 19, 2024 15:11

Merge branch 'refs/heads/master' into outer-level-enum-formatting

256c041

Some fixes and tests improvement [skip ci]

5d73864

tyranron requested changes Jul 19, 2024

View reviewed changes

clippy.toml Outdated Show resolved Hide resolved

tests/display.rs Show resolved Hide resolved

tests/compile_fail/display/shared_format_positional_placeholders.stderr Outdated Show resolved Hide resolved

JelteF mentioned this pull request Jul 20, 2024

Only put Display-like bounds on type variables #387

Merged

1 task

tyranron requested changes Jul 24, 2024

View reviewed changes

tyranron self-assigned this Jul 24, 2024

tyranron added 3 commits July 24, 2024 19:57

Re-impl, vol.1

dc223cf

Re-impl, vol.2

4a506c4

Fix

b30e717

JelteF commented Jul 24, 2024

View reviewed changes

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

JelteF commented Jul 24, 2024

View reviewed changes

impl/src/fmt/display.rs Outdated Show resolved Hide resolved

impl/src/fmt/display.rs Outdated Show resolved Hide resolved

impl/src/fmt/display.rs Outdated Show resolved Hide resolved

impl/src/fmt/display.rs Outdated Show resolved Hide resolved

tyranron and others added 7 commits July 25, 2024 10:10

Update impl/doc/display.md

b07d550

Co-authored-by: Jelte Fennema-Nio <[email protected]>

Unraw syn::Ident for non-fields variants

986d8f5

Fix shared format transparency for format_args!() expansions

decb3af

Cover raw idents in tests

9612477

Update impl/src/fmt/display.rs

43c8464

Co-authored-by: Jelte Fennema-Nio <[email protected]>

Merge remote-tracking branch 'origin/outer-level-enum-formatting' int…

e614fbd

…o outer-level-enum-formatting

Simplify

9690fe0

tyranron previously approved these changes Jul 25, 2024

View reviewed changes

JelteF commented Jul 25, 2024

View reviewed changes

tests/display.rs Show resolved Hide resolved

Add test case for variant transparency

e570a25

tyranron dismissed their stale review via e570a25 July 25, 2024 10:10

tyranron self-requested a review July 25, 2024 10:14

tyranron approved these changes Jul 25, 2024

View reviewed changes

tyranron merged commit 8a172f2 into master Jul 25, 2024
17 checks passed

tyranron deleted the outer-level-enum-formatting branch July 25, 2024 11:40

tyranron mentioned this pull request Aug 9, 2024

Fix transparency check on shared attribute formatting on enums (#377) #395

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support _variant in outer level enum formatting for Display #377

Support _variant in outer level enum formatting for Display #377

JelteF commented Jul 2, 2024 •

edited

Loading

JelteF commented Jul 2, 2024

JelteF commented Jul 3, 2024

tyranron left a comment •

edited

Loading

tyranron commented Jul 24, 2024 •

edited

Loading

JelteF left a comment

tyranron commented Jul 25, 2024

Support _variant in outer level enum formatting for Display #377

Support _variant in outer level enum formatting for Display #377

Conversation

JelteF commented Jul 2, 2024 • edited Loading

Synopsis

Solution

Checklist

JelteF commented Jul 2, 2024

JelteF commented Jul 3, 2024

tyranron left a comment • edited Loading

Choose a reason for hiding this comment

tyranron commented Jul 24, 2024 • edited Loading

JelteF left a comment

Choose a reason for hiding this comment

tyranron commented Jul 25, 2024

JelteF commented Jul 2, 2024 •

edited

Loading

tyranron left a comment •

edited

Loading

tyranron commented Jul 24, 2024 •

edited

Loading