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

Propagate either feature and enable by default from the facade #1151

Closed
wants to merge 1 commit into from

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jul 31, 2018

No description provided.

@cramertj
Copy link
Member

Is this primarily for streams and sinks? Because we can no longer implement Future for Either.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 31, 2018

I didn’t actually check what it enabled, I just noticed it wasn’t propagated when looking at the compat stuff.

Maybe it would be better to just drop the either stuff now that it can’t be implemented for Future, that was @MajorBreakfast’s sentiment on #1152

@cramertj
Copy link
Member

I still think that some form or another of the either stuff is still needed so long as we don't have async/await like syntax for writing Streams or Sinks that delegate to one of several possible children.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Jul 31, 2018

We could introduce a Coalesce enum generated via a macro (like Join)

Coalesce, Coalesce3, Coalesce4, Coalesce5

e.g.

enum Coalesce3<T1, T2, T3> {
    Variant1(T1),
    Variant2(T2),
    Variant3(T3),
}

That enum would be local to our crate. We can add a correct Unpin impl

impl<T1: Unpin, T2: Unpin, T3: Unpin> Unpin for Coalesce3 {}

and Future and Stream impls

The main advantage is of course that we can add support for more than two variants. I'd say five, like we have for Join, should be enough.

Edit: I don't think that we should add combinator methods for this. At least I think so :)

@cramertj
Copy link
Member

@MajorBreakfast I have a macro that does this in fuchsia already. However, I realized that it is unsafe under 0.3, since there's nothing stopping the user from implementing Unpin for the resulting type unconditionally. In the 0.3 branch I renamed the macro to unsafe_many_futures and noted that it was unsound to implement Unpin or Drop for the resulting type. However, you're right that we could force the issue by implementing Unpin and Drop ourselves, preventing the user from implementing either. However, that potentially becomes unsound if rust-lang/rust#29864 ever becomes stable, as it would allow overlapping impls of Unpin (permitting the user to write an unconditional impl). I don't know how much we should concern ourselves with this particular case, though. WDTY?

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Jul 31, 2018

@cramertj I actually didn't consider that we could make the macro public. But it makes sense to do so. If it's prefixed with unsafe_ with an explanation in the docs, then it's IMO quite okay to make it public.

I think it makes sense to expose the macro because I'm certain that someone can come up with a use case that requires 12 variants.

@cramertj
Copy link
Member

cramertj commented Jul 31, 2018

@MajorBreakfast okay! yeah, in that case you can just drag-and-drop https://fuchsia.googlesource.com/garnet/+/sandbox/cramertj/asyncing/public/rust/crates/fuchsia-async/src/lib.rs#46 (although you'd want to change the PinMut and Future references to $crate::core_reexport:...-- the PinMuts in that version that are unprefixed).

You'd also want to add the Stream and Sink impls.

The left_sink(), right_sink(), left_stream(), and right_stream() extension methods are also occasionally more convenient, so it might be a good idea to continue offering those (via the Either type). What do you think?

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Jul 31, 2018

I'd prefer to remove the either dependency entirely because we can't implement it consistently for all traits. Also, offering two solutions for the same thing seems weird.

I suggest to not add any combinator methods. If we added methods for Coalesce to Coalesce5, we would need to add 14 methods. We could of course only add the two methods for Coalesce, bit IMO the Coalesce::Variant1(future) notation is nice enough.

@Nemo157 Nemo157 closed this Nov 15, 2018
@Nemo157 Nemo157 deleted the propagate-either branch February 16, 2019 16:16
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.

3 participants