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

Provide better names for builtin deriving-generated attributes

First attempt at fixing rust-lang#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`?)

Author: bors

src/librustc/middle/liveness.rs

@@ -184,6 +184,7 @@ impl<'a, 'tcx> Visitor<'tcx> for IrMaps<'a, 'tcx> {
                 b: hir::BodyId, s: Span, id: NodeId) {
         visit_fn(self, fk, fd, b, s, id);
     }
+
     fn visit_local(&mut self, l: &'tcx hir::Local) { visit_local(self, l); }
     fn visit_expr(&mut self, ex: &'tcx Expr) { visit_expr(self, ex); }
     fn visit_arm(&mut self, a: &'tcx hir::Arm) { visit_arm(self, a); }
@@ -361,6 +362,16 @@ fn visit_fn<'a, 'tcx: 'a>(ir: &mut IrMaps<'a, 'tcx>,
     // swap in a new set of IR maps for this function body:
     let mut fn_maps = IrMaps::new(ir.tcx);
 
+    // Don't run unused pass for #[derive()]
+    if let FnKind::Method(..) = fk {
+        let parent = ir.tcx.hir.get_parent(id);
+        if let Some(hir::map::Node::NodeItem(i)) = ir.tcx.hir.find(parent) {
+            if i.attrs.iter().any(|a| a.check_name("automatically_derived")) {
+                return;
+            }
+        }
+    }
+
     debug!("creating fn_maps: {:?}", &fn_maps as *const IrMaps);
 
     let body = ir.tcx.hir.body(body_id);

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 = {

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,

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 = {

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();

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,

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,

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 {

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,

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));
         }
 

src/libsyntax_pos/symbol.rs

@@ -53,6 +53,10 @@ impl Ident {
     pub fn modern(self) -> Ident {
         Ident::new(self.name, self.span.modern())
     }
+
+    pub fn gensym(self) -> Ident {
+        Ident::new(self.name.gensymed(), self.span)
+    }
 }
 
 impl PartialEq for Ident {

src/test/run-pass-fulldeps/auxiliary/custom_derive_partial_eq.rs

@@ -58,7 +58,7 @@ fn expand_deriving_partial_eq(cx: &mut ExtCtxt, span: Span, mitem: &MetaItem, it
         name: "eq",
         generics: LifetimeBounds::empty(),
         explicit_self: borrowed_explicit_self(),
-        args: vec![borrowed_self()],
+        args: vec![(borrowed_self(), "other")],
         ret_ty: Literal(deriving::generic::ty::Path::new_local("bool")),
         attributes: attrs,
         is_unsafe: false,

src/test/run-pass-fulldeps/deriving-hygiene.rs

@@ -0,0 +1,25 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(rustc_private)]
+extern crate serialize;
+
+pub const other: u8 = 1;
+pub const f: u8 = 1;
+pub const d: u8 = 1;
+pub const s: u8 = 1;
+pub const state: u8 = 1;
+pub const cmp: u8 = 1;
+
+#[derive(Ord,Eq,PartialOrd,PartialEq,Debug,Decodable,Encodable,Hash)]
+struct Foo {}
+
+fn main() {
+}

src/test/run-pass/format-hygiene.rs

@@ -0,0 +1,15 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+pub const arg0: u8 = 1;
+
+pub fn main() {
+    format!("{}", 1);
+}