swarm/src/behaviour: Make NetworkBehaviourAction generic over TBehaviour

[mxinden]

Follow up on libp2p#2191 (comment).

//CC @thomaseizinger

[thomaseizinger]

Interesting that clippy flags TBehaviour as unused!

[thomaseizinger]

How often do we use these map_in and map_out functions actually? If it simplifies things, it may be worth sacrificing them. In the end, they are just convenience functions from what I can see.

From my own experience, I don't think I ever used these although implementations like the EitherHandler that we had the other day do become a lot cleaner.

Instead of providing individual map_ functions, we could provide just a single one, with 3 closures that map all elements of NetworkBehaviourAction to those of a new NetworkBehaviour.

[mxinden]

How often do we use these map_in and map_out functions actually? If it simplifies things, it may be worth sacrificing them. In the end, they are just convenience functions from what I can see.

Considering libp2p#2191 as the status quo, we use them within libp2p-gossipsub and Toggle behaviour.

I don't have a strong opinion here. As of today, I am in favor of keeping them, despite the complexity it adds to NetworkBehaviourAction's type parameters.

[thomaseizinger]

How often do we use these map_in and map_out functions actually? If it simplifies things, it may be worth sacrificing them. In the end, they are just convenience functions from what I can see.

Considering libp2p#2191 as the status quo, we use them within libp2p-gossipsub and Toggle behaviour.

I don't have a strong opinion here. As of today, I am in favor of keeping them, despite the complexity it adds to NetworkBehaviourAction's type parameters.

What do you think of doing the following instead:

enum NetworkBehaviourAction<TBehaviour> {
	GenerateEvent(TBehaviour::OutEvent),
	// ...
}

impl<TBehaviour> NetworkBehaviourAction<TBehaviour> {

	// (signature not complete)
	fn map<TOther>(self, map_event: impl Fn(TBehaviour::OutEvent) -> TOther::OutEvent, map_handler: impl Fn) -> NetworkBehaviourAction<TOther> {
		// ...
	}

}

Would this work for all of our usecases?

[mxinden]

Thanks for following up here.

Off the top of my head, yes, the map taking 3 Fns would work (though a bit less idiomatic I would say).

Still, the issue of TBehaviour not being used by NetworkBehaviourAction blocks this pull request, though that might be fixed by removing the default parameters making the map_x methods generic over TBehaviour and TNewBehaviour only.

I will explore this option next week.

[thomaseizinger]

though that might be fixed by removing the default parameters making the map_x methods generic over TBehaviour and TNewBehaviour only.

That was the idea! If we are referring to associated types, it cannot possible be flagged as unused!

The downside is that we need to put a trait-bound on the type declaration itself. I consider that an anti-pattern because it makes the trait-bound creep through all usages of the type. That being said, the type is probably not named much outside of rust-libp2p.