Improve errors #601
Improve errors #601
Conversation
Now that we have `matches` because of the recent MSRV bump we can ergonomically test error types without using `PartialEq`/`Eq`. Remove all derives from error types except `Debug`.
Do trivial refactor to reduce the clutter in the `source` method. Refactor only, no logic changes.
| assert_eq!(full.recover_ecdsa(&msg, &sigr), Ok(pk)); | ||
| let vrfy_res = vrfy.recover_ecdsa(&msg, &sigr); | ||
| let full_res = full.recover_ecdsa(&msg, &sigr); | ||
| assert!(matches!(vrfy_res, full_res)); |
There was a problem hiding this comment.
In 645238f:
This is not correct -- you can't use matches! on two variables like this; the result is to bind vrfy_res to full_res and then check that the binding succeeded (which it always will because there are no further restrictions). See https://stackoverflow.com/questions/72537643/apparent-unused-variable-in-match-statement
In general I'm a bit suspicious of these allow(unused_variables) lines because I think they're masking similar mistakes .... and in retrospect we should revisit what's going on in rust-bitcoin with these.
There was a problem hiding this comment.
ok, I need to read the matches code then before using it willy-nilly. Just had a thought, perhaps we could do this on error types
#[derive(Debug)]
#[non_exhaustive]
#[allow(missing_copy_implementations)]
#[cfg_attr(test, derive(Copy, PartialEq, Eq, Clone))]Then all the test changes in this PR are not required and matches! use goes away.
There was a problem hiding this comment.
Why are we removing PartialEq in secp256k1? The biggest reason to remove it is that we may add std::io::Error inside one day but I don't expect this to happen in this crate. Same with Clone. I'd avoid Copy since adding something that allocates is quite plausible.
There was a problem hiding this comment.
Ah, that's a good point -- I had half-forgotten that the reason for "error types don't impl any traits" is that io::Error tends to poison our types. But you're right that we're extremely unlikely to ever have I/O errors from this pure-math crate.
There was a problem hiding this comment.
Oh, I thought that the "great error cleanup", across the org, meant that all errors should only derive Debug? Are you guys saying that for every error type I/we have to think about what traits to derive? That sounds like unnecessary work, can't we just decide on a minimum set and use that (which is what I thought we did and how we got to only deriving Debug)?
There was a problem hiding this comment.
Just lazy typing by me, I should have written PartialEq/Eq :)
There was a problem hiding this comment.
oh I didn't type it, lazy typing by @Kixunil (I assume) - double smiley face.
There was a problem hiding this comment.
Not necessarily. We may avoid promising Eq to avoid future breakage like what happened with LockTime. But I'm not entirely sure about this one, there's a good chance Eq is just fine.
There was a problem hiding this comment.
The LockTime breakage was about Ord. Having a type that is PartialEq but not Eq is a pretty obscure use-case, and I'm doubtful that crypto errors will meet it.
There was a problem hiding this comment.
Indeed, I'm very open to having Eq.
|
Could be closed, setting as draft so I don't loose the discussion and can re-visit what to do with errors in secp (if anything). |
|
This will be redundant after #635 |
Patch 1: Remove derives from error types
Patch 2: Trivial refactor