Skip to content

Provide better names for builtin deriving-generated attributes #49986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/librustc/middle/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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); }
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 5 additions & 5 deletions src/libsyntax_ext/deriving/cmp/ord.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"]);
Expand All @@ -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
Expand All @@ -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 = {
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax_ext/deriving/cmp/partial_eq.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,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,
Expand Down
12 changes: 6 additions & 6 deletions src/libsyntax_ext/deriving/cmp/partial_ord.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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 = {
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax_ext/deriving/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax_ext/deriving/decodable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"decoder"?

Copy link
Contributor Author

@zofrex zofrex Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also from the trait. Relatedly... are these (decodable/encodable) even used? I couldn't find any references to these in the docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are, they're from a deprecated builtin thing. See the rustc_serialize crate.

ret_ty:
Literal(Path::new_(pathvec_std!(cx, result::Result),
None,
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax_ext/deriving/encodable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"encoder"?

ret_ty: Literal(Path::new_(
pathvec_std!(cx, result::Result),
None,
Expand Down
24 changes: 12 additions & 12 deletions src/libsyntax_ext/deriving/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
/// }
Expand All @@ -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)
/// }
Expand Down Expand Up @@ -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
/// }
/// }
Expand Down Expand Up @@ -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>>();

Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax_ext/deriving/hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 9 additions & 7 deletions src/libsyntax_ext/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This had to go all the way up here because... something to do with E0382. Self gets moved into the expression below with pieces? This can go further down if we have a mutable array in the loop and push names onto it, but I assumed we would prefer to avoid mutability.

self.ecx.ident_of(&format!("arg{}", i)).gensym()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed opportunity: arg0 or arg_0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shrug this is never exposed so it doesn't matter

}).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);
Expand All @@ -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));
Expand All @@ -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));
}

Expand Down
4 changes: 4 additions & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
25 changes: 25 additions & 0 deletions src/test/run-pass-fulldeps/deriving-hygiene.rs
Original file line number Diff line number Diff line change
@@ -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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spaces between commas

(also change the commit message to just "test deriving hygiene")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(noted, although I was advised on IRC that I should squash the commits before merging anyway so I've been giving the commit messages a little less care – let me know if that's not the case)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're free to, but I personally prefer them to stay logically separate if possible, provided they build individually.

So squash the fixups together into logical changes, but you can leave them separate otherwise. Or just squash it all if you want, either way.

struct Foo {}

fn main() {
}
15 changes: 15 additions & 0 deletions src/test/run-pass/format-hygiene.rs
Original file line number Diff line number Diff line change
@@ -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);
}