Drop custom Either types

[thomaseizinger]

Description

We have a lot of custom Either types within libp2p-core, some of which (I think) could go away by implementing our traits directly on the Either type that we are already depending on.

Motivation

Less code to maintain.

Checklist

• EitherTransport: refactor(core)!: remove EitherTransport #3338
• EitherError: refactor(core)!: remove EitherError in favor of either::Either #3337
• EitherOutput: refactor(core)!: remove EitherOutput #3341
• EitherUpgrade: refactor(core)!: remove EitherUpgrade #3339
• EitherName: Cannot be removed at the moment because it would conflict with the blanket implementation of ProtocolName over T. This will however be resolved as soon as misc/multistream-select: Treat protocol as String and not [u8] #2831 is fixed.
• EitherFuture:
  • In the current state of the codebase, we cannot remove this because we need an implementation of Future that returns Either and does not force the same Output on both types.
  • We can remove some of the usages with core: Remove Transport::{Dial,ListenUpgrade} in favor of using trait objects #3347.
  • The other usage is within SelectUpgrade. That one I believe we cannot remove because SelectUpgrade is for example used to choose between two muxers which is functionality we definitely want. However, in order to make Re-organising crate structure #3271 easier, we can at least move this dedicated EitherFuture type into the same crate as SelectUpgrade because it has only one user.
• EitherIter: Cannot be removed because Either does not implement IntoIterator. If we still need this after misc/multistream-select: Treat protocol as String and not [u8] #2831 is merged, we can request it upstream.

Are you planning to do it yourself in a pull request?

No / Maybe. Could be a good first issue. What do you think @mxinden?

[mxinden]

Sounds good to me. Also, should we be using either::Either or futures::future::Either or both?

[thomaseizinger]

Sounds good to me. Also, should we be using either::Either or futures::future::Either or both?

I am not sure how well maintained either::Either is but feels like the more appropriate one, given that Future is part of std now?

I've had a brief look and for example the Error impl in either::Either is not overriding all methods so it may need a bit of upstream work until we can completely replace our types.

I think as part of this ticket, one would need to do a bit more research to see if we can drop future::Either for example. Would be interesting to know, why they have their own Either type in the first place.

[maschad]

examining it naively the future::Either could be dropped as it seems to serve as a convenience enum type, which could be replaced by what you mentioned earlier by implementing our traits directly on the Either type or implementing IntoFuture to be awaited.

[thomaseizinger]

I've had a brief look and for example the Error impl in either::Either is not overriding all methods so it may need a bit of upstream work until we can completely replace our types.

This is fixed now: rayon-rs/either#69

There is also a new release coming: rayon-rs/either#73

[thomaseizinger]

I've opened an issue regarding pin projection which is another blocker for some of our Either use cases: rayon-rs/either#76

[thomaseizinger]

I opened an issue with either to see if we can get AsyncRead + AsyncWrite traits. Then we could drop the EitherOutput type: rayon-rs/either#79

[thomaseizinger]

I opened an issue with either to see if we can get AsyncRead + AsyncWrite traits. Then we could drop the EitherOutput type: bluss/either#79

Update: As suggested in the linked issue, we could simply use the futures::Either type for all cases where we need implementations of AsyncRead etc.

I think that is a sensible way forward. Using the same type everywhere would be better but using two already existing ones is already an improvement over defining our own types in my opinion.

[thomaseizinger]

I opened an issue with either to see if we can get AsyncRead + AsyncWrite traits. Then we could drop the EitherOutput type: bluss/either#79

Update: As suggested in the linked issue, we could simply use the futures::Either type for all cases where we need implementations of AsyncRead etc.

I think that is a sensible way forward. Using the same type everywhere would be better but using two already existing ones is already an improvement over defining our own types in my opinion.

Unfortunately, this doesn't quite work.

1. Our EitherFuture type only works on TryFutures and directly transposes the result.
2. We use our EitherFuture type in multiple places. In some of them, the resulting Output should be an Either that implements futures traits and in some it should be the regular Either.
3. The futures::Either type implements Future such that both futures need to evaluate to the same type which is a bit silly IMO and probably the reason we have our own type.

The bottom line is that I think we should convince either that they should provide a Future etc implementation that doesn't try to be clever but simply sets all associated types like Future::Output and Stream::Item to Either.

[thomaseizinger]

The bottom line is that I think we should convince either that they should provide a Future etc implementation that doesn't try to be clever but simply sets all associated types like Future::Output and Stream::Item to Either.

That is also not possible because they already implement Future in the exact same way as futures::Either. Unfortunately, this means we are stuck with a custom EitherFuture type that behaves the way we need it. The good news is that we might only need it for SelectUpgrade if we end up doing #3347.

[thomaseizinger]

EitherFuture is here to stay for a while so I am considering this to be completed because everything that is actionable has been done.