Remove PartialEq impl from Error
#741
Conversation
We will want to start boxing our errors to avoid generic error types (and to reduce the size of error types, by collapsing multiple variants into one). In order to do this, we need a trait that we can box which expresses fmt::Debug and fmt::Display. (Ideally we would also bound on std::error::Error but we can't express this bound without std until Rust 1.81, and we can't have a std-only bound since it would make the std feature non-additive.) This IS A BREAKING CHANGE because it now requires all key parsing errors to have a 'static bound. But the alternatives are pretty bad: * We could continue dropping parsing errors and replacing them with Strings (or not replacing them at all..); or * We could add a <Pk> bound to all of our error types
In the next commit we will remove this.
This might annoy some people, but it will be necessary in order to hold generic pubkey error types inside of our error returns. Our alternatives are: * Implement some manual hacky notion of equality for pubkey error (either "never equal" or "equal based on comparing output of fmt::Display or something) * Add a PartialEq bound on all the <Pk as FromStr>::Err types, which would make it impossible to use anything that e.g. contained an io error. I don't think ether of these are good, and given that io::Error has "set the standard" for error types to have basically no useful trait bounds other than Debug and Display, we are okay to do this.
|
Interesting, I was wondering about |
|
Within reason, yes. |
| @@ -17,6 +17,11 @@ use core::{fmt, hash}; | |||
|
|
|||
| use crate::MiniscriptKey; | |||
|
|
|||
| /// Auxiliary trait indicating that a type implements both `Debug` and `Display`. | |||
There was a problem hiding this comment.
| /// Auxiliary trait indicating that a type implements both `Debug` and `Display`. | |
| /// Auxiliary trait indicating that a type implements `'static`, `Debug` and `Display`. |
There was a problem hiding this comment.
"implements 'static" isn't the right terminology ... if we want to say this would say it "is 'static".
But IMHO it's fine as-is. The name of the trait exactly describes what it does.
|
Then this seems like a reasonable step IMO. |
There was a problem hiding this comment.
ACK 57bbde8.
which to avoid a Pk parameter on Error, will need to be boxed
It will be super annoying to have Pk on Error. If we have to remove PartialEq for this; I think is it is worth it.
This will break downstream users; but hopefully the fix is easy for them. If we get strong complaints we implement some reasonable manual version of PartialEq and make a non-breaking point release.
This is a short PR in terms of LOC, but may be controversial. I think it is necessary though.
Currently we have
PartialEqon our giantErrorenum (though amusingly notEq) so that we can useassert_eqin unit tests. We can get away with this because our error enum does not contain anystd::io::Errors anywhere, which can't be compared, nor does it contain any boxed errors.In the next couple of PRs I am going to clean up the
expressionmodule to make the parser non-recursive and the error types strongly typed and detailed. To do this, I want to store<Pk as FromStr>::Errs, which to avoid aPkparameter onError, will need to be boxed. When we box the type we need to declare upfront what traits the error types have, and these traits will become requirements of theMiniscriptKeytype.It's not really reasonable to require
PartialEqorEqsince these are not common traits for errors. Really the only things we can reasonaby require areDebug,Displayand'static. (I would like to also requirestd::error::Errorbut as far as I can tell there is no way to do this while retaining both nostd compatibility and additive features. See this users.rust-lang.org thread about this).