Parse int error by Kixunil · Pull Request #1129 · rust-bitcoin/rust-bitcoin

Kixunil · 2022-07-26T13:01:29Z

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

tcharding · 2022-07-26T22:45:31Z

#994 was rebased to fix the CI fails we are seeing here.

tcharding

Thanks for doing this man, I learned a bunch while playing around with it.

src/blockdata/locktime.rs

tcharding · 2022-07-26T23:49:24Z

src/blockdata/locktime.rs

It would be nice to get rid of the duplication here, I tried but was unable to.

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.

tcharding · 2022-07-26T23:51:35Z

src/blockdata/locktime.rs

I was amazed by the ($to $(:: $fn)?). How does this work, why is the whitespace allowed?

Whitespace is always allowed. It's just not used by convention outside of macros like this one. :)

Oh cool, thanks man.

tcharding · 2022-07-27T00:23:31Z

src/blockdata/locktime.rs

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);

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.

src/error.rs

tcharding · 2022-07-27T00:43:37Z

src/error.rs

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.

Yeah, I was planning to do these in a followup PR.

src/parse.rs

tcharding · 2022-07-27T01:23:48Z

Oh the embedded CI fail is real, flagging incase you assume all the red is because of the LockTime PR (as I did).

EDIT: Just need to add to parse.rs: use crate::prelude::*;

apoelstra · 2022-07-27T16:15:37Z

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

Kixunil · 2022-07-27T17:02:12Z

Finally! I guess I will have some champagne tonight to celebrate... 🍾

In other news, I just noticed we forgot to implement parsing for Sequence. Will add here since it's relevant. Edit: done.

`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`.

apoelstra · 2022-07-27T21:39:23Z

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.

apoelstra

ACK 071a1c0

tcharding

Just a few nits, thanks man!

ACK 071a1c0

tcharding · 2022-07-27T22:45:53Z

src/error.rs

+
+/// Impls std::error::Error for the specified type with appropriate attributes, possibly returning
+/// source.
+#[macro_export]


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.

Ah, this is the way! Damn! I was confused why the others work and this one not. Macro visibility is confusing me.

tcharding · 2022-07-27T22:46:51Z

src/error.rs

+/// source.
+#[macro_export]
+macro_rules! impl_std_error {
+    // No source available


Suggested change

// No source available

// Field is not an error source.

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 could also be no field. :) Maybe "No field is an error source"?

tcharding · 2022-07-27T22:47:27Z

src/error.rs

+        #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
+        impl std::error::Error for $type {}
+    };
+    // Struct with $field as source


Suggested change

// Struct with $field as source

// $field is an error source (implements `std::error::Error`)

tcharding · 2022-07-27T22:50:57Z

src/parse.rs

+    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)


These three lines are hot ;)

Kixunil · 2022-07-28T11:13:28Z

Since the nits don't affect our API I request to address them in a followup PR to speed up the release.

apoelstra · 2022-07-28T13:03:26Z

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

Kixunil force-pushed the parse-int-error branch from 3ba3e60 to 4fa2b7c Compare July 26, 2022 13:03

Kixunil added enhancement API break This PR requires a version bump for the next release error handling Issues and PRs related to error handling, mainly error refactoring epic labels Jul 26, 2022

Kixunil mentioned this pull request Jul 26, 2022

Release tracking PR - v0.29.0 #1109

Merged

3 tasks

tcharding reviewed Jul 27, 2022

View reviewed changes

Kixunil force-pushed the parse-int-error branch from 4fa2b7c to 73bf61d Compare July 27, 2022 09:18

Kixunil force-pushed the parse-int-error branch from 73bf61d to c39bc39 Compare July 27, 2022 17:00

Kixunil mentioned this pull request Jul 27, 2022

Impl std error using macro #1136

Merged

Kixunil added the P-high High priority label Jul 27, 2022

Kixunil force-pushed the parse-int-error branch from 0733b4b to 009f917 Compare July 27, 2022 18:34

Kixunil added this to the 0.29.0 milestone Jul 27, 2022

Implement string parsing for Sequence

071a1c0

`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`.

Kixunil force-pushed the parse-int-error branch from 009f917 to 071a1c0 Compare July 27, 2022 18:46

apoelstra approved these changes Jul 27, 2022

View reviewed changes

tcharding approved these changes Jul 27, 2022

View reviewed changes

apoelstra merged commit ed3fb45 into rust-bitcoin:master Jul 28, 2022

Kixunil deleted the parse-int-error branch July 28, 2022 13:25

	// Struct with $field as source
	// $field is an error source (implements `std::error::Error`)

Conversation

Kixunil commented Jul 26, 2022

Uh oh!

tcharding commented Jul 26, 2022

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Jul 27, 2022

Uh oh!

Kixunil commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Jul 27, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Jul 28, 2022

Uh oh!

apoelstra commented Jul 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kixunil Jul 27, 2022 •

edited

Loading

tcharding Jul 27, 2022 •

edited

Loading

tcharding commented Jul 27, 2022 •

edited

Loading

Kixunil commented Jul 27, 2022 •

edited

Loading

tcharding Jul 27, 2022 •

edited

Loading