paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
290 stars 218 forks source link

fixed-hash: Derive PartialEq and Eq instead of implementing them manually #742

Closed fhartwig closed 1 year ago

fhartwig commented 1 year ago

The motivation for this is that Rust only lets you use named constants (as opposed to literals) for pattern matching if the value's type has derived PartialEq and Eq implementations. So trying to compile the following code:

use ethereum_types::H160;

const ONE: H160 = H160::repeat_byte(1);;
const TWO: H160 = H160::repeat_byte(2);

fn main() {
    let x = H160::repeat_byte(3);
    match x {
        ONE => println!("it's a one!"),
        TWO => println!("it's a two!"),
        _ => println!("something else"),
    }
}

currently results in the following compilation error:

error: to use a constant of type `H160` in a pattern, `H160` must be annotated with `#[derive(PartialEq, Eq)]`
 --> src/main.rs:9:9
  |
9 |         ONE => println!("it's a one!"),
  |         ^^^

error: to use a constant of type `H160` in a pattern, `H160` must be annotated with `#[derive(PartialEq, Eq)]`
  --> src/main.rs:10:9
   |
10 |         TWO => println!("it's a two!"),
   |         ^^^

As far as I can tell, the existing explicit PartialEq implementation exists for historical reasons and isn't necessary any more, the derived implementation should be equivalent. Assuming that is true, the only potential drawback of this change that I can see is that it poses a backwards compatibility hazard, i.e. if in the future you would want to manually implement PartialEq again for some reason, doing so would break the code of anyone using e.g. named H160 constants in pattern matches.

fhartwig commented 1 year ago

Sure, done!

ordian commented 1 year ago

Thanks! Only cargo +nightly fmt is missing: https://github.com/paritytech/parity-common/actions/runs/4769435148/jobs/8484483754?pr=742

fhartwig commented 1 year ago

Sorry about that, fixed.