refactor: expose and use THandlerOutEvent type alias
#3368
Conversation
| peer_id: PeerId, | ||
| /// The produced event. | ||
| event: THandlerOutEvent<THandler>, | ||
| event: <<THandler as IntoConnectionHandler>::Handler as ConnectionHandler>::OutEvent, |
There was a problem hiding this comment.
I temporarily inlined this one so that I can delete the 2nd type alias named THandlerOutEvent defined in swarm::behaviour.
This will be simplified to ConnectionHandler::OutEvent in #3254.
swarm/src/behaviour.rs
Outdated
| /// } | ||
| /// ``` | ||
| pub trait NetworkBehaviour: 'static { | ||
| pub trait NetworkBehaviour: Sized + 'static { |
There was a problem hiding this comment.
This requires a Sized bound now but I'd honestly not know why a NetworkBehaviourwouldn't be sized ...
There was a problem hiding this comment.
How is the requirement to be Sized related to this change?
There was a problem hiding this comment.
I think it is because I am now using Self within a function signature via a type-alias. To be frank, I just followed the compiler 🙃
There was a problem hiding this comment.
I would prefer to avoid casually introducing trait bounds without careful consideration. Is there any way around introducing this trait bound? Is this pull request worth doing given the added complexity.
I would have to dig deeper here. Based on intuition, adding Sized, Send or Sync unintentionally has not proven to be good ideas in the past.
There was a problem hiding this comment.
Fair point! I never looked deeply into it but apparently, requiring Sized prohibits the use as a trait object: https://doc.rust-lang.org/std/marker/trait.Sized.html
I'll try to work around it.
There was a problem hiding this comment.
Turns out it is not needed, lol 😂
There was a problem hiding this comment.
Fair point! I never looked deeply into it but apparently, requiring
Sizedprohibits the use as a trait object:I'll try to work around it.
yeah, that's why you can't have Box<dyn Clone> for example
There was a problem hiding this comment.
Thanks for investigating. Great to see it not being needed.
|
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Other than the introduction of Sized, this looks good to me.
swarm/src/behaviour.rs
Outdated
| /// } | ||
| /// ``` | ||
| pub trait NetworkBehaviour: 'static { | ||
| pub trait NetworkBehaviour: Sized + 'static { |
There was a problem hiding this comment.
I would prefer to avoid casually introducing trait bounds without careful consideration. Is there any way around introducing this trait bound? Is this pull request worth doing given the added complexity.
I would have to dig deeper here. Based on intuition, adding Sized, Send or Sync unintentionally has not proven to be good ideas in the past.
Previously, we used the full reference to the `OutEvent` of the `ConnectionHandler` in all implementations of `NetworkBehaviour`. Not only is this very verbose, it is also more brittle to changes. With the current implementation plan for libp2p#2824, we will be removing the `IntoConnectionHandler` abstraction. Using a type-alias to refer to the `OutEvent` makes the migration much easier.
Description
Previously, we used the full reference to the
OutEventof theConnectionHandlerin all implementations ofNetworkBehaviour. Not only is this very verbose, it is also more brittle to changes. With the current implementation plan for #2824, we will be removing theIntoConnectionHandlerabstraction. Using a type-alias to refer to theOutEventmakes the migration much easier.Notes
This will reduce the diff in #3254.
Links to any relevant issues
Open Questions
Change checklist
I have added tests that prove my fix is effective or that my feature worksA changelog entry has been made in the appropriate crates