JelteF / derive_more

Some more derive(Trait) options
MIT License
1.74k stars 123 forks source link

Fix incorrect `fmt::Pointer` implementations attempt two #381

Closed JelteF closed 3 months ago

JelteF commented 4 months ago

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

JelteF commented 3 months ago

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

JelteF commented 3 months ago

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