Apply clippy::clone_on_copy throughout stacks core

[jferrant]

No description provided.

[kantai]

I think I would prefer deleting the Copy implementations for many of these types.

[jbencin]

@kantai I assume you're referring mostly to 32 byte hash/key types, right? Do we want to establish an upper size limit for what types are allowed to implement Copy? Maybe anything bigger than u128 should not implement Copy to encourage passing by reference instead?

[kantai]

@kantai I assume you're referring mostly to 32 byte hash/key types, right? Do we want to establish an upper size limit for what types are allowed to implement Copy? Maybe anything bigger than u128 should not implement Copy to encourage passing by reference instead?

Yeah -- I think something along this kind of patch at least:

diff --git a/stacks-common/src/util/macros.rs b/stacks-common/src/util/macros.rs
index 4e332179e6..5a3a873410 100644
--- a/stacks-common/src/util/macros.rs
+++ b/stacks-common/src/util/macros.rs
@@ -414,8 +414,6 @@ macro_rules! impl_array_newtype {
             }
         }
 
-        impl Copy for $thing {}
-
         impl ::std::hash::Hash for $thing {
             #[inline]
             fn hash<H>(&self, state: &mut H)
diff --git a/stacks-common/src/util/secp256k1.rs b/stacks-common/src/util/secp256k1.rs
index e569a8ba0d..ca60a622fe 100644
--- a/stacks-common/src/util/secp256k1.rs
+++ b/stacks-common/src/util/secp256k1.rs
@@ -35,7 +35,7 @@ use crate::util::hash::{hex_bytes, to_hex};
 // per-thread Secp256k1 context
 thread_local!(static _secp256k1: Secp256k1<secp256k1::All> = Secp256k1::new());
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize, Hash)]
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Hash)]
 pub struct Secp256k1PublicKey {
     // serde is broken for secp256k1, so do it ourselves
     #[serde(
@@ -46,7 +46,7 @@ pub struct Secp256k1PublicKey {
     compressed: bool,
 }
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)]
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)]
 pub struct Secp256k1PrivateKey {
     // serde is broken for secp256k1, so do it ourselves
     #[serde(

I am little hesitant to establish an upper size limit: for my part, I kind of think Copy should be avoided almost whenever possible.

[jbencin]

I kind of think Copy should be avoided almost whenever possible

Copy is great for small types, it lets you make assignments without the extra noise of clone() or &s in your code. The standard library implements Copy for all primitive types, but also for small-to-medium sized structs/enums like Duration and SocketAddrV6. Anything that's the size of usize or less doesn't benefit from passing by reference (because usize is the size of the pointer it needs to copy), so we should at least implement Copy for those types. I think it would be reasonable to limit Copy in our code to <= 128-bit types, curious to know what others think...

[obycode]

I would agree with @kantai here. I would default to only have Copy for simple types, things that would go into a register.

[jcnelson]

From the call: Consensus is to get rid of Copy implementations, since .clone() and Copy have the same behavior for these types.

[jferrant]

Gonna close and replace.