Remove feature gated enum variants by tcharding · Pull Request #874 · rust-bitcoin/rust-bitcoin

tcharding · 2022-03-10T05:52:22Z

Feature gating enum variants makes code that uses the library brittle while we do not have non_exhaustive, we should avoid doing so. Instead we can add a dummy type that is available when the feature is not turned on. Doing so enables the compiler to enforce that we do not create the error type that is feature gated when the feature is not enabled.

Remove the feature gating around bitcoincensensus featured error enum variants.

Patch 1 is a trivial preparatory change to the Display impl.

Closes: #645

In preparation for writing a formatted string refactor to call `write!` on every match arm.

Feature gating enum variants makes code that uses the library brittle while we do not have `non_exhaustive`, we should avoid doing so. Instead we can add a dummy type that is available when the feature is not turned on. Doing so enables the compiler to enforce that we do not create the error type that is feature gated when the feature is not enabled. Remove the feature gating around `bitcoincensensus` featured error enum variants. Closes: rust-bitcoin#645

Kixunil

The extra traits defeat the purpose, so they definitely have to be removed. I'm still not convinced this is useful. It's not an RC fix and we will bump MSRV in the next version. I hope one of the first tasks. If you want this to be an RC fix, I'm not strongly opposed. It's not a super hard change but a bit tricky - e.g. one can shoot himself into the foot by implementing extra traits. :) I would require a careful review.

Kixunil · 2022-03-10T11:18:25Z

src/blockdata/script.rs

+
+/// Dummy error type used when `bitcoinconsensus` feature is not enabled.
+#[cfg(not(feature = "bitcoinconsensus"))]
+#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Clone, Copy)]


For the backcompat trick to work you may only implement traits that bitcoinconsensus::Error implements

Regarding derives on dumb error types: while we should not underderive, we can overderive. I.e., if the upstream adds derives to the respective error types this will not be a breaking change for them, but it will break our code. Thus, I suggest to do all possible derives for dumb errors, namely Copy, Eq, PartialEq, Ord, PartialOrd, Hash, Debug (Default for errors is a bad practice, I assume).

@dr-orlovsky I think you're wrong here because the feature gate removes our type and replaces it with bitcoinconsensus::Error, meaning that if we overderive, the feature-gate will remove derives and potentially cause compilation breakage.

This is tested in CI in this project, while underderivation may break downstream without CI in this project detecting it (when upstream adds more derives to the error type).

Ha! This discussion is mute now, the link above is to bitcoinconsensus v0.17.1 but we are depending on v0.19.0-3 which has all the derives on it that we want.

My bad for blindly following web rendered docs without checking the version or the code :(

@dr-orlovsky we can not test others people code in the CI and overderiving makes this entire PR pointless. @apoelstra is perfectly correct.

However @tcharding has a point with bitcoinconsensus version, so it's OK.

BTW, sorry for being late, I was super busy and then sick. I feel better now but still not 100%.

Good to have you back man, hope you get fully recovered soon.

apoelstra · 2022-03-10T14:13:44Z

@Kixunil did you mean to comment on a different PR? I don't see any new traits here.

apoelstra · 2022-03-10T14:14:50Z

src/blockdata/script.rs

-            #[cfg(feature="bitcoinconsensus")]
-            Error::UnknownSpentOutput(ref _point) => write!(f, "unknown spent output Transaction::verify()"),
-            #[cfg(feature="bitcoinconsensus")]
+            Error::BitcoinConsensus(ref err) => write!(f, "bitcoinconsensus verification failed: {:?}", err),


In d9d2755:

I think we're not supposed to prefix errors, instead using source/cause for them to let smart error formatters do smart things with them. But I won't hold up this PR -- probably it needs to wait for the MSRV bump so that we can use source and do a more general error cleanup.

apoelstra

ACK d9d2755

@Kixunil

Oops, I just understood what @Kixunil meant. Good catch!

Kixunil · 2022-03-10T18:58:20Z

Explanation in case it's still confusing to someone else: the point of having dummy type is that whatever code does with it is also valid with actual error type when the feature is turned on to ensure additiveness of features. Thus the dummy type can only have functionality equal or less than the actual type. (Liskov principle in some sense.) In this case the traits Copy , Hash, Ord, PartialOrd are not implemented for bitcoinconsensus::Error.

tcharding · 2022-03-10T20:38:41Z

I was just cleaning up old issues, with the comments above I think I'll just close this one. I'm not really sure what's the best approach to closing #645 once we bump MSRV, perhaps one of you could throw a comment on the issue if you have a sec. Otherwise I'll come back to it later, no doubt.

apoelstra · 2022-03-10T20:49:18Z

@tcharding I think it is still worth fixing this, since the current situation is really bad. If you remove the extra derived traits I'll ACK the PR as-is.

Kixunil · 2022-03-15T15:40:31Z

Hmm, given how long releases take I'm thinking of agreeing. Well, I wasn't strongly opposed to it anyway.

Once we bump MSRV we can just slap #[non_exhaustive] on the Error enum and it's done. :)

tcharding · 2022-03-15T22:56:36Z

I've rebased the branch on my tree, removed the first patch, fixed the derive issue, made type alias pub and checked the docs buid is sane. I do not have permissions to re-open the PR, are you able to do that @apoelstra please? I don't want to open a new PR and break the thread.

tcharding · 2022-03-15T22:57:30Z

Might want an rc-fix label too so we don't forget to merge it?

Kixunil · 2022-03-16T17:03:58Z

I believe you have to create a new one because of force push.

tcharding · 2022-03-16T20:04:08Z

ok, will do. Cheers

6ad2902 Remove feature gated enum variants (Tobin Harding) Pull request description: This is the updated version of #874 (which I closed, force pushed, and then was unable to re-open - my bad). Feature gating enum variants makes code that uses the library brittle while we do not have `non_exhaustive`, we should avoid doing so. Instead we can add a dummy type that is available when the feature is not turned on. Doing so enables the compiler to enforce that we do not create the error type that is feature gated when the feature is not enabled. Remove the feature gating around `bitcoinconsensus` error enum variants. Closes: #645 ACKs for top commit: sanket1729: tACK 6ad2902. This is an improvment. dr-orlovsky: ACK 6ad2902 Tree-SHA512: 07d8c6b500d2d5b92e367b89e296b86bec046bab4fe9f624eb087d52ea24a900d7f7a41a98065949c67b307a1f374a7f4cf1b77cb93b6cf19e3d779c27fd7f1d

tcharding added 2 commits March 10, 2022 16:37

Use write! for each match arm

ea9fb93

In preparation for writing a formatted string refactor to call `write!` on every match arm.

Kixunil requested changes Mar 10, 2022

View reviewed changes

apoelstra reviewed Mar 10, 2022

View reviewed changes

apoelstra previously approved these changes Mar 10, 2022

View reviewed changes

tcharding closed this Mar 10, 2022

tcharding mentioned this pull request Mar 16, 2022

Remove feature gated enum variants #881

Merged

Conversation

tcharding commented Mar 10, 2022

Uh oh!

Kixunil 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Mar 10, 2022

Uh oh!

tcharding commented Mar 10, 2022

Uh oh!

apoelstra commented Mar 10, 2022

Uh oh!

Kixunil commented Mar 15, 2022

Uh oh!

tcharding commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Mar 16, 2022

Uh oh!

tcharding commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Mar 15, 2022 •

edited

Loading

tcharding commented Mar 15, 2022 •

edited

Loading