`alloc`-free parse errors by Kixunil · Pull Request #1297 · rust-bitcoin/rust-bitcoin

Kixunil · 2022-09-20T15:19:54Z

This implements various helpers for parse errors that will not require alloc. This PR is useless while all of the crates require alloc and is thus a draft so that you can look at the design.

apoelstra · 2022-09-20T16:05:10Z

Neat! I'm a little scared of having error types with lifetimes on them because they can be hard to propagate upward. I guess you've thought about that and think it'll be ok?

Kixunil · 2022-09-20T16:09:05Z

CannotParse is not an error type but a display adapter similar to bitcoin::amount::Display or std::path::Display, so it won't be propagated, just used to implement Display for an error type carrying InputString

tcharding

Cool, concept ACK.

tcharding · 2022-09-20T22:48:31Z

internals/src/error.rs

Not that it matters since this is draft but did you mean to use the newer form error.rs and error/ instead of error/mod.rs?

Honestly, I don't care about these forms. I normally use mod.rs because that's what I'm used to from the time when there was no other option but if there already exists non-directory foo.rs and I need to add files I skip renaming it.

If we want to have a policy about this (I'm lightly against) then foo.rs is better than foo/mod.rs because it's easier to add things.

Just flagging because I noticed while reviewing that have error.rs on this branch. I believe since we had this chat in 2022 we decided on using foo/mod.rs.

EDIT: post review comment because the github view lost context - this refers to original review discussion in 2022.

stevenroose · 2023-05-05T18:53:17Z

I'm not sure the benefits of having this merit the complexity of this code.. I mean errors are great and all, but this code is quite non-trivial to maintain. I think it's a conversation worth having in this context.

Kixunil · 2023-05-30T06:41:30Z

Undrafting since this is going to be used in bitcoin-units.

@stevenroose this code is actually doing just trivial shoveling of data, so should be fine. It's a bit large but it'll be used in numerous places. There were already people asking for no_alloc support, so this is pretty much required if you don't want the others to waste ton of tie debugging errors without context.

tcharding

Looks good, a few docs suggestions. Also it would be nice to have a usage of this code along with the PR, perhaps bitcoin::address::Error::UnknownAddressType (for unknown variant) and bitcoin::string::Error::MissingPrefix (for display_cannot_parse)?

internals/Cargo.toml

tcharding · 2023-05-30T06:56:21Z

internals/src/error.rs

Just flagging because I noticed while reviewing that have error.rs on this branch. I believe since we had this chat in 2022 we decided on using foo/mod.rs.

EDIT: post review comment because the github view lost context - this refers to original review discussion in 2022.

internals/src/error/parse_error.rs

tcharding · 2023-05-30T07:35:04Z

internals/src/error/input_string.rs

I had a go at using this PR on top of the new bitcoin-units crate, I think this should be:

Suggested change

/// This is created by `display_cannot_parse` method and should be used as

/// `write_err!("{}", self.input.display_cannot_parse("what is parsed"); self.source)` in parse

/// error [`Display`](fmt::Display) imlementation.

/// This is created by `display_cannot_parse` method and should be used as

/// `write!("{}", self.input.display_cannot_parse("what is parsed"))` in parse

/// error [`Display`](fmt::Display) imlementation.

It actually depends on whether the error has another source or not.

What? I thought InputString replaces a String in an error variant, how can it have another source?

FTR this is a snippet of how I used it:

UnknownDenomination(ref d) => write!(f, "unknown denomination: {}", d.display_cannot_parse("a bitcoin denomination")), PossiblyConfusingDenomination(ref d) => { write!(f, "the first character of denomination has a technical meaning but that denomination is uncommon so it is possibly confusing: {}", d.display_cannot_parse("a bitcoin denomination"))

No worries, I can't think of a clear example, I rekon it would be better to include the most simple case in this doc but I won't hold the PR up for this.

how can it have another source?

It doesn't. The error it's in may have. E.g. our ParseIntError has both input string and core::num::ParseIntError which is the source.

Oh there is literally a macro that defines a type like this :)

internals/src/error/input_string.rs

The macro is useful for all other crates thus it is moved to the internals crate in this commit.

Kixunil · 2023-05-30T16:39:37Z

That formatting is horrible. I had to do some serious hacks to make it sane. Perhaps we should increase line size to 120 or more.

tcharding · 2023-05-31T03:31:02Z

No matter what the limit one always hits it eventually and gets annoyed at the formatting, right? This even happens when formatting manually (in C for example) when one is trying to maintain some line length. I don't care what it is as long as its uniform.

tcharding · 2023-05-31T04:01:35Z

internals/src/error/input_string.rs

Commenting because this is the second time I reviewed this code and both times I did a double take at the What generic, I personally prefer a plain old T so I don't go "what is What" then have to look back at <'a, What, and go "oh its a generic with a name, that's unexpected". libp2p does this, it used to drive me nuts there too. Open to expanding my world view if you have a valid technical reason to back it though :)

Lately I started using bit more descriptive names for generics but it's true it's not clear it's generic. Hard to say what's better. We could use some silly prefix like GWhat but I'd rather not.

Another good name for What would be Object but it could look like OOP object while it actually refers to a word in a phrase (same category as "subject").

None of these make the code easier to read than plain old T in my opinion.

I found it helpful for more complicated code but this is simple, so I don't care.

tcharding · 2023-05-31T04:03:14Z

I'd like to clear up the usage/source thing before I ack just to make sure I'm not misunderstanding or that we are not missing a usecase. Also the What vs T thing please. Everything else looks good to me.

Kixunil · 2023-05-31T10:21:25Z

No matter what the limit one always hits it eventually and gets annoyed at the formatting, right?

Yes, but longer limits make this less frequent and the screens these days are far larger than in 70s where terminals were fixed to 80 chars. I guess even 200 would be fine.

Kixunil · 2023-05-31T10:27:56Z

I'd like to clear up the usage/source thing before I ack just to make sure I'm not misunderstanding or that we are not missing a usecase.

I think we should either merge #1225 first without no-alloc support or change this PR to use the added stuff in the code and rebase bitcoin-units. Which do you prefer? I personally don't care.

apoelstra · 2023-05-31T13:13:02Z

Yes, but longer limits make this less frequent and the screens these days are far larger than in 70s where terminals were fixed to 80 chars. I guess even 200 would be fine.

Unfortunately rustfmt makes this complicated by unwrapping lines to the limit, which still causes churn and is almost always bad for readability. If the limit were really just "at this point it's excessive, you are required to wrap" I'd be fine with 200. But as-is I think our current limit of 100 is a reasonable compromise. (If the limit were merely a limit I really wouldn't care at all and leave this whole discussion to people more interested in format wars....but alas, Rust can't resist making everything needlessly political.)

For my part, my entire laptop screen at my usual terminal font has 225 chars. So after adding line numbers and error highlights, 100 is about the limit if I want to have two terminals open side-by-side. I don't mind some one-off lines wrapping but I'd really rather not have that be the default case for function declarations etc.

Kixunil · 2023-05-31T14:58:50Z

Heh, I actually hate multi-line function declarations (where clause is OK) so my preference would be to have higher limit for those only. But that would mess up with your workflow. Anyway, doesn't look like we will resolve this one.

And yes, I do side-by-side too, I just have weird preferences. :)

tcharding · 2023-06-04T04:34:23Z

Hey @Kixunil, I'm unsure of the status of this and #1225. Are you planning on pushing any changes to this or you want it merged as is? I still don't like the What but if someone else acks I'll ack so this can go in.

Once merged, I can rebase 1225 on top of this work.

Kixunil · 2023-06-04T08:13:41Z

In #1297 (comment) I've asked you whether you prefer to do this one first or the other one. So I take your message as your answer. I'll look into changing What.

This implements basic facilities to conditionally carry string inputs in parse errors. This includes: * `InputString` type that may carry the input and format it * `parse_error_type!` macro creating a special type for parse errors * `impl_parse` implementing parsing for various types as well as its `serde`-supporting alternative

tcharding · 2023-06-05T05:30:00Z

Thanks man, I always prefer your PRs to go in first so I carry the rebase burden.

tcharding

ACK 2b6bcf0

Kixunil · 2023-06-05T07:17:40Z

You forgot to hit approve.

apoelstra

ACK 2b6bcf0

tcharding · 2023-06-06T04:13:52Z

You forgot to hit approve.

F***. Since starting traveling I can't get my ack-tip function to work, I think its because of github access tokens but I cannot get it to configure properly. If anyone has time can you check your access token perms for me please. its settings -> developer settings -> personal access tokens -> fine-grained tokens.

My repository access looks fishy, "owned by you" - I don't own rust-bitcoin? @Kixunil could you please look at yours because Andrew owns it so his will be different I assume.

I have:

Repository permissions

Read access to Dependabot alerts and metadata
Read and Write access to code, commit statuses, deployments, discussions, issues, merge queues, and pull requests

Repository access

This token has access to all repositories owned by you.

User permissions

None

ack-tip

ack-tip () {
        git log -1 --format=full @
        read "ack?Really ACK this commit? "
        if [[ "$ack" =~ ^[Yy]$ ]]
        then
                gh pr review -a -b "ACK $(git rev-parse @)"
        fi
}

tcharding · 2023-06-06T04:16:41Z

@apoelstra do you have a setting in the rust-bitcoin org to allow write access to tokens? You will have given me access before if so because on my desktop at home I can use this function but not on my laptop. Thanks

Kixunil · 2023-06-06T08:40:28Z

@tcharding I don't even have access to settings of this repo, so clearly you have elevated permissions.

tcharding · 2023-06-06T08:58:24Z

I don't have access to settings in this repo either.

apoelstra · 2023-06-07T15:26:45Z

@tcharding I have as a "Maintainer" while @Kixunil I have as "Write". I don't know why I did this but it probably wasn't on purpose (because I don't know, and probably never knew, the difference between those levels :)). But I'll leave it be since it seems like it's been working so far. I am an "Admin" FWIW.

As for Settings->Developer Settings, that is only available for my personal settings. If you are having token issues I think it's a problem with your setup, not related to the repo.

tcharding · 2023-06-08T04:48:09Z

Cool thanks man.

Kixunil added enhancement 1.0 Issues and PRs required or helping to stabilize the API error handling Issues and PRs related to error handling, mainly error refactoring epic labels Sep 20, 2022

Kixunil added the crate smashing Issues and PRs splitting out parts of the main crate to smaller crates label Sep 20, 2022

Kixunil mentioned this pull request Sep 20, 2022

Add bitcoin-units crate #1225

Merged

tcharding reviewed Sep 20, 2022

View reviewed changes

tcharding mentioned this pull request Sep 21, 2022

Add Target and Work types #1197

Merged

Kixunil force-pushed the input-string branch 2 times, most recently from 55487fc to 2b3c398 Compare September 21, 2022 07:35

Kixunil mentioned this pull request Feb 10, 2023

Remove Into requirement from FromHexStr #1637

Closed

Kixunil mentioned this pull request May 30, 2023

Error refactoring #820

Open

8 tasks

Kixunil force-pushed the input-string branch from 2b3c398 to 987eebd Compare May 30, 2023 06:34

Kixunil marked this pull request as ready for review May 30, 2023 06:34

Kixunil force-pushed the input-string branch from 987eebd to caf3d04 Compare May 30, 2023 06:49

tcharding reviewed May 30, 2023

View reviewed changes

Kixunil force-pushed the input-string branch 4 times, most recently from 291d3b8 to 090a06e Compare May 30, 2023 15:35

Move impl_std_error to bitcoin-internals

783e1e8

The macro is useful for all other crates thus it is moved to the internals crate in this commit.

Kixunil force-pushed the input-string branch 2 times, most recently from d6acd8d to 262a6ec Compare May 30, 2023 16:38

tcharding reviewed May 31, 2023

View reviewed changes

Kixunil force-pushed the input-string branch from 262a6ec to 2b6bcf0 Compare June 4, 2023 19:15

tcharding reviewed Jun 5, 2023

View reviewed changes

apoelstra approved these changes Jun 5, 2023

View reviewed changes

tcharding approved these changes Jun 6, 2023

View reviewed changes

apoelstra merged commit 0750168 into rust-bitcoin:master Jun 7, 2023

Kixunil deleted the input-string branch June 11, 2023 12:46

apoelstra mentioned this pull request Jun 23, 2023

internals: Bump version to 0.2.0 #1916

Merged

-/// This is created by `display_cannot_parse` method and should be used as
-/// `write_err!("{}", self.input.display_cannot_parse("what is parsed"); self.source)` in parse
-/// error [`Display`](fmt::Display) imlementation.
+/// This is created by `display_cannot_parse` method and should be used as
+/// `write!("{}", self.input.display_cannot_parse("what is parsed"))` in parse
+/// error [`Display`](fmt::Display) imlementation.

Conversation

Kixunil commented Sep 20, 2022

Uh oh!

apoelstra commented Sep 20, 2022

Uh oh!

Kixunil commented Sep 20, 2022

Uh oh!

tcharding left a comment

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 May 30, 2023 • 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!

stevenroose commented May 5, 2023

Uh oh!

Kixunil commented May 30, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding May 30, 2023 • 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!

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 May 31, 2023 • 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!

Uh oh!

Kixunil commented May 30, 2023

Uh oh!

tcharding commented May 31, 2023

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 commented May 31, 2023

Uh oh!

Kixunil commented May 31, 2023

Uh oh!

Kixunil commented May 31, 2023

Uh oh!

apoelstra commented May 31, 2023

Uh oh!

Kixunil commented May 31, 2023

Uh oh!

tcharding commented Jun 4, 2023

Uh oh!

Kixunil commented Jun 4, 2023

Uh oh!

tcharding commented Jun 5, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Jun 5, 2023

tcharding May 30, 2023 •

edited

Loading

tcharding May 30, 2023 •

edited

Loading

tcharding May 31, 2023 •

edited

Loading

apoelstra commented Jun 7, 2023 •

edited

Loading