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

Change impl Future for Either to Output = Either #79

Open
thomaseizinger opened this issue Jan 11, 2023 · 14 comments
Open

Change impl Future for Either to Output = Either #79

thomaseizinger opened this issue Jan 11, 2023 · 14 comments

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jan 11, 2023

Currently, the implementation of Future for Either requires that both Futures resolve to the same type:

either/src/lib.rs

Lines 1127 to 1141 in af9f5fb

/// `Either<L, R>` is a future if both `L` and `R` are futures.
impl<L, R> Future for Either<L, R>
where
L: Future,
R: Future<Output = L::Output>,
{
type Output = L::Output;
fn poll(
self: Pin<&mut Self>,
cx: &mut core::task::Context<'_>,
) -> core::task::Poll<Self::Output> {
for_both!(self.as_pin_mut(), inner => inner.poll(cx))
}
}

This limits, which kinds of Futures can be expressed using Either. It would be more useful to set the Output type to Either<A::Output, B::Output>. This is an API breaking change.

The original behaviour can be restored using the Either::into_inner function. In case both Futures, A and B have the same Output, the into_inner function can be used to "unwrap" the Either:

either/src/lib.rs

Lines 916 to 930 in af9f5fb

impl<T> Either<T, T> {
/// Extract the value of an either over two equivalent types.
///
/// ```
/// use either::*;
///
/// let left: Either<_, u32> = Left(123);
/// assert_eq!(left.into_inner(), 123);
///
/// let right: Either<u32, _> = Right(123);
/// assert_eq!(right.into_inner(), 123);
/// ```
pub fn into_inner(self) -> T {
for_both!(self, inner => inner)
}

@thomaseizinger thomaseizinger changed the title Implement AsyncRead and AsyncWrite? Implement traits from futures? Jan 11, 2023
@cuviper
Copy link
Member

cuviper commented Jan 11, 2023

Is there a reason you're not using futures::future::Either for that?

@thomaseizinger
Copy link
Contributor Author

Is there a reason you're not using futures::future::Either for that?

I guess we could yeah! I'll try and report back.

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Jan 17, 2023

It doesn't work at the moment because project is not exposed publicly. However, I think from the previous responses, people are okay with adding it so I'll submit a PR now.

Edit: PR submitted here: rust-lang/futures-rs#2691

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Jan 17, 2023

Another thing I just discovered is that futures::Either has a - IMO - very weird implementation of Future:

impl<A, B> Future for Either<A, B>
where
    A: Future,
    B: Future<Output = A::Output>,
{
    type Output = A::Output;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        match self.project() {
            Either::Left(x) => x.poll(cx),
            Either::Right(x) => x.poll(cx),
        }
    }
}

I would have expected that a Either<F1, F2> yields me another Either type but the implementation in futures forces both futures to resolve to the same type.

@thomaseizinger
Copy link
Contributor Author

Would you be up for merging an implementation of the futures traits that doesn't try to be clever and simply sets all associated types to Either, i.e. Future::Output, Stream::Item, etc

@cuviper
Copy link
Member

cuviper commented Jan 17, 2023

Ah, note that Future is a re-export from core, and #77 already implemented that with a unified Output. I'm not sure why I did that -- probably just because that's how we treat Iterator too.

@thomaseizinger
Copy link
Contributor Author

Ah, note that Future is a re-export from core, and #77 already implemented that with a unified Output. I'm not sure why I did that -- probably just because that's how we treat Iterator too.

Dang so that would be a breaking change :(

I guess I'll have to stick with my custom Either type then.

@thomaseizinger
Copy link
Contributor Author

FWIW, a not unified output would have been more powerful because you can always call into_inner if they end up being the same type.

Not sure if you ever plan on doing 2.0 but that could be a consideration :)

@cuviper
Copy link
Member

cuviper commented Jan 19, 2024

FWIW, a not unified output would have been more powerful because you can always call into_inner if they end up being the same type.

It's not exactly equivalent -- they can be different Future types (so into_inner won't work) yet still have the same Future::Output, enabling the current Future for Either.

@thomaseizinger
Copy link
Contributor Author

FWIW, a not unified output would have been more powerful because you can always call into_inner if they end up being the same type.

It's not exactly equivalent -- they can be different Future types (so into_inner won't work) yet still have the same Future::Output, enabling the current Future for Either.

I am not sure I follow? What I meant was:

impl<A, B> Future for Either<A, B> {
	type Output = Either<A::Output, B::Output>;

	// ...
}

If A and B happen to resolve to the same type (like u32) you end up with Either<u32, u32> at which point you can call .into_inner.

Am I missing something?

@cuviper
Copy link
Member

cuviper commented Jan 20, 2024

Oh, calling into_inner on the output, after polling/await -- yes that would work.

@thomaseizinger
Copy link
Contributor Author

Oh, calling into_inner on the output, after polling/await -- yes that would work.

Would you like me to re-word this issue for changing the Future implementation (in a potential 2.0) release?

@cuviper
Copy link
Member

cuviper commented Jan 20, 2024

Sure, it wouldn't hurt to have a clean summary of proposed changes.

@thomaseizinger thomaseizinger changed the title Implement traits from futures? Change impl Future for Either to Output = Either Jan 21, 2024
@thomaseizinger
Copy link
Contributor Author

Sure, it wouldn't hurt to have a clean summary of proposed changes.

Done!

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

No branches or pull requests

2 participants