tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.92k stars 504 forks source link

feat: derive Copy trait for messages where possible #950

Closed caspermeijn closed 5 months ago

caspermeijn commented 11 months ago

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:

Generated code for Protobuf enums already derives Copy.

gibbz00 commented 6 months ago

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

caspermeijn commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago
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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

caspermeijn commented 6 months ago

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.