Skip to content

Add explicit impls for std::error::Error for all error types#5919

Open
mpbagot wants to merge 8 commits intorust-bitcoin:masterfrom
mpbagot:feature/error-impl-explicit
Open

Add explicit impls for std::error::Error for all error types#5919
mpbagot wants to merge 8 commits intorust-bitcoin:masterfrom
mpbagot:feature/error-impl-explicit

Conversation

@mpbagot
Copy link
Copy Markdown
Contributor

@mpbagot mpbagot commented Mar 29, 2026

The std::error::Error impls on many error types rely on the default implementation for the trait, which returns None. While these are (presumably) correct, explicit return values make the functionality more obvious to later developers without requiring pre-existing knowledge of the trait or needing to search documentation.

  • Patch 1 changes the std::error::Error impls in units to be explicit in their function.
  • Patch 2 changes the std::error::Error impls in primitives to be explicit in their function.
  • Patch 3 changes the std::error::Error impls in consensus_encoding to be explicit in their function.
  • Patch 4 changes the std::error::Error impls in hashes to be explicit in their function.
  • Patch 5 changes the std::error::Error impls in base58 to be explicit in their function.
  • Patch 6 changes the std::error::Error impls in bitcoin to be explicit in their function.
  • Patch 7 changes the std::error::Error impls in p2p to be explicit in their function.
  • Patch 8 updates the API files

Closes #5908

@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-consensus_encoding PRs modifying the consensus-encoding crate C-primitives C-base58 PRs modifying the base58 crate C-p2p labels Mar 29, 2026
@mpbagot mpbagot marked this pull request as ready for review March 29, 2026 17:04
0xa507ea32, 0x6fab9537, 0x17406110, 0x0d8cd6f1, 0xcdaa3b6d, 0xc0bbbe37, 0x83613bda,
0xdb48a363, 0x0b02e931, 0x6fd15ca7, 0x521afaca, 0x31338431, 0x6ed41a95, 0x6d437890,
0xc39c91f2, 0x9eccabbd, 0xb5c9a0e6, 0x532fb63c, 0xd2c741c6, 0x07237ea3, 0xa4954b68,
0x4c191d76,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original format is better, I'm surprised to see this since this code is old as hell. I 100% do not understand exactly when the formatter runs on files. These need a skip please mate.

mut msg2_b,
mut msg3_a,
mut msg3_b,
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original is better IMO.

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 670acd8

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 30, 2026

Mad, thanks man!

Did you do anything to check if there were already impls on enums that have all variants without inner errors that do not match on self? Said another way, are you confident that we match explicitly on all error enums now?

@mpbagot
Copy link
Copy Markdown
Contributor Author

mpbagot commented Mar 30, 2026

Did you do anything to check if there were already impls on enums that have all variants without inner errors that do not match on self? Said another way, are you confident that we match explicitly on all error enums now?

I didn't do anything intelligent here. I only did a regex search in VSCode for source\(&self\) -> Option<&\(dyn std::error::Error \+ 'static\)> \{.*None \} and clicked through the 63 results and checked the error type. Every one of them is an impl on a struct error type.
In response to your question I also asked Claude to double check if it noticed anything. It (as LLMs are wont to do) agreed that it was fine.

On another note, it brought up the enums that have matches with ors on the arms, like:

#[cfg(feature = "std")]
impl std::error::Error for PrevoutsIndexError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self {
            Self::InvalidOneIndex | Self::InvalidAllIndex => None,
        }
    }
}

I used an arm for each enum variant. Should I add a patch to change errors like these to use an arm per variant, or should I go back and change the new ones to use ors? Or is a mismatch fine in this case?

@tcharding
Copy link
Copy Markdown
Member

I don't think its too bigger deal (I didn't notice during review) but I"d personally favour using the | form just because it achieves the same goal, is just as readable, and is more terse.

@tcharding
Copy link
Copy Markdown
Member

In response to your question I also asked Claude to double check if it noticed anything.

What about prompt it with 'print the impls of all error::Error functions that are define on enums' and then just grep that for None?

@apoelstra
Copy link
Copy Markdown
Member

670acd8 needs rebase

@tcharding
Copy link
Copy Markdown
Member

The formatting changes are problematic. Fixed in #5927

@tcharding
Copy link
Copy Markdown
Member

All good except fmt stuff. Reviewed 31d7595

mpbagot added 8 commits April 1, 2026 01:32
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None.
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None. For enum errors, also return None for each match arm.
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None.
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None.
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None.
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None. For enum errors, also return None for each match arm.
The std::error::Error impls on many error types rely on the default
implementation for the trait, which returns None. While these are
correct, explicit return values make the functionality more obvious to
later developers without requiring pre-existing knowledge of the trait
or needing to search documentation.

Replace all default impls of std::error::Error with explicit impls
returning None. For enum errors, also return None for each match arm.
As part of introducing explicit impls for std::error::Error, various
error types are now recorded as including a source() impl. To satisfy
the API checker, the files must be updated.

Update API files with just check-api
@mpbagot mpbagot force-pushed the feature/error-impl-explicit branch from 31d7595 to d2cfff4 Compare March 31, 2026 14:33
@mpbagot
Copy link
Copy Markdown
Contributor Author

mpbagot commented Mar 31, 2026

Rebased and dropped the formatting patch

@tcharding
Copy link
Copy Markdown
Member

d2cfff4 needs rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-base58 PRs modifying the base58 crate C-bitcoin PRs modifying the bitcoin crate C-consensus_encoding PRs modifying the consensus-encoding crate C-hashes PRs modifying the hashes crate C-p2p C-primitives C-units PRs modifying the units crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Be uniform in impls of std::error::Error when returning None

3 participants