Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add non_exhaustive to all error enums #1026

Merged

Conversation

tcharding
Copy link
Member

Adding an error variant to a public enum is an API breaking change, this means making, what could be, small refactorings or improvements harder. If we use non_exhaustive for error types then we mitigate this cost.

There is a tradeoff however, downstream users who explicitly match on our public error types must include a wildcard pattern.

Adding an error variant to a public enum is an API breaking change, this
means making what could be small refactorings or improvements harder. If
we use `non_exhaustive` for error types then we mitigate this cost.
There is a tradeoff however, downstream users who explicitly match on
our public error types must include a wildcard pattern.
@dpc
Copy link
Contributor

dpc commented May 31, 2022

I could imagine there could be errors that are 99% guaranteed not to need addition in foreseeable feature, but #[non_exhaustive] is a good default.

@tcharding tcharding added the API break This PR requires a version bump for the next release label May 31, 2022
@tcharding
Copy link
Member Author

I thought there might be but I do not feel qualified to judge.

Perhaps we default to using non_exhaustive on all errors with an explicit // comment if not needed? If anyone wants to throw a quick comment on this thread about current errors that do not need non_exhaustive I can add them to this PR.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 99f565f

@apoelstra
Copy link
Member

I agree that there are probably error types we don't need this for, but I'd rather have this everywhere than nowhere, and I'm not very interested in finding and arguing for exceptions.

@tcharding
Copy link
Member Author

lol, I love it :)

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 99f565f

Regarding exceptions, I agree it's better to add too many non_exhaustive than too few. In my experience/opinion the best candidates for committing to the variants are errors of simple encodings - hex, base58, bech32 could probably be exceptions one day but even then I'd only do it after some time and careful thinking. Removing it is backwards-compatible, so that's fine.

Note that I often hide my error types completely by putting enums into structs. The point is I'm not even promising there will be any variants existing or that it'll be enum forever. Inspecting the type can still be provided using carefully designed methods.

@apoelstra apoelstra merged commit 50489c8 into rust-bitcoin:master Jun 1, 2022
@tcharding tcharding deleted the 05-31-non-exhaustive-errors branch June 1, 2022 21:37
@sanket1729
Copy link
Member

sanket1729 commented Jun 1, 2022

Agreed, this is a good default. I might come back with a few complaints asking for exceptions after migrating to next release.

ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
… enums

99f565f Add non_exhaustive to all error enums (Tobin C. Harding)

Pull request description:

  Adding an error variant to a public enum is an API breaking change, this means making, what could be, small refactorings or improvements harder. If we use `non_exhaustive` for error types then we mitigate this cost.

  There is a tradeoff however, downstream users who explicitly match on our public error types must include a wildcard pattern.

ACKs for top commit:
  apoelstra:
    ACK 99f565f
  Kixunil:
    ACK 99f565f

Tree-SHA512: ff329f87d52b3fbe24654f32e4062ddae73173cba5a13d511591158e68ee278e9bdc0a70a3e0b42d6606b369255923f9c46d8b3d1b2ff75f8461a82567df80cd
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API break This PR requires a version bump for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants