Add #[non_exhaustive] to protocol event enums

[dhuseby]

While working on PR #4503 a short discussion came up about how adding an emitted event to the Kademlia protocol was a breaking change. I wondered out loud to @thomaseizinger if we should also add #[non_exhaustive] to the event enum as part of this breaking change so that in the future it won't be a breaking change to add more events. I brought the discussion here to surface it and build some consensus.

Proposal:

• Add #[non_exhaustive] to the event enum for protocols as a one-time breaking change.

Pros:

• Adding events to the protocol enum in the future won't trigger any compile warnings.
• Doesn't affect any code within the protocol crate is that attribute is ignored per the RFC.
• Changing an existing event does trigger compile warnings.
• Most downstream code already has a wildcard arm: _ => {}

Cons:

• All downstream dependent code that matches against the event enum must have a wildcard arm: _ => {}
• The only way to get notified of new events in the enum is to enable a—currently unstable—lint check: Tracking Issue for #![feature(non_exhaustive_omitted_patterns_lint)] rust-lang/rust#89554

Thoughts? @thomaseizinger mentioned that we had considered it before but I couldn't find any previous posts in theses discussions or on discuss.libp2p.io.

[thomaseizinger]

The more I think about it, the more I like this idea. I'll bring it up in today's maintainer call: #4556

[thomaseizinger]

Here is the full description of this feature: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute

One thing that is nice and I didn't know is that it can also be applied to enum variants which allows you to add more fields to it without it being a breaking change.

As an example, if we would have non_exhaustive on FromSwarm, we could ship #4371 as a non-breaking change which would be quite nice.

Similiarly, if all Events would have non_exhaustive applied to their variants, we could have shipped #4527 as a non-breaking change.

[thomaseizinger]

Perhaps a suitable policy would be to at least apply this to enums from libp2p-core and libp2p-swarm. Breaking changes to those have a cascading effect across the workspace and often hold us back in shipping certain features.

[dhuseby]

Don't forget that adding the attribute is itself a breaking change so we'll have to do it carefully and in a controlled way.

[mxinden]

I am in favor of the proposal. I think we should consider #[non_exhaustive] for multiple pub enums across the code base. Thanks @dhuseby for the suggestion and thanks @thomaseizinger for bringing it up in the last open maintainers call.

Another con: in case we ever have a semantically breaking change, we can no longer force a user to handle the case of the change by adding a new enum variant. Though off the top of my head, I can not think of such a need in the past.

[dhuseby]

Next steps are to decide on which enums get this and which ones don't. @thomaseizinger you said FromSwarm and ToSwarm? What else?

[thomaseizinger]

Probably:

• SwarmEvent
• ConnectionHandlerEvent
• ConnectionEvent

[thomaseizinger]

I think we must not release such changes to libp2p-swarm-derive as patch releases.

Any ideas on how we can prevent that? I can already see us running into this problem because it is super subtle and we just forget about that.

Should we add a comment next to version = in libp2p-swarm-derive's Cargo.toml that says something like "READ THIS BEFORE YOU BUMP THE VERSION" and link to a document explaining how to do the bump?

[thomaseizinger]

So, actually, I think the best solution to this is to always pin the libp2p-swarm-derive dependency in libp2p-swarm. That way, it can never get out of sync, regardless of which version upgrades we are making.

[mxinden]

So, actually, I think the best solution to this is to always pin the libp2p-swarm-derive dependency in libp2p-swarm. That way, it can never get out of sync, regardless of which version upgrades we are making.

Sounds like the best option to me. I can not see a downside to this, given that we control both libp2p-swarm and libp2p-swarm-derive.

[dhuseby]

draft PR #4581 submitted