From 9249e6a1e2ef57bb6e329e6477beed31647236b1 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Wed, 3 Feb 2016 19:40:59 -0500 Subject: [PATCH] shallow Clone for #[derive(Copy,Clone)] Changes #[derive(Copy, Clone)] to use a faster impl of Clone when both derives are present, and there are no generics in the type. The faster impl is simply returning *self (which works because the type is also Copy). See the comments in libsyntax_ext/deriving/clone.rs for more details. There are a few types which are Copy but not Clone, in violation of the definition of Copy. These include large arrays and tuples. The very existence of these types is arguably a bug, but in order for this optimization not to change the applicability of #[derive(Copy, Clone)], the faster Clone impl also injects calls to a new function, core::clone::assert_receiver_is_clone, to verify that all members are actually Clone. This is not a breaking change, because pursuant to RFC 1521, any type that implements Copy should not do any observable work in its Clone impl. --- src/libcore/clone.rs | 11 ++ src/libsyntax_ext/deriving/clone.rs | 122 +++++++++++++++----- src/libsyntax_ext/deriving/decodable.rs | 2 +- src/libsyntax_ext/deriving/encodable.rs | 2 +- src/libsyntax_ext/deriving/generic/mod.rs | 2 + src/test/compile-fail/deriving-copyclone.rs | 48 ++++++++ src/test/run-pass/copy-out-of-array-1.rs | 2 - src/test/run-pass/deriving-copyclone.rs | 48 ++++++++ src/test/run-pass/hrtb-opt-in-copy.rs | 2 - src/test/run-pass/issue-13264.rs | 2 - src/test/run-pass/issue-3121.rs | 2 - 11 files changed, 203 insertions(+), 40 deletions(-) create mode 100644 src/test/compile-fail/deriving-copyclone.rs create mode 100644 src/test/run-pass/deriving-copyclone.rs diff --git a/src/libcore/clone.rs b/src/libcore/clone.rs index a793502e58d..ad2a205a823 100644 --- a/src/libcore/clone.rs +++ b/src/libcore/clone.rs @@ -75,6 +75,17 @@ pub trait Clone : Sized { } } +// FIXME(aburka): this method is used solely by #[derive] to +// assert that every component of a type implements Clone. +// +// This should never be called by user code. +#[doc(hidden)] +#[inline(always)] +#[unstable(feature = "derive_clone_copy", + reason = "deriving hack, should not be public", + issue = "0")] +pub fn assert_receiver_is_clone(_: &T) {} + #[stable(feature = "rust1", since = "1.0.0")] impl<'a, T: ?Sized> Clone for &'a T { /// Returns a shallow copy of the reference. diff --git a/src/libsyntax_ext/deriving/clone.rs b/src/libsyntax_ext/deriving/clone.rs index 0085182d1ac..c81198d4729 100644 --- a/src/libsyntax_ext/deriving/clone.rs +++ b/src/libsyntax_ext/deriving/clone.rs @@ -11,26 +11,68 @@ use deriving::generic::*; use deriving::generic::ty::*; -use syntax::ast::{MetaItem, Expr, VariantData}; +use syntax::ast::{Expr, ItemKind, Generics, MetaItem, VariantData}; +use syntax::attr::{self, AttrMetaMethods}; use syntax::codemap::Span; use syntax::ext::base::{ExtCtxt, Annotatable}; use syntax::ext::build::AstBuilder; use syntax::parse::token::InternedString; use syntax::ptr::P; +#[derive(PartialEq)] +enum Mode { Deep, Shallow } + pub fn expand_deriving_clone(cx: &mut ExtCtxt, span: Span, mitem: &MetaItem, item: &Annotatable, push: &mut FnMut(Annotatable)) { + // check if we can use a short form + // + // the short form is `fn clone(&self) -> Self { *self }` + // + // we can use the short form if: + // - the item is Copy (unfortunately, all we can check is whether it's also deriving Copy) + // - there are no generic parameters (after specialization this limitation can be removed) + // if we used the short form with generics, we'd have to bound the generics with + // Clone + Copy, and then there'd be no Clone impl at all if the user fills in something + // that is Clone but not Copy. and until specialization we can't write both impls. + let bounds; + let substructure; + match *item { + Annotatable::Item(ref annitem) => { + match annitem.node { + ItemKind::Struct(_, Generics { ref ty_params, .. }) | + ItemKind::Enum(_, Generics { ref ty_params, .. }) + if ty_params.is_empty() + && attr::contains_name(&annitem.attrs, "derive_Copy") => { + + bounds = vec![Literal(path_std!(cx, core::marker::Copy))]; + substructure = combine_substructure(Box::new(|c, s, sub| { + cs_clone("Clone", c, s, sub, Mode::Shallow) + })); + } + + _ => { + bounds = vec![]; + substructure = combine_substructure(Box::new(|c, s, sub| { + cs_clone("Clone", c, s, sub, Mode::Deep) + })); + } + } + } + + _ => cx.span_bug(span, "#[derive(Clone)] on trait item or impl item") + } + let inline = cx.meta_word(span, InternedString::new("inline")); let attrs = vec!(cx.attribute(span, inline)); let trait_def = TraitDef { span: span, attributes: Vec::new(), path: path_std!(cx, core::clone::Clone), - additional_bounds: Vec::new(), + additional_bounds: bounds, generics: LifetimeBounds::empty(), is_unsafe: false, methods: vec!( @@ -42,9 +84,7 @@ pub fn expand_deriving_clone(cx: &mut ExtCtxt, ret_ty: Self_, attributes: attrs, is_unsafe: false, - combine_substructure: combine_substructure(Box::new(|c, s, sub| { - cs_clone("Clone", c, s, sub) - })), + combine_substructure: substructure, } ), associated_types: Vec::new(), @@ -56,14 +96,24 @@ pub fn expand_deriving_clone(cx: &mut ExtCtxt, fn cs_clone( name: &str, cx: &mut ExtCtxt, trait_span: Span, - substr: &Substructure) -> P { + substr: &Substructure, + mode: Mode) -> P { let ctor_path; let all_fields; - let fn_path = cx.std_path(&["clone", "Clone", "clone"]); + let fn_path = match mode { + Mode::Shallow => cx.std_path(&["clone", "assert_receiver_is_clone"]), + Mode::Deep => cx.std_path(&["clone", "Clone", "clone"]), + }; let subcall = |field: &FieldInfo| { let args = vec![cx.expr_addr_of(field.span, field.self_.clone())]; - cx.expr_call_global(field.span, fn_path.clone(), args) + let span = if mode == Mode::Shallow { + // set the expn ID so we can call the unstable method + Span { expn_id: cx.backtrace(), .. trait_span } + } else { + field.span + }; + cx.expr_call_global(span, fn_path.clone(), args) }; let vdata; @@ -89,29 +139,41 @@ fn cs_clone( } } - match *vdata { - VariantData::Struct(..) => { - let fields = all_fields.iter().map(|field| { - let ident = match field.name { - Some(i) => i, - None => { - cx.span_bug(trait_span, - &format!("unnamed field in normal struct in \ - `derive({})`", name)) - } - }; - cx.field_imm(field.span, ident, subcall(field)) - }).collect::>(); + match mode { + Mode::Shallow => { + cx.expr_block(cx.block(trait_span, + all_fields.iter() + .map(subcall) + .map(|e| cx.stmt_expr(e)) + .collect(), + Some(cx.expr_deref(trait_span, cx.expr_self(trait_span))))) + } + Mode::Deep => { + match *vdata { + VariantData::Struct(..) => { + let fields = all_fields.iter().map(|field| { + let ident = match field.name { + Some(i) => i, + None => { + cx.span_bug(trait_span, + &format!("unnamed field in normal struct in \ + `derive({})`", name)) + } + }; + cx.field_imm(field.span, ident, subcall(field)) + }).collect::>(); - cx.expr_struct(trait_span, ctor_path, fields) - } - VariantData::Tuple(..) => { - let subcalls = all_fields.iter().map(subcall).collect(); - let path = cx.expr_path(ctor_path); - cx.expr_call(trait_span, path, subcalls) - } - VariantData::Unit(..) => { - cx.expr_path(ctor_path) + cx.expr_struct(trait_span, ctor_path, fields) + } + VariantData::Tuple(..) => { + let subcalls = all_fields.iter().map(subcall).collect(); + let path = cx.expr_path(ctor_path); + cx.expr_call(trait_span, path, subcalls) + } + VariantData::Unit(..) => { + cx.expr_path(ctor_path) + } + } } } } diff --git a/src/libsyntax_ext/deriving/decodable.rs b/src/libsyntax_ext/deriving/decodable.rs index 49f14c937e9..9387cf05ac7 100644 --- a/src/libsyntax_ext/deriving/decodable.rs +++ b/src/libsyntax_ext/deriving/decodable.rs @@ -74,7 +74,7 @@ fn expand_deriving_decodable_imp(cx: &mut ExtCtxt, }, explicit_self: None, args: vec!(Ptr(Box::new(Literal(Path::new_local(typaram))), - Borrowed(None, Mutability::Mutable))), + Borrowed(None, Mutability::Mutable))), ret_ty: Literal(Path::new_( pathvec_std!(cx, core::result::Result), None, diff --git a/src/libsyntax_ext/deriving/encodable.rs b/src/libsyntax_ext/deriving/encodable.rs index a05bd7869b2..daa352275d8 100644 --- a/src/libsyntax_ext/deriving/encodable.rs +++ b/src/libsyntax_ext/deriving/encodable.rs @@ -150,7 +150,7 @@ 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))), + Borrowed(None, Mutability::Mutable))), ret_ty: Literal(Path::new_( pathvec_std!(cx, core::result::Result), None, diff --git a/src/libsyntax_ext/deriving/generic/mod.rs b/src/libsyntax_ext/deriving/generic/mod.rs index b8ba1a58f21..56df0ff0e5b 100644 --- a/src/libsyntax_ext/deriving/generic/mod.rs +++ b/src/libsyntax_ext/deriving/generic/mod.rs @@ -858,6 +858,7 @@ impl<'a> MethodDef<'a> { explicit_self: ast::ExplicitSelf, arg_types: Vec<(Ident, P)> , body: P) -> ast::ImplItem { + // create the generics that aren't for Self let fn_generics = self.generics.to_generics(cx, trait_.span, type_ident, generics); @@ -991,6 +992,7 @@ impl<'a> MethodDef<'a> { body = cx.expr_match(trait_.span, arg_expr.clone(), vec!( cx.arm(trait_.span, vec!(pat.clone()), body) )) } + body } diff --git a/src/test/compile-fail/deriving-copyclone.rs b/src/test/compile-fail/deriving-copyclone.rs new file mode 100644 index 00000000000..92fb7c5737a --- /dev/null +++ b/src/test/compile-fail/deriving-copyclone.rs @@ -0,0 +1,48 @@ +// Copyright 2016 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// this will get a no-op Clone impl +#[derive(Copy, Clone)] +struct A { + a: i32, + b: i64 +} + +// this will get a deep Clone impl +#[derive(Copy, Clone)] +struct B { + a: i32, + b: T +} + +struct C; // not Copy or Clone +#[derive(Clone)] struct D; // Clone but not Copy + +fn is_copy(_: T) {} +fn is_clone(_: T) {} + +fn main() { + // A can be copied and cloned + is_copy(A { a: 1, b: 2 }); + is_clone(A { a: 1, b: 2 }); + + // B can be copied and cloned + is_copy(B { a: 1, b: 2 }); + is_clone(B { a: 1, b: 2 }); + + // B cannot be copied or cloned + is_copy(B { a: 1, b: C }); //~ERROR Copy + is_clone(B { a: 1, b: C }); //~ERROR Clone + + // B can be cloned but not copied + is_copy(B { a: 1, b: D }); //~ERROR Copy + is_clone(B { a: 1, b: D }); +} + diff --git a/src/test/run-pass/copy-out-of-array-1.rs b/src/test/run-pass/copy-out-of-array-1.rs index 5c5765454d4..54147c73ff0 100644 --- a/src/test/run-pass/copy-out-of-array-1.rs +++ b/src/test/run-pass/copy-out-of-array-1.rs @@ -12,8 +12,6 @@ // // (Compare with compile-fail/move-out-of-array-1.rs) -// pretty-expanded FIXME #23616 - #[derive(Copy, Clone)] struct C { _x: u8 } diff --git a/src/test/run-pass/deriving-copyclone.rs b/src/test/run-pass/deriving-copyclone.rs new file mode 100644 index 00000000000..96d0406d9eb --- /dev/null +++ b/src/test/run-pass/deriving-copyclone.rs @@ -0,0 +1,48 @@ +// Copyright 2016 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +//! Test that #[derive(Copy, Clone)] produces a shallow copy +//! even when a member violates RFC 1521 + +use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT, Ordering}; + +/// A struct that pretends to be Copy, but actually does something +/// in its Clone impl +#[derive(Copy)] +struct Liar; + +/// Static cooperating with the rogue Clone impl +static CLONED: AtomicBool = ATOMIC_BOOL_INIT; + +impl Clone for Liar { + fn clone(&self) -> Self { + // this makes Clone vs Copy observable + CLONED.store(true, Ordering::SeqCst); + + *self + } +} + +/// This struct is actually Copy... at least, it thinks it is! +#[derive(Copy, Clone)] +struct Innocent(Liar); + +impl Innocent { + fn new() -> Self { + Innocent(Liar) + } +} + +fn main() { + let _ = Innocent::new().clone(); + // if Innocent was byte-for-byte copied, CLONED will still be false + assert!(!CLONED.load(Ordering::SeqCst)); +} + diff --git a/src/test/run-pass/hrtb-opt-in-copy.rs b/src/test/run-pass/hrtb-opt-in-copy.rs index b40f4d27a9c..f0214d3f37b 100644 --- a/src/test/run-pass/hrtb-opt-in-copy.rs +++ b/src/test/run-pass/hrtb-opt-in-copy.rs @@ -16,8 +16,6 @@ // did not consider that a match (something I would like to revise in // a later PR). -// pretty-expanded FIXME #23616 - #![allow(dead_code)] use std::marker::PhantomData; diff --git a/src/test/run-pass/issue-13264.rs b/src/test/run-pass/issue-13264.rs index 7acabf31c85..383c1aef234 100644 --- a/src/test/run-pass/issue-13264.rs +++ b/src/test/run-pass/issue-13264.rs @@ -8,8 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// pretty-expanded FIXME #23616 - use std::ops::Deref; struct Root { diff --git a/src/test/run-pass/issue-3121.rs b/src/test/run-pass/issue-3121.rs index 777e5bf7a6d..6e9ee7fb15c 100644 --- a/src/test/run-pass/issue-3121.rs +++ b/src/test/run-pass/issue-3121.rs @@ -8,8 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// pretty-expanded FIXME #23616 - #![allow(unknown_features)] #![feature(box_syntax)]