Triage needed: possible semver violation in libp2p-gossipsub

[obi1kenobi]

libp2p-gossipsub at commit hash 3ec7c797 has begun failing semver-checks due to a presumed upstream dependency semver violation. Auto-traits are viral, so the upstream break leaks into libp2p-gossipsub's own public API and breaks its semver too:

$ git checkout 3ec7c797e56a3bdffa9194e685ee83e1c21831c4

$ cargo semver-checks check-release --package libp2p-gossipsub
    Updating index
     Parsing libp2p-gossipsub v0.44.0 (current)
     Parsing libp2p-gossipsub v0.44.0 (baseline, cached)
    Checking libp2p-gossipsub v0.44.0 -> v0.44.0 (no change)
   Completed [   2.165s] 41 checks; 40 passed, 1 failed, 0 unnecessary

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.18.3/src/lints/auto_trait_impl_removed.ron

Failed in:
  type Behaviour is no longer Sync, in /.../rust-libp2p/protocols/gossipsub/src/behaviour.rs:213
       Final [   2.367s] semver requires new major version: 1 major and 0 minor checks failed

To find the problem, I've added this small snippet of code in behaviour.rs that will cause a compilation error with a pretty error message:

fn send_check(_x: impl Sync) {}

fn verifier(b: Behaviour) {
    send_check(b)
}

Compiler error output

    Checking libp2p-gossipsub v0.44.0 (/.../rust-libp2p/protocols/gossipsub)
error[E0277]: `(dyn muxing::boxed::AsyncReadWrite + std::marker::Send + 'static)` cannot be shared between threads safely
   --> protocols/gossipsub/src/behaviour.rs:207:16
    |
207 |     send_check(b)
    |     ---------- ^ `(dyn muxing::boxed::AsyncReadWrite + std::marker::Send + 'static)` cannot be shared between threads safely
    |     |
    |     required by a bound introduced by this call
    |
    = help: the trait `Sync` is not implemented for `(dyn muxing::boxed::AsyncReadWrite + std::marker::Send + 'static)`
    = note: required for `Unique<(dyn muxing::boxed::AsyncReadWrite + std::marker::Send + 'static)>` to implement `Sync`
    = note: required because it appears within the type `Box<dyn AsyncReadWrite + Send>`
    = note: required because it appears within the type `Pin<Box<dyn AsyncReadWrite + Send>>`
    = note: required because it appears within the type `SubstreamBox`
    = note: required because it appears within the type `State<SubstreamBox>`
    = note: required because it appears within the type `Negotiated<SubstreamBox>`
    = note: required because it appears within the type `Fuse<Negotiated<SubstreamBox>, GossipsubCodec>`
    = note: required because it appears within the type `FramedWrite2<Fuse<Negotiated<SubstreamBox>, GossipsubCodec>>`
    = note: required because it appears within the type `FramedRead2<FramedWrite2<Fuse<Negotiated<SubstreamBox>, GossipsubCodec>>>`
    = note: required because it appears within the type `Framed<Negotiated<SubstreamBox>, GossipsubCodec>`
note: required because it appears within the type `OutboundSubstreamState`
   --> protocols/gossipsub/src/handler.rs:148:6
    |
148 | enum OutboundSubstreamState {
    |      ^^^^^^^^^^^^^^^^^^^^^^
    = note: required because it appears within the type `Option<OutboundSubstreamState>`
note: required because it appears within the type `Handler`
   --> protocols/gossipsub/src/handler.rs:85:12
    |
85  | pub struct Handler {
    |            ^^^^^^^
    = note: required because it appears within the type `NetworkBehaviourAction<Event, Handler, HandlerIn>`
    = note: required for `Unique<NetworkBehaviourAction<Event, handler::Handler, HandlerIn>>` to implement `Sync`
    = note: required because it appears within the type `RawVec<NetworkBehaviourAction<Event, Handler, HandlerIn>>`
    = note: required because it appears within the type `VecDeque<NetworkBehaviourAction<Event, Handler, HandlerIn>>`
note: required because it appears within the type `Behaviour`
   --> protocols/gossipsub/src/behaviour.rs:221:12
    |
221 | pub struct Behaviour<D = IdentityTransform, F = AllowAllSubscriptionFilter> {
    |            ^^^^^^^^^
note: required by a bound in `send_check`
   --> protocols/gossipsub/src/behaviour.rs:202:24
    |
202 | fn send_check(_x: impl Sync) {
    |                        ^^^^ required by this bound in `send_check`

For more information about this error, try `rustc --explain E0277`.

I am not currently seeing this problem on the current tip of master (1a9cf4f7760724032b729c43165716c7ecd842ad).

To be honest, I'm not exactly sure how that's possible, since the problematic commit only started failing in the last 48h — I know because it's part of the cargo-semver-checks integration suite and it wasn't failing 48h ago but failed now: https://github.com/obi1kenobi/cargo-semver-checks/actions/runs/4267893704/jobs/7429959838

This is very unusual, and probably worth investigating. I'd love your help!

[obi1kenobi]

It seems that the libp2p-gossipsub-v0.44.0 tag is actually in the future relative to the affected hash (3ec7c79). This is strange since the affected hash already shows v0.44.0 in the Cargo.toml for that package. Does your workflow perhaps cause updates to the version number in Cargo.toml before the version is published?

Bisection shows that this is the commit where the semver violation stops being reported: caed1fe2#diff-368386d6c30a7eb4d60fcddda689bba01e9e215451a387fa58ff93b12fb914de

[obi1kenobi]

I noticed at https://crates.io/crates/libp2p-gossipsub/versions that v0.44.0 was just released ~16h ago, so I'm guessing that libp2p-gossipsub uses "future versioning" where the Cargo.toml version number may either be the most-recently-released version or a still-unreleased version.

In that case, there's no semver violation here since the baseline ("crates.io 0.44.0") is actually newer than the "current" (the git hash in question).

But there is a hazard for a future semver violation that you may not have noticed: the Behaviour type is now Sync (as of the bisected commit), and semver requires a major version if it ever stops being Sync. This is a good example of something that might be a good warn-level API evolution lint for the future.

[thomaseizinger]

uses "future versioning" where the Cargo.toml version number may either be the most-recently-released version or a still-unreleased version.

Yes we bump the version as part of PRs to ensure we don't forget that there was a breaking change. I do want to change that though, see #2902.

But there is a hazard for a future semver violation that you may not have noticed: the Behaviour type is now Sync (as of the bisected commit), and semver requires a major version if it ever stops being Sync. This is a good example of something that might be a good warn-level API evolution lint for the future.

Damn. Sometimes I wish auto-traits wouldn't exist in Rust and you would just have to write #[derive(Send, Sync)].