New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse int error #1129
Parse int error #1129
Conversation
#994 was rebased to fix the CI fails we are seeing here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this man, I learned a bunch while playing around with it.
src/blockdata/locktime.rs
Outdated
/// 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)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to get rid of the duplication here, I tried but was unable to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, with fresh mind I got an idea.
Oh, the idea doesn't work because of complicated macro issues. Maybe there's a hack around it but it'd likely be too horrible.
src/blockdata/locktime.rs
Outdated
impl TryFrom<String> for PackedLockTime { | ||
type Error = ParseIntError; | ||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
parse_int(s).map($to $(:: $fn)?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was amazed by the ($to $(:: $fn)?)
. How does this work, why is the whitespace allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace is always allowed. It's just not used by convention outside of macros like this one. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, thanks man.
src/blockdata/locktime.rs
Outdated
($($from:ty, $to:ident $(, $fn:ident)?);*) => { | ||
$( | ||
impl TryFrom<$from> for $to { | ||
type Error = ParseIntError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change this to type Error = Error
and add an extra ?
below
parse_int(s).map($to $(:: $fn)?)?
We can then use impl_tryfrom_str!
for Height
and Time
as well. But ... we need the slightly ugly helper functions
// Helper function used by `impl_tryfrom_str!`.
fn _from_consensus(n: u32) -> Result<LockTime, Error> {
Ok(Self::from_consensus(n))
}
and
// Helper function used by `impl_tryfrom_str!`.
fn _from_u32(n: u32) -> Result<PackedLockTime, Error> {
Ok(Self::from_consensus(n))
}
so we have
impl_tryfrom_str!(PackedLockTime, _from_u32);
impl_tryfrom_str!(LockTime, _from_consensus);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking about de-duplicating but couldn't figure out anything that's not horrible. Maybe with proc macros it'd be fine but those are slooooow to compile.
/// Impls std::error::Error for the specified type with appropriate attributes, possibly returning | ||
/// source. | ||
#[macro_export] | ||
macro_rules! impl_std_error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of other struct errors that we can use this for too
- ConversionError
- SighashTypeParseError
- CommandStringError
- ParseLengthError
I did all of these, I'll pushed a branch review-parse-int-error
on my fork in case you want to look at it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was planning to do these in a followup PR.
Oh the embedded CI fail is real, flagging incase you assume all the red is because of the EDIT: Just need to add to |
Merged #994, we can rebase on master. |
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 rust-bitcoin#1113
Finally! I guess I will have some champagne tonight to celebrate... 🍾 In other news, I just noticed we forgot to implement parsing for |
`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`.
Have a similar intuition to both of you that this ought to be possible to clean up or simplify, but I don't see how :). So looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 071a1c0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits, thanks man!
ACK 071a1c0
|
||
/// Impls std::error::Error for the specified type with appropriate attributes, possibly returning | ||
/// source. | ||
#[macro_export] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We do not need to export this. We can make it pub crate only by using the trick that we use in internal_macros.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is the way! Damn! I was confused why the others work and this one not. Macro visibility is confusing me.
/// source. | ||
#[macro_export] | ||
macro_rules! impl_std_error { | ||
// No source available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// No source available | |
// Field is not an error source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit. My aim was to make the macro more approachable for devs new to our error code boiler plate code and/or macros in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could also be no field. :) Maybe "No field is an error source"?
#[cfg_attr(docsrs, doc(cfg(feature = "std")))] | ||
impl std::error::Error for $type {} | ||
}; | ||
// Struct with $field as source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Struct with $field as source | |
// $field is an error source (implements `std::error::Error`) |
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three lines are hot ;)
Since the nits don't affect our API I request to address them in a followup PR to speed up the release. |
Sounds good. |
071a1c0 Implement string parsing for `Sequence` (Martin Habovstiak) c39bc39 Extend `ParseIntError` to carry more context (Martin Habovstiak) Pull request description: 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 Depends on #994 ACKs for top commit: apoelstra: ACK 071a1c0 tcharding: ACK 071a1c0 Tree-SHA512: 31cb84b9e4d5fe3bdeb1cd48b85da2cbe9b9d17d93d029c2f95e0eed5b8842d7a553afafcf8b4a87c075aa53cf0274776e893bed6dca37e7dbc2e1ee1d602b8e
110b5d8 Bump version to v0.29.0 (Tobin C. Harding) Pull request description: Add changelog notes and bump the version number to v0.29.0. ## Notes I attempted to go through all the PRs since last release, please sing out if you had a PR merged that is not mentioned and you would like it mentioned. The changelog notes can be changed or improved, please do not take me writing them to imply I know exactly what goes on round here - I just made an effort to save others having to do it. ## TODO - [x] merge all 'required' PRs - #1131 - #1137 - #1129 - #1151 - #1165 (add release notes still) - [x] Ensure all green from the CI run on: rust-bitcoin/rust-miniscript#450 - [ ] Carry out (and improve) the #1106 ACKs for top commit: tcharding: ACK 110b5d8 Kixunil: ACK 110b5d8 apoelstra: ACK 110b5d8 sanket1729: reACK 110b5d8 Tree-SHA512: d00c70534476794c01cd694ea9a23affef947c4f913b14344405272bc99cc00763f1ac755cc677e7afbd3dbef573d786251c9093d5dbafd76ee0cf86ca3b0ebd
86218ad Use macro to implement `std::erorr::Error` (Martin Habovstiak) Pull request description: There was a bunch of manual implemntations that can be converted to macro call. This commit replaces them except for enums because those are currently not supported by the macro and we want to protect against forgetting to handle newly added variants. Depends on #1129 and is **not** urgent for the next release. ACKs for top commit: apoelstra: ACK 86218ad tcharding: ACK 86218ad Tree-SHA512: a4ca91e023d66b5ad9408004b201cfe4cea85efb5a6e7f2241367934a954659c9196561295a491d2b2ed15c1a69c0ffb17a297d710cec4ce1d0f1ec8c12492e6
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 theone from
core
as well as additional information. ItsDisplay
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
Depends on #994