Skip to content

Commit

Permalink
shallow Clone for #[derive(Copy,Clone)]
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
durka committed Apr 26, 2016
1 parent 42ea682 commit 9249e6a
Show file tree
Hide file tree
Showing 11 changed files with 203 additions and 40 deletions.
11 changes: 11 additions & 0 deletions src/libcore/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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: Clone + ?Sized>(_: &T) {}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, T: ?Sized> Clone for &'a T {
/// Returns a shallow copy of the reference.
Expand Down
122 changes: 92 additions & 30 deletions src/libsyntax_ext/deriving/clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand All @@ -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(),
Expand All @@ -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<Expr> {
substr: &Substructure,
mode: Mode) -> P<Expr> {
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;
Expand All @@ -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::<Vec<_>>();

cx.expr_struct(trait_span, ctor_path, fields)
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)))))
}
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)
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::<Vec<_>>();

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)
}
}
}
}
}
2 changes: 1 addition & 1 deletion src/libsyntax_ext/deriving/decodable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax_ext/deriving/encodable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/libsyntax_ext/deriving/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,7 @@ impl<'a> MethodDef<'a> {
explicit_self: ast::ExplicitSelf,
arg_types: Vec<(Ident, P<ast::Ty>)> ,
body: P<Expr>) -> ast::ImplItem {

// create the generics that aren't for Self
let fn_generics = self.generics.to_generics(cx, trait_.span, type_ident, generics);

Expand Down Expand Up @@ -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
}

Expand Down
48 changes: 48 additions & 0 deletions src/test/compile-fail/deriving-copyclone.rs
Original file line number Diff line number Diff line change
@@ -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 <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.

// 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<T> {
a: i32,
b: T
}

struct C; // not Copy or Clone
#[derive(Clone)] struct D; // Clone but not Copy

fn is_copy<T: Copy>(_: T) {}
fn is_clone<T: 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<i32> can be copied and cloned
is_copy(B { a: 1, b: 2 });
is_clone(B { a: 1, b: 2 });

// B<C> cannot be copied or cloned
is_copy(B { a: 1, b: C }); //~ERROR Copy
is_clone(B { a: 1, b: C }); //~ERROR Clone

// B<D> can be cloned but not copied
is_copy(B { a: 1, b: D }); //~ERROR Copy
is_clone(B { a: 1, b: D });
}

2 changes: 0 additions & 2 deletions src/test/run-pass/copy-out-of-array-1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down
48 changes: 48 additions & 0 deletions src/test/run-pass/deriving-copyclone.rs
Original file line number Diff line number Diff line change
@@ -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 <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.

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

2 changes: 0 additions & 2 deletions src/test/run-pass/hrtb-opt-in-copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions src/test/run-pass/issue-13264.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 0 additions & 2 deletions src/test/run-pass/issue-3121.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]

Expand Down

0 comments on commit 9249e6a

Please sign in to comment.