feat(swarm): rename associated types for message passing

[tcoratger]

Description

Previously, the associated types on NetworkBehaviour and ConnectionHandler carried generic names like InEvent and OutEvent. These names are correct in that OutEvents are passed out and InEvents are passed in but they don't help users understand how these types are used.

In theory, a ConnectionHandler could be used separately from NetworkBehaviours but that is highly unlikely. Thus, we rename these associated types to indicate, where the message is going to be sent to:

• NetworkBehaviour::OutEvent is renamed to ToSwarm: It describes the message(s) a NetworkBehaviour can emit to the Swarm. The user is going to receive those in SwarmEvent::Behaviour.
• ConnectionHandler::InEvent is renamed to FromBehaviour: It describes the message(s) a ConnectionHandler can receive from its behaviour via ConnectionHandler::on_swarm_event. The NetworkBehaviour can send it via the ToSwarm::NotifyHandler command.
• ConnectionHandler::OutEvent is renamed to ToBehaviour: It describes the message(s) a ConnectionHandler can send back to the behaviour via the now also renamed ConnectionHandlerEvent::NotifyBehaviour (previously ConnectionHandlerEvent::Custom)

Resolves: #2854.

Notes & open questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• A changelog entry has been made in the appropriate crates

[mergify]

This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏

[thomaseizinger]

Thank you!

As someone that is relatively new to the codebase: Does this make things easier to understand? What is your opinion?

[tcoratger]

Thank you!

As someone that is relatively new to the codebase: Does this make things easier to understand? What is your opinion?

@thomaseizinger Yes this logic helps me personally to better understand how traits work with these explicit names which in any case help the user. Here are some additional remarks:

• At the beginning I didn't understand why there is no FromSwarm event in the NetworkBehaviour trait.
• What was really confusing to me reading these few parts of the code was the fact that there are also InEvent and OutEvent items in many other places in the code for other implementations that have absolutely nothing to do with each other. One example: in the SubstreamHandler trait there also these two items. So maybe we should open other issues to give also explicit name to these other items and avoid generic InEvent and OutEvent usage, what do you think?

[thomaseizinger]

Thank you!

As someone that is relatively new to the codebase: Does this make things easier to understand? What is your opinion?

@thomaseizinger Yes this logic helps me personally to better understand how traits work with these explicit names which in any case help the user. Here are some additional remarks:

• At the beginning I didn't understand why there is no FromSwarm event in the NetworkBehaviour trait.

There actually is a FromSwarm event but there isn't an associated type in NetworkBehaviour which is probably what you meant :)

The reason is that you can directly call functions on NetworkBehaviour via Swarm::behaviour_mut and thus no message passing is needed to "talk" to the NetworkBehaviour.

• What was really confusing to me reading these few parts of the code was the fact that there are also InEvent and OutEvent items in many other places in the code for other implementations that have absolutely nothing to do with each other. One example: in the SubstreamHandler trait there also these two items. So maybe we should open other issues to give also explicit name to these other items and avoid generic InEvent and OutEvent usage, what do you think?

Ugh yes. I really want to get rid of all of StreamHandler. That was what I now consider a failed experiment.

[drHuangMHT]

Looking forward to merging this! The naming can be rather confusing.

• At the beginning I didn't understand why there is no FromSwarm event in the NetworkBehaviour trait.

There actually is a FromSwarm event but there isn't an associated type in NetworkBehaviour which is probably what you meant :)

The reason is that you can directly call functions on NetworkBehaviour via Swarm::behaviour_mut and thus no message passing is needed to "talk" to the NetworkBehaviour.

By the way, is there any plan to run different NetworkBehaviour on different tasks?

[mxinden]

By the way, is there any plan to run different NetworkBehaviour on different tasks?

What would be the motivation to do so? In case it is performance, thus far I have seen no indication that it is a bottleneck in any deployment. (Note absence of evidence does not mean evidence of absence, though when it comes to performance "improvements" I am reluctant of optimization without proof.) Also, in case our NetworkBehaviour hierarchy is a bottleneck, I would prefer first introducing a smarter polling logic, along the lines of FuturesUnordered, only polling the NetworkBehaviour that triggered the wake-up of the task.

[mergify]

This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏

[thomaseizinger]

Thanks! I am really looking forward to merging this, I think it will make understanding our abstractions a lot easier.

Left some minor comments.

[thomaseizinger]

Thanks! One question and two suggestions.

[thomaseizinger]

Where is this warning going to show up? log is only an API crate and needs a concrete implementation like env_logger to actually print the messages. Does cargo automatically configure a logger when it runs proc-macros?

[tcoratger]

@thomaseizinger I thought so, but you're right it's not, I added env_logger::init(); to put it simply, in this way the warning should be displayed correctly.

[thomaseizinger]

should be displayed correctly.

@tcoratger Have you confirmed that it is actually displayed? :)

I am asking because there is an entire nightly library feature that deals with diagnostics in proc macros and I think it is not as easy as just emitting a log :)

I think we can work around this by emitting code that has deprecation warnings on it. Here is an idea:

1. If the user users out_event, also generate a trait: trait OutEventAttributeIsDeprecatedAndHasBeenRenamedToToSwarm { }
2. Also put a #[deprecated] attribute on this trait
3. Implement the trait for the generated out event

I think this should then show up as a deprecation warning in the user's code.

[tcoratger]

@thomaseizinger Can you enlighten me on the subject, I am not very familiar with proc macros, I noticed the following things:

• When I do a simple println! I thought it was the right approach but in fact the warning is generated only at the first compilation but not at each use of the code, because I imagine that it is a piece of code that runs only at compilation.
• On the another hand when I use an approach like:

if meta.path().is_ident("out_event") {
  #[deprecated(
      since = "0.33.0",
      note = "The 'out_event' key is deprecated, use 'to_swarm' instead."
  )]
  trait OutEventAttributeIsDeprecatedAndHasBeenRenamedToToSwarm {}

  impl OutEventAttributeIsDeprecatedAndHasBeenRenamedToToSwarm for Meta {}
}

then the deprecation warning appears each time even if we use to_swarm, this is normal because we face a general trait implementation that is deprecated and this is not what we want. Any advice here?

[thomaseizinger]

I think we don't want to implement it for Meta but for the type that the user specifies in out_event.

Also, we need to wrap this in quote! to generate this code for the user.

The idea is, when out_event is used, we emit code that contains deprecated symbols. The compiler then has to compile this code, encounters the #[deprecated] attribute and emits a warning. At least that is the hypothesis, not sure if it works as intended :D

[thomaseizinger]

I couldn't get this to work. For some reason, I am not seeing any warnings that should be produced for the generated code. I swear I saw some of them in the past (which is why we have allow(clippy) attributes in there).

I've found https://github.com/ggwpez/proc-macro-warning but couldn't get it to work. Let's just leave it out and document that it is deprecated.

[thomaseizinger]

We could use https://docs.rs/proc-macro-error/1.0.4/proc_macro_error/macro.emit_warning.html then at least the users on nightly will get a warning!

[tcoratger]

@thomaseizinger Yes on my side also I looked a lot and I did not find anything conclusive. You mean doing an implementation like:

if meta.path().is_ident("to_swarm") || meta.path().is_ident("out_event") {
  if meta.path().is_ident("out_event") {
      quote! {
          macro_rules! emit_warning {
              ($span:expr, $($tts:tt)*) => {
                  println!("warning: {}", format_args!($($tts)*));
              };
          }
      };

      let emit_warning_call = quote! {
          emit_warning!(#meta, "The 'out_event' key is deprecated, use 'to_swarm' instead.");
      };

      use quote::ToTokens;
      quote! {
          #emit_warning_call
      }
      .to_token_stream();
  }
....
}

I've tried that a little bit yesterday switching my rustup to nightly and did work properly, what do you think?

[thomaseizinger]

Ha, that is also a creative idea but not quite what I meant :)

We can just depend on the proc-macro-error crate directly and use their emit_warning! macro! On nightly, that should allow us to issue a diagnostic, similar to how the Rust compiler or clippy issues them.

[thomaseizinger]

Exciting news over at proc-macro-warning: ggwpez/proc-macro-warning#2 (comment).

[mergify]

This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏

[mxinden]

I'd like @mxinden to have a look over this too.

I like the new names. Thanks to you two for pushing this forward!

[thomaseizinger]

@tcoratger Would you mind exploring whether the deprecation warning works with what is documented in ggwpez/proc-macro-warning#2?

I think that would make it a lot easier for users to migrate.

[mergify]

This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏

[tcoratger]

@tcoratger Would you mind exploring whether the deprecation warning works with what is documented in ggwpez/proc-macro-warning#2?

I think that would make it a lot easier for users to migrate.

I just pushed an implementation that works:

• Inside the parse_attributes function, if out_event appears as an attribute then I add a TokenStream deprecation_tokenstream to the output structure.
• Then I take this TokenStream which contains the warning and I add it in the final_quote of the build_struct function
• In this way, as soon as out_event appears, I have the warning of deprecation while nothing is displayed when it is not there.

@thomaseizinger Feel free to modify if something doesn't suit you but I think it's a good compromise. We will then be good to merge I think.

[thomaseizinger]

@tcoratger Thank you! I've pushed a unit test that assert the warning we see. I also updated the deprecation message.

[mxinden]

Great to see this merged. Thanks for the work. Also neat warnings AND warning tests!