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

swarm-derive/: Add where clause of behaviour to generated out event #2819

Merged
merged 2 commits into from Aug 17, 2022

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Aug 15, 2022

Description

When generating the OutEvent for a NetworkBehaviour, add the where clause of the
NetworkBehaviour struct to the generated enum.

Links to any relevant issues

Follow up to #2792

Open Questions

Change checklist

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

When generating the `OutEvent` for a `NetworkBehaviour`, add the `where` clause of the
`NetworkBehaviour` `struct` to the generated `enum`.
@mxinden mxinden merged commit 67266c6 into libp2p:master Aug 17, 2022
@elenaf9
Copy link
Contributor

elenaf9 commented Aug 18, 2022

Won't this cause an error in case of

    #[derive(NetworkBehaviour)]
    struct Foo<T>
    where
        T: Copy
    {
        ping: libp2p::ping::Ping,
        #[behaviour(ignore)]
        bar: T,
    }

which will generate

pub enum FooEvent<T> // Error: unused Parameter
where
    T: Copy
{
    Ping(<libp2p::ping::Ping as NetworkBehaviour>::OutEvent)
}

Am I missing something?

@mxinden
Copy link
Member Author

mxinden commented Aug 20, 2022

@elenaf9 that is correct. Good catch. Though why would someone use the OutEvent mechanism in combination with #[behaviour(ignore)]?

Once we remove NetworkBehaviourEventProcess (#2751) I don't think there is any use for poll_method nor ignore at all. See #2751 (comment).

With that in mind, I would leave this as is. Do you agree @elenaf9?

@elenaf9
Copy link
Contributor

elenaf9 commented Aug 20, 2022

Though why would someone use the OutEvent mechanism in combination with #[behaviour(ignore)]?

Once we remove NetworkBehaviourEventProcess (#2751) I don't think there is any use for poll_method nor ignore at all. See #2751 (comment).

I am a bit confused. Why would we not need #[behaviour(ignore)] anymore? Isn't it still needed to signal the derive macro that this field is not a network behaviour? E.g. when implementing the methods of the NetworkBehaviour trait and we pass the event to our inner behaviours?
And with OutEvent I would think we stilll need it to mark that this field won't generate an event:

  • If the OutEvent is provided by the user, we should not need the trait bound From<<FIELD as NetworkBehaviour>::OutEvent> for this field
  • If the OutEvent is generated, we should not generated an enum variant for this field. See above code example.

@mxinden what am I missing here?

@mxinden
Copy link
Member Author

mxinden commented Aug 20, 2022

I am a bit confused. Why would we not need #[behaviour(ignore)] anymore?

Let me rephrase:

With NetworkBehaviourEventProcess users sometimes needed an events: VecDeque which they could push events into from NetworkBehaviourEventProcess and pop events from within their custom poll method. events would not implement NetworkBehaviour and thus needed the #[behaviour(ignore)].

With the removal of NetworkBehaviourEventProcess there is no more need for fields on the root NetworkBehaviour that do not themselves implement NetworkBehaviour, at least I can not think of a use-case.

Can you think of a use case for #[behaviour(ignore)] and a custom poll method once NetworkBehaviourEventProcess is removed @elenaf9?

@elenaf9
Copy link
Contributor

elenaf9 commented Aug 20, 2022

Thanks for elaborating @mxinden! Makes sense now.

Can you think of a use case for #[behaviour(ignore)] and a custom poll method once NetworkBehaviourEventProcess is removed @elenaf9?

I guess the only use case is if the wrapping NetworkBehaviour wants to implement some additional logic on top of the inner behaviours and / or hide some of the implementation details about them. But this can also be implemented on top of the Swarm, so I agree that we don't need #[behaviour(ignore)] or #[behaviour(poll = ..)] anymore.

With that in mind, I would leave this as is. Do you agree @elenaf9?

Yep :)

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 this pull request may close these issues.

None yet

3 participants