Add `check_pub_error` macro by tcharding · Pull Request #1743 · rust-bitcoin/rust-bitcoin

tcharding · 2023-03-23T23:21:59Z

Public errors are part of the public API, as such we need to be careful not to change them. One thing that is hard to notice is adding/removing Send/Sync.

Add a macro that asserts that an error type implements Send, Sync and std::error::Error (feature guarded on test and feature = "std").

Based on idea by @Kixunil: #1127 (comment)

Notes since first raising PR

All the release job discussion below is resolved now.

tcharding · 2023-03-23T23:26:07Z

I'm not sure why the release job cargo publish -p bitcoin --dry-run fails to find the new macro?

tcharding · 2023-03-23T23:48:05Z

All the force pushes are me rebasing on #1742 trying to get it past CI.

sanket1729

concept ACK. Left one question

sanket1729 · 2023-03-23T23:56:25Z

internals/src/error.rs

Why is this pub and why do we need a trait? Any drawbacks of creating this in #cfg(test) for this? We can also do this with function without the trait.

#[cfg(test) fn assert_send_sync<T: Send + Sync + Debug> {} macro_rules check_pub_error { #[cfg(test)] assert_send_sync<$type>(); }

It's pub because it's in internals and reused. We can define the function inside macro, hygiene should take care of conflicts. Unless someone can prove it slows down compilation.

I ended up using a trait still but feature guarding so the trait is not built in in non-test builds. Ok?

Sure. I am okay as long it is only in tests.

Kixunil · 2023-03-24T06:15:04Z

assert Debug is implemented

We should just check for std::error::Error on std.

apoelstra · 2023-03-24T13:30:24Z

Hmm, shitty.

So @tcharding the reason CI is failing is that cargo publish tries to use the version of internals that's already on crates.io. So if we add literally anything to internals ... or to hashes ... then the release job will fail.

On the one hand, this is exactly what we want to happen (it's telling us that we can't release bitcoin without releasing internals fist). But on the other hand, we're about 48 hours into this release and we're already getting a failure from the CI job, so I guess we've gotta figure out how to get it to be yellow instead of red :)

Kixunil · 2023-03-24T15:53:02Z

Sounds like detecting release PRs is a better solution.

apoelstra · 2023-03-24T20:33:20Z

@tcharding could you try adding a commit which does that, to see if it makes CI pass?

I think that wrapping the publish checks in if git diff --quiet */Cargo.toml; then ... fi will do it.

tcharding · 2023-03-28T02:52:27Z

Spent more time playing with bash than I wanted to, will try and fix the release thing again tomorrow.

Kixunil

Concept ACK.

Kixunil · 2023-05-10T08:16:30Z

internals/src/error.rs

Perhaps we should also check that no_std errors are still Send + Sync?

apoelstra · 2023-05-10T15:16:02Z

Neat! ACK. Part of me feels like bikeshedding the name, but this is good.

Perhaps we should have similar macros for primitives e.g. to make sure that they all implement clone/partialeq/eq/hash/debug/display/fromstr. Unsure. At some point it gets unweildy to do this with macros that we have to remember to call inline.

apoelstra

ACK 928635ded299622bb08436fc44fb7028b0377b3a

stevenroose

Left a suggestion.

stevenroose · 2023-05-10T16:39:30Z

internals/src/error.rs

Still not super happy this is pub.

I would suggest the following changes:

have a single macro and have a ($type:ty) => {}; rule for non-test, non-empty rule for test

do Sanket's approach for methods, have one method fn check_sendsync<T: Send + Sync>() and fn check_error<T: std::error::Error>() and gate the second on std.

Currently there is a bug that non-std test runs will not be able to build.

I'm not happy about it either but seems less work for compiler (and compile times!). It's hidden which is a clear signal people shouldn't use it (and internals is also such signal). If anyone is insane enough to depend on it they deserve the consequences.

I used you suggestions @stevenroose except I was unable to put an attribute on the arms of the macro so I kept the dummy macro for not test builds.

tcharding · 2023-05-10T23:07:43Z

Changes in force push:

Uses suggestion by @sanket1729 to use a function instead of a trait.
Uses suggestion by @stevenroose to separate the std and non-std checks.

I also checked that the macro is used for all errors (I grepped for std::error::Error so ironically if any error types are missing this impl then I'll have missed them).

apoelstra · 2023-05-11T19:06:51Z

I'm a little surprised that this works now without the functions being pub

apoelstra

ACK 70054bcffa51a43e60fb11f0fb99035418b1c7f1

tcharding · 2023-05-12T01:17:51Z

I'm a little surprised that this works now without the functions being pub

I was surprise also, I tried to work out why it works and I cannot. (Also the comment on dead_code is wrong, will fix it and force push.)

Kixunil · 2023-05-16T06:44:25Z

internals/src/error.rs

Maybe we should use bare cfg, not feature, so this doesn't get pulled in when --all-features is used?

I used #[cfg(trait_checks)] and I enabled it in all builds by exporting RUSFLAGS from the top level contrib/test.sh script.

We should have at least one build without it in case something gets messed up.

Oh yes, very good point.

Kixunil · 2023-05-16T06:45:06Z

internals/src/error.rs

Could also check Debug + Display for no_std.

Done as a separate trait.

Kixunil · 2023-05-16T06:46:17Z

bitcoin/src/taproot.rs

Unrelated but it looks like this should be called HiddenNodesError or be withing an error module.

Noted, thanks.

tcharding · 2023-05-16T23:25:55Z

Changes in force push:

Use #[cfg(trait_checks)] instead of feature
Check for Display/Debug for all builds

tcharding · 2023-05-17T22:43:54Z

Changes in force push:

Changed cfg option to be check_traits (instead of trait_checks) so its a verb in line with "bench" and "bitcoin_fuzz".
Changed CI to explicitly call cargo build with RUSTFLAGS set instead of setting for all builds.

tcharding · 2023-05-18T00:19:14Z

ooo, I had missed all the public errors that were structs (as opposed to enums). Now included.

tcharding · 2023-05-18T02:18:20Z

Sweeet, macro finds to unusual error types, they were added in commit: 1a2cf2681 Implement consensus encoding adapter for serde by you @Kixunil. Are we happy with DecodeError and DecodeInitError not implementing the usual traits?

I've added FIXME to mark the two errors.

Kixunil · 2023-05-18T11:38:22Z

Good question, IIRC those errors are meant to be converted to serde errors. But adding the impls wouldn't be too bad. I think we should check Send + Sync at least.

tcharding · 2023-05-18T23:32:01Z

I elected to implement Display and std::error::Error for both types, added as an initial patch.

bitcoin/src/consensus/serde.rs

Kixunil · 2023-05-19T08:36:42Z

bitcoin/src/consensus/serde.rs

write_err

I'm not totally sure on the best content to put in the error message of write_err, noting it here but can discuss on #1862.

internals/src/error.rs

We have two "helper" error types (used within serde code to hide implementation details) that do not implement the "standard" error traits that we implement. While not totally necessary it does not hurt to implement them, adding only a few lines of easy to maintain code. Implement `Display` and `std::error::Error` for helper error types in the `consensus::serde` module (`DecodeError` and `DecodeInitError`).

tcharding · 2023-06-06T06:35:31Z

I rebased this one to fix merge conflicts, but to do the rebase properly I have to go through the whole codebase again and check no new errors were introduced since I last did this. I can't be bothered doing that right now. Can we just agree that this PR might miss adding the macro to some errors, and keep an eye out?

Public errors are part of the public API, as such we need to be careful not to change them. One thing that is hard to notice is adding/removing `Send`/`Sync`. Add a macro that asserts that an error type implements `Send` and `Sync`. While we are at it, assert `Debug` is implemented as well because we would like all errors to implement `Debug`.

sanket1729

ACK c406cc2. This looks great. Did not check whether all parts are covered

apoelstra · 2023-06-07T12:31:02Z

Honestly now I'm a little tempted to scrap this PR in favor of just using some creative greping on the output of #1880, which would be resilient against new errors being introduced.

tcharding · 2023-06-08T05:08:06Z

That's not a bad idea because it would put further "coding policy" things into the same place instead of having a bunch of macros that one must remember to call (assuming we come up with more things like that introduced here).

tcharding · 2023-06-09T15:44:14Z

Draft until #1880 resolves.

tcharding · 2023-12-06T04:14:07Z

Looks like we are going to use cargo public-api to solve this problem.

ref: rust-bitcoin/hex-conservative#34

Kixunil · 2023-12-06T10:49:34Z

@tcharding it'd still be nice to have some tool to check that the API is good in the first place (e.g. an error type implements std::error::Error). cargo public-api helps with the biggest footguns though.

apoelstra · 2023-12-06T18:10:00Z

@Kixunil right -- after bringing in cargo public-api we had a vague plan to write some shell scripts (or Perl or whatever) that would run in CI and do sanity checks like "if a type has Error in its name then it impls std::error::Error".

Unfortunately every specific rule we've thought of has had a bunch of exceptions :) but if you have ones in mind it'd be awesome to start doing it. Maybe in hex-conservative first where we have a much smaller API.

Kixunil · 2023-12-06T18:43:01Z

Well, I think the one you mentioned doesn't have an exception. :) I'd throw in Clone (later, when we get rid of io::Error, Send, and Sync as well, maybe also PartialEq. Another that comes to mind: <T as FromStr>::Err and <T as TryFrom<U>>::Error should end with Error. IIRC there are also some interesting clippy lints which I'd prefer instead of shell scripts.

tcharding force-pushed the 03-24-check-pub-error branch from ee312da to 90a0bf6 Compare March 23, 2023 23:29

tcharding mentioned this pull request Mar 23, 2023

Add policy section to docs #1127

Merged

tcharding force-pushed the 03-24-check-pub-error branch from 90a0bf6 to acd6a1f Compare March 23, 2023 23:47

tcharding force-pushed the 03-24-check-pub-error branch from acd6a1f to 33b04f4 Compare March 23, 2023 23:54

sanket1729 reviewed Mar 23, 2023

View reviewed changes

tcharding force-pushed the 03-24-check-pub-error branch 2 times, most recently from 4f2b992 to b6ca19f Compare March 28, 2023 02:52

tcharding force-pushed the 03-24-check-pub-error branch from b6ca19f to 27d1ec1 Compare March 28, 2023 23:26

tcharding marked this pull request as draft March 28, 2023 23:26

tcharding force-pushed the 03-24-check-pub-error branch from 27d1ec1 to 928635d Compare May 9, 2023 23:29

tcharding marked this pull request as ready for review May 9, 2023 23:29

Kixunil reviewed May 10, 2023

View reviewed changes

internals/src/error.rs Outdated

Copy link
Copy Markdown

Collaborator

Kixunil May 10, 2023

Choose a reason for hiding this comment

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

Perhaps we should also check that no_std errors are still Send + Sync?

apoelstra previously approved these changes May 10, 2023

View reviewed changes

stevenroose requested changes May 10, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via b8c3cb2 May 10, 2023 23:05

tcharding force-pushed the 03-24-check-pub-error branch from 928635d to b8c3cb2 Compare May 10, 2023 23:05

tcharding force-pushed the 03-24-check-pub-error branch 2 times, most recently from 5e4bead to 70054bc Compare May 10, 2023 23:34

apoelstra previously approved these changes May 11, 2023

View reviewed changes

Kixunil reviewed May 16, 2023

View reviewed changes

tcharding force-pushed the 03-24-check-pub-error branch from d442ed9 to ba16ab7 Compare May 16, 2023 23:24

tcharding force-pushed the 03-24-check-pub-error branch from ba16ab7 to 44b3ede Compare May 17, 2023 22:42

tcharding force-pushed the 03-24-check-pub-error branch from 44b3ede to 4ceabb6 Compare May 18, 2023 00:18

tcharding force-pushed the 03-24-check-pub-error branch from 4ceabb6 to 7ecb4d1 Compare May 18, 2023 02:15

tcharding force-pushed the 03-24-check-pub-error branch from 7ecb4d1 to 981cb9b Compare May 18, 2023 23:31

Kixunil reviewed May 19, 2023

View reviewed changes

tcharding force-pushed the 03-24-check-pub-error branch from 981cb9b to e48e282 Compare May 19, 2023 22:17

tcharding force-pushed the 03-24-check-pub-error branch from e48e282 to 2dfce3f Compare June 6, 2023 06:36

tcharding force-pushed the 03-24-check-pub-error branch from 2dfce3f to c406cc2 Compare June 6, 2023 07:07

sanket1729 approved these changes Jun 6, 2023

View reviewed changes

tcharding marked this pull request as draft June 9, 2023 15:44

tcharding closed this Dec 6, 2023

tcharding deleted the 03-24-check-pub-error branch May 22, 2024 23:11

Conversation

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

Uh oh!

Notes since first raising PR

Uh oh!

tcharding commented Mar 23, 2023

Uh oh!

tcharding commented Mar 23, 2023

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Kixunil commented Mar 24, 2023

Uh oh!

apoelstra commented Mar 24, 2023

Uh oh!

Kixunil commented Mar 24, 2023

Uh oh!

apoelstra commented Mar 24, 2023

Uh oh!

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

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented May 10, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

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

tcharding commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented May 11, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tcharding commented Mar 23, 2023 •

edited

Loading

Kixunil Mar 24, 2023 •

edited

Loading

tcharding commented Mar 28, 2023 •

edited

Loading

tcharding commented May 10, 2023 •

edited

Loading

Kixunil May 17, 2023 •

edited

Loading

tcharding commented Jun 6, 2023 •

edited

Loading

Kixunil commented Dec 6, 2023 •

edited by tcharding

Loading