Skip to content
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

[RFC] Split Copy trait in two traits: Pod and Copy #23016

Closed
wants to merge 8 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Mar 4, 2015

DO NOT MERGE NEEDS AN APPROVED RFC.

  • The Pod trait is used to mark types that can be cloned by simply copying its bytes on the stack.
  • The Copy trait is used to mark types that are implictly copyable.
  • Pod can only be derived by structs/enums whose fields are also Pod
  • Copy can only be derived by types that are Pod, i.e. trait Copy: Pod {}
  • To avoid a massive breakage, the #[derive(Copy)] syntax extension now derives both the Pod and Copy traits.
  • A #[derive(Pod)] syntax extension have been added to derive the Pod trait
  • Cell<T>'s T: Copy bound has been relaxed to T: Pod, this lets you use it with a wider variety of types like iterators, which are not implicitly copyable.

[breaking-change]s

  • If you were manually implementing the Copy trait, you'll have to manually implement Pod as well:
  impl<T> Copy for MyType<T> {}
+ impl<T> Pod for MyType<T> {}
  • To have #![derive(Copy)] expand into two items, it was necessary to change the unstable #[derive] API. This will likely break third-party #[derive] extensions.

I plon to finish writing a proper RFC by tomorrow, but here's the current draft.
RFC

cc @aturon @nikomatsakis @tbu-
cc #21846

@rust-highfive
Copy link
Collaborator

r? @kmcallister

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

@tbu-
Copy link
Contributor

tbu- commented Mar 4, 2015

@japaric Very nice work, thank you!

I haven't looked into the details, but do you also derive Clone for #[derive(Pod)] or #[derive(Copy)]?

@japaric
Copy link
Member Author

japaric commented Mar 4, 2015

I haven't looked into the details, but do you also derive Clone for #[derive(Pod)] or #[derive(Copy)]?

No, the plan was to do that in the type system with a blanket impl for Pod implementors:

impl Clone for T where T: Pod {
    fn clone(&self) -> T {
        // NB safe! this a memcpy of Pod data (no owned/refcounted/`drop`able data here)
        unsafe {
            mem::transmute_copy(self)
        }
    }
}

But! It can't be done right now because it needs negative bounds to handle some coherence problem (overlapping impls).

I think it may be possible to do it in syntax extension (have #[derive(Pod)] derive Pod and Clone, and have #[derive(Copy)] derive Pod, Clone and Copy), but I feel uncomfortable with that approach because a bug in the expansion may yield a struct that's not Pod (let's say the Clone impl missed a T: Pod, and now the struct is not Pod) but can be cloned via a memcpy (transmute_copy)! The blanket impl approach is immune to that class of bug.

@bors
Copy link
Contributor

bors commented Mar 12, 2015

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

@japaric
Copy link
Member Author

japaric commented Mar 20, 2015

Closing, since we are taking the lint route.

@japaric japaric closed this Mar 20, 2015
@aturon
Copy link
Member

aturon commented Mar 20, 2015

@japaric Thanks for pushing this along and raising the discussion!

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

Successfully merging this pull request may close these issues.

6 participants