refactor(core)!: remove `EitherOutput` #3341

thomaseizinger · 2023-01-17T13:11:00Z

Description

The trick with this one is to use futures::Either everywhere where we may wrap something that implements any of the futures traits. This includes the output of EitherFuture itself. We also need to implement StreamMuxer on future::Either because StreamMuxers may be the the Output of InboundUpgrade.

Notes

~~Draft because it doesn't compile yet.~~

This should unblock the compile errors I've been getting while resolving the merge conflicts in #3254.

Links to any relevant issues

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

)

…port

…ure2

thomaseizinger · 2023-01-18T12:45:56Z

swarm/src/handler/either.rs

-impl<LOP, ROP, LOOI, ROOI>
-    FullyNegotiatedOutbound<Either<SendWrapper<LOP>, SendWrapper<ROP>>, Either<LOOI, ROOI>>
-where
-    LOP: OutboundUpgradeSend,
-    ROP: OutboundUpgradeSend,
-{
-    fn transpose(
-        self,
-    ) -> Either<FullyNegotiatedOutbound<LOP, LOOI>, FullyNegotiatedOutbound<ROP, ROOI>> {
-        match self {
-            FullyNegotiatedOutbound {
-                protocol: EitherOutput::First(protocol),
-                info: Either::Left(info),
-            } => Either::Left(FullyNegotiatedOutbound { protocol, info }),
-            FullyNegotiatedOutbound {
-                protocol: EitherOutput::Second(protocol),
-                info: Either::Right(info),
-            } => Either::Right(FullyNegotiatedOutbound { protocol, info }),
-            _ => unreachable!(),
-        }
-    }
-}
-
-impl<LOP, ROP, LOOI, ROOI>
-    DialUpgradeError<Either<LOOI, ROOI>, Either<SendWrapper<LOP>, SendWrapper<ROP>>>
-where
-    LOP: OutboundUpgradeSend,
-    ROP: OutboundUpgradeSend,
-{
-    fn transpose(self) -> Either<DialUpgradeError<LOOI, LOP>, DialUpgradeError<ROOI, ROP>> {
-        match self {
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(Either::Left(error))),
-                info: Either::Left(info),
-            } => Either::Left(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(error)),
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(Either::Right(error))),
-                info: Either::Right(info),
-            } => Either::Right(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(error)),
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(error)),
-                info: Either::Left(info),
-            } => Either::Left(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(error)),
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(error)),
-                info: Either::Right(info),
-            } => Either::Right(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(error)),
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timer,
-                info: Either::Left(info),
-            } => Either::Left(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timer,
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timer,
-                info: Either::Right(info),
-            } => Either::Right(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timer,
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timeout,
-                info: Either::Left(info),
-            } => Either::Left(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timeout,
-                info,
-            }),
-            DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timeout,
-                info: Either::Right(info),
-            } => Either::Right(DialUpgradeError {
-                error: ConnectionHandlerUpgrErr::Timeout,
-                info,
-            }),
-            _ => unreachable!(),
-        }
-    }
-}


These can now go away because we unified the either types and there is already an implementation for it.

thomaseizinger · 2023-01-20T03:49:12Z

This is ready for review @mxinden @jxs.

jxs

LGTM Thanks Thomas. The unsafe code seems fine, but if possible I'd rather have it upstream.

jxs · 2023-01-20T10:36:35Z

core/src/either.rs

+/// pinned projections of the inner variants.
+///
+/// Local function until <https://github.com/rust-lang/futures-rs/pull/2691> is merged.
+fn as_pin_mut<A, B>(


Should we ask for project to be public? As it seems the same purpose and would allow us to not have unsafe code here.

update:
sorry only now saw rayon-rs/either#76, can't we use as_pin_mut from either::Either?

Should we ask for project to be public? As it seems the same purpose and would allow us to not have unsafe code here.

See rust-lang/futures-rs#2629 (comment) :)

This is the future::Either type, we can't use either::Either here because it doesn't implement AsyncRead etc

yeah I saw after, thanks Thomas :)
It it would be nice for either::Either to implement the futures traits as you asked on rayon-rs/either#79. But it seems you also submitted rust-lang/futures-rs#2691 which will allow us to remove the unsafe code right? Ideally what would be really nice was for a single Either type 😀 which will be possible when rust-lang/futures-rs#2691 is merged right? Nonetheless thanks for your work simplifying this Thomas!

yeah I saw after, thanks Thomas :)
It it would be nice for either::Either to implement the futures traits as you asked on rayon-rs/either#79. But it seems you also submitted rust-lang/futures-rs#2691 which will allow us to remove the unsafe code right?

Correct!

Ideally what would be really nice was for a single Either type 😀 which will be possible when rust-lang/futures-rs#2691 is merged right?

Not quite unfortunately, we also use Either in places where it needs to implement std::error::Error for example and futures::Either doesn't AFAIK. I agree that it would be nice but at least after these patches, we don't have a problem with there being two different Either types.

Nonetheless thanks for your work simplifying this Thomas!

You are welcome ☺️

wdyt of then leaving a TODO on the unsafe code to be removed when a futures gets released with rust-lang/futures-rs#2691 ?

wdyt of then leaving a TODO on the unsafe code to be removed when a futures gets released with rust-lang/futures-rs#2691 ?

Do you mean like line 93? 🙈

ah! right LOL 😂

thomaseizinger · 2023-01-20T12:41:56Z

LGTM Thanks Thomas. The unsafe code seems fine, but if possible I'd rather have it upstream.

Yeah me too. It will merge eventually and it is a private function so I don't think it is blocking :)

thomaseizinger · 2023-01-23T05:52:45Z

Merging here to move forward with #3254.

thomaseizinger · 2023-01-23T11:45:43Z

@Mergifyio requeue

mergify · 2023-01-23T11:46:18Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

thomaseizinger · 2023-01-23T11:46:45Z

@Mergifyio queue

mergify · 2023-01-23T11:47:17Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 4b41f5a

thomaseizinger added 22 commits January 11, 2023 16:13

Use single quotes to avoid running commands

3a84c14

Use intermediary variable

458aa53

Merge branch 'master' into no-run-title-as-command

341d096

refactor(relay): introduce Handler::new functions (#3326)

aed5363

fix: revert "refactor(relay): introduce Handler::new functions" (#3333

5e97d0e

)

Replace EitherError with Either

da97e8f

Add changelog entry

2ce805d

Remove EitherTransport in favor of Either

0a2729d

Update core/CHANGELOG.md

3397266

Merge branch '2650-remove-either-error' into 2650-remove-either-trans…

5802177

…port

Add changelog entry

7060f4b

Merge branch 'no-run-title-as-command' into 2650-remove-either-error

82eb6c2

Merge branch 'master' into no-run-title-as-command

955ff80

Merge branch 'no-run-title-as-command' into 2650-remove-either-error

bb84906

Merge branch '2650-remove-either-error' into 2650-remove-either-trans…

fdfaf72

…port

Update changelog entry

59f6465

Remove EitherUpgrade

8872039

Update core/CHANGELOG.md

d3e79cf

Remove EitherFuture2

1526051

add changelog entry

1b25ec1

Merge branch '2650-remove-either-upgrade' into 2650-remove-either-fut…

4164aa8

…ure2

WIP: Remove EitherOutput

0c37e77

thomaseizinger mentioned this pull request Jan 17, 2023

Drop custom Either types #2650

Closed

7 tasks

Base automatically changed from 2650-remove-either-future2 to master January 18, 2023 10:17

thomaseizinger added 2 commits January 18, 2023 23:36

Merge branch 'master' into 2650-remove-either-output

37215eb

Make it compile

6301ff3

thomaseizinger commented Jan 18, 2023

View reviewed changes

thomaseizinger added 2 commits January 18, 2023 23:49

Fix rustdoc link

04e0682

Add changelog entry

aa377be

thomaseizinger marked this pull request as ready for review January 18, 2023 13:13

thomaseizinger requested review from mxinden and jxs January 18, 2023 13:14

Merge branch 'master' into 2650-remove-either-output

ff64c4d

jxs approved these changes Jan 20, 2023

View reviewed changes

thomaseizinger added the send-it label Jan 23, 2023

Merge branch 'master' into 2650-remove-either-output

79b80dd

Merge branch 'master' into 2650-remove-either-output

7218b93

mergify bot merged commit 4b41f5a into master Jan 23, 2023

mergify bot deleted the 2650-remove-either-output branch January 23, 2023 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core)!: remove `EitherOutput` #3341

refactor(core)!: remove `EitherOutput` #3341

thomaseizinger commented Jan 17, 2023 •

edited

Loading

thomaseizinger Jan 18, 2023

thomaseizinger commented Jan 20, 2023

jxs left a comment

jxs Jan 20, 2023 •

edited

Loading

thomaseizinger Jan 20, 2023

jxs Jan 20, 2023

thomaseizinger Jan 21, 2023

jxs Jan 21, 2023

thomaseizinger Jan 21, 2023

jxs Jan 21, 2023

thomaseizinger commented Jan 20, 2023

thomaseizinger commented Jan 23, 2023

thomaseizinger commented Jan 23, 2023

mergify bot commented Jan 23, 2023

thomaseizinger commented Jan 23, 2023

mergify bot commented Jan 23, 2023 •

edited

Loading

refactor(core)!: remove EitherOutput #3341

refactor(core)!: remove EitherOutput #3341

Conversation

thomaseizinger commented Jan 17, 2023 • edited Loading

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger Jan 18, 2023

Choose a reason for hiding this comment

thomaseizinger commented Jan 20, 2023

jxs left a comment

Choose a reason for hiding this comment

jxs Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

thomaseizinger Jan 20, 2023

Choose a reason for hiding this comment

jxs Jan 20, 2023

Choose a reason for hiding this comment

thomaseizinger Jan 21, 2023

Choose a reason for hiding this comment

jxs Jan 21, 2023

Choose a reason for hiding this comment

thomaseizinger Jan 21, 2023

Choose a reason for hiding this comment

jxs Jan 21, 2023

Choose a reason for hiding this comment

thomaseizinger commented Jan 20, 2023

thomaseizinger commented Jan 23, 2023

thomaseizinger commented Jan 23, 2023

mergify bot commented Jan 23, 2023

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

thomaseizinger commented Jan 23, 2023

mergify bot commented Jan 23, 2023 • edited Loading

✅ The pull request has been merged automatically

refactor(core)!: remove `EitherOutput` #3341

refactor(core)!: remove `EitherOutput` #3341

thomaseizinger commented Jan 17, 2023 •

edited

Loading

jxs Jan 20, 2023 •

edited

Loading

mergify bot commented Jan 23, 2023 •

edited

Loading