Skip to content

Copy: Clone has made dropck implementation unsound! #24895

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

Closed
pnkfelix opened this issue Apr 28, 2015 · 4 comments · Fixed by #24906
Closed

Copy: Clone has made dropck implementation unsound! #24895

pnkfelix opened this issue Apr 28, 2015 · 4 comments · Fixed by #24906
Labels
P-high High priority

Comments

@pnkfelix
Copy link
Member

Sample code (playpen):

#![allow(unused_variables, unused_assignments)]

struct D<T:Copy>(T, &'static str);

#[derive(Copy)]
struct S<'a>(&'a D<i32>, &'static str);
impl<'a> Clone for S<'a> {
    fn clone(&self) -> S<'a> {
        println!("cloning `S(_, {})` and thus accessing: {}", self.1, (self.0).0);
        S(self.0, self.1)
    }
}

impl<T:Copy> Drop for D<T> {
    fn drop(&mut self) {
        println!("calling Drop for {}", self.1);
        let _call = self.0.clone();
    }
}

fn main() {
    let (d2, d1);
    d1 = D(34, "d1");
    d2 = D(S(&d1, "inner"), "d2");
}

The Drop Check rule, as specified in RFC 769, requires that no methods be attached to the trait in question. The trait Copy used to qualify under that criteria, but with the addition of the Copy: Clone bound, it no longer does.

The original dropck implementation took a short-cut to enforcing the criteria (by only accepting the known builtin bounds, under the assumption that none had user-defined methods), and so now it is unsound.

@pnkfelix
Copy link
Member Author

To my mind, we should obviously fix the dropck implementation to check for methods properly. (I have a patch that does this; that's how I actually found this problem -- because the regression test I added was also checking that it still accepts Copy, but of course it shouldn't, by the reasoning above.)

The remaining question is: Do we continue to have Copy: Clone? Or does we remove that relationship, at least for the short term (i.e. remove it for the 1.0 release, with the expectation that we would put it back if we had some way of enforcing that the Clone implementation were in fact just the direct copy of the data).

@pnkfelix
Copy link
Member Author

cc @nikomatsakis

@pnkfelix
Copy link
Member Author

triage: P-high

@rust-highfive rust-highfive added the P-high High priority label Apr 28, 2015
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 28, 2015
[breaking-change]

What does this break?  Basically, code that implements `Drop` and is
using `T:Copy` for one of its type parameters and is relying on the
Drop Check rule not applying to it.

Here is an example:

```rust
#![allow(dead_code,unused_variables,unused_assignments)]
struct D<T:Copy>(T);
impl<T:Copy> Drop for D<T> { fn drop(&mut self) { } }

trait UserT { fn c(&self) { } }
impl<T:Copy> UserT for T { }
struct E<T:UserT>(T);
impl<T:UserT> Drop for E<T> { fn drop(&mut self) { } }

// This one will start breaking.
fn foo() { let (d2, d1); d1 = D(34); d2 = D(&d1); }

#[cfg(this_one_does_and_should_always_break)]
fn bar() { let (e2, e1); e1 = E(34); e2 = E(&e1); }

fn main() {
    foo();
}
```
bors added a commit that referenced this issue Apr 28, 2015
dropck: Remove `Copy` from special-cased traits

Fix #24895.

[breaking-change]

What does this break?  Basically, code that implements `Drop` and is
using `T:Copy` for one of its type parameters and is relying on the
Drop Check rule not applying to it.

Here is an example:

```rust
#![allow(dead_code,unused_variables,unused_assignments)]
struct D<T:Copy>(T);
impl<T:Copy> Drop for D<T> { fn drop(&mut self) { } }

trait UserT { fn c(&self) { } }
impl<T:Copy> UserT for T { }
struct E<T:UserT>(T);
impl<T:UserT> Drop for E<T> { fn drop(&mut self) { } }

// This one will start breaking.
fn foo() { let (d2, d1); d1 = D(34); d2 = D(&d1); }

#[cfg(this_one_does_and_should_always_break)]
fn bar() { let (e2, e1); e1 = E(34); e2 = E(&e1); }

fn main() {
    foo();
}
```
alexcrichton pushed a commit to alexcrichton/rust that referenced this issue Apr 30, 2015
[breaking-change]

What does this break?  Basically, code that implements `Drop` and is
using `T:Copy` for one of its type parameters and is relying on the
Drop Check rule not applying to it.

Here is an example:

```rust
#![allow(dead_code,unused_variables,unused_assignments)]
struct D<T:Copy>(T);
impl<T:Copy> Drop for D<T> { fn drop(&mut self) { } }

trait UserT { fn c(&self) { } }
impl<T:Copy> UserT for T { }
struct E<T:UserT>(T);
impl<T:UserT> Drop for E<T> { fn drop(&mut self) { } }

// This one will start breaking.
fn foo() { let (d2, d1); d1 = D(34); d2 = D(&d1); }

#[cfg(this_one_does_and_should_always_break)]
fn bar() { let (e2, e1); e1 = E(34); e2 = E(&e1); }

fn main() {
    foo();
}
```
@RalfJung
Copy link
Member

RalfJung commented May 9, 2015

Wow, that's surprising - I expected it to be illegal to have a type that is Copy, and has an explicit clone. I mean, is there any usecase for a type where the implicit copies performed by the compiler are different from the explicit copies created by calling clone? I would consider that very surprising, unexpected behavior. Not even C++ allows this: Either you overload the copy constructor, and it is used in all places, or you don't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants