Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect fmt::Pointer implementations attempt two #381

Merged
merged 6 commits into from
Jul 31, 2024

Conversation

JelteF
Copy link
Owner

@JelteF JelteF commented Jul 3, 2024

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

#[derive(Display)]
#[display("{_0:o}")]
struct OctalInt(i32);

The way this works is by introducing a local binding in the macro expansion:

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:

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

Checklist

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

@JelteF JelteF changed the base branch from master to outer-level-enum-formatting July 4, 2024 07:38
@JelteF JelteF changed the title Fix incorrect fmt::Pointer implementations attempt two Fix incorrect fmt::Pointer implementations attempt two Jul 4, 2024
@JelteF JelteF changed the base branch from outer-level-enum-formatting to master July 4, 2024 08:58
@JelteF JelteF force-pushed the fix-fmt-pointer-attempt2 branch 2 times, most recently from 1d1d9fb to 528cf06 Compare July 4, 2024 10:03
@tyranron tyranron added this to the 1.0.0 milestone Jul 4, 2024
@JelteF JelteF force-pushed the fix-fmt-pointer-attempt2 branch 2 times, most recently from 0240938 to 1127455 Compare July 4, 2024 10:36
@JelteF JelteF requested a review from tyranron July 19, 2024 01:34
@JelteF JelteF force-pushed the fix-fmt-pointer-attempt2 branch 2 times, most recently from 679e0a4 to 6bfad73 Compare July 20, 2024 11:48
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@JelteF could you update this PR to the latest master? I tried, but there are some non-trivial conflicts bound to older implementation of #377.

@JelteF
Copy link
Owner Author

JelteF commented Jul 25, 2024

Rebased now, there's still a few more tests I want to add though.

@JelteF
Copy link
Owner Author

JelteF commented Jul 26, 2024

Okay it turned out that the tests I wanted to add already existed, so this is ready for review again.

@JelteF JelteF requested a review from tyranron July 26, 2024 08:08
tests/debug.rs Outdated Show resolved Hide resolved
impl/doc/debug.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

I've extracted the similar code to the parent module and implemented the 2nd suggestion regarding expansion.

LGTM for me now, and I'm ready to merge. Feel free to leave any notices if you still have ones.

@tyranron tyranron self-assigned this Jul 31, 2024
@tyranron tyranron merged commit 7179498 into master Jul 31, 2024
17 checks passed
@tyranron tyranron deleted the fix-fmt-pointer-attempt2 branch July 31, 2024 18:10
@tyranron tyranron mentioned this pull request Jul 31, 2024
6 tasks
JelteF pushed a commit that referenced this pull request Jul 31, 2024
Resolves
#381 (comment)

## Synopsis

It has sense to bump MSRV to 1.75 for using RPITIT in implementations.

## Solution

- [x] Bump up MSRV to 1.75.
- [x] Switch to RPITIT in extension traits.
- [x] Remove `cfg(msrv)`. 

## Checklist

- [x] Documentation is updated
- [x] Tests are added/updated
- [x] [CHANGELOG entry](/CHANGELOG.md) is added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants