special-case #[derive(Copy, Clone)] with a shallow clone

[durka]

If a type is Copy then its Clone implementation can be a no-op. Currently #[derive(Clone)] generates a deep clone anyway. This can lead to lots of code bloat.

This PR detects the case where Copy and Clone are both being derived (the general case of "is this type Copy" can't be determined by a syntax extension) and generates the shallow Clone impl. Right now this can only be done if there are no type parameters (see #31085 (comment)), but this restriction can be removed after specialization.

Fixes #31085.

[rust-highfive]

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

[nagisa]

syntax::attr::contains_name(item.attrs, "derive_Copy")

[nagisa]

So this branch is not trigerred for non-trait/impl items? Consider: functions, type aliases, mod items, etc?

[durka]

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

[durka]

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

[durka]

I'm wrong again because it could be a different kind of Item. So it should just ICE with "... on non-struct/enum item"

[nagisa]

Right, I’m surprised this branch is necessary, when any sort of item “files” just fine behind the Item variant.

[durka]

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.

[nagisa]

I'm wrong again because it could be a different kind of Item. So it should just ICE with "... on non-struct/enum item"

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 */
}

[durka]

It doesn't work because it's enum Annotatable { Item(P<Item>), ... } and you can't match through a P. At least, I can't. I tried, with #![feature(box_patterns)] but the ptr field is private so I can't get to the inner Box.

[bluss]

According to #31085 (comment) this kind of improvement can improve compile times (that's a case with a lot of structs).

According to me @ #31085 (comment) using the Copy-based clone can improve codegen for loops calling .clone().

[bors]

☔️ The latest upstream changes (presumably #31400) made this pull request unmergeable. Please resolve the merge conflicts.

[durka]

It occurs to me that the shallow Clone can also be done in this situation:

#[derive(Copy, Clone)]
struct S<T: Copy>(T);

i.e. when there are generic parameters but they are all already constrained to be Copy. Is it worth it to add this check as well?

Edit: seems like it would be somewhat unworkably unhygienic. We'd have to check for the literal name "Copy", since all this happens before resolve.

[durka]

Rebased.

[nagisa]

This PR LGTM, but I’d like an opinion from somebody more familiar with the code as well.

[brson]

Can you put this in its own rmake test case instead of piggy-backing on this one?

[durka]

I'll split the tests and revert my changes to the Makefile for this one.

[brson]

Do we have any concrete numbers that this gives to either compile time or runtime? @retep998 said he worked around this on winapi because of bad compile times. Perhaps we can change winapi to use deriving again and see if this improves perf there.

[durka]

Travis failed because I didn't run the full test suite locally. Will fix later tonight.

[durka]

I split the tests and tried to fix the fallout in the rpass-full tests. I'm getting linker errors locally which I think will be solved by make clean, we'll see.

[durka]

@brson @bluss here are the runtime benchmarks: gist

The performance of clone_from_slice on #[derive(Copy, Clone)] struct S(u8, u32); is doubled.

[durka]

@brson @retep998 and compile time: gist

A #[derive(Copy, Clone)] struct with 1000 members compiles 13 times faster.

[durka]

As a bonus, this extends the reach of #[derive(Clone)] -- you can now #[derive(Copy, Clone)] on structs containing types which are Copy but not Clone, such as [u8; 100]. gist

[bluss]

Compile time win for structs and enums might be the best thing with this. A bit unnerving that it enables deriving Clone on more things, though. That makes it hard to go back on this optimization.

[durka]

@bluss I don't see why we'd want to go back on it. Plus, more motivation to fix the issue where types can be magically Copy but not Clone :)

[brson]

Wow, great numbers.

I'm also uneasy about the semantic change this brings, making derive Copy/Clone work on types containing those that aren't clone. It exposes the optimization - semantically we may actually want derive Clone to be defined as if it was implemented the naive way.

I assume this doesn't magically make just derive(Clone) work for types containing types that don't implement Clone, but only derive(Copy, Clone), and that's super weird.

[durka]

I assume this doesn't magically make just derive(Clone) work for types containing types that don't implement Clone, but only derive(Copy, Clone)

Correct. The derives just do their little syntactical shufflings and leave it to typeck to enforce the constraints later.

and that's super weird.

I agree, but it's only observable if you have a type that's Copy and not Clone, which isn't supposed to be possible. So it only comes up with corner cases like arrays over size 32 or tuples over size 12.

[durka]

We could revert the semantic change by generating code like this:

impl Clone for Foo {
    fn clone(&self) -> Foo {
        if false { /* naive implementation */ }
        else { *self }
    }
}

but we'd of course lose the compile-time wins.

[sfackler]

I think we optimize if false ... in the frontend so that branch would at least not make it to LLVM.

Can't we also do something like

struct Foo {
    bar: Bar,
}

// generated
impl Clone for Foo {
    fn clone(&self) -> Foo {
        fn is_clone<C: Clone>() {}
        is_clone::<Bar>();
        *self
    }
}

which should avoid the compile time sadness.

[sfackler]

That does still leave the behavior change that bar's Clone impl isn't invoked obviously.

[durka]

@sfackler I'm trying this. I still think it's insignificant since the existence of T: Copy + !Clone is a bug.

[eddyb]

@durka Good question, it would likely make winapi worse, not better. Maybe replace #[derive(Copy, Clone)] with #[derive_CopyClone] or something like that?

[durka]

@Manishearth sorry about my slowness. I still need to fix the spans so I
can make core::assert_receiver_is_clone unstable. And I was hoping if I
stared at the deriving code long enough I could remove the breakage, but I
don't see how. So punt on this if you want, otherwise I'll really try to
get it ready to land this weekend.
On Apr 22, 2016 17:36, "Eduard-Mihai Burtescu" notifications@github.com
wrote:

@durka https://github.com/durka Good question, it would likely make
winapi worse, not better. Maybe replace #[derive(Copy, Clone)] with
#[derive_CopyClone] or something like that?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#31414 (comment)

[Manishearth]

Nah, it's okay, we can wait a few days 😄

I wasn't sure what the status of this PR was.

[retep998]

@eddyb I don't use #[derive(Clone)] at all in winapi.

[durka]

@retep998 but you could in a parallel universe where #[derive(Clone)]
didn't slow things down and winapi didn't care about 1.x backcompat.
On Apr 22, 2016 21:51, "Peter Atashian" notifications@github.com wrote:

@eddyb https://github.com/eddyb I don't use #[derive(Clone)] at all in
winapi.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#31414 (comment)

[retep998]

@durka But I can't due some Copy types not being Clone such as function pointers and large arrays which winapi has quite a lot of.

[bluss]

How lovely, winapi needs the bugs that this PR takes care to not expose. 😄 Can't please them all

[durka]

The stage1-rmake test passes now. And the stage2-rpass-full one. And I guess travis will try them all.

[durka]

@eddyb and I are now certain we can do this without the breaking change. un-cc #31645 and my apologies for delaying the batch :)

[durka]

Breaking change removed. Performance gains retained.

[alexcrichton]

Instead of no-pretty-expanded I think you can just remove these directives altogether

[durka]

Oh is it opt-in? Ok. I thought it was opt-out. I also figured if I left the mention of the method name here, then someone in the future stabilizing or removing this hack might grep for it and realize they could re-enable the test. But it's fine with me to remove it.

[alexcrichton]

Nah yeah it's fine to just remove everywhere, right now that phase of testing is opt-in

[durka]

Fair enough. Does that mean when I write a new run-pass test I should try
to add "// pretty-expanded"? Do we have a policy on that?

On Mon, Apr 25, 2016 at 2:24 PM, Alex Crichton notifications@github.com
wrote:

In src/test/run-pass/issue-3121.rs
#31414 (comment):

@@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

-// pretty-expanded FIXME #23616
+// no-pretty-expanded FIXME #23616 assert_receiver_is_clone

Nah yeah it's fine to just remove everywhere, right now that phase of
testing is opt-in

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/31414/files/64d22d453d5b71b33b149dac3c28b2d626446d9f#r60962453

[alexcrichton]

Nah it's fine to just remove these comments

[durka]

Fixed.

[alexcrichton]

I think we have src/test/pretty for this, right? Always just good to avoid run-make wherever possible :)

[durka]

Hmm what does src/test/pretty actually do? This test asserts that the pretty expansion is exactly as intended but src/test/pretty doesn't seem to have expected output.

[eddyb]

There's one .pp file in there which is the expected output for one test. If nobody has removed it yet for being painful to update, that is.

[durka]

Oh, so there is. I can migrate this test to src/test/pretty then. But
there are more run-make tests of the same class (pretty-expanded,
pretty-expanded-hygiene). I guess they can be migrated in a separate PR.

On Mon, Apr 25, 2016 at 2:04 PM, Eduard-Mihai Burtescu <
notifications@github.com> wrote:

In src/test/run-make/expand-derive/input_pp.rs
#31414 (comment):

+// minimal junk
+#![feature(no_core)]
+#![no_core]
+
+struct A {

• a: i32,
  +}
  +#[automatically_derived]
  +#[allow(unused_qualifications)]
  +impl ::clone::Clone for A {
• #[inline]
• fn clone(&self) -> A {

•    match *self {
  

•        A { a: ref __self_0_0 } => {
  

•            ::clone::assert_receiver_is_clone(&(*__self_0_0));
  

There's one .pp file in there which is the expected output for one test.
If nobody has removed it yet for being painful to update, that is.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/31414/files/64d22d453d5b71b33b149dac3c28b2d626446d9f#r60958975

[alexcrichton]

Yeah I'd also be fine just leaving this test out, it'd end up being pretty brittle :(

[durka]

I'm trying to think of a way to test the shallowness of a Clone impl without pp-exactness.

[durka]

I think this test will do the job.

[durka]

In other words this is a behavior change, but it is now OKed by accepted rust-lang/rfcs#1521.

[alexcrichton]

Yeah that seems like a good test to me.

[durka]

OK, I deleted the pretty-printing test and added the Innocent(Liar) test.

[alexcrichton]

Thanks again for the PR @durka! Everything looks good to me, wanna squash and I'll r+?

[durka]

Squashed and wrote a more extensive commit message.

On Tue, Apr 26, 2016 at 1:06 PM, Alex Crichton notifications@github.com
wrote:

Thanks again for the PR @durka https://github.com/durka! Everything
looks good to me, wanna squash and I'll r+?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#31414 (comment)

[alexcrichton]

@bors: r+ 9249e6a

[bors]

⌛️ Testing commit 9249e6a with merge 8f55218...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild