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

feat: derive Copy trait for messages where possible #950

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

caspermeijn
Copy link
Collaborator

@caspermeijn caspermeijn commented Nov 24, 2023

Rust primitive types can be copied by simply copying the bits. Rust structs can also have this property by deriving the Copy trait.

Automatically derive Copy for:

  • messages that only have fields with primitive types
  • the Rust enum for one-of fields
  • messages whose field type are messages that also implement Copy

Generated code for Protobuf enums already derives Copy.

@caspermeijn caspermeijn marked this pull request as draft November 24, 2023 15:26
@caspermeijn caspermeijn changed the title WIP: feat: derive Copy trait for messages where possible feat: derive Copy trait for messages where possible Feb 29, 2024
@caspermeijn caspermeijn marked this pull request as ready for review February 29, 2024 10:09
@gibbz00
Copy link
Contributor

gibbz00 commented Apr 19, 2024

Would it be possible to make this configurable? I personally like to avoid automatic Copy derives to avoid implicit cloning.

@caspermeijn
Copy link
Collaborator Author

Would it be possible to make this configurable? I personally like to avoid automatic Copy derives to avoid implicit cloning.

What do you mean with implicit cloning? The trait Copy means that the bytes can simply be copied to create a second object.

The docs says: When should my type be Copy? Generally speaking, if your type can implement Copy, it should.

https://doc.rust-lang.org/std/marker/trait.Copy.html

@gibbz00
Copy link
Contributor

gibbz00 commented Apr 19, 2024

This example is perhaps trivial, but my experience is that these things can become really tricky to pin down in large projects.

fn main() {
    let copyable = 1;
    read(&copyable);
}

fn read(x: &u32) {
    update(*x); // implicit clone
    dbg!(x); // 1
}

fn update(mut x: u32) {
    x += 1;
    dbg!(x); // 2
}

@caspermeijn
Copy link
Collaborator Author

update(*x); // implicit clone

I would not call that an implicit clone, as you explicitly deref x. With a deref, you choose to copy the value from behind the reference to a local variable. So with * you choose to copy, am I right?

https://micouy.github.io/rust-dereferencing/

@gibbz00
Copy link
Contributor

gibbz00 commented Apr 19, 2024

Well things get a bit tricky because derefmut and deref both use the same symbol *.

Sure, my example was maybe not the best. I'm also just playing a bit of devil's advocate.

There's still the basic case of implicit cloning mentioned in the link you posted above:

#[derive(Copy, Clone)]
struct GiantStruct(u32);

fn main() {
    let copyable0 = GiantStruct(1);
    let copyable1 = copyable0;
}

@caspermeijn
Copy link
Collaborator Author

There's still the basic case of implicit cloning mentioned in the link you posted above:

Well, that is how the language is supposed to work, right? If you want two variables pointing to the same instance, then you want to take a reference: let copyable1 = &copyable0;.

Sorry, but I don't understand in what situation you don't want an impl Copy for your types. From my perspective, it is always useful.

@gibbz00
Copy link
Contributor

gibbz00 commented Apr 20, 2024

You're probably right. I usually want the compiler to tell me that I must copy things if that's what I'm trying to do.

The Rust API Guidelines book also recommends implementing Copy for public types: https://rust-lang.github.io/api-guidelines/interoperability.html?highlight=copy#types-eagerly-implement-common-traits-c-common-traits

Rust primitive types can be copied by simply copying the bits. Rust structs can also have this property by deriving the Copy trait.

Automatically derive Copy for:
- messages that only have fields with primitive types
- the Rust enum for one-of fields
- messages whose field type are messages that also implement Copy

Generated code for Protobuf enums already derives Copy.
@caspermeijn caspermeijn force-pushed the derive_copy branch 2 times, most recently from 449b016 to 93a39d8 Compare April 24, 2024 11:41
Clippy reports: warning: using `clone` on type `Timestamp` which implements the `Copy` trait
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work! I like this a lot. Do you think we should release this as part of a breaking change or not?

@caspermeijn
Copy link
Collaborator Author

Really nice work! I like this a lot. Do you think we should release this as part of a breaking change or not?

I agree this is a breaking change.

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

Successfully merging this pull request may close these issues.

None yet

3 participants