-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
special-case #[derive(Copy, Clone)] with a shallow clone #31414
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this depend on the ordering of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't, but I should probably add a test. It's checking for |
||
|
||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this branch is not trigerred for non-trait/impl items? Consider: functions, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK those should never reach this path because they error out here: https://github.com/rust-lang/rust/blob/master/src/libsyntax_ext/deriving/mod.rs#L122 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I misinterpreted what you meant. There are only three choices for Annotatable: http://manishearth.github.io/rust-internals-docs/syntax/ext/base/enum.Annotatable.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wrong again because it could be a different kind of Item. So it should just ICE with "... on non-struct/enum item" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I’m surprised this branch is necessary, when any sort of item “files” just fine behind the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I'm going back to my second comment. There's a nested match. This unreachable catchall arm is in the outer one, which differentiates between Item vs TraitItem/ImplItem. The inner one does the check to see which kind of Clone we can derive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think you could just have something like match *item {
Annotatable::Item(ast::Item { node: ast::ItemStruct(...) }) |
Annotatable::Item(ast::Item { node: ast::ItemEnum(...) })
if /* the other conditions */ => {
// this is copy branch
}
_ => /* the clone branch */
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't work because it's |
||
} | ||
|
||
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<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; | ||
|
@@ -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) | ||
} | ||
} | ||
} | ||
} | ||
} |
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 }); | ||
} | ||
|
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)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be stable or unstable? It needs a new feature name and updated version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should be unstable.