Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[derive(NetworkBehaviour)] fails on structs with generics #2879

Closed
dmitry-markin opened this issue Sep 8, 2022 · 11 comments · Fixed by #2907
Closed

#[derive(NetworkBehaviour)] fails on structs with generics #2879

dmitry-markin opened this issue Sep 8, 2022 · 11 comments · Fixed by #2907

Comments

@dmitry-markin
Copy link
Contributor

dmitry-markin commented Sep 8, 2022

Summary

It looks like there is a regression in libp2p-0.48.0: #[derive(NetworkBehaviour)] fails to generate code if the original struct has generic parameters.

Expected behaviour

#[derive(NetworkBehaviour)] generates the implementation if the struct has generic parameters.

Actual behaviour

Compilation fails with the error:

error: proc-macro derive produced unparseable tokens

Please see the minimal example: main.rs

(In this case the compilation would fail anyway, because Option<T> does not implement NetworkBehaviour, but the compilation fails earlier with the same error as in the real-life example in Substrate, where the templatized type actually implements NetworkBehaviour.)

Version

  • libp2p version: 0.48.0

Additional information

It also looks like there is another, may be connected, bug: #[derive(NetworkBehaviour)] fails to generate <STRUCT_NAME>Event if no #[behaviour(out_event = "...")] provided (even without generics):

error[E0433]: failed to resolve: use of undeclared type `NetworkBehaviour`

Would you like to work on fixing this bug?

No, unfortunately I don't understand what's happening.

CC @mxinden

@dariusc93
Copy link
Contributor

I may be wrong but It may be related to #2842 and I dont believe substrate upgraded to 0.48 where the changes would be needed (assuming they are ignoring those fields) though i havent looked at their source so i wouldnt know what they are doing. It also sounds like youre looking for Toggle<T> though T would need to implement NetworkBehaviour.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Sep 9, 2022

Yes, using types that don't implement NetworkBehaviour (like Option) are no longer supported.

We have a test where the annotated NetworkBehaviour has a type parameter, so it can't be the generic itself that makes the macro choke.

#[test]
fn where_clause() {
#[allow(dead_code)]
#[derive(NetworkBehaviour)]
struct Foo<T>
where
T: Copy + NetworkBehaviour,
<T as NetworkBehaviour>::OutEvent: Debug,
{
ping: libp2p::ping::Ping,
bar: T,
}
}

@mxinden
Copy link
Member

mxinden commented Sep 11, 2022

@dmitry-markin would the following fix your example?

  #[derive(NetworkBehaviour)]
  #[behaviour(out_event = "BadOutEvent<T>")]
- struct BadBehaviour<T> {
+ struct BadBehaviour<T: NetworkBehaviour> {
      mdns: Mdns,
-     template: Option<T>,
+     template: Toggle<T>,
  }

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Sep 12, 2022

In the example above I wasn't able to implement From<Toggle<T>::OutEvent> for BadOutEvent<T> because the declaration conflicts with the default std From implementation, but I was able to prepare another example. Taking some dummy CustomBehaviour NetworkBehaviour that is pretty useless, but nevertheless compiles, the following compiles fine:
https://github.com/dmitry-markin/libp2p-behaviour/blob/16a3a6e3dc73f90d9d2b8e178762611a9b30efeb/src/main.rs

But when I make CustomBehaviour depend on a template parameter, the following fails:
dmitry-markin/libp2p-behaviour@acb9e47

The error is the same:

error: proc-macro derive produced unparseable tokens

Most likely I just don't understand something quite trivial and make a silly mistake. But can't spot it, so will appreciate the help a lot.

@dariusc93
Copy link
Contributor

@dmitry-markin Going from the top of my head from looking over your code, I would assume this would probably fix the error youre getting

#[derive(NetworkBehaviour)]
#[behaviour(out_event = "BadOutEvent")]
struct BadBehaviour<T: 'static + Send> {
    mdns: Mdns,
    custom: CustomBehaviour<T>,
}

I dont believe it likes #[behaviour(out_event = "BadOutEvent<T>")] because of BadOutEvent<T> defined there in the macro (though I never tried doing that so im not sure if its correct or not so maybe somebody else could chime in on that), however that may not address your From impl. I would assume it would look something like

impl<T: 'static + Send> From<<CustomBehaviour<T> as NetworkBehaviour>::OutEvent>
    for BadOutEvent<T>
{
    fn from(e: T) -> Self {
        Self::Template(e)
    }
}

Since you wouldnt be using Toggle<T> in From (since all youre doing is having a way to convert the event to BadOutEvent and not touching the behaviour in the struct, which may or may not be behind a Toggle)

Can test this sometime tomorrow though but im pretty sure by skimming over your code in that repo that there might need more changes since OutEvent is void::Void in your NetworkBehaviour impl which might cause problems with the other From impl.

@dmitry-markin
Copy link
Contributor Author

@dariusc93 thanks, that fixes at least one problem: removing template parameter from #[behaviour(out_event = "BadOutEvent<T>")] and making it #[behaviour(out_event = "BadOutEvent")] made it compile (example).

Unfortunately, in Substrate we have different number of template parameters in a behaviour struct and its out event. Here is the toy example for this: main.rs

In this case the compilation fails with the following error:

error[E0107]: this enum takes 0 generic arguments but 1 generic argument was supplied
  --> src/main.rs:11:10
   |
11 |   #[derive(NetworkBehaviour)]
   |  __________^^^^^^^^^^^^^^^^-
   | |          |
   | |          expected 0 generic arguments
12 | | #[behaviour(out_event = "BadOutEvent")]
13 | | struct BadBehaviour<T: 'static + Send> {
   | |______________________________________- help: remove these generics
   |
note: enum defined here, with 0 generic parameters
  --> src/main.rs:19:6
   |
19 | enum BadOutEvent {
   |      ^^^^^^^^^^^
   = note: this error originates in the derive macro `NetworkBehaviour` (in Nightly builds, run with -Z macro-backtrace for more info)

@thomaseizinger
Copy link
Contributor

Unfortunately, in Substrate we have different number of template parameters in a behaviour struct and its out event. Here is the toy example for this: main.rs

I think that is something we should support.

Does it work if you let the macro generate the outevent for you?

@dmitry-markin
Copy link
Contributor Author

Unfortunately, in Substrate we have different number of template parameters in a behaviour struct and its out event. Here is the toy example for this: main.rs

I think that is something we should support.

It looks like the generated code implies that the template parameters of the OutEvent are the same as of the NetworkBehaviour...

Does it work if you let the macro generate the outevent for you?

Yes, it works, I just needed to import libp2p::swarm::NetworkBehaviour additionally to NetworkBehaviour derive macro. It works both in Substrate and the toy example above.

@thomaseizinger
Copy link
Contributor

I've pushed a fix to this in #2907. Please review that the added test corresponds to your usecase. Also, could you try and patch that branch into your application and see if it makes the error go away?

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Sep 15, 2022

Yes, the test covers the use case with the only difference that our behavior has two template parameters, and out event one, and in the test they are one and zero correspondingly. Patching the branch into Substrate also made the error go away.

@thomaseizinger thanks a lot for the bugfix!

@thomaseizinger
Copy link
Contributor

Yes, the test covers the use case with the only difference that our behavior has two template parameters, and out event one, and in the test they are one and zero correspondingly. Patching the branch into Substrate also made the error go away.

@thomaseizinger thanks a lot for the bugfix!

Good to hear! I'll wait for a review of #2907 from @mxinden and then we can merge that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants