Merge #741: Remove PartialEq impl from Error

57bbde8 remove PartialEq bound from top-level Error type (Andrew Poelstra)
3649850 tests: remove all uses of Eq on error types (Andrew Poelstra)
4f52fb7 introduce `StaticDebugAndDisplay` blanket trait, use it for all errors (Andrew Poelstra)

Pull request description:

  This is a short PR in terms of LOC, but may be controversial. I think it is necessary though.

  Currently we have `PartialEq` on our giant `Error` enum (though amusingly not `Eq`) so that we can use `assert_eq` in unit tests. We can get away with this because our error enum does not contain any `std::io::Error`s 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 `expression` module to make the parser non-recursive and the error types strongly typed and detailed. To do this, I want to store `<Pk as FromStr>::Err`s, which to avoid a `Pk` parameter on `Error`, 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 the `MiniscriptKey` type.

  It's not really reasonable to require `PartialEq` or `Eq` since these are not common traits for errors. Really the only things we *can* reasonaby require are `Debug`, `Display` and `'static`. (I would like to also require `std::error::Error` but 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](https://users.rust-lang.org/t/how-to-box-an-error-type-retaining-std-error-only-when-std-is-enabled/)).

ACKs for top commit:
  sanket1729:
    ACK 57bbde8.

Tree-SHA512: 58400af38e50da8ebee39ad8cb14be3532d801311d8afd2b3854446f89d22019d2e1711d58b971244aefd0f7685b50ca3f6c2da1bcbec95c49962306cbbcf014

Author: apoelstra

src/blanket_traits.rs

@@ -17,6 +17,11 @@ use core::{fmt, hash};
 
 use crate::MiniscriptKey;
 
+/// Auxiliary trait indicating that a type implements both `Debug` and `Display`.
+pub trait StaticDebugAndDisplay: fmt::Debug + fmt::Display + 'static {}
+
+impl<T: fmt::Debug + fmt::Display + 'static> StaticDebugAndDisplay for T {}
+
 /// Blanket trait describing a key where all associated types implement `FromStr`,
 /// and all `FromStr` errors can be displayed.
 pub trait FromStrKey:
@@ -36,7 +41,7 @@ pub trait FromStrKey:
         + fmt::Debug
         + hash::Hash;
     /// Dummy type. Do not use.
-    type _Sha256FromStrErr: fmt::Debug + fmt::Display;
+    type _Sha256FromStrErr: StaticDebugAndDisplay;
     /// Dummy type. Do not use.
     type _Hash256: FromStr<Err = Self::_Hash256FromStrErr>
         + Clone
@@ -46,7 +51,7 @@ pub trait FromStrKey:
         + fmt::Debug
         + hash::Hash;
     /// Dummy type. Do not use.
-    type _Hash256FromStrErr: fmt::Debug + fmt::Display;
+    type _Hash256FromStrErr: StaticDebugAndDisplay;
     /// Dummy type. Do not use.
     type _Ripemd160: FromStr<Err = Self::_Ripemd160FromStrErr>
         + Clone
@@ -56,7 +61,7 @@ pub trait FromStrKey:
         + fmt::Debug
         + hash::Hash;
     /// Dummy type. Do not use.
-    type _Ripemd160FromStrErr: fmt::Debug + fmt::Display;
+    type _Ripemd160FromStrErr: StaticDebugAndDisplay;
     /// Dummy type. Do not use.
     type _Hash160: FromStr<Err = Self::_Hash160FromStrErr>
         + Clone
@@ -66,9 +71,9 @@ pub trait FromStrKey:
         + fmt::Debug
         + hash::Hash;
     /// Dummy type. Do not use.
-    type _Hash160FromStrErr: fmt::Debug + fmt::Display;
+    type _Hash160FromStrErr: StaticDebugAndDisplay;
     /// Dummy type. Do not use.
-    type _FromStrErr: fmt::Debug + fmt::Display;
+    type _FromStrErr: StaticDebugAndDisplay;
 }
 
 impl<T> FromStrKey for T
@@ -78,11 +83,11 @@ where
     Self::Hash256: FromStr,
     Self::Ripemd160: FromStr,
     Self::Hash160: FromStr,
-    <Self as FromStr>::Err: fmt::Debug + fmt::Display,
-    <<Self as MiniscriptKey>::Sha256 as FromStr>::Err: fmt::Debug + fmt::Display,
-    <Self::Hash256 as FromStr>::Err: fmt::Debug + fmt::Display,
-    <Self::Ripemd160 as FromStr>::Err: fmt::Debug + fmt::Display,
-    <Self::Hash160 as FromStr>::Err: fmt::Debug + fmt::Display,
+    <Self as FromStr>::Err: StaticDebugAndDisplay,
+    <<Self as MiniscriptKey>::Sha256 as FromStr>::Err: StaticDebugAndDisplay,
+    <Self::Hash256 as FromStr>::Err: StaticDebugAndDisplay,
+    <Self::Ripemd160 as FromStr>::Err: StaticDebugAndDisplay,
+    <Self::Hash160 as FromStr>::Err: StaticDebugAndDisplay,
 {
     type _Sha256 = <T as MiniscriptKey>::Sha256;
     type _Sha256FromStrErr = <<T as MiniscriptKey>::Sha256 as FromStr>::Err;

src/lib.rs

@@ -409,7 +409,7 @@ pub trait ForEachKey<Pk: MiniscriptKey> {
 
 /// Miniscript
 
-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub enum Error {
     /// Opcode appeared which is not part of the script subset
     InvalidOpcode(Opcode),

src/miniscript/mod.rs

@@ -1472,13 +1472,16 @@ mod tests {
     fn duplicate_keys() {
         // You cannot parse a Miniscript that has duplicate keys
         let err = Miniscript::<String, Segwitv0>::from_str("and_v(v:pk(A),pk(A))").unwrap_err();
-        assert_eq!(err, Error::AnalysisError(crate::AnalysisError::RepeatedPubkeys));
+        assert!(matches!(err, Error::AnalysisError(crate::AnalysisError::RepeatedPubkeys)));
 
         // ...though you can parse one with from_str_insane
         let ok_insane =
             Miniscript::<String, Segwitv0>::from_str_insane("and_v(v:pk(A),pk(A))").unwrap();
         // ...but this cannot be sanity checked.
-        assert_eq!(ok_insane.sanity_check().unwrap_err(), crate::AnalysisError::RepeatedPubkeys);
+        assert!(matches!(
+            ok_insane.sanity_check().unwrap_err(),
+            crate::AnalysisError::RepeatedPubkeys
+        ));
         // ...it can be lifted, though it's unclear whether this is a deliberate
         // choice or just an accident. It seems weird given that duplicate public
         // keys are forbidden in several other places.
@@ -1492,7 +1495,10 @@ mod tests {
             "and_v(v:and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))",
         )
         .unwrap_err();
-        assert_eq!(err, Error::AnalysisError(crate::AnalysisError::HeightTimelockCombination));
+        assert!(matches!(
+            err,
+            Error::AnalysisError(crate::AnalysisError::HeightTimelockCombination)
+        ));
 
         // Though you can in an or() rather than and()
         let ok_or = Miniscript::<String, Segwitv0>::from_str(
@@ -1512,10 +1518,10 @@ mod tests {
             ok_insane.sanity_check().unwrap_err(),
             crate::AnalysisError::HeightTimelockCombination
         );
-        assert_eq!(
+        assert!(matches!(
             ok_insane.lift().unwrap_err(),
             Error::LiftError(crate::policy::LiftError::HeightTimelockCombination)
-        );
+        ));
     }
 
     #[test]