Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Derive Hash while manually implementing PartialEq #381

Closed
tcharding opened this issue May 2, 2022 · 1 comment · Fixed by #402
Closed

Derive Hash while manually implementing PartialEq #381

tcharding opened this issue May 2, 2022 · 1 comment · Fixed by #402

Comments

@tcharding
Copy link
Member

For the Miniscript struct

#[derive(Clone, Hash)]
pub struct Miniscript<Pk: MiniscriptKey, Ctx: ScriptContext> {

Clippy warns us that we are deriving Hash and using a custom implementation of PartialEq, this is likely wrong as explained in the lint docs.

Here is the clippy warning

error: you are deriving `Hash` but have implemented `PartialEq` explicitly
  --> src/miniscript/mod.rs:60:17
   |
60 | #[derive(Clone, Hash)]
   |                 ^^^^
   |
   = note: `#[deny(clippy::derive_hash_xor_eq)]` on by default
note: `PartialEq` implemented here
  --> src/miniscript/mod.rs:93:1
   |
93 | / impl<Pk: MiniscriptKey, Ctx: ScriptContext> PartialEq for Miniscript<Pk, Ctx> {
94 | |     fn eq(&self, other: &Miniscript<Pk, Ctx>) -> bool {
95 | |         self.node.eq(&other.node)
96 | |     }
97 | | }
   | |_^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
   = note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)
@apoelstra
Copy link
Member

Nice, good catch.

It's probably okay here since (in theory) self.node implies all the other fields but if we're going to make that explicit for PartialEq we should do the same for Hash as well.

sanket1729 added a commit that referenced this issue May 15, 2022
4ac68c9 Implement hash::Hash for Miniscript (Tobin C. Harding)

Pull request description:

  We manually implement a bunch of traits on `Miniscript` that pass through to the `node` field (e.g. `PartialEq`). We should do the same for `hash::Hash` instead of deriving it.

  Found by clippy.

  Fixes: #381

ACKs for top commit:
  apoelstra:
    ACK 4ac68c9

Tree-SHA512: 7920481034bee12fbfdbdfac2f38e9598e25a13a8a89b6b50cc7f63ce528c2de47e959ad073ac1c8f1ab64d4b0e432d264a5caf74ddb656c1a0ffbd8923faf6f
joemphilips pushed a commit to joemphilips/rust-miniscript that referenced this issue Dec 9, 2022
We manually implement a bunch of traits on `Miniscript` that pass
through to the `node` field (e.g. `PartialEq`). We should do the same
for `hash::Hash` instead of deriving it.

Found by clippy.

Fixes: rust-bitcoin#381
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants