Implement `std::error::Error` for the new MSRV #987

tcharding · 2022-05-04T06:00:44Z

Now that we have MSRV of 1.41.1 we should use source instead of cause. Audit the whole codebase and implement source for every error type we have.

The first three patches are preparatory cleanup, patch 3 is particularly shameful (adds parenthesis to make my editor work).

CC @Kixunil because he is championing the error stuff.

apoelstra

ACK 74dc8ef

Didn't check that all the source impls are correct, but it looks like you did a better job than the existing cause ones..

I forgive you for patch 3.

tcharding · 2022-05-04T23:33:54Z

I forgive you for patch 3.

If you feel like it could you remove the parenthesis and see if your emacs handles it or if its something strange in my environment?

apoelstra · 2022-05-05T13:59:52Z

@tcharding I am using vim without any extensions other than a syntax highlighting dictionary that I installed many years ago. My editor has no problems with anything* :)

*actually it goes crazy handling git merge-conflict markers in postscript. But it never has problems with Rust.

RCasatta · 2022-05-05T14:59:07Z

IIRC some guidelines recommend using source when the error is not also printed in Display otherwise in some cases error information could be printed twice. I see we have some cases such as ParseOutPointError::Txid

tcharding · 2022-05-05T22:03:35Z

I am using vim without any extensions

Oh, from a previous comment you once made on which key shortcuts you use I thought you used emacs. I won't hold it against you ;)

tcharding · 2022-05-05T22:41:21Z

IIRC some guidelines recommend using source when the error is not also printed in Display otherwise in some cases error information could be printed twice. I see we have some cases such as ParseOutPointError::Txid

Thanks man, from my understanding, all the Display impls will get an audit/overhaul with #820. I added a link on the 820 thread to your original comment above.

Put the match arms in the same order as the enum that defines them.

In an effort to be uniform throughout the codebase; put the `std::error::Error` impl block below the `Display` impl block.

Parenthesis are not needed around this expression but my editor is going mad and cannot format the code without them. Since it does not hurt readability add parenthesis around the expression.

Audit ever error type we have and implement `source` for each.

tcharding · 2022-05-19T06:40:21Z

Face palm, I did this same work in rust-bitcoin and rust-miniscript but I did two things differently in each

In one I use fully quallified path std::error::Error and in one I import std::error.
In one I put source above Display and in the other I put it below.

My OCD cannot handle that so I'll fix them both tomorrow, sending this to draft. Please comment if you have an opinion on these otherwise I'll arbitrarily pick and do them all the same.

elichai · 2022-05-19T13:19:11Z

What's the benefit of implementing source that always returns None? isn't that the provided impl already?

apoelstra · 2022-05-19T13:59:28Z

@elichai being explicit so that we know we didn't just forget about one of the impls.

Also explicitly enumerating all the variants forces us to update the impl in case we later add a non-None variant.

sanket1729

Did a review and all sources that covered it. Did not check whether we covered all possible Error enums in the repository.

sanket1729 · 2022-05-20T02:11:10Z

src/network/mod.rs

+
+        match self {
+            Io(e) => Some(e),
+            SocketMutexPoisoned | SocketNotConnectedToPeer => None,


I could not find any instances of SocketMutexPiosoned and SocketNotConnectedError. These should perhaps be removed.

Cheers. This should be caught by the linter, now edition 2018 is here we can start thinking about getting the codebase linting cleanly.

apoelstra

ACK 97a5bb1

Kixunil · 2022-05-24T08:24:13Z

Since this got merged, we will really need to fix Display impls before the release to avoid double printing.

(I'm back, should be able to go through most issues/PRs during this week.)

tcharding · 2022-05-25T02:36:49Z

Damn! @apoelstra, after all our chats about not implicitly de-referencing I did the opposite and matched on self in every source function :(

Old habits die hard. Please do pull me up if you happen to notice this ever in any PRs.

Kixunil · 2022-05-25T06:23:27Z

Doesn't clippy complain about the opposite?

apoelstra · 2022-05-25T16:11:07Z

Lol @tcharding I didn't even notice. Unfortunately these checks are basically impossible to reliably do mentally.

Also lol if Clippy actually complains about not using weak-typing antifeatures.

Kixunil · 2022-05-25T17:02:46Z

I don't think it's anti-feature since even though types are weird it's quite obvious what it does. I remember when it wasn't in the language and writing Rust those days was quite annoying. I did welcome the change.

tcharding · 2022-05-25T21:50:38Z

For context @Kixunil (just out of interest and because its closed so you may not see it) Andrew and I had some discussions on this PR. Its totally unnecessary nerding, you'll probably love it :)

@Kixunil

57dd673 Do not print error when displaying for std builds (Tobin C. Harding) b80cfee Bind to error_kind instead of e (Tobin C. Harding) 241ec72 Bind to b instead of e (Tobin C. Harding) 01f481b Bind to s instead of e (Tobin C. Harding) 5c6d369 network: Remove unused error variants (Tobin C. Harding) e67e97b Put From impl below std::error::Error impl (Tobin C. Harding) 6ca98e5 Remove error TODO (Tobin C. Harding) Pull request description: As part of the ongoing error improvement work and as a direct result of [this comment](#987 (comment)) improve the `Display` implementations of all our error types so as to not repeat the source error when printing. The first 5 patches are trivial clean ups around the errors. Patch 6 is the real work. EDIT: ~CC @Kixunil, have I got the right idea here bro?~ Patch 6 now includes a macro as suggested. ACKs for top commit: Kixunil: ACK 57dd673 apoelstra: ACK 57dd673 sanket1729: ACK 57dd673. Did not check if we covered all cases. We need to remember to use `write_err!` instead of `write!` in future. Tree-SHA512: 1ed26b0cc5f9a0f71684c431cbb9f94404c116c9136be696434c56a2f56fd93cb5406b0955edbd0dc6f8612e77345c93fa70a70650118968cc58e680333a41de

@Kixunil

…r the new MSRV 97a5bb1 Implement std::error::source codebase wide (Tobin C. Harding) 0a9191b Add parenthesis around left hand side of companion (Tobin C. Harding) 7cf8af2 Put Error impl block below Display (Tobin C. Harding) 2384712 Re-order Display match arms (Tobin C. Harding) Pull request description: Now that we have MSRV of 1.41.1 we should use `source` instead of `cause`. Audit the whole codebase and implement `source` for _every_ error type we have. The first three patches are preparatory cleanup, patch 3 is particularly shameful (adds parenthesis to make my editor work). CC @Kixunil because he is championing the error stuff. ACKs for top commit: apoelstra: ACK 97a5bb1 Tree-SHA512: 46313a28929445f32e01e30ca3b0246b30bc9d5e43db5754d4b441e9c30d3e427efaf247100eb6b452f98beec5a4fcde1daba7943a772114aa34f78ab52cbc60

@Kixunil

…ntations 57dd673 Do not print error when displaying for std builds (Tobin C. Harding) b80cfee Bind to error_kind instead of e (Tobin C. Harding) 241ec72 Bind to b instead of e (Tobin C. Harding) 01f481b Bind to s instead of e (Tobin C. Harding) 5c6d369 network: Remove unused error variants (Tobin C. Harding) e67e97b Put From impl below std::error::Error impl (Tobin C. Harding) 6ca98e5 Remove error TODO (Tobin C. Harding) Pull request description: As part of the ongoing error improvement work and as a direct result of [this comment](rust-bitcoin/rust-bitcoin#987 (comment)) improve the `Display` implementations of all our error types so as to not repeat the source error when printing. The first 5 patches are trivial clean ups around the errors. Patch 6 is the real work. EDIT: ~CC @Kixunil, have I got the right idea here bro?~ Patch 6 now includes a macro as suggested. ACKs for top commit: Kixunil: ACK 57dd673 apoelstra: ACK 57dd673 sanket1729: ACK 57dd673. Did not check if we covered all cases. We need to remember to use `write_err!` instead of `write!` in future. Tree-SHA512: 1ed26b0cc5f9a0f71684c431cbb9f94404c116c9136be696434c56a2f56fd93cb5406b0955edbd0dc6f8612e77345c93fa70a70650118968cc58e680333a41de

tcharding changed the title ~~Implement std::error::Error for the new MSRV~~ Implement std::error::Error for the new MSRV May 4, 2022

apoelstra previously approved these changes May 4, 2022

View reviewed changes

tcharding mentioned this pull request May 5, 2022

Error refactoring #820

Open

8 tasks

tcharding added 4 commits May 19, 2022 16:33

Re-order Display match arms

2384712

Put the match arms in the same order as the enum that defines them.

Put Error impl block below Display

7cf8af2

In an effort to be uniform throughout the codebase; put the `std::error::Error` impl block below the `Display` impl block.

Add parenthesis around left hand side of companion

0a9191b

Parenthesis are not needed around this expression but my editor is going mad and cannot format the code without them. Since it does not hurt readability add parenthesis around the expression.

Implement std::error::source codebase wide

97a5bb1

Audit ever error type we have and implement `source` for each.

tcharding mentioned this pull request May 19, 2022

Implement source for error types rust-bitcoin/rust-miniscript#410

Merged

tcharding marked this pull request as draft May 19, 2022 06:42

tcharding dismissed apoelstra’s stale review via 97a5bb1 May 20, 2022 00:11

tcharding force-pushed the error-cause branch from 74dc8ef to 97a5bb1 Compare May 20, 2022 00:11

tcharding marked this pull request as ready for review May 20, 2022 00:27

sanket1729 approved these changes May 20, 2022

View reviewed changes

apoelstra approved these changes May 21, 2022

View reviewed changes

apoelstra merged commit 0e82376 into rust-bitcoin:master May 21, 2022

tcharding deleted the error-cause branch May 23, 2022 22:39

tcharding mentioned this pull request May 25, 2022

Improve error Display implementations #1003

Merged

tcharding mentioned this pull request Jun 24, 2022

Errors should return source instead of displaying it #831

Closed

tcharding modified the milestone: 0.29.0 Jun 24, 2022

Kixunil added the minor API Change This PR should get a minor version bump label Aug 1, 2022

Kixunil added this to the 0.29.0 milestone Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `std::error::Error` for the new MSRV #987

Implement `std::error::Error` for the new MSRV #987

tcharding commented May 4, 2022 •

edited

apoelstra left a comment

tcharding commented May 4, 2022

apoelstra commented May 5, 2022

RCasatta commented May 5, 2022

tcharding commented May 5, 2022

tcharding commented May 5, 2022 •

edited

tcharding commented May 19, 2022

elichai commented May 19, 2022

apoelstra commented May 19, 2022

sanket1729 left a comment

sanket1729 May 20, 2022

tcharding May 24, 2022

apoelstra left a comment

Kixunil commented May 24, 2022

tcharding commented May 25, 2022 •

edited

Kixunil commented May 25, 2022

apoelstra commented May 25, 2022

Kixunil commented May 25, 2022

tcharding commented May 25, 2022

Implement std::error::Error for the new MSRV #987

Implement std::error::Error for the new MSRV #987

Conversation

tcharding commented May 4, 2022 • edited

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented May 4, 2022

apoelstra commented May 5, 2022

RCasatta commented May 5, 2022

tcharding commented May 5, 2022

tcharding commented May 5, 2022 • edited

tcharding commented May 19, 2022

elichai commented May 19, 2022

apoelstra commented May 19, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 May 20, 2022

Choose a reason for hiding this comment

tcharding May 24, 2022

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented May 24, 2022

tcharding commented May 25, 2022 • edited

Kixunil commented May 25, 2022

apoelstra commented May 25, 2022

Kixunil commented May 25, 2022

tcharding commented May 25, 2022

Implement `std::error::Error` for the new MSRV #987

Implement `std::error::Error` for the new MSRV #987

tcharding commented May 4, 2022 •

edited

tcharding commented May 5, 2022 •

edited

tcharding commented May 25, 2022 •

edited