From c986b2f620a8e411bb499db741c419691ae1f1ee Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 1 Nov 2024 16:10:16 +1100 Subject: [PATCH 1/5] internals: Move error.rs to error/mod.rs We use the `foo/mod.rs` style in this repo; `error.rs` is an anomaly, move it to `error/mod.rs`. --- internals/src/{error.rs => error/mod.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename internals/src/{error.rs => error/mod.rs} (100%) diff --git a/internals/src/error.rs b/internals/src/error/mod.rs similarity index 100% rename from internals/src/error.rs rename to internals/src/error/mod.rs From c90f4b60331f89750d530526c684212e6529662d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 1 Nov 2024 17:04:03 +1100 Subject: [PATCH 2/5] Fix bug in error output `ParseTimeError` should say "block time" not "block height". This looks like a cut'n pasta error because the `ParseHeightError` uses the same string. --- units/src/locktime/absolute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index 69c80a52b..79bdde330 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -204,7 +204,7 @@ pub struct ParseTimeError(ParseError); impl fmt::Display for ParseTimeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - self.0.display(f, "block height", LOCK_TIME_THRESHOLD, u32::MAX) + self.0.display(f, "block time", LOCK_TIME_THRESHOLD, u32::MAX) } } From 9b7a706bfdd83e3215901b896b257f7e54d52e65 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 1 Nov 2024 16:22:21 +1100 Subject: [PATCH 3/5] Remove From The errors in `units::locktime::absolute` are complex, I'd like to make them more simple so they are more understandable. I have no clue why this is implemented - remove it. --- units/src/locktime/absolute.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index 79bdde330..60a99da21 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -10,7 +10,6 @@ use internals::write_err; #[cfg(feature = "alloc")] use crate::parse; -use crate::parse::ParseIntError; /// The Threshold for deciding whether a lock time value is a height or a time (see [Bitcoin Core]). /// @@ -360,12 +359,6 @@ impl ParseError { } } -impl From for ParseError { - fn from(value: ParseIntError) -> Self { - Self::InvalidInteger { source: value.source, input: value.input } - } -} - impl From for ParseError { fn from(value: ConversionError) -> Self { Self::Conversion(value.input.into()) } } From aa5c78430c5197bf2511857aa0c020b8cc7d4cd0 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 1 Nov 2024 16:54:45 +1100 Subject: [PATCH 4/5] Replace invalidInteger with ParseIntError We have a special type for wrapping integer parsing errors, use it. To test this I added the following tests: #[test] pub fn debug_absolute_error_conversion_height() { let invalid_height = LOCK_TIME_THRESHOLD + 1; let _ = Height::from_consensus(invalid_height).unwrap(); } #[test] pub fn debug_absolute_error_conversion_time() { let invalid_time = LOCK_TIME_THRESHOLD - 1; let _ = Time::from_consensus(invalid_time).unwrap(); } #[test] #[cfg(feature = "std")] pub fn debug_absolute_error_conversion_height_string() { let invalid_height = std::format!("{:x}", LOCK_TIME_THRESHOLD + 1); let _ = Height::from_hex(&invalid_height).unwrap(); } #[test] #[cfg(feature = "std")] pub fn debug_absolute_error_conversion_time_string() { let invalid_time = std::format!("{:x}", LOCK_TIME_THRESHOLD - 1); let _ = Time::from_hex(&invalid_time).unwrap(); } #[test] #[cfg(feature = "std")] pub fn debug_absolute_error_height_invalid_hex_string() { let _ = Height::from_hex("somerandomshit").unwrap(); } #[test] #[cfg(feature = "std")] pub fn debug_absolute_error_time_invalid_hex_string() { let _ = Time::from_hex("somerandomshit").unwrap(); } Which resulted in the following output Before: ---- locktime::absolute::tests::debug_absolute_error_conversion_height stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_height' panicked at units/src/locktime/absolute.rs:431:56: called `Result::unwrap()` on an `Err` value: ConversionError { unit: Blocks, input: 500000001 } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- locktime::absolute::tests::debug_absolute_error_conversion_height_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_height_string' panicked at units/src/locktime/absolute.rs:444:51: called `Result::unwrap()` on an `Err` value: ParseHeightError(Conversion(500000001)) ---- locktime::absolute::tests::debug_absolute_error_conversion_time stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_time' panicked at units/src/locktime/absolute.rs:437:52: called `Result::unwrap()` on an `Err` value: ConversionError { unit: Seconds, input: 499999999 } ---- locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string' panicked at units/src/locktime/absolute.rs:457:52: called `Result::unwrap()` on an `Err` value: ParseHeightError(InvalidInteger { source: ParseIntError { kind: InvalidDigit }, input: "somerandomshit" }) ---- locktime::absolute::tests::debug_absolute_error_conversion_time_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_time_string' panicked at units/src/locktime/absolute.rs:451:47: called `Result::unwrap()` on an `Err` value: ParseTimeError(Conversion(499999999)) ---- locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string' panicked at units/src/locktime/absolute.rs:464:50: called `Result::unwrap()` on an `Err` value: ParseTimeError(InvalidInteger { source: ParseIntError { kind: InvalidDigit }, input: "somerandomshit" }) After: ---- locktime::absolute::tests::debug_absolute_error_conversion_height stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_height' panicked at units/src/locktime/absolute.rs:432:56: called `Result::unwrap()` on an `Err` value: ConversionError { unit: Blocks, input: 500000001 } ---- locktime::absolute::tests::debug_absolute_error_conversion_height_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_height_string' panicked at units/src/locktime/absolute.rs:445:51: called `Result::unwrap()` on an `Err` value: ParseHeightError(Conversion(500000001)) ---- locktime::absolute::tests::debug_absolute_error_conversion_time stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_time' panicked at units/src/locktime/absolute.rs:438:52: called `Result::unwrap()` on an `Err` value: ConversionError { unit: Seconds, input: 499999999 } ---- locktime::absolute::tests::debug_absolute_error_conversion_time_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_conversion_time_string' panicked at units/src/locktime/absolute.rs:452:47: called `Result::unwrap()` on an `Err` value: ParseTimeError(Conversion(499999999)) ---- locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string' panicked at units/src/locktime/absolute.rs:458:52: called `Result::unwrap()` on an `Err` value: ParseHeightError(ParseInt(ParseIntError { input: "somerandomshit", bits: 32, is_signed: false, source: ParseIntError { kind: InvalidDigit } })) ---- locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string stdout ---- thread 'locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string' panicked at units/src/locktime/absolute.rs:465:50: called `Result::unwrap()` on an `Err` value: ParseTimeError(ParseInt(ParseIntError { input: "somerandomshit", bits: 32, is_signed: false, source: ParseIntError { kind: InvalidDigit } })) --- units/src/locktime/absolute.rs | 17 +++++++++-------- units/src/parse.rs | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index 60a99da21..484439724 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -8,6 +8,7 @@ use core::fmt; use internals::write_err; +use crate::parse::ParseIntError; #[cfg(feature = "alloc")] use crate::parse; @@ -300,7 +301,7 @@ impl fmt::Display for LockTimeUnit { /// Internal - common representation for height and time. #[derive(Debug, Clone, Eq, PartialEq)] enum ParseError { - InvalidInteger { source: core::num::ParseIntError, input: String }, + ParseInt(ParseIntError), // unit implied by outer type // we use i64 to have nicer messages for negative values Conversion(i64), @@ -310,7 +311,7 @@ internals::impl_from_infallible!(ParseError); impl ParseError { fn invalid_int>(s: S) -> impl FnOnce(core::num::ParseIntError) -> Self { - move |source| Self::InvalidInteger { source, input: s.into() } + move |source| Self::ParseInt(ParseIntError { input: s.into(), bits: 32, is_signed: true , source }) } fn display( @@ -325,13 +326,13 @@ impl ParseError { use ParseError::*; match self { - InvalidInteger { source, input } if *source.kind() == IntErrorKind::PosOverflow => { + ParseInt(ParseIntError { input, bits: _, is_signed: _, source }) if *source.kind() == IntErrorKind::PosOverflow => { write!(f, "{} {} is above limit {}", subject, input, upper_bound) } - InvalidInteger { source, input } if *source.kind() == IntErrorKind::NegOverflow => { + ParseInt(ParseIntError { input, bits: _, is_signed: _, source }) if *source.kind() == IntErrorKind::NegOverflow => { write!(f, "{} {} is below limit {}", subject, input, lower_bound) } - InvalidInteger { source, input } => { + ParseInt(ParseIntError { input, bits: _, is_signed: _, source }) => { write_err!(f, "failed to parse {} as {}", input, subject; source) } Conversion(value) if *value < i64::from(lower_bound) => { @@ -351,9 +352,9 @@ impl ParseError { use ParseError::*; match self { - InvalidInteger { source, .. } if *source.kind() == IntErrorKind::PosOverflow => None, - InvalidInteger { source, .. } if *source.kind() == IntErrorKind::NegOverflow => None, - InvalidInteger { source, .. } => Some(source), + ParseInt(ParseIntError { source, .. }) if *source.kind() == IntErrorKind::PosOverflow => None, + ParseInt(ParseIntError { source, .. }) if *source.kind() == IntErrorKind::NegOverflow => None, + ParseInt(ParseIntError { source, .. }) => Some(source), Conversion(_) => None, } } diff --git a/units/src/parse.rs b/units/src/parse.rs index 1ba8fbb3a..02df36f6a 100644 --- a/units/src/parse.rs +++ b/units/src/parse.rs @@ -22,11 +22,11 @@ use internals::write_err; pub struct ParseIntError { pub(crate) input: String, // for displaying - see Display impl with nice error message below - bits: u8, + pub(crate) 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, + pub(crate) is_signed: bool, pub(crate) source: core::num::ParseIntError, } From 3f2e760d1fef2951f93a2554cd53340b0d7a6e0b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 1 Nov 2024 17:15:06 +1100 Subject: [PATCH 5/5] Replace String with InputString in ParseIntError Currently the `ParseIntError` contains an owned copy of the input string, this is causing us to have to use `alloc` everywhere. We already have a alloc-friendly string replacement type, the `InputString` - use it. --- units/src/locktime/absolute.rs | 18 ++++++++++-------- units/src/parse.rs | 13 ++++--------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index 484439724..92283468d 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -6,7 +6,7 @@ use alloc::{boxed::Box, string::String}; use core::fmt; -use internals::write_err; +use internals::error::InputString; use crate::parse::ParseIntError; #[cfg(feature = "alloc")] @@ -221,7 +221,7 @@ impl From for ParseTimeError { fn parser(f: F) -> impl FnOnce(S) -> Result where E: From, - S: AsRef + Into, + S: AsRef + Into, F: FnOnce(u32) -> Result, { move |s| { @@ -234,7 +234,7 @@ where fn parse_hex(s: S, f: F) -> Result where E: From, - S: AsRef + Into, + S: AsRef + Into, F: FnOnce(u32) -> Result, { let n = i64::from_str_radix(parse::hex_remove_optional_prefix(s.as_ref()), 16) @@ -310,7 +310,7 @@ enum ParseError { internals::impl_from_infallible!(ParseError); impl ParseError { - fn invalid_int>(s: S) -> impl FnOnce(core::num::ParseIntError) -> Self { + fn invalid_int>(s: S) -> impl FnOnce(core::num::ParseIntError) -> Self { move |source| Self::ParseInt(ParseIntError { input: s.into(), bits: 32, is_signed: true , source }) } @@ -327,13 +327,15 @@ impl ParseError { match self { ParseInt(ParseIntError { input, bits: _, is_signed: _, source }) if *source.kind() == IntErrorKind::PosOverflow => { - write!(f, "{} {} is above limit {}", subject, input, upper_bound) + // Outputs "failed to parse as absolute Height/Time ( is above limit )" + write!(f, "{} ({} is above limit {})", input.display_cannot_parse("absolute Height/Time"), subject, upper_bound) } ParseInt(ParseIntError { input, bits: _, is_signed: _, source }) if *source.kind() == IntErrorKind::NegOverflow => { - write!(f, "{} {} is below limit {}", subject, input, lower_bound) + // Outputs "failed to parse as absolute Height/Time ( is below limit )" + write!(f, "{} ({} is below limit {})", input.display_cannot_parse("absolute Height/Time"), subject, lower_bound) } - ParseInt(ParseIntError { input, bits: _, is_signed: _, source }) => { - write_err!(f, "failed to parse {} as {}", input, subject; source) + ParseInt(ParseIntError { input, bits: _, is_signed: _, source: _ }) => { + write!(f, "{} ({})", input.display_cannot_parse("absolute Height/Time"), subject) } Conversion(value) if *value < i64::from(lower_bound) => { write!(f, "{} {} is below limit {}", subject, value, lower_bound) diff --git a/units/src/parse.rs b/units/src/parse.rs index 02df36f6a..a2ee0e4db 100644 --- a/units/src/parse.rs +++ b/units/src/parse.rs @@ -7,6 +7,7 @@ use core::fmt; use core::str::FromStr; use internals::write_err; +use internals::error::InputString; /// Error with rich context returned when a string can't be parsed as an integer. /// @@ -20,7 +21,7 @@ use internals::write_err; #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub struct ParseIntError { - pub(crate) input: String, + pub(crate) input: InputString, // for displaying - see Display impl with nice error message below pub(crate) bits: u8, // We could represent this as a single bit but it wouldn't actually derease the cost of moving @@ -30,16 +31,10 @@ pub struct ParseIntError { pub(crate) source: core::num::ParseIntError, } -impl ParseIntError { - /// Returns the input that was attempted to be parsed. - pub fn input(&self) -> &str { &self.input } -} - 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) + write_err!(f, "{} ({}, {}-bit)", self.input.display_cannot_parse("integer"), signed, self.bits; self.source) } } @@ -74,7 +69,7 @@ impl_integer!(u8, i8, u16, i16, u32, i32, u64, i64, u128, i128); /// /// 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 fn int + Into>(s: S) -> Result { +pub fn int + Into>(s: S) -> Result { s.as_ref().parse().map_err(|error| { ParseIntError { input: s.into(),