swarm*/: Remove NetworkBehaviourEventProcess and generate OutEvent

[mxinden]

Description

This pull request does two things:

1. Remove support for NetworkBehaviourEventProcess.

   I am of the opinion that NetworkBehaviourEventProcess is not worth the complexity, especially for newcomers. I am not aware of a use-case that can not be supported without.

2. If user does not specify out_event = "", generate NetworkBehaviour::OutEvent enum

   Instead of requiring the user to provide an enum as the OutEvent of the derived NetworkBehaviour we can generate this enum in the procedural macro itself if not specified.

   See swarm-derive/tests/test.rs for examples how much this simplifies things.

Links to any relevant issues

Open Questions

Change checklist

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

[mxinden]

The only problem I see is that the user has to interact with generated code: the enum.

Good point. Agree that this adds mental complexity. That said, if documented well, I think it is worth the complexity.

Letting the user write the enum

I would like the user to not have to write the enum out manually. I find this tedious and error prone.

[thomaseizinger]

Letting the user write the enum

I would like the user to not have to write the enum out manually. I find this tedious and error prone.

It is tedious, I agree. I do want to mention though that generating the enum for the user also hosts other problems:

• It takes away the ability for the user to control the visibility: pub, pub(super), etc
• It is harder for the user to add more derives, be it their own or well-known ones like serde
• A user may not want to actually track all variants and discard some of them

All of these are perhaps solvable in one way or another but the bigger picture is that we are taking away a lot of control from the user by generating the enum for them.

This ties in with the "really good proc-macros" from above: If the use of a piece of generated code is completely internal to the generated-code, you are in full control of all requirements. Once you expose a piece of generated code to the user, you are opening the door to more and more feature requests like the above list.

I am not sure if the added benefit of not writing the enum (which is a one-time cost) is worth this complexity.

[dignifiedquire]

Regarding the enum, I would prefer having a gradual approach. Full generation, custom enum with automatic from generations, full custom. This will reduce the boilerplate but still allow users to upgrade to fully customized versions if desired.

[mxinden]

It takes away the ability for the user to control the visibility: pub, pub(super), etc

Why would a user want the event to have a different visibility than the behaviour?

It is harder for the user to add more derives, be it their own or well-known ones like serde

I can not think of a scenario where one would need additional derive statements. As far as I know none of the sub-behaviour events derive serde, thus deriving serde on the top level enum would be in vain, right?

(I think I am missing to add the standard (e.g. Debug, PartialEq, ...) on the generated enum. Thanks for triggering that thought!)

A user may not want to actually track all variants and discard some of them

They can do so when matching on the event returned by Swarm. Note that this wasn't possible with event-process=false either.

Regarding the enum, I would prefer having a gradual approach. Full generation, custom enum with automatic from generations, full custom. This will reduce the boilerplate but still allow users to upgrade to fully customized versions if desired.

Can you expand on the concrete use-cases for each of these @dignifiedquire?

I would like to make the easy things easy, and the hard things possible? In my opinion this pull request makes the common case really easy and thus far I don't see any uncommon use-cases being impossible.

[rkuhn]

How about a hybrid approach regarding the event type: have the user spell out the precise enum they want to use (which is not much work and provides occasion for some doc comments) and do all the wiring in the proc macro.

pub(crate) enum MyEvent {
    Ping(PingEvent),
    ...
}

#[derive(NetworkBehaviour)]
#[behaviour(out_event = MyEvent)]
struct MyNet {
    #[event(Ping)]
    ping: ping::Behaviour,
    // events from this one will be ignored
    identify: Identify,
    ...
}

This could be extended to allow calling a function for turning an event into the out_event type (perhaps just form a path <out_event>::<ident> and invoke with a single argument, which covers all cases already).

[thomaseizinger]

It takes away the ability for the user to control the visibility: pub, pub(super), etc

Why would a user want the event to have a different visibility than the behaviour?

Like I tried to point out above, I raised these points as examples of usecases, there are surely more than what I currently think of but these examples show that not having control over the generated enum can be a blocker that can only be resolved by patching the derive, which will trigger PR discussions because a solution has to be designed, requiring a release, etc.

It is harder for the user to add more derives, be it their own or well-known ones like serde

I can not think of a scenario where one would need additional derive statements. As far as I know none of the sub-behaviour events derive serde, thus deriving serde on the top level enum would be in vain, right?

You can't say that for sure. I user could use this derive with only behaviours that they write themselves where all events derive serde. Again, this example was meant as a thought experiment rather than a concrete problem that we need to solve.

(I think I am missing to add the standard (e.g. Debug, PartialEq, ...) on the generated enum. Thanks for triggering that thought!)

I think the above point may actually result in a real problem here: We don't which behaviours a user will compose with this macro and thus, we cannot possibly know, which "standard" derives to add. E.g. I don't think all our events will implement Ord so we can't derive that. But what if a user wants to have their OutEvent implement Ord and all their events implement Ord?

As much as I'd love for ergonomics of this macro to get better, I don't think removing control over the out event completely is a good solution.

How about the following (I've outlined it above briefly already, but here in more detail):

1. Define the convention on what the out-event should be called (we can infer this from the NB name)
2. Provide a derive(OutEvent) proc-macro that implements From for all branches

For example:

#[derive(NetworkBehaviour)]
struct Foo {
    ping: libp2p::ping::Ping,
}

#[derive(OutEvent)]
enum FooEvent {
	Ping(libp2p::ping::PingEvent)
}

• The boilerplate is minimal (writing the enum is a one-time cost)
• The macro is simple because we don't need any additional configuration options
• We can enforce the conversion to the enum via From in the NB-derive macro
• derive(OutEvent) would generate the From implementations
• The user has full control over the enum without having to bother us with feature-requests

[rkuhn]

What would be the difference between #[derive(OutEvent)] and #[derive(derive_more::From)]?

[thomaseizinger]

What would be the difference between #[derive(OutEvent)] and #[derive(derive_more::From)]?

Functionally, nothing but it:

• saves users from adding another dependency
• allows us to "hide" some of the details on how things work internally which should make things easier to understand for newcomers

[mxinden]

Meta level: Thanks for all the input here!

With the above arguments, agreed that there should be a way to bring your own OutEvent.

Define the convention on what the out-event should be called (we can infer this from the NB name)

I would prefer making this explicit instead of inferring the enum based on its name. Thus I am would be in favor of sticking with #[behaviour(out_event = MyEvent)].

Provide a derive(OutEvent) proc-macro that implements From for all branches

Haven't put enough thought into this. Off the top of my head I would prefer #[derive(derive_more::From)] as suggested by @rkuhn. I too want to reduce the amount of dependencies in rust-libp2p, but in this case the dependency is just a perfect match.

allows us to "hide" some of the details on how things work internally which should make things easier to understand for newcomers

I expect users to be very familiar with the From trait. Thus I am not sure the custom generation is easier to understand than the well known From concept.

---

I would like to make the following suggestion. Note that I am still suggesting to additionally support the lazy case, where the OutEvent is generated by the procedural macro.

• Have the user write their own enum used as the OutEvent.
• Have the user specify the custom OutEvent in #[behaviour(out_event = MyEvent)].
• Require OutEvent to implement From. Document that users can use derive_more.
• If the user does not specify an out_event, generate the enum for the user.

What do you all think? Would you be fine with the option to generate the OutEvent if there is the alternative to bring your own?

[rkuhn]

That proposal covers all aspects but one — and I’m not certain that it is an important one so it might be fine: impl From can only distinguish based on source type, so if there are two nested behaviours that emit the same event type (e.g. two instances of a behaviour, parameterised differently) then the only way to distinguish their events is to use the generated OutEvent option. One way to fix this retroactively (i.e. if the situation actually does come up frequently enough) is to consider using From the default and handling a new attribute in the derive macro to customise the transformation.

So +1 from me :-)

[thomaseizinger]

Define the convention on what the out-event should be called (we can infer this from the NB name)

I would prefer making this explicit instead of inferring the enum based on its name. Thus I am would be in favor of sticking with #[behaviour(out_event = MyEvent)].

Convention over configuration is a well established way of dealing with such problems but I if people feels strongly about wanting to configure the enum name, then I am happy to go with that.

As a user, I wouldn't care about the name, on the contrary, having the enum enforce it creates consistency!

Provide a derive(OutEvent) proc-macro that implements From for all branches

Haven't put enough thought into this. Off the top of my head I would prefer #[derive(derive_more::From)] as suggested by @rkuhn. I too want to reduce the amount of dependencies in rust-libp2p, but in this case the dependency is just a perfect match.

Implementation-wise it is a perfect match yes. I suggested this because it hides what people have to understand about this derive. From by itself is easy to understand. How the compiler or a macro composes together a tree of poll-style state machines is not. But to understand why you need From you need to think about that.

Contrarily, people are used to macros working in a way of: "Put this sticker here, this other one there and leave the rest to us!"

It also gives as flexibility to add things to the derive later if we ever need it. Not sure how good of an argument that is.

• Have the user write their own enum used as the OutEvent.
• Have the user specify the custom OutEvent in #[behaviour(out_event = MyEvent)].
• Require OutEvent to implement From. Document that users can use derive_more.
• If the user does not specify an out_event, generate the enum for the user.

What do you all think? Would you be fine with the option to generate the OutEvent if there is the alternative to bring your own?

I am okay with this solution too but I also think that it carries a higher complexity because there are more moving parts a user needs to understand.

Bottom line: These are my points, I'll leave the decision to you :)

[elenaf9]

@mxinden:

I would like to make the following suggestion. Note that I am still suggesting to additionally support the lazy case, where the OutEvent is generated by the procedural macro.

• Have the user write their own enum used as the OutEvent.
• Have the user specify the custom OutEvent in #[behaviour(out_event = MyEvent)].
• Require OutEvent to implement From. Document that users can use derive_more.
• If the user does not specify an out_event, generate the enum for the user.

What do you all think? Would you be fine with the option to generate the OutEvent if there is the alternative to bring your own?

👍 sounds good to me.

@thomaseizinger:

The macro is simple because we don't need any additional configuration options

How about the following (I've outlined it above briefly already, but here in more detail):

1. Define the convention on what the out-event should be called (we can infer this from the NB name)

2. Provide a `derive(OutEvent)` proc-macro that implements `From` for all branches

For example:

#[derive(NetworkBehaviour)]
struct Foo {
    ping: libp2p::ping::Ping,
}

#[derive(OutEvent)]
enum FooEvent {
	Ping(libp2p::ping::PingEvent)
}

(...) - The macro is simple because we don't need any additional configuration options

We would still need the configuration option to inform the macro that we use a custom event, no? I am also more in favor of making the enum name explicit.

[mxinden]

I would like to make the following suggestion. Note that I am still suggesting to additionally support the lazy case, where the OutEvent is generated by the procedural macro.

* Have the user write their own `enum` used as the `OutEvent`.

* Have the user specify the custom `OutEvent` in `#[behaviour(out_event = MyEvent)]`.

* Require `OutEvent` to implement `From`. Document that users can use `derive_more`.

* If the user does not specify an `out_event`, generate the `enum` for the user.

What do you all think? Would you be fine with the option to generate the OutEvent if there is the alternative to bring your own?

I adjusted the pull request to implement the suggestion above. I would very much appreciate reviews.

---

Regarding #[derive(OutEvent)] vs #[derive(derive_more::From)], I don't have a strong opinion on this. This pull request does not implement the former, nor does it use the latter. Still I think the pull request adds value in its status quo, thus I am suggesting to merge without either of the two. Is that OK for folks?

[thomaseizinger]

Answering for completness, not to question the implementation

- The macro is simple because we don't need any additional configuration options

We would still need the configuration option to inform the macro that we use a custom event, no?

@elenaf9 The idea was to always require a custom event, i.e. to not auto-generate anything but expect the user to provide a type in the same module that follows the naming convention of NetworkBehaviourName + Event, i.e.FooBehaviourEvent for a NB FooBehaviour.

With this assumption, no configuration would be necessary.

[mxinden]

Replacing this pull request with https://github.com/libp2p/rust-libp2p/pull/2840 on top of the latest v0.47.0 release.