Fix incorrect fmt::Pointer implementations (#381, #328)

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 <tyranron@gmail.com>

CHANGELOG.md

@@ -104,6 +104,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Hygiene of macro expansions in presence of custom `core` crate.
   ([#327](https://github.com/JelteF/derive_more/pull/327))
 - Fix documentation of generated methods in `IsVariant` derive.
+- Make `{field:p}` do the expected thing in format strings for `Display` and
+  `Debug`. Also document weirdness around `Pointer` formatting when using
+  expressions, due to field variables being references.
+  ([#381](https://github.com/JelteF/derive_more/pull/381))
 
 ## 0.99.10 - 2020-09-11
 

impl/doc/debug.md

@@ -14,8 +14,28 @@ This derive macro is a clever superset of `Debug` from standard library. Additio
 You supply a format by placing an attribute on a struct or enum variant, or its particular field:
 `#[debug("...", args...)]`. The format is exactly like in [`format!()`] or any other [`format_args!()`]-based macros.
 
-The variables available in the arguments is `self` and each member of the struct or enum variant, with members of tuple
-structs being named with a leading underscore and their index, i.e. `_0`, `_1`, `_2`, etc.
+The variables available in the arguments is `self` and each member of the
+struct or enum variant, with members of tuple structs being named with a
+leading underscore and their index, i.e. `_0`, `_1`, `_2`, etc. Due to
+ownership/lifetime limitations the member variables are all references to the
+fields, except when used directly in the format string. For most purposes this
+detail doesn't matter, but it is quite important when using `Pointer`
+formatting. If you don't use the `{field:p}` syntax, you have to dereference
+once to get the address of the field itself, instead of the address of the
+reference to the field:
+
+```rust
+use derive_more::Debug;
+
+#[derive(Debug)]
+#[debug("{field:p} {:p}", *field)]
+struct RefInt<'a> {
+    field: &'a i32,
+}
+
+let a = &123;
+assert_eq!(format!("{:?}", RefInt{field: &a}), format!("{a:p} {:p}", a));
+```
 
 
 ### Generic data types
@@ -90,8 +110,8 @@ If the top-level `#[debug("...", args...)]` attribute (the one for a whole struc
 and can be trivially substituted with a transparent delegation call to the inner type, then all the additional
 [formatting parameters][1] do work as expected:
 ```rust
-# use derive_more::Debug;
-#
+use derive_more::Debug;
+
 #[derive(Debug)]
 #[debug("{_0:o}")] // the same as calling `Octal::fmt()`
 struct MyOctalInt(i32);
@@ -110,8 +130,8 @@ assert_eq!(format!("{:07?}", MyBinaryInt(2)), "10");
 If, for some reason, transparency in trivial cases is not desired, it may be suppressed explicitly
 either with the [`format_args!()`] macro usage:
 ```rust
-# use derive_more::Debug;
-#
+use derive_more::Debug;
+
 #[derive(Debug)]
 #[debug("{}", format_args!("{_0:o}"))] // `format_args!()` obscures the inner type
 struct MyOctalInt(i32);
@@ -121,8 +141,8 @@ assert_eq!(format!("{:07?}", MyOctalInt(9)), "11");
 ```
 Or by adding [formatting parameters][1] which cause no visual effects:
 ```rust
-# use derive_more::Debug;
-#
+use derive_more::Debug;
+
 #[derive(Debug)]
 #[debug("{_0:^o}")] // `^` is centering, but in absence of additional width has no effect
 struct MyOctalInt(i32);

impl/doc/display.md

@@ -22,9 +22,28 @@ inner variable. If there is no such variable, or there is more than 1, an error
 You supply a format by attaching an attribute of the syntax: `#[display("...", args...)]`.
 The format supplied is passed verbatim to `write!`.
 
-The variables available in the arguments is `self` and each member of the variant,
-with members of tuple structs being named with a leading underscore and their index,
-i.e. `_0`, `_1`, `_2`, etc.
+The variables available in the arguments is `self` and each member of the
+struct or enum variant, with members of tuple structs being named with a
+leading underscore and their index, i.e. `_0`, `_1`, `_2`, etc. Due to
+ownership/lifetime limitations the member variables are all references to the
+fields, except when used directly in the format string. For most purposes this
+detail doesn't matter, but it is quite important when using `Pointer`
+formatting. If you don't use the `{field:p}` syntax, you have to dereference
+once to get the address of the field itself, instead of the address of the
+reference to the field:
+
+```rust
+# use derive_more::Display;
+#
+#[derive(Display)]
+#[display("{field:p} {:p}", *field)]
+struct RefInt<'a> {
+    field: &'a i32,
+}
+
+let a = &123;
+assert_eq!(format!("{}", RefInt{field: &a}), format!("{a:p} {:p}", a));
+```
 
 For enums you can also specify a shared format on the enum itself instead of
 the variant. This format is used for each of the variants, and can be
@@ -55,7 +74,7 @@ E.g., for a structure `Foo` defined like this:
 # trait Trait { type Type; }
 #
 #[derive(Display)]
-#[display("{} {} {:?} {:p}", a, b, c, d)]
+#[display("{a} {b} {c:?} {d:p}")]
 struct Foo<'a, T1, T2: Trait, T3> {
     a: T1,
     b: <T2 as Trait>::Type,

impl/src/fmt/debug.rs

@@ -13,7 +13,7 @@ use crate::utils::{
 
 use super::{
     trait_name_to_attribute_name, ContainerAttributes, ContainsGenericsExt as _,
-    FmtAttribute,
+    FieldsExt as _, FmtAttribute,
 };
 
 /// Expands a [`fmt::Debug`] derive macro.
@@ -250,9 +250,22 @@ impl<'a> Expansion<'a> {
     fn generate_body(&self) -> syn::Result<TokenStream> {
         if let Some(fmt) = &self.attr.fmt {
             return Ok(if let Some((expr, trait_ident)) = fmt.transparent_call() {
-                quote! { derive_more::core::fmt::#trait_ident::fmt(&(#expr), __derive_more_f) }
+                let expr = if self
+                    .fields
+                    .fmt_args_idents()
+                    .into_iter()
+                    .any(|field| expr == field)
+                {
+                    quote! { #expr }
+                } else {
+                    quote! { &(#expr) }
+                };
+
+                quote! { derive_more::core::fmt::#trait_ident::fmt(#expr, __derive_more_f) }
             } else {
-                quote! { derive_more::core::write!(__derive_more_f, #fmt) }
+                let deref_args = fmt.additional_deref_args(self.fields);
+
+                quote! { derive_more::core::write!(__derive_more_f, #fmt, #(#deref_args),*) }
             });
         };
 
@@ -288,12 +301,16 @@ impl<'a> Expansion<'a> {
                             exhaustive = false;
                             Ok::<_, syn::Error>(out)
                         }
-                        Some(FieldAttribute::Right(fmt_attr)) => Ok(quote! {
-                            derive_more::__private::DebugTuple::field(
-                                #out,
-                                &derive_more::core::format_args!(#fmt_attr),
-                            )
-                        }),
+                        Some(FieldAttribute::Right(fmt_attr)) => {
+                            let deref_args = fmt_attr.additional_deref_args(self.fields);
+
+                            Ok(quote! {
+                                derive_more::__private::DebugTuple::field(
+                                    #out,
+                                    &derive_more::core::format_args!(#fmt_attr, #(#deref_args),*),
+                                )
+                            })
+                        }
                         None => {
                             let ident = format_ident!("_{i}");
                             Ok(quote! {
@@ -319,29 +336,38 @@ impl<'a> Expansion<'a> {
                     )
                 };
                 let out = named.named.iter().try_fold(out, |out, field| {
-                        let field_ident = field.ident.as_ref().unwrap_or_else(|| {
-                            unreachable!("`syn::Fields::Named`");
-                        });
-                        let field_str = field_ident.to_string();
-                        match FieldAttribute::parse_attrs(&field.attrs, self.attr_name)?
-                            .map(Spanning::into_inner)
-                        {
-                            Some(FieldAttribute::Left(_skip)) => {
-                                exhaustive = false;
-                                Ok::<_, syn::Error>(out)
-                            }
-                            Some(FieldAttribute::Right(fmt_attr)) => Ok(quote! {
+                    let field_ident = field.ident.as_ref().unwrap_or_else(|| {
+                        unreachable!("`syn::Fields::Named`");
+                    });
+                    let field_str = field_ident.to_string();
+                    match FieldAttribute::parse_attrs(&field.attrs, self.attr_name)?
+                        .map(Spanning::into_inner)
+                    {
+                        Some(FieldAttribute::Left(_skip)) => {
+                            exhaustive = false;
+                            Ok::<_, syn::Error>(out)
+                        }
+                        Some(FieldAttribute::Right(fmt_attr)) => {
+                            let deref_args =
+                                fmt_attr.additional_deref_args(self.fields);
+
+                            Ok(quote! {
                                 derive_more::core::fmt::DebugStruct::field(
                                     #out,
                                     #field_str,
-                                    &derive_more::core::format_args!(#fmt_attr),
+                                    &derive_more::core::format_args!(
+                                        #fmt_attr, #(#deref_args),*
+                                    ),
                                 )
-                            }),
-                            None => Ok(quote! {
-                                derive_more::core::fmt::DebugStruct::field(#out, #field_str, &#field_ident)
-                            }),
+                            })
                         }
-                    })?;
+                        None => Ok(quote! {
+                            derive_more::core::fmt::DebugStruct::field(
+                                #out, #field_str, &#field_ident
+                            )
+                        }),
+                    }
+                })?;
                 Ok(if exhaustive {
                     quote! { derive_more::core::fmt::DebugStruct::finish(#out) }
                 } else {

impl/src/fmt/display.rs

@@ -11,7 +11,7 @@ use crate::utils::{attr::ParseMultiple as _, Spanning};
 
 use super::{
     trait_name_to_attribute_name, ContainerAttributes, ContainsGenericsExt as _,
-    FmtAttribute,
+    FieldsExt as _, FmtAttribute,
 };
 
 /// Expands a [`fmt::Display`]-like derive macro.
@@ -278,13 +278,30 @@ impl<'a> Expansion<'a> {
             body = match &self.attrs.fmt {
                 Some(fmt) => {
                     if has_shared_attr {
-                        quote! { &derive_more::core::format_args!(#fmt) }
+                        let deref_args = fmt.additional_deref_args(self.fields);
+
+                        quote! { &derive_more::core::format_args!(#fmt, #(#deref_args),*) }
                     } else if let Some((expr, trait_ident)) = fmt.transparent_call() {
+                        let expr = if self
+                            .fields
+                            .fmt_args_idents()
+                            .into_iter()
+                            .any(|field| expr == field)
+                        {
+                            quote! { #expr }
+                        } else {
+                            quote! { &(#expr) }
+                        };
+
                         quote! {
-                            derive_more::core::fmt::#trait_ident::fmt(&(#expr), __derive_more_f)
+                            derive_more::core::fmt::#trait_ident::fmt(#expr, __derive_more_f)
                         }
                     } else {
-                        quote! { derive_more::core::write!(__derive_more_f, #fmt) }
+                        let deref_args = fmt.additional_deref_args(self.fields);
+
+                        quote! {
+                            derive_more::core::write!(__derive_more_f, #fmt, #(#deref_args),*)
+                        }
                     }
                 }
                 None if self.fields.is_empty() => {
@@ -332,8 +349,10 @@ impl<'a> Expansion<'a> {
 
         if has_shared_attr {
             if let Some(shared_fmt) = &self.shared_attr {
+                let deref_args = shared_fmt.additional_deref_args(self.fields);
+
                 let shared_body = quote! {
-                    derive_more::core::write!(__derive_more_f, #shared_fmt)
+                    derive_more::core::write!(__derive_more_f, #shared_fmt, #(#deref_args),*)
                 };
 
                 body = if body.is_empty() {

impl/src/fmt/mod.rs

@@ -9,7 +9,7 @@ pub(crate) mod display;
 mod parsing;
 
 use proc_macro2::TokenStream;
-use quote::{format_ident, ToTokens};
+use quote::{format_ident, quote, ToTokens};
 use syn::{
     parse::{Parse, ParseStream},
     punctuated::Punctuated,
@@ -113,14 +113,16 @@ impl Parse for FmtAttribute {
     fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
         Self::check_legacy_fmt(input)?;
 
-        Ok(Self {
+        let mut parsed = Self {
             lit: input.parse()?,
             comma: input
                 .peek(token::Comma)
                 .then(|| input.parse())
                 .transpose()?,
             args: input.parse_terminated(FmtArgument::parse, token::Comma)?,
-        })
+        };
+        parsed.args.pop_punct();
+        Ok(parsed)
     }
 }
 
@@ -273,6 +275,33 @@ impl FmtAttribute {
         })
     }
 
+    /// Returns an [`Iterator`] over the additional formatting arguments doing the dereferencing
+    /// replacement in this [`FmtAttribute`] for those [`Placeholder`] representing the provided
+    /// [`syn::Fields`] and requiring it
+    fn additional_deref_args<'fmt: 'ret, 'fields: 'ret, 'ret>(
+        &'fmt self,
+        fields: &'fields syn::Fields,
+    ) -> impl Iterator<Item = TokenStream> + 'ret {
+        let used_args = Placeholder::parse_fmt_string(&self.lit.value())
+            .into_iter()
+            .filter_map(|placeholder| match placeholder.arg {
+                Parameter::Named(name) => Some(name),
+                _ => None,
+            })
+            .collect::<Vec<_>>();
+
+        fields
+            .fmt_args_idents()
+            .into_iter()
+            .filter_map(move |field_name| {
+                (used_args.iter().any(|arg| field_name == arg)
+                    && !self.args.iter().any(|arg| {
+                        arg.alias.as_ref().map_or(false, |(n, _)| n == &field_name)
+                    }))
+                .then(|| quote! { #field_name = *#field_name })
+            })
+    }
+
     /// Errors in case legacy syntax is encountered: `fmt = "...", (arg),*`.
     fn check_legacy_fmt(input: ParseStream<'_>) -> syn::Result<()> {
         let fork = input.fork();
@@ -523,6 +552,7 @@ where
     }
 }
 
+/// Extension of a [`syn::Type`] and a [`syn::Path`] allowing to travers its type parameters.
 trait ContainsGenericsExt {
     /// Checks whether this definition contains any of the provided `type_params`.
     fn contains_generics(&self, type_params: &[&syn::Ident]) -> bool;
@@ -638,6 +668,26 @@ impl ContainsGenericsExt for syn::Path {
     }
 }
 
+/// Extension of [`syn::Fields`] providing helpers for a [`FmtAttribute`].
+trait FieldsExt {
+    /// Returns an [`Iterator`] over [`syn::Ident`]s representing these [`syn::Fields`] in a
+    /// [`FmtAttribute`] as [`FmtArgument`]s or named [`Placeholder`]s.
+    ///
+    /// [`syn::Ident`]: struct@syn::Ident
+    // TODO: Return `impl Iterator<Item = syn::Ident> + '_` once MSRV is bumped up to 1.75 or
+    //       higher.
+    fn fmt_args_idents(&self) -> Vec<syn::Ident>;
+}
+
+impl FieldsExt for syn::Fields {
+    fn fmt_args_idents(&self) -> Vec<syn::Ident> {
+        self.iter()
+            .enumerate()
+            .map(|(i, f)| f.ident.clone().unwrap_or_else(|| format_ident!("_{i}")))
+            .collect()
+    }
+}
+
 #[cfg(test)]
 mod fmt_attribute_spec {
     use itertools::Itertools as _;

impl/src/parsing.rs

@@ -71,6 +71,12 @@ impl Parse for Expr {
     }
 }
 
+impl PartialEq<syn::Ident> for Expr {
+    fn eq(&self, other: &syn::Ident) -> bool {
+        self.ident().map_or(false, |i| i == other)
+    }
+}
+
 impl ToTokens for Expr {
     fn to_tokens(&self, tokens: &mut TokenStream) {
         match self {