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

Allow access to the Either type #5

Open
seanmonstar opened this issue Aug 1, 2018 · 16 comments
Open

Allow access to the Either type #5

seanmonstar opened this issue Aug 1, 2018 · 16 comments
Labels
feature New feature or request rfc Extra attention is needed

Comments

@seanmonstar
Copy link
Owner

seanmonstar commented Aug 1, 2018

The Filter::or combinator returns an Either<A, B> type, but it currently isn't exported publicly, making it impossible to match on the variants. It does implement Reply, making the full framework function, but it's currently impossible to use or to retrieve either A or B.

Some reasons why it hasn't been exposed yet:

  • It may be beneficial to change from Either<A, B> to instead more like Coprod<A, Coprod<B, Never>>. The benefit of doing this is that nesting Either<A, B> will result in the enums getting fatter and fatter, where as the Coprod approach makes unlimited nested ors still only need space for 1 variant tag (turns out I was wrong about Coprod, it also grows the size).
@davidbarsky
Copy link

Adding a small note—I'm writing a generator for Twirp. It'd be pretty nice to be able to write standalone, reusable filters that accept either application/json or application/protobuf, but the or combinator changes the return type of the filter (I'm currently using Hyper directly).

On an aside, is there any reasonable way to hide the Self::Extract tuple as seen here?

(This problem, of course, would go away if Rust had anonymous sum types.)

@seanmonstar
Copy link
Owner Author

I believe with trait specialization, single value extractions can be just T, instead of (T,).

@davidbarsky
Copy link

Ah, gotcha—thank you for clarifying! I suppose specialization would be a better solution in this case than anon. sum types.

@seanmonstar
Copy link
Owner Author

seanmonstar commented Aug 8, 2018

I do think it's probably best to change the current Either to be more like frunk's Coprod type, but figuring out the nicest way to do that may take some time (and be brutal to name).

I have wanted at times to be able to sort of "unify" over an Either, when I don't care about which exact case it was, and they are all the same type. Something like the either crate's into_inner(). It could be possible to come up with a trait that's implemented over Either<T, T> that does this, and then it could be used in user code like this:

let path_id = warp::path::param::<u64>();
let header_id = warp::header::<u64>("x-todo-id");

path_id
    .or(header_id)
    .map(handle_id);

// naming is hard
fn handle_id(id: impl EitherUnify<u64>) {
    // ...
}

An alternative is to add some unify or either combinator on Filter that does this for you...

@davidbarsky
Copy link

I do think it's probably best to change the current Either to be more like frunk's Coprod type, but figuring out the nicest way to do that may take some time (and be brutal to name).

Yeah, that's fair. It's kinda challenging and exposing Coproduct directly seems... not right?

I have wanted at times to be able to sort of "unify" over an Either, when I don't care about which exact case it was, and they are all the same type. Something like the either crate's into_inner(). It could be possible to come up with a trait that's implemented over Either<T, T> that does this, and then it could be used in user code like this: [code omitted]:

Oh, I like that API a lot! To be clear, if a String and a u64 is extracted, the type of EitherUnify<_> would be impl EitherUnify<String, u64>?

@seanmonstar
Copy link
Owner Author

To be clear, if a String and a u64 is extracted, the type of EitherUnify<_> would be impl EitherUnify<String, u64>?

Oh... No, I had meant specifically when all the types in the Either are the same. So, Either<u64, u64> could be unified to a single u64. If they are different types, then it sounds like you need to match on the Either, right?

@davidbarsky
Copy link

Oh... No, I had meant specifically when all the types in the Either are the same.

I got that part! I can see how Either<u64, u64> can be unified to a single u64. I phrased my followup question poorly—namely, I dropped the tuple in impl EitherUnify<(String, u64)>, trying to write pseudocode that extracts both a string and a u64. I believe specialization would cover this use-case better (hence me commenting here).

I'm sorry if I'm not making sense—I've got too many meetings that are melting my brain.

@seanmonstar
Copy link
Owner Author

Ah, yes, it should, since it'd work for any T, as long as both sides of Either are the same T.

So, for now, I added a Filter::unify() combinator, which will allow anyone to convert to the inner T when using things like or to combine 2 filters that provide the same type.

@seanmonstar seanmonstar added feature New feature or request rfc Extra attention is needed labels Aug 13, 2018
@olivia-fl
Copy link

I'm not sure whether this is the right approach here, but I'm trying to do something like this:

fn render<T>(template: T) -> impl warp::Reply {
    match template.render() {
        Ok(html) => Either::Left(warp::reply::html(html)),
        Err(err) => {
            error!("template rendering failed: {} (for {:#?})", err, template);
            Either::Right(warp::http::StatusCode::INTERNAL_SERVER_ERROR)
    }
}

Since Either isn't exposed, there's no good way to return two different types of Reply from the same function. Is there another approach I should take here, or is waiting for Either the way to go?

@olivia-fl
Copy link

Another thing I just thought of, is there any reason not to just use the either crate for this?

@remexre
Copy link
Contributor

remexre commented Jan 7, 2019

I'm encountering a similar situation; I'd be strongly in favor of either using the either crate's Either type or just using frunk's Coproduct. (If frunk is chosen, the Hlist in src/generic.rs could be nixed too.)

I've got a branch here using the either crate if you need this now, though I'd prefer warp to lean on frunk more heavily instead.

@olivia-fl
Copy link

I'm not sure this issue is the right place to mention or discuss this, but in my fairly limited experience using warp, it seems like extensibility is really hurt by the fact that many of the types or traits needed to write things are private. Because you don't have access to many of the parts that make the built in parts of warp function, extending it in a composable way is very hacky.

@remexre
Copy link
Contributor

remexre commented Jan 21, 2019

@seanmonstar would a PR to frunkify Either be accepted?

This'd essentially just change anything using Either<A, B> to use Coprod!(A, B) instead for now, but it'd be nice to see .or() use .embed() to make most cases of .unify() unnecessary. That'd be a separate PR though, and a very breaking change.

@seanmonstar
Copy link
Owner Author

I had initially thought that if nested enums were used such that the final type were uninhabitable, Rust could optimize the layout to not need space for all the nested tags. I was wrong.

So, maybe, instead there could be something like the Tuple/HList stuff but as a set of enums, like Either2, Either3, etc. Then, combining filters with or could convert an Either2 into an Either3... I haven't tried this out in practice though.

@remexre
Copy link
Contributor

remexre commented Jan 21, 2019

Eh....... that seems a lot grosser; I'd honestly rather wait to see language support for "first-class" anonymous sums instead. RFC1154 got closed for prioritization reasons; I might try a revival of it as my first RFC after #35121/RFC1216 lands.

@remexre
Copy link
Contributor

remexre commented Feb 2, 2019

Would a PR of https://github.com/remexre/warp/tree/either-crate be accepted for 0.2, with the change to first-class enums (or Coproduct if optimized tags happen first) being pushed off to a later breaking release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request rfc Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants
@seanmonstar @davidbarsky @remexre @olivia-fl and others