From c39bc3909d35fe992a04b5885355aaf1c592fbfb Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 26 Jul 2022 14:50:28 +0200 Subject: [PATCH 1/2] 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 | 92 +++++++++++++++++------------------- src/blockdata/transaction.rs | 6 +-- src/error.rs | 25 ++++++++++ src/internal_macros.rs | 2 +- src/lib.rs | 2 + src/parse.rs | 88 ++++++++++++++++++++++++++++++++++ src/util/address.rs | 4 +- 7 files changed, 165 insertions(+), 54 deletions(-) create mode 100644 src/error.rs create mode 100644 src/parse.rs diff --git a/src/blockdata/locktime.rs b/src/blockdata/locktime.rs index 440006b1..84427f88 100644 --- a/src/blockdata/locktime.rs +++ b/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 for u32 { } } -impl FromStr for PackedLockTime { - type Err = ParseIntError; +/// 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 from_str(s: &str) -> Result { - s.parse().map(PackedLockTime) + fn try_from(s: $from) -> Result { + parse::int(s).map($to $(:: $fn)?) + } + } + )* } } -impl TryFrom<&str> for PackedLockTime { - type Error = ParseIntError; +/// Implements `TryFrom<{&str, String, Box}> 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, $to $(, $fn)?); + + impl FromStr for $to { + type Err = ParseIntError; + + fn from_str(s: &str) -> Result { + parse::int(s).map($to $(:: $fn)?) + } + } - fn try_from(s: &str) -> Result { - PackedLockTime::from_str(s) } } -impl TryFrom for PackedLockTime { - type Error = ParseIntError; - - fn try_from(s: String) -> Result { - 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 { - s.parse().map(LockTime::from_consensus) - } -} - -impl TryFrom<&str> for LockTime { - type Error = ParseIntError; - - fn try_from(s: &str) -> Result { - LockTime::from_str(s) - } -} - -impl TryFrom for LockTime { - type Error = ParseIntError; - - fn try_from(s: String) -> Result { - LockTime::from_str(&s) - } -} +impl_tryfrom_str!(LockTime, from_consensus); impl From for LockTime { fn from(h: Height) -> Self { @@ -503,7 +491,7 @@ impl FromStr for Height { type Err = Error; fn from_str(s: &str) -> Result { - let n = s.parse::().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 { - Height::from_str(s) + let n = parse::int(s)?; + Height::from_consensus(n) } } @@ -520,7 +509,7 @@ impl TryFrom for Height { type Error = Error; fn try_from(s: String) -> Result { - let n = s.parse::().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 { - let n = s.parse::().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 { - Time::from_str(s) + let n = parse::int(s)?; + Time::from_consensus(n) } } @@ -602,7 +592,7 @@ impl TryFrom for Time { type Error = Error; fn try_from(s: String) -> Result { - let n = s.parse::().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 for Error { } } +impl From 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 { diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index c4cb4837..e4affe71 100644 --- a/src/blockdata/transaction.rs +++ b/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 { 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::("lol").unwrap_err()))); assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:42"), Ok(OutPoint{ diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 00000000..9a641fbe --- /dev/null +++ b/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) + } + } + }; +} diff --git a/src/internal_macros.rs b/src/internal_macros.rs index c617058a..ccce11a0 100644 --- a/src/internal_macros.rs +++ b/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")] { diff --git a/src/lib.rs b/src/lib.rs index fa44570a..a92c40cd 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/parse.rs b/src/parse.rs new file mode 100644 index 00000000..9f17880a --- /dev/null +++ b/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 for core::num::ParseIntError { + fn from(value: ParseIntError) -> Self { + value.source + } +} + +impl AsRef 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 + TryFrom + 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` which is not used later it's better to pass it as +/// owned since it avoids allocation in error case. +pub(crate) fn int + Into>(s: S) -> Result { + s.as_ref().parse().map_err(|error| { + ParseIntError { + input: s.into(), + bits: u8::try_from(core::mem::size_of::() * 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); diff --git a/src/util/address.rs b/src/util/address.rs index 52943aba..e73fb725 100644 --- a/src/util/address.rs +++ b/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 { - let version: u8 = s.parse().map_err(Error::UnparsableWitnessVersion)?; + let version: u8 = crate::parse::int(s).map_err(Error::UnparsableWitnessVersion)?; WitnessVersion::try_from(version) } } From 071a1c02b7318ec96edee7673eb7076f383985ee Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 27 Jul 2022 19:18:00 +0200 Subject: [PATCH 2/2] Implement string parsing for `Sequence` `Sequence` didn't have `FromStr` nor `TryFrom<{stringly type}>` implemented by accident. This moves a macro for implementing them from `locktime` module to the `parse` module, renames it for clarity and uses it to implement parsing for `Sequence`. --- src/blockdata/locktime.rs | 36 +++--------------------------------- src/blockdata/transaction.rs | 3 +++ src/parse.rs | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/blockdata/locktime.rs b/src/blockdata/locktime.rs index 84427f88..ebdae544 100644 --- a/src/blockdata/locktime.rs +++ b/src/blockdata/locktime.rs @@ -28,6 +28,7 @@ use crate::consensus::encode::{self, Decodable, Encodable}; use crate::io::{self, Read, Write}; use crate::prelude::*; use crate::internal_macros::write_err; +use crate::impl_parse_str_through_int; /// The Threshold for deciding whether a lock time value is a height or a time (see [Bitcoin Core]). /// @@ -135,38 +136,7 @@ impl From for u32 { } } -/// 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 { - parse::int(s).map($to $(:: $fn)?) - } - } - )* - } -} - -/// Implements `TryFrom<{&str, String, Box}> 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, $to $(, $fn)?); - - impl FromStr for $to { - type Err = ParseIntError; - - fn from_str(s: &str) -> Result { - parse::int(s).map($to $(:: $fn)?) - } - } - - } -} - -impl_tryfrom_str!(PackedLockTime); +impl_parse_str_through_int!(PackedLockTime); impl fmt::LowerHex for PackedLockTime { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { @@ -376,7 +346,7 @@ impl LockTime { } } -impl_tryfrom_str!(LockTime, from_consensus); +impl_parse_str_through_int!(LockTime, from_consensus); impl From for LockTime { fn from(h: Height) -> Self { diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index e4affe71..5ffa542d 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -32,6 +32,7 @@ use crate::hash_types::{Sighash, Txid, Wtxid}; use crate::VarInt; use crate::util::sighash::UINT256_ONE; use crate::internal_macros::{impl_consensus_encoding, serde_string_impl, serde_struct_human_string_impl, write_err}; +use crate::impl_parse_str_through_int; #[cfg(doc)] use crate::util::sighash::SchnorrSighashType; @@ -422,6 +423,8 @@ impl fmt::Display for RelativeLockTimeError { } } +impl_parse_str_through_int!(Sequence); + #[cfg(feature = "std")] #[cfg_attr(docsrs, doc(cfg(feature = "std")))] impl std::error::Error for RelativeLockTimeError { diff --git a/src/parse.rs b/src/parse.rs index 9f17880a..54f82415 100644 --- a/src/parse.rs +++ b/src/parse.rs @@ -86,3 +86,38 @@ pub(crate) fn int + Into>(s: S) -> Result for $to` using `parse::int`, mapping the output using `fn` +#[macro_export] +macro_rules! impl_tryfrom_str_through_int_single { + ($($from:ty, $to:ident $(, $fn:ident)?);*) => { + $( + impl core::convert::TryFrom<$from> for $to { + type Error = $crate::error::ParseIntError; + + fn try_from(s: $from) -> Result { + $crate::parse::int(s).map($to $(:: $fn)?) + } + } + )* + } +} + +/// Implements `FromStr` and `TryFrom<{&str, String, Box}> for $to` using `parse::int`, mapping the output using `fn` +/// +/// The `Error` type is `ParseIntError` +#[macro_export] +macro_rules! impl_parse_str_through_int { + ($to:ident $(, $fn:ident)?) => { + $crate::impl_tryfrom_str_through_int_single!(&str, $to $(, $fn)?; String, $to $(, $fn)?; Box, $to $(, $fn)?); + + impl core::str::FromStr for $to { + type Err = $crate::error::ParseIntError; + + fn from_str(s: &str) -> Result { + $crate::parse::int(s).map($to $(:: $fn)?) + } + } + + } +}