Extend `ParseIntError` to carry more context

When debugging parsing errors it's very useful to know some context:
what the input was and what integer type was parsed. `ParseIntError`
from `core` doesn't contain this information.

In this commit a custom `ParseIntError` type is crated that carries the
one from `core` as well as additional information. Its `Display`
implementation displays this additional information as a well-formed
English sentence to aid users with understanding the problem. A helper
function parses any integer type from common string types returning the
new `ParseIntError` type on error.

To clean up the error code a bit some new macros are added and used.
New modules are added to organize the types, functions and macros.

Closes #1113

src/blockdata/locktime.rs

@@ -21,7 +21,8 @@ use core::{mem, fmt};
 use core::cmp::{PartialOrd, Ordering};
 use core::convert::TryFrom;
 use core::str::FromStr;
-use core::num::ParseIntError;
+use crate::error::ParseIntError;
+use crate::parse;
 
 use crate::consensus::encode::{self, Decodable, Encodable};
 use crate::io::{self, Read, Write};
@@ -134,29 +135,38 @@ impl From<PackedLockTime> for u32 {
     }
 }
 
-impl FromStr for PackedLockTime {
+/// Implements `TryFrom<$from> for $to` using `parse::int`, mapping the output using `fn`
+macro_rules! impl_tryfrom_str_single {
+    ($($from:ty, $to:ident $(, $fn:ident)?);*) => {
+        $(
+        impl TryFrom<$from> for $to {
+            type Error = ParseIntError;
+
+            fn try_from(s: $from) -> Result<Self, Self::Error> {
+                parse::int(s).map($to $(:: $fn)?)
+            }
+        }
+        )*
+    }
+}
+
+/// Implements `TryFrom<{&str, String, Box<str>}> for $to` using `parse::int`, mapping the output using `fn`
+macro_rules! impl_tryfrom_str {
+    ($to:ident $(, $fn:ident)?) => {
+        impl_tryfrom_str_single!(&str, $to $(, $fn)?; String, $to $(, $fn)?; Box<str>, $to $(, $fn)?);
+
+        impl FromStr for $to {
             type Err = ParseIntError;
 
             fn from_str(s: &str) -> Result<Self, Self::Err> {
-        s.parse().map(PackedLockTime)
+                parse::int(s).map($to $(:: $fn)?)
             }
         }
 
-impl TryFrom<&str> for PackedLockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: &str) -> Result<Self, Self::Error> {
-        PackedLockTime::from_str(s)
     }
 }
 
-impl TryFrom<String> for PackedLockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: String) -> Result<Self, Self::Error> {
-        PackedLockTime::from_str(&s)
-    }
-}
+impl_tryfrom_str!(PackedLockTime);
 
 impl fmt::LowerHex for PackedLockTime {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -366,29 +376,7 @@ impl LockTime {
     }
 }
 
-impl FromStr for LockTime {
-    type Err = ParseIntError;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        s.parse().map(LockTime::from_consensus)
-    }
-}
-
-impl TryFrom<&str> for LockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: &str) -> Result<Self, Self::Error> {
-        LockTime::from_str(s)
-    }
-}
-
-impl TryFrom<String> for LockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: String) -> Result<Self, Self::Error> {
-        LockTime::from_str(&s)
-    }
-}
+impl_tryfrom_str!(LockTime, from_consensus);
 
 impl From<Height> for LockTime {
     fn from(h: Height) -> Self {
@@ -503,7 +491,7 @@ impl FromStr for Height {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s.to_owned()))?;
+        let n = parse::int(s)?;
         Height::from_consensus(n)
     }
 }
@@ -512,7 +500,8 @@ impl TryFrom<&str> for Height {
     type Error = Error;
 
     fn try_from(s: &str) -> Result<Self, Self::Error> {
-        Height::from_str(s)
+        let n = parse::int(s)?;
+        Height::from_consensus(n)
     }
 }
 
@@ -520,7 +509,7 @@ impl TryFrom<String> for Height {
     type Error = Error;
 
     fn try_from(s: String) -> Result<Self, Self::Error> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s))?;
+        let n = parse::int(s)?;
         Height::from_consensus(n)
     }
 }
@@ -585,7 +574,7 @@ impl FromStr for Time {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s.to_owned()))?;
+        let n = parse::int(s)?;
         Time::from_consensus(n)
     }
 }
@@ -594,7 +583,8 @@ impl TryFrom<&str> for Time {
     type Error = Error;
 
     fn try_from(s: &str) -> Result<Self, Self::Error> {
-        Time::from_str(s)
+        let n = parse::int(s)?;
+        Time::from_consensus(n)
     }
 }
 
@@ -602,7 +592,7 @@ impl TryFrom<String> for Time {
     type Error = Error;
 
     fn try_from(s: String) -> Result<Self, Self::Error> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s))?;
+        let n = parse::int(s)?;
         Time::from_consensus(n)
     }
 }
@@ -626,7 +616,7 @@ pub enum Error {
     /// An error occurred while operating on lock times.
     Operation(OperationError),
     /// An error occurred while parsing a string into an `u32`.
-    Parse(ParseIntError, String),
+    Parse(ParseIntError),
 }
 
 impl fmt::Display for Error {
@@ -636,7 +626,7 @@ impl fmt::Display for Error {
         match *self {
             Conversion(ref e) => write_err!(f, "error converting lock time value"; e),
             Operation(ref e) => write_err!(f, "error during lock time operation"; e),
-            Parse(ref e, ref s) => write_err!(f, "error parsing string: {}", s; e),
+            Parse(ref e) => write_err!(f, "failed to parse lock time from string"; e),
         }
     }
 }
@@ -650,7 +640,7 @@ impl std::error::Error for Error {
         match *self {
             Conversion(ref e) => Some(e),
             Operation(ref e) => Some(e),
-            Parse(ref e, _) => Some(e),
+            Parse(ref e) => Some(e),
         }
     }
 }
@@ -667,6 +657,12 @@ impl From<OperationError> for Error {
     }
 }
 
+impl From<ParseIntError> for Error {
+    fn from(e: ParseIntError) -> Self {
+        Error::Parse(e)
+    }
+}
+
 /// An error that occurs when converting a `u32` to a lock time variant.
 #[derive(Debug, Clone, Eq, PartialEq, Hash)]
 pub struct ConversionError {

src/blockdata/transaction.rs

@@ -107,7 +107,7 @@ pub enum ParseOutPointError {
     /// Error in TXID part.
     Txid(hashes::hex::Error),
     /// Error in vout part.
-    Vout(::core::num::ParseIntError),
+    Vout(crate::error::ParseIntError),
     /// Error in general format.
     Format,
     /// Size exceeds max.
@@ -151,7 +151,7 @@ fn parse_vout(s: &str) -> Result<u32, ParseOutPointError> {
             return Err(ParseOutPointError::VoutNotCanonical);
         }
     }
-    s.parse().map_err(ParseOutPointError::Vout)
+    crate::parse::int(s).map_err(ParseOutPointError::Vout)
 }
 
 impl core::str::FromStr for OutPoint {
@@ -1308,7 +1308,7 @@ mod tests {
         assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c945X:1"),
                    Err(ParseOutPointError::Txid(Txid::from_hex("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c945X").unwrap_err())));
         assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:lol"),
-                   Err(ParseOutPointError::Vout(u32::from_str("lol").unwrap_err())));
+                   Err(ParseOutPointError::Vout(crate::parse::int::<u32, _>("lol").unwrap_err())));
 
         assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:42"),
                    Ok(OutPoint{

src/error.rs

@@ -0,0 +1,25 @@
+//! Contains error types and other error handling tools.
+
+pub use crate::parse::ParseIntError;
+
+/// Impls std::error::Error for the specified type with appropriate attributes, possibly returning
+/// source.
+#[macro_export]
+macro_rules! impl_std_error {
+    // No source available
+    ($type:ty) => {
+        #[cfg(feature = "std")]
+        #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
+        impl std::error::Error for $type {}
+    };
+    // Struct with $field as source
+    ($type:ty, $field:ident) => {
+        #[cfg(feature = "std")]
+        #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
+        impl std::error::Error for $type {
+            fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+                Some(&self.$field)
+            }
+        }
+    };
+}

src/internal_macros.rs

@@ -581,7 +581,7 @@ pub(crate) use user_enum;
 /// because `e.source()` is only available in std builds, without this macro the error source is
 /// lost for no-std builds.
 macro_rules! write_err {
-    ($writer:expr, $string:literal $(, $args:expr),*; $source:expr) => {
+    ($writer:expr, $string:literal $(, $args:expr)*; $source:expr) => {
         {
             #[cfg(feature = "std")]
             {

src/lib.rs

@@ -78,11 +78,13 @@ mod test_macros;
 mod internal_macros;
 #[cfg(feature = "serde")]
 mod serde_utils;
+mod parse;
 
 #[macro_use]
 pub mod network;
 pub mod blockdata;
 pub mod consensus;
+pub mod error;
 pub mod hash_types;
 pub mod policy;
 pub mod util;

src/parse.rs

@@ -0,0 +1,88 @@
+use crate::internal_macros::write_err;
+use crate::impl_std_error;
+use core::fmt;
+use core::str::FromStr;
+use core::convert::TryFrom;
+use crate::prelude::*;
+
+/// Error with rich context returned when a string can't be parsed as an integer.
+///
+/// This is an extension of [`core::num::ParseIntError`], which carries the input that failed to
+/// parse as well as type information. As a result it provides very informative error messages that
+/// make it easier to understand the problem and correct mistakes.
+///
+/// Note that this is larger than the type from `core` so if it's passed through a deep call stack
+/// in a performance-critical application you may want to box it or throw away the context by
+/// converting to `core` type.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct ParseIntError {
+    input: String,
+    // for displaying - see Display impl with nice error message below
+    bits: u8,
+    // We could represent this as a single bit but it wouldn't actually derease the cost of moving
+    // the struct because String contains pointers so there will be padding of bits at least
+    // pointer_size - 1 bytes: min 1B in practice.
+    is_signed: bool,
+    source: core::num::ParseIntError,
+}
+
+impl ParseIntError {
+    /// Returns the input that was attempted to be parsed.
+    pub fn input(&self) -> &str {
+        &self.input
+    }
+}
+
+impl From<ParseIntError> for core::num::ParseIntError {
+    fn from(value: ParseIntError) -> Self {
+        value.source
+    }
+}
+
+impl AsRef<core::num::ParseIntError> for ParseIntError {
+    fn as_ref(&self) -> &core::num::ParseIntError {
+        &self.source
+    }
+}
+
+impl fmt::Display for ParseIntError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let signed = if self.is_signed { "signed" } else { "unsigned" };
+        let n = if self.bits == 8 { "n" } else { "" };
+        write_err!(f, "failed to parse '{}' as a{} {}-bit {} integer", self.input, n, self.bits, signed; self.source)
+    }
+}
+
+/// Not strictly neccessary but serves as a lint - avoids weird behavior if someone accidentally
+/// passes non-integer to the `parse()` function.
+pub(crate) trait Integer: FromStr<Err=core::num::ParseIntError> + TryFrom<i8> + Sized {}
+
+macro_rules! impl_integer {
+    ($($type:ty),* $(,)?) => {
+        $(
+        impl Integer for $type {}
+        )*
+    }
+}
+
+impl_integer!(u8, i8, u16, i16, u32, i32, u64, i64, u128, i128);
+
+/// Parses the input string as an integer returning an error carrying rich context.
+///
+/// If the caller owns `String` or `Box<str>` which is not used later it's better to pass it as
+/// owned since it avoids allocation in error case.
+pub(crate) fn int<T: Integer, S: AsRef<str> + Into<String>>(s: S) -> Result<T, ParseIntError> {
+    s.as_ref().parse().map_err(|error| {
+        ParseIntError {
+            input: s.into(),
+            bits: u8::try_from(core::mem::size_of::<T>() * 8).expect("max is 128 bits for u128"),
+            // We detect if the type is signed by checking if -1 can be represented by it
+            // this way we don't have to implement special traits and optimizer will get rid of the
+            // computation.
+            is_signed: T::try_from(-1i8).is_ok(),
+            source: error,
+        }
+    })
+}
+
+impl_std_error!(ParseIntError, source);

src/util/address.rs

@@ -26,7 +26,7 @@ use crate::prelude::*;
 
 use core::convert::TryFrom;
 use core::fmt;
-use core::num::ParseIntError;
+use crate::error::ParseIntError;
 use core::str::FromStr;
 
 use secp256k1::{Secp256k1, Verification, XOnlyPublicKey};
@@ -239,7 +239,7 @@ impl FromStr for WitnessVersion {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let version: u8 = s.parse().map_err(Error::UnparsableWitnessVersion)?;
+        let version: u8 = crate::parse::int(s).map_err(Error::UnparsableWitnessVersion)?;
         WitnessVersion::try_from(version)
     }
 }