alloc-free parse errors
#1297
alloc-free parse errors
#1297
Conversation
|
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? |
|
|
| @@ -6,6 +6,11 @@ | |||
| //! Error handling macros and helpers. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
55487fc to
2b3c398
Compare
|
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. |
|
Undrafting since this is going to be used in @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 |
| @@ -6,6 +6,11 @@ | |||
| //! Error handling macros and helpers. | |||
There was a problem hiding this comment.
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/input_string.rs
Outdated
| /// 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. |
There was a problem hiding this comment.
I had a go at using this PR on top of the new bitcoin-units crate, I think this should be:
| /// 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. |
There was a problem hiding this comment.
It actually depends on whether the error has another source or not.
There was a problem hiding this comment.
What? I thought InputString replaces a String in an error variant, how can it have another source?
There was a problem hiding this comment.
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"))There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh there is literally a macro that defines a type like this :)
291d3b8 to
090a06e
Compare
The macro is useful for all other crates thus it is moved to the internals crate in this commit.
d6acd8d to
262a6ec
Compare
|
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. |
|
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. |
internals/src/error/input_string.rs
Outdated
| /// Displays a message saying `failed to parse <self> as <what>`. | ||
| /// | ||
| /// This is normally used whith the `write_err!` macro. | ||
| pub fn display_cannot_parse<'a, What>(&'a self, what: &'a What) -> CannotParse<'a, What> |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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").
There was a problem hiding this comment.
None of these make the code easier to read than plain old T in my opinion.
There was a problem hiding this comment.
I found it helpful for more complicated code but this is simple, so I don't care.
|
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 |
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. |
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 |
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. |
|
Heh, I actually hate multi-line function declarations ( And yes, I do side-by-side too, I just have weird preferences. :) |
|
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 |
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
|
Thanks man, I always prefer your PRs to go in first so I carry the rebase burden. |
|
You forgot to hit approve. |
F***. Since starting traveling I can't get my 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
Repository accessThis token has access to all repositories owned by you. User permissionsNone ack-tip |
|
@apoelstra do you have a setting in the |
|
@tcharding I don't even have access to settings of this repo, so clearly you have elevated permissions. |
|
I don't have access to settings in this repo either. |
|
@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. |
|
Cool thanks man. |
This implements various helpers for parse errors that will not require
alloc. This PR is useless while all of the crates requireallocand is thus a draft so that you can look at the design.