Auto merge of #49986 - zofrex:better-derived-argument-names, r=Manishearth

Provide better names for builtin deriving-generated attributes

First attempt at fixing #49967

Not in love with any choices here, don't be shy if you aren't happy with anything :)

I've tested that this produces nicer names in documentation, and that it no longer has issues conflicting with constants with the same name. (I guess we _could_ make a test for that... unsure if that would be valuable)

In all cases I took the names from the methods as declared in the relevant trait.

In some cases I had to prepend the names with _ otherwise there were errors about un-used variables. I'm uneasy with the inconsistency... do they all need to be like that? Is there a way to generate an alternate impl or use a different name (`_`?) in the cases where the arguments are not used?

Lastly the gensym addition to Ident I implemented largely as suggested, but I want to point out it's a little circuitous (at least, as far as I understand it). `cx.ident_of(name)` is just `Ident::from_str`, so we create an Ident then another Ident from it. `Ident::with_empty_ctxt(Symbol::gensym(string))` may or may not be equivalent, I don't know if it's important to intern it _then_ gensym it. It seems like either we could use that, or if we do want a new method to make this convenient, it could be on Ident instead (`from_str_gensymed`?)

9 files changed, 41 insertions, 39 deletions

diff --git a/src/libsyntax_ext/deriving/cmp/ord.rs b/src/libsyntax_ext/deriving/cmp/ord.rs
index 1a392ac3765..99b6f752e94 100644
--- a/src/libsyntax_ext/deriving/cmp/ord.rs
+++ b/src/libsyntax_ext/deriving/cmp/ord.rs
@@ -38,7 +38,7 @@ pub fn expand_deriving_ord(cx: &mut ExtCtxt,
                           name: "cmp",
                           generics: LifetimeBounds::empty(),
                           explicit_self: borrowed_explicit_self(),
-                          args: vec![borrowed_self()],
+                          args: vec![(borrowed_self(), "other")],
                           ret_ty: Literal(path_std!(cx, cmp::Ordering)),
                           attributes: attrs,
                           is_unsafe: false,
@@ -64,7 +64,7 @@ pub fn ordering_collapsed(cx: &mut ExtCtxt,
 }
 
 pub fn cs_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<Expr> {
-    let test_id = cx.ident_of("__cmp");
+    let test_id = cx.ident_of("cmp").gensym();
     let equals_path = cx.path_global(span, cx.std_path(&["cmp", "Ordering", "Equal"]));
 
     let cmp_path = cx.std_path(&["cmp", "Ord", "cmp"]);
@@ -77,9 +77,9 @@ pub fn cs_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<Expr> {
     // ::std::cmp::Ordering::Equal => {
     // ...
     // }
-    // __cmp => __cmp
+    // cmp => cmp
     // },
-    // __cmp => __cmp
+    // cmp => cmp
     // }
     //
     cs_fold(// foldr nests the if-elses correctly, leaving the first field
@@ -88,7 +88,7 @@ pub fn cs_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<Expr> {
             |cx, span, old, self_f, other_fs| {
         // match new {
         //     ::std::cmp::Ordering::Equal => old,
-        //     __cmp => __cmp
+        //     cmp => cmp
         // }
 
         let new = {
diff --git a/src/libsyntax_ext/deriving/cmp/partial_eq.rs b/src/libsyntax_ext/deriving/cmp/partial_eq.rs
index 81ca7e73228..c259733d81a 100644
--- a/src/libsyntax_ext/deriving/cmp/partial_eq.rs
+++ b/src/libsyntax_ext/deriving/cmp/partial_eq.rs
@@ -78,7 +78,7 @@ pub fn expand_deriving_partial_eq(cx: &mut ExtCtxt,
                 name: $name,
                 generics: LifetimeBounds::empty(),
                 explicit_self: borrowed_explicit_self(),
-                args: vec![borrowed_self()],
+                args: vec![(borrowed_self(), "other")],
                 ret_ty: Literal(path_local!(bool)),
                 attributes: attrs,
                 is_unsafe: false,
diff --git a/src/libsyntax_ext/deriving/cmp/partial_ord.rs b/src/libsyntax_ext/deriving/cmp/partial_ord.rs
index 9560fd0570a..d71527fd0ed 100644
--- a/src/libsyntax_ext/deriving/cmp/partial_ord.rs
+++ b/src/libsyntax_ext/deriving/cmp/partial_ord.rs
@@ -34,7 +34,7 @@ pub fn expand_deriving_partial_ord(cx: &mut ExtCtxt,
                 name: $name,
                 generics: LifetimeBounds::empty(),
                 explicit_self: borrowed_explicit_self(),
-                args: vec![borrowed_self()],
+                args: vec![(borrowed_self(), "other")],
                 ret_ty: Literal(path_local!(bool)),
                 attributes: attrs,
                 is_unsafe: false,
@@ -59,7 +59,7 @@ pub fn expand_deriving_partial_ord(cx: &mut ExtCtxt,
         name: "partial_cmp",
         generics: LifetimeBounds::empty(),
         explicit_self: borrowed_explicit_self(),
-        args: vec![borrowed_self()],
+        args: vec![(borrowed_self(), "other")],
         ret_ty,
         attributes: attrs,
         is_unsafe: false,
@@ -123,7 +123,7 @@ pub fn some_ordering_collapsed(cx: &mut ExtCtxt,
 }
 
 pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<Expr> {
-    let test_id = cx.ident_of("__cmp");
+    let test_id = cx.ident_of("cmp").gensym();
     let ordering = cx.path_global(span, cx.std_path(&["cmp", "Ordering", "Equal"]));
     let ordering_expr = cx.expr_path(ordering.clone());
     let equals_expr = cx.expr_some(span, ordering_expr);
@@ -138,9 +138,9 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<
     // ::std::option::Option::Some(::std::cmp::Ordering::Equal) => {
     // ...
     // }
-    // __cmp => __cmp
+    // cmp => cmp
     // },
-    // __cmp => __cmp
+    // cmp => cmp
     // }
     //
     cs_fold(// foldr nests the if-elses correctly, leaving the first field
@@ -149,7 +149,7 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<
             |cx, span, old, self_f, other_fs| {
         // match new {
         //     Some(::std::cmp::Ordering::Equal) => old,
-        //     __cmp => __cmp
+        //     cmp => cmp
         // }
 
         let new = {
diff --git a/src/libsyntax_ext/deriving/debug.rs b/src/libsyntax_ext/deriving/debug.rs
index c007285cd85..b546f5df157 100644
--- a/src/libsyntax_ext/deriving/debug.rs
+++ b/src/libsyntax_ext/deriving/debug.rs
@@ -40,7 +40,7 @@ pub fn expand_deriving_debug(cx: &mut ExtCtxt,
                           name: "fmt",
                           generics: LifetimeBounds::empty(),
                           explicit_self: borrowed_explicit_self(),
-                          args: vec![fmtr],
+                          args: vec![(fmtr, "f")],
                           ret_ty: Literal(path_std!(cx, fmt::Result)),
                           attributes: Vec::new(),
                           is_unsafe: false,
@@ -70,7 +70,7 @@ fn show_substructure(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<E
     // We want to make sure we have the ctxt set so that we can use unstable methods
     let span = span.with_ctxt(cx.backtrace());
     let name = cx.expr_lit(span, ast::LitKind::Str(ident.name, ast::StrStyle::Cooked));
-    let builder = Ident::from_str("__debug_trait_builder");
+    let builder = Ident::from_str("debug_trait_builder").gensym();
     let builder_expr = cx.expr_ident(span, builder.clone());
 
     let fmt = substr.nonself_args[0].clone();
diff --git a/src/libsyntax_ext/deriving/decodable.rs b/src/libsyntax_ext/deriving/decodable.rs
index 46dada256b8..7618fe63ab3 100644
--- a/src/libsyntax_ext/deriving/decodable.rs
+++ b/src/libsyntax_ext/deriving/decodable.rs
@@ -67,8 +67,8 @@ fn expand_deriving_decodable_imp(cx: &mut ExtCtxt,
                                                             PathKind::Global)])],
                           },
                           explicit_self: None,
-                          args: vec![Ptr(Box::new(Literal(Path::new_local(typaram))),
-                                         Borrowed(None, Mutability::Mutable))],
+                          args: vec![(Ptr(Box::new(Literal(Path::new_local(typaram))),
+                                         Borrowed(None, Mutability::Mutable)), "d")],
                           ret_ty:
                               Literal(Path::new_(pathvec_std!(cx, result::Result),
                                                  None,
diff --git a/src/libsyntax_ext/deriving/encodable.rs b/src/libsyntax_ext/deriving/encodable.rs
index d579b3abd45..2c6c18cc51c 100644
--- a/src/libsyntax_ext/deriving/encodable.rs
+++ b/src/libsyntax_ext/deriving/encodable.rs
@@ -148,8 +148,8 @@ fn expand_deriving_encodable_imp(cx: &mut ExtCtxt,
                     ],
                 },
                 explicit_self: borrowed_explicit_self(),
-                args: vec![Ptr(Box::new(Literal(Path::new_local(typaram))),
-                           Borrowed(None, Mutability::Mutable))],
+                args: vec![(Ptr(Box::new(Literal(Path::new_local(typaram))),
+                           Borrowed(None, Mutability::Mutable)), "s")],
                 ret_ty: Literal(Path::new_(
                     pathvec_std!(cx, result::Result),
                     None,
diff --git a/src/libsyntax_ext/deriving/generic/mod.rs b/src/libsyntax_ext/deriving/generic/mod.rs
index 1f80385cfbd..1ef0d2b0b49 100644
--- a/src/libsyntax_ext/deriving/generic/mod.rs
+++ b/src/libsyntax_ext/deriving/generic/mod.rs
@@ -252,7 +252,7 @@ pub struct MethodDef<'a> {
     pub explicit_self: Option<Option<PtrTy<'a>>>,
 
     /// Arguments other than the self argument
-    pub args: Vec<Ty<'a>>,
+    pub args: Vec<(Ty<'a>, &'a str)>,
 
     /// Return type
     pub ret_ty: Ty<'a>,
@@ -915,9 +915,9 @@ impl<'a> MethodDef<'a> {
             explicit_self
         });
 
-        for (i, ty) in self.args.iter().enumerate() {
+        for (ty, name) in self.args.iter() {
             let ast_ty = ty.to_ty(cx, trait_.span, type_ident, generics);
-            let ident = cx.ident_of(&format!("__arg_{}", i));
+            let ident = cx.ident_of(name).gensym();
             arg_tys.push((ident, ast_ty));
 
             let arg_expr = cx.expr_ident(trait_.span, ident);
@@ -1004,10 +1004,10 @@ impl<'a> MethodDef<'a> {
     ///
     /// // equivalent to:
     /// impl PartialEq for A {
-    ///     fn eq(&self, __arg_1: &A) -> bool {
+    ///     fn eq(&self, other: &A) -> bool {
     ///         match *self {
     ///             A {x: ref __self_0_0, y: ref __self_0_1} => {
-    ///                 match *__arg_1 {
+    ///                 match *other {
     ///                     A {x: ref __self_1_0, y: ref __self_1_1} => {
     ///                         __self_0_0.eq(__self_1_0) && __self_0_1.eq(__self_1_1)
     ///                     }
@@ -1020,10 +1020,10 @@ impl<'a> MethodDef<'a> {
     /// // or if A is repr(packed) - note fields are matched by-value
     /// // instead of by-reference.
     /// impl PartialEq for A {
-    ///     fn eq(&self, __arg_1: &A) -> bool {
+    ///     fn eq(&self, other: &A) -> bool {
     ///         match *self {
     ///             A {x: __self_0_0, y: __self_0_1} => {
-    ///                 match __arg_1 {
+    ///                 match other {
     ///                     A {x: __self_1_0, y: __self_1_1} => {
     ///                         __self_0_0.eq(&__self_1_0) && __self_0_1.eq(&__self_1_1)
     ///                     }
@@ -1134,14 +1134,14 @@ impl<'a> MethodDef<'a> {
     /// // is equivalent to
     ///
     /// impl PartialEq for A {
-    ///     fn eq(&self, __arg_1: &A) -> ::bool {
-    ///         match (&*self, &*__arg_1) {
+    ///     fn eq(&self, other: &A) -> ::bool {
+    ///         match (&*self, &*other) {
     ///             (&A1, &A1) => true,
     ///             (&A2(ref self_0),
     ///              &A2(ref __arg_1_0)) => (*self_0).eq(&(*__arg_1_0)),
     ///             _ => {
     ///                 let __self_vi = match *self { A1(..) => 0, A2(..) => 1 };
-    ///                 let __arg_1_vi = match *__arg_1 { A1(..) => 0, A2(..) => 1 };
+    ///                 let __arg_1_vi = match *other { A1(..) => 0, A2(..) => 1 };
     ///                 false
     ///             }
     ///         }
@@ -1240,7 +1240,7 @@ impl<'a> MethodDef<'a> {
         let vi_idents: Vec<ast::Ident> = self_arg_names.iter()
             .map(|name| {
                 let vi_suffix = format!("{}_vi", &name[..]);
-                cx.ident_of(&vi_suffix[..])
+                cx.ident_of(&vi_suffix[..]).gensym()
             })
             .collect::<Vec<ast::Ident>>();
 
@@ -1616,7 +1616,7 @@ impl<'a> TraitDef<'a> {
         let mut ident_exprs = Vec::new();
         for (i, struct_field) in struct_def.fields().iter().enumerate() {
             let sp = struct_field.span.with_ctxt(self.span.ctxt());
-            let ident = cx.ident_of(&format!("{}_{}", prefix, i));
+            let ident = cx.ident_of(&format!("{}_{}", prefix, i)).gensym();
             paths.push(ident.with_span_pos(sp));
             let val = cx.expr_path(cx.path_ident(sp, ident));
             let val = if use_temporaries {
diff --git a/src/libsyntax_ext/deriving/hash.rs b/src/libsyntax_ext/deriving/hash.rs
index b192ab2527e..67096cdb49a 100644
--- a/src/libsyntax_ext/deriving/hash.rs
+++ b/src/libsyntax_ext/deriving/hash.rs
@@ -44,8 +44,8 @@ pub fn expand_deriving_hash(cx: &mut ExtCtxt,
                               bounds: vec![(typaram, vec![path_std!(cx, hash::Hasher)])],
                           },
                           explicit_self: borrowed_explicit_self(),
-                          args: vec![Ptr(Box::new(Literal(arg)),
-                                         Borrowed(None, Mutability::Mutable))],
+                          args: vec![(Ptr(Box::new(Literal(arg)),
+                                         Borrowed(None, Mutability::Mutable)), "state")],
                           ret_ty: nil_ty(),
                           attributes: vec![],
                           is_unsafe: false,
diff --git a/src/libsyntax_ext/format.rs b/src/libsyntax_ext/format.rs
index c78decb1eb9..6b155b6596d 100644
--- a/src/libsyntax_ext/format.rs
+++ b/src/libsyntax_ext/format.rs
@@ -543,6 +543,10 @@ impl<'a, 'b> Context<'a, 'b> {
         let mut pats = Vec::new();
         let mut heads = Vec::new();
 
+        let names_pos: Vec<_> = (0..self.args.len()).map(|i| {
+            self.ecx.ident_of(&format!("arg{}", i)).gensym()
+        }).collect();
+
         // First, build up the static array which will become our precompiled
         // format "string"
         let pieces = self.ecx.expr_vec_slice(self.fmtsp, self.str_pieces);
@@ -560,7 +564,7 @@ impl<'a, 'b> Context<'a, 'b> {
         // of each variable because we don't want to move out of the arguments
         // passed to this function.
         for (i, e) in self.args.into_iter().enumerate() {
-            let name = self.ecx.ident_of(&format!("__arg{}", i));
+            let name = names_pos[i];
             let span =
                 DUMMY_SP.with_ctxt(e.span.ctxt().apply_mark(self.ecx.current_expansion.mark));
             pats.push(self.ecx.pat_ident(span, name));
@@ -570,14 +574,12 @@ impl<'a, 'b> Context<'a, 'b> {
             heads.push(self.ecx.expr_addr_of(e.span, e));
         }
         for pos in self.count_args {
-            let name = self.ecx.ident_of(&match pos {
-                Exact(i) => format!("__arg{}", i),
-                _ => panic!("should never happen"),
-            });
-            let span = match pos {
-                Exact(i) => spans_pos[i],
+            let index = match pos {
+                Exact(i) => i,
                 _ => panic!("should never happen"),
             };
+            let name = names_pos[index];
+            let span = spans_pos[index];
             counts.push(Context::format_arg(self.ecx, self.macsp, span, &Count, name));
         }