Suggestion: replace all the NetworkBehaviour::inject_* methods with a single one

[tomaka]

After #1515, the SwarmEvent is very redundant with the methods in the NetworkBehaviour.

Instead of having several inject_* methods, one idea would be to have a single inject_event method (it could be named on_event or something else as well) that accepts an enum as parameter.

This would considerably simplify writing implementations of NetworkBehaviour that are built on top of another NetworkBehaviour.

cc #1021 which is slightly related

[twittner]

One difference would be that adding a new method with a default implementation can be a minor change, thus requiring only a minor semver increment, whereas a new enum constructor is always a major change unless we annotate the enum with #[non_exhaustive].

[romanb]

I like the idea in general and using #[non_exhaustive] sounds good to me as well. It may even be considered to just pass the NetworkEvent to the behaviour though that would remove some flexibility w.r.t. custom extensions and event mappings done by the swarm and possibly introduce some lifetime woes.

[mxinden]

Only concerning #[non-exhaustive]:

As a downstream crate owner I would very much appreciate if my compiler can tell me that there was a new event added in an upstream crate that I depend on. Marking the enum as non_exhaustive takes this notification mechanism away as I can't exhaustively match on the enum anymore. While additions to enums can imply a new feature (not a breaking change) it can as well imply behavior changes (breaking change).

Is the trade-off (improving the new-feature path) worth the lost safety?

[twittner]

I did not argue in favour of adding #[non_exhaustive] but wanted to point out the implications the proposed change has for semver compatible updates. In that regard the current implementation is more expressive than an enum-based approach as one can choose between minor and major changes for every added method, switching to an enum either makes every addition a major change or ― with #[non_exhaustive] ― none, although in the latter case one could still decide to increment the major version to alert users of the importance of the change. Finally, clients may choose to add a default handler anyway so even without #[non_exhaustive] they may ignore added constructors, a new trait method without implementation on the other hand can not be ignored.

[tomaka]

The problem is that I can imagine adding new events that can reasonably be ignored by users, but I can also imagine adding new events that the user should not ignore.

For example (assuming #1519 is merged), we have this ConnectionOpened/ConnectionClosed combination of events, and the user can for example add an entry in a local hashmap when a connection is open, and remove that entry when it is closed.

If later for some reason we add a new event, separate from ConnectionClosed but that also notifies of a connection being closed, then the user will then effectively have a state mismatch and/or a memory leak.

[twittner]

The problem is that I can imagine adding new events that can reasonably be ignored by users, but I can also imagine adding new events that the user should not ignore.

On its own this would suggest to keep the current approach instead of switching to an enum.

The original motivation for the change was:

This would considerably simplify writing implementations of NetworkBehaviour that are built on top of another NetworkBehaviour.

Could you expand on this maybe?

[thomaseizinger]

Closing in favor of #2832.