Define and enforce standard derives

[sgeisler]

Many standard traits can be easily derived but are often forgotten. In #511 the discussion of what to derive arose for Error types in particular. We should define a standard set of traits to implement or derive for every struct/enum to make them interoperable with algorithms and data structures from std. Ideally we could enforce that via CI too, either with some heuristic using regex or something fancy like rustql (rust-corpus/rustql#6).

[apoelstra]

I think this is a good idea. Interesting idea to use rustsq. I like it. We could enforce a lot of things in CI with that.

[sanket1729]

Have we decided on what the standard trait derives should be?
For starters, I think we should have the following

#[derive] attributes

Comparison traits: Eq, PartialEq.
Clone, to create T from &T via a copy.
Hash, to compute a hash from &T.
Debug, to format a value using the {:?} formatter.

And additionally Display and Error.

I can go either ways on Ord.

[apoelstra]

Regarding Hash, I think that in bitcoin_hashes we should impl this more intelligently for hashtypes than just #deriving it. e.g. by taking a wrapping sum of the 64-bit words or something.

I like that list. We should additionally impl Copy everywhere we can, except iterators.

I think we should impl PartialOrd and Ord everywhere. It causes pain otherwise when using BTreeSets or when trying to sort vectors of composite objects.

[dr-orlovsky]

Agree with @apoelstra and the original @sanket1729 list. Just would like to ask to extend it with Default and Copy where possible. So the list is the following:

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)]

I do paste thisline before each struct and enum and then remove those derives which does not compile (after some investigation of what prevents from derivation and whether this can be changed). I assume this is what Rust API Guidelines call opportunistic derivation of types. Propose to do the same with all types in rust bitcoin, hashes and secp256k1 (+invoices, miniscript etc)

For errors this must be always accompanied with Display and std::error::Error implementation.

Also, guidelines highly recommend to do types Sync and Send whenever possible, but I am not doing that usually (but probably should)

[thomaseizinger]

Also, guidelines highly recommend to do types Sync and Send whenever possible, but I am not doing that usually (but probably should)

Send and Sync are automatically implemented by the compiler if appropriate. Manually implementing them is actually unsafe and usually not required unless you are building low-level synchronization primitives like locks.

Just would like to ask to extend it with Default and Copy where possible.

Agree with Copy.
Default is kind of a double-edged sword. I usually only implement Default if it makes sense semantically. Amount could implement Default by means of Amount::ZERO. But what would Default on Transaction mean?
However, thinking about it, it is already possible to construct instances of Transaction that are completely empty so I guess making that easier by deriving Default doesn't do any harm.

I would only vote against Default derives on types that otherwise enforce invariants in their constructors like Address.

[dr-orlovsky]

But what would Default on Transaction mean?

It is implemented today exactly as you described. Generally seems Default stands not for semantically default value, but for an "empty" value, if we look into rust API. Anyway, default is recommended to eagerly implement: https://rust-lang.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits

Send and Sync are automatically implemented by the compiler if appropriate. Manually implementing them is actually unsafe and usually not required unless you are building low-level synchronization primitives like locks.

Clear, was wrong reading API guidelines - overlooked phrase "automatically by compiler"

[sgeisler]

I implemented a proof-of-concept query with qrates that should get us all traits for all types. For example the output for my currently checked-out rust-bitcoin looks like that. We'd probably need to filter out private types (although that would require a way to deal with public re-exports afaik) and should check if it actually catches all types.

[sgeisler]

@apoelstra I'd be interested in the rationale behind not implementing Copy for iterators. Cloning them is definitely valid in some situations. And a copy only seems indicate that the duplication can happen by directly copying bytes instead of doing anything fancy. Is it the added magic of automatically making copies when it's necessary that puts you off?

[Kixunil]

I'd be careful about implementing too many traits. There are cases where they don't make sense or are harmful. (e.g. There's a reason f64 doesn't implement Eq.) I was bitten by this at least once. Enforcing traits seems too much. If you want to enforce anything then maybe make it so that people can somehow mark a trait as "I didn't forget, just don't think it makes sense".

As an example where I don't see a clear implementation of a trait is PartialOrd on Transaction.

A safer approach would be to assign each type a category and then make sure to implement appropriate traits. E.g. identifiers should definitely impl Eq, PartialEq, Hash, Ord. Unambiguous collection should implement IntoIterator... ("Unambiguous" is very important. Without looking at anything else, do you instinctively perceive std::path::Path as a collection? If yes, then collection of what?)

@sgeisler my understanding is that people often want to change the state of iterator (using &mut). Copy is a footgun as it can cause the iterator to silently not change its state but state of copy instead.

[sgeisler]

If you want to enforce anything then maybe make it so that people can somehow mark a trait as "I didn't forget, just don't think it makes sense".

That was always a hard requirement in my mind, I should have written that. I still think that some form of automated reminder is useful to not forget to derive/impl stuff (which often happens to me when I just try to solve a certain problem and later I'm annoyed because I need to patch in some derive). It's probably best to first agree on a common nice-to-have set of traits and then add exceptions to the rule as we need them. CI enforcement in a nice-to-have thing here, but can come at a later point.

[Kixunil]

If this is added to CI as informational only (not block merges) then I'd agree. Same for some questionable clippy lints. (Didn't check which are turned on here but I remember facepalming about some ridiculous instances elsewhere.)

[apoelstra]

@sgeisler the "no Copy for iterators" thing is not mine, this is also true of the standard library. None of the iterators impl Copy. And yes, it is because it makes it far too easy to accidentally iterate on a copy, which does not mutate the original iterator and will lead to surprising double-iteration.

[stevenroose]

Just my 2 cents: I think it makes sense to keep a sort of best-effort list of our target trait support in the CONTRIBUTING.md, do a single pass over all types now and from now on just try to keep the list in mind when adding/accepting new types. But I'd definitely recommend to not just blindly auto-derive where possible or so because often the traits have meaning that is not correctly conveyed by just deriving them. Like Default is indeed a very tricky one in most occasions.

[dr-orlovsky]

Proposed a version which looks like a consensus from the discussion above here: c4147f6 (this is a part of larger PR #587 proposing CONTRIBUTING.md)

[dr-orlovsky]

Not yet closed because of #560