*: Don't leak prost dependency in error types

[thomaseizinger]

Description

Whilst reviewing #3050, we discovered that we are currently leaking the prost dependency through our error types in various places. This would make https://github.com/libp2p/rust-libp2p/pull/3050/files a breaking change despite being entirely an implementation detail about our build(!) setup.

This PR aims to resolve that hiding the concrete error in the variants with a newtype error that forwards source and Display to its inner type. This preserves the Display chain when using wrappers like anyhow.

Patch-by-patch review is recommended.

Links to any relevant issues

• Migrate from protoc to rust-protobuf #3050

Open Questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[mxinden]

Looks good to me. Thanks for the many simplifications, e.g. using prost-codec. Thanks for the patch-by-patch pull request.

[rkuhn]

While this change improved the code in many places, I wonder about the definition of “breaking change”: is it not the case that wrapping prost errors in io::Error shifts the breakage from compile-time to runtime? The dynamic error type will change in any case, so why is it preferable to not catch this using the type system?

[thomaseizinger]

While this change improved the code in many places, I wonder about the definition of “breaking change”: is it not the case that wrapping prost errors in io::Error shifts the breakage from compile-time to runtime? The dynamic error type will change in any case, so why is it preferable to not catch this using the type system?

It depends on what we consider the public API to be. In my view, handing an opaque error type to someone should communicate that "this is all you are gonna get". If I wanted to commit to a more specific API, I would have used an error enum with concrete variants.

Depending on the Display text or downcasting the inner error via source to something specific is outside of the public API in my opinion and thus changing it is not a breaking change to me.

Do you have a particular case at hand where changing our protobuf implementation (with this patch in place) breaks things for you?

[rkuhn]

This is a reasonable stance, but it makes it impossible to selectively recover from those hidden error causes without risking runtime breakage. I don’t have an example with prost, but I do have some fresh experience with trying to detect the effects of a TCP simultaneous open.

In this (PR) case I think it makes sense to hide the detailed cause. After your above explanation my only remaining point is that this obfuscation should in general be a conscious and justified decision — not “leaking a dependency” is not strong enough for my taste (the error type could just be re-exported). Not leaking an aspect of the build setup is stronger.

[mxinden]

not “leaking a dependency” is not strong enough for my taste (the error type could just be re-exported). Not leaking an aspect of the build setup is stronger.

The motivation here is that we want to move away from prost. Instead of making the breaking change when moving away, this pull request does the breaking change early by removing prost from our public API already`. Does that make sense @rkuhn?

While this change improved the code in many places, I wonder about the definition of “breaking change”: is it not the case that wrapping prost errors in io::Error shifts the breakage from compile-time to runtime? The dynamic error type will change in any case, so why is it preferable to not catch this using the type system?

It depends on what we consider the public API to be. In my view, handing an opaque error type to someone should communicate that "this is all you are gonna get". If I wanted to commit to a more specific API, I would have used an error enum with concrete variants.

Depending on the Display text or downcasting the inner error via source to something specific is outside of the public API in my opinion and thus changing it is not a breaking change to me.

For what it is worth, I share this view.

[rkuhn]

Yes, we are in agreement.