librustc: Make `Copy` opt-in. #17864

pcwalton · 2014-10-08T00:55:08Z

This change makes the compiler no longer infer whether types (structures
and enumerations) implement the Copy trait (and thus are implicitly
copyable). Rather, you must implement Copy yourself via impl Copy for MyType {}.

This breaks code like:

#[deriving(Show)]
struct Point2D {
    x: int,
    y: int,
}

fn main() {
    let mypoint = Point2D {
        x: 1,
        y: 1,
    };
    let otherpoint = mypoint;
    println!("{}{}", mypoint, otherpoint);
}

Change this code to:

#[deriving(Show)]
struct Point2D {
    x: int,
    y: int,
}

impl Copy for Point2D {}

fn main() {
    let mypoint = Point2D {
        x: 1,
        y: 1,
    };
    let otherpoint = mypoint;
    println!("{}{}", mypoint, otherpoint);
}

This is the backwards-incompatible part of #13231.

Part of RFC #3.

[breaking-change]

r? @nikomatsakis

rust-highfive · 2014-10-08T00:55:13Z

Warning

These commits modify unsafe code. Please review it carefully!

sfackler · 2014-10-08T03:14:41Z

It'd be nice to allow #[deriving(Copy)].

killercup · 2014-10-08T12:54:29Z

Link to the RFC

nikomatsakis · 2014-10-08T14:24:16Z

Exciting! Can't wait to review.

One nit. You write:

This is the backwards-incompatible part of #13231.

But in fact there is another backwards incompatible part, concerning whether *T is send or not.

Manishearth · 2014-10-09T07:29:51Z

#[deriving(Copy)] should be easy enough, we just need to copy the code for Eq with an empty combine_substructure or something similar.

thestinger · 2014-10-09T10:25:27Z

If you implement Clone, your type is cloneable, meaning that it moves from place to place, but it can be explicitly cloned. This is suitable for cases where copying is expensive.

If you implement Copy, your type is copyable, meaning that it is just copied by default without the need for an explicit clone. This is suitable for small bits of data like ints or points.

As long as the Copy guarantees are being leveraged by types like Cell, the RFC is incorrect. It's incorrect to leave out a Copy implementation based on how expensive it would be. The lack of an automatic Clone implementation for all Copy types is a significant language wart. The system in C++ is not as transparent about costs but at least it's coherent.

bstrie · 2014-10-09T13:56:55Z

@thestinger, is it mentioned anywhere that we don't want Copy to imply Clone? I know it's not possible today, but I was under the impression that trait reform would make such blanket impls feasible.

thestinger · 2014-10-09T14:21:57Z

#17884

flaper87 · 2014-10-12T20:37:34Z

cc @flaper87

flaper87 · 2014-10-12T20:59:47Z

src/librustc/middle/typeck/coherence/mod.rs

@@ -476,6 +481,114 @@ impl<'a, 'tcx> CoherenceChecker<'a, 'tcx> {
            }
        }
    }
+
+    /// Ensures that implementations of the built-in trait `Copy` are legal.
+    fn check_implementations_of_copy(&self) {


Can this method be made more generic in order to cover also other built-in traits?

In my last PR I had a visitor that would run the checks below for each implementation of a built-in trait.

I like the implementation of this PR better but there's probably something we can rescue from the old one since for other built-in traits we'll need the same checks on fields and variants.

pcwalton · 2014-10-15T22:01:41Z

r? @nikomatsakis

nikomatsakis · 2014-10-16T19:12:37Z

r+ modulo bare fns and the nit about just calling through to assemble_builtin_candidates unconditionally

mitsuhiko · 2014-10-26T23:59:34Z

Has there been a discussion about keeping the old Copy trait stick around under a new name for code like Cell can make assumptions about it? The compiler could do what it did for Copy in the past but for a new trait called Pod for instance which has no user facing API implications.

thestinger · 2014-10-27T00:18:45Z

Every case where a type had Copy before but not after the commits is a regression unless there's a soundness issue with having it. The timeval and timespec types in liblibc should remain Copy, among others. If going through all the types and making decisions is too large of a maintenance burden then there's a problem with this design decision. It's going to break a huge amount of third party code and there is no possible migration path in cases where at least one field is private.

sfackler · 2014-10-27T00:22:02Z

@mitsuhiko I'm not sure I understand how Pod would be distinct from current Copy. Could types under a Pod bound be copied? How would it have no user facing API implications if it were usable by users?

thestinger · 2014-10-27T00:26:43Z

I think there needs to be a lint to determine when a type is able to implement Copy, and all of the warnings should be fixed. It can be allowed on the few (5-10?) types that are supposed to lack a Copy implementation but are able to provide it. This would catch all of the regressions.

mitsuhiko · 2014-10-27T07:33:38Z

@sfackler Copy had two meanings: copies implicitly and can be copied with memcpy. Even under the new system it makes absolutely no sense that Copy could be implemented for non pod types. As such it makes sense to have an automatic Pod type and to use that to aid the developer (warn if copy is missing, error if copy cannot be a memcpy). Likewise for data structures like Cell currently relying on Copy it makes sense to switch over to Pod and to also support the types that can no longer be copied implicitly.

mitsuhiko · 2014-10-27T08:16:39Z

(To add to that: figuring out if something is a Pod type or not is something that is very useful. C++ for instance comes with an std::is_pod template)

nikomatsakis · 2014-10-28T13:41:05Z

@mitsuhiko the semantics of Copy are not changed by this patch. The only change is that it becomes opt-in.

mitsuhiko · 2014-10-28T22:07:08Z

@mitsuhiko the semantics of Copy are not changed by this patch. The only change is that it becomes opt-in.

I understand that, but why not have an always implemented Pod trait that is a superset of it? There are situations in which you might want the guarantee of it being safe for memcpy but still have the implicit copy disabled.

pcwalton · 2014-11-04T23:08:03Z

@thestinger "It's too hard to change right now" was explicitly not considered a valid reason to not make this change.

pcwalton · 2014-11-04T23:45:49Z

That said I'm adding the lint right now, for public types without any type parameters.

pcwalton · 2014-11-11T06:58:58Z

r? @nikomatsakis (lint added per @thestinger's suggestion)

nikomatsakis · 2014-11-11T16:14:37Z

r+ here are comments (from IRC):

[08:12:49] <nmatsakis> pcwalton: do we want to impose a rule that you cannot impl Copy if you impl Drop? (and do you impose such a rule?)
[08:12:56] <nmatsakis> pcwalton: it's not obvious to me that this restriction is NECESSARY
[08:13:16] <nmatsakis> pcwalton: though perhaps at minimum we shouldn't lint about it I guess
[08:13:24] <nmatsakis> pcwalton: but basically r+
[08:14:02] <nmatsakis> pcwalton: I read the changes to middle::traits and middle:coherence, I assume the rest is fallout. <nag>it'd be helpful to segregate that into its own commit.</nag>
[08:14:04] <nmatsakis> I'll leave a comment

tamird · 2014-12-01T00:22:07Z

looks like this needs a rebase

nikomatsakis · 2014-12-04T23:19:02Z

@pcwalton r+ but needs rebase

This change makes the compiler no longer infer whether types (structures and enumerations) implement the `Copy` trait (and thus are implicitly copyable). Rather, you must implement `Copy` yourself via `impl Copy for MyType {}`. A new warning has been added, `missing_copy_implementations`, to warn you if a non-generic public type has been added that could have implemented `Copy` but didn't. This breaks code like: #[deriving(Show)] struct Point2D { x: int, y: int, } fn main() { let mypoint = Point2D { x: 1, y: 1, }; let otherpoint = mypoint; println!("{}{}", mypoint, otherpoint); } Change this code to: #[deriving(Show)] struct Point2D { x: int, y: int, } impl Copy for Point2D {} fn main() { let mypoint = Point2D { x: 1, y: 1, }; let otherpoint = mypoint; println!("{}{}", mypoint, otherpoint); } This is the backwards-incompatible part of rust-lang#13231. Part of RFC rust-lang#3. [breaking-change]

alexcrichton · 2014-12-08T19:15:37Z

Closing in favor of #19566

fix: Build and run build scripts in lsif command

rust-highfive assigned nikomatsakis Oct 8, 2014

pcwalton force-pushed the oibit2 branch from ef66b5f to ef717e5 Compare October 8, 2014 02:59

pcwalton force-pushed the oibit2 branch from ef717e5 to 4f7f757 Compare October 8, 2014 06:58

pcwalton force-pushed the oibit2 branch from 4f7f757 to c5140b1 Compare October 9, 2014 03:29

nikomatsakis mentioned this pull request Oct 10, 2014

Implement where clauses #17657

Closed

7 tasks

flaper87 reviewed Oct 12, 2014
View reviewed changes

aturon mentioned this pull request Oct 15, 2014

Implicitly copyable iterators are mega confusing #18045

Closed

pcwalton force-pushed the oibit2 branch 3 times, most recently from 7cc1145 to a3c7530 Compare October 15, 2014 22:01

pcwalton force-pushed the oibit2 branch 2 times, most recently from c0689c7 to eac462d Compare October 21, 2014 21:02

pcwalton force-pushed the oibit2 branch from eac462d to d772af8 Compare October 23, 2014 19:08

pcwalton force-pushed the oibit2 branch 2 times, most recently from 051aa46 to a9fd710 Compare November 11, 2014 06:57

pcwalton force-pushed the oibit2 branch from a9fd710 to 3d54aef Compare November 11, 2014 16:10

pcwalton force-pushed the oibit2 branch from 3d54aef to 00662f4 Compare November 11, 2014 16:31

pcwalton force-pushed the oibit2 branch from 00662f4 to 2d439c7 Compare December 4, 2014 22:31

pcwalton force-pushed the oibit2 branch from 2d439c7 to b517e81 Compare December 5, 2014 02:07

pcwalton force-pushed the oibit2 branch from b517e81 to 09f1bfa Compare December 5, 2014 05:03

pcwalton force-pushed the oibit2 branch from 09f1bfa to 5859dc1 Compare December 5, 2014 12:07

alexcrichton closed this Dec 8, 2014

blaenk mentioned this pull request Dec 15, 2014

make TypeId derive Copy, like the real one chris-morgan/anymap#12

Closed

lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 13, 2024

Auto merge of rust-lang#17864 - Veykril:lsif, r=Veykril

e66f3db

fix: Build and run build scripts in lsif command

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librustc: Make `Copy` opt-in. #17864

librustc: Make `Copy` opt-in. #17864

pcwalton commented Oct 8, 2014

rust-highfive commented Oct 8, 2014

sfackler commented Oct 8, 2014

killercup commented Oct 8, 2014

nikomatsakis commented Oct 8, 2014

Manishearth commented Oct 9, 2014

thestinger commented Oct 9, 2014

bstrie commented Oct 9, 2014

thestinger commented Oct 9, 2014

flaper87 commented Oct 12, 2014

flaper87 Oct 12, 2014

pcwalton commented Oct 15, 2014

nikomatsakis commented Oct 16, 2014

mitsuhiko commented Oct 26, 2014

thestinger commented Oct 27, 2014

sfackler commented Oct 27, 2014

thestinger commented Oct 27, 2014

mitsuhiko commented Oct 27, 2014

mitsuhiko commented Oct 27, 2014

nikomatsakis commented Oct 28, 2014

mitsuhiko commented Oct 28, 2014

pcwalton commented Nov 4, 2014

pcwalton commented Nov 4, 2014

pcwalton commented Nov 11, 2014

nikomatsakis commented Nov 11, 2014

tamird commented Dec 1, 2014

nikomatsakis commented Dec 4, 2014

alexcrichton commented Dec 8, 2014

librustc: Make Copy opt-in. #17864

librustc: Make Copy opt-in. #17864

Conversation

pcwalton commented Oct 8, 2014

rust-highfive commented Oct 8, 2014

sfackler commented Oct 8, 2014

killercup commented Oct 8, 2014

nikomatsakis commented Oct 8, 2014

Manishearth commented Oct 9, 2014

thestinger commented Oct 9, 2014

bstrie commented Oct 9, 2014

thestinger commented Oct 9, 2014

flaper87 commented Oct 12, 2014

flaper87 Oct 12, 2014

Choose a reason for hiding this comment

pcwalton commented Oct 15, 2014

nikomatsakis commented Oct 16, 2014

mitsuhiko commented Oct 26, 2014

thestinger commented Oct 27, 2014

sfackler commented Oct 27, 2014

thestinger commented Oct 27, 2014

mitsuhiko commented Oct 27, 2014

mitsuhiko commented Oct 27, 2014

nikomatsakis commented Oct 28, 2014

mitsuhiko commented Oct 28, 2014

pcwalton commented Nov 4, 2014

pcwalton commented Nov 4, 2014

pcwalton commented Nov 11, 2014

nikomatsakis commented Nov 11, 2014

tamird commented Dec 1, 2014

nikomatsakis commented Dec 4, 2014

alexcrichton commented Dec 8, 2014

librustc: Make `Copy` opt-in. #17864

librustc: Make `Copy` opt-in. #17864