Fix incorrect fmt::Pointer implementations attempt two

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
@@ -25,8 +45,8 @@ are bound by respective formatting trait.
 
 E.g., for a structure `Foo` defined like this:
 ```rust
-use derive_more::Debug;
-
+# use derive_more::Debug;
+#
 #[derive(Debug)]
 struct Foo<'a, T1, T2: Trait, T3, T4> {
     #[debug("{a}")]
@@ -64,9 +84,9 @@ possible. Our aim is to avoid imposing additional bounds, as they can be added w
 In the example below, we can infer only that `V: Display`, other bounds have to be supplied by the user:
 
 ```rust
-use std::fmt::Display;
-use derive_more::Debug;
-
+# use std::fmt::Display;
+# use derive_more::Debug;
+#
 #[derive(Debug)]
 #[debug(bound(T: MyTrait, U: Display))]
 struct MyStruct<T, U, V, F> {
@@ -137,9 +157,9 @@ assert_eq!(format!("{:07?}", MyOctalInt(9)), "11");
 ## Example usage
 
 ```rust
-use std::path::PathBuf;
-use derive_more::Debug;
-
+# use std::path::PathBuf;
+# use derive_more::Debug;
+#
 #[derive(Debug)]
 struct MyInt(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 {