`Either` rework broke `option_layer`

[davidpdrsn]

In https://github.com/tower-rs/tower/pull/637 we changed Either to not change the error but instead require the two inner services have the same error type. This broke option_layer as demonstrated by:

fn main() {
    let svc = ServiceBuilder::new()
        .option_layer(Some(TimeoutLayer::new(Duration::from_secs(1))))
        .service_fn(|()| async { Ok::<_, io::Error>(()) });

    assert_service(svc);
}

fn assert_service<S, R, T, E>(svc: S) -> S
where
    S: Service<R, Response = T, Error = E>,
{
    svc
}

This fails with

Checking foo v0.1.0 (/Users/davidpdrsn/Desktop/foo)
error[E0271]: type mismatch resolving `<impl Future<Output = Result<(), std::io::Error>> as Future>::Output == Result<_, Box<(dyn std::error::Error + Send + Sync + 'static)>>`
  --> src/main.rs:11:20
   |
11 |     assert_service(svc);
   |     -------------- ^^^ expected struct `Box`, found struct `std::io::Error`
   |     |
   |     required by a bound introduced by this call
   |
   = note: expected enum `Result<_, Box<(dyn std::error::Error + Send + Sync + 'static)>>`
              found enum `Result<(), std::io::Error>`
   = note: required because of the requirements on the impl of `Service<()>` for `ServiceFn<[closure@src/main.rs:9:21: 9:58]>`
   = note: 1 redundant requirement hidden
   = note: required because of the requirements on the impl of `Service<()>` for `Either<tower::timeout::Timeout<ServiceFn<[closure@src/main.rs:9:21: 9:58]>>, ServiceFn<[closure@src/main.rs:9:21: 9:58]>>`
note: required by a bound in `assert_service`
  --> src/main.rs:16:8
   |
14 | fn assert_service<S, R, T, E>(svc: S) -> S
   |    -------------- required by a bound in this
15 | where
16 |     S: Service<R, Response = T, Error = E>,
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `assert_service`

For more information about this error, try `rustc --explain E0271`.
error: could not compile `foo` due to previous error

The compiler runs into BoxError and io::Error not being the same type.

The workaround is to add a .map_err(Into::<BoxError>::into):

let svc = ServiceBuilder::new()
    .option_layer(Some(TimeoutLayer::new(Duration::from_secs(1))))
    .map_err(Into::<BoxError>::into)
    .service_fn(|()| async { Ok::<_, io::Error>(()) });

The doc test for option_layer didn't catch this because it didn't actually check that the resulting service implements Service.

I think we should consider divorcing option_layer from Either such that option_layer always uses BoxError as the error type.

As a separate note I think as part of tower-service 1.0 we should consider adding impl Layer<S> for Option<L>. This was discussed back when we added option_layer but I've since come around and I think having impl Layer<S> for Option<L> would be good. Thats a separate discussion though.

[w4]

We've been waiting for option_layer & Either to support services returning the same error to clean up some of the duplicate types/boxing in our server implementation, so many thanks for that change.

option_layer(T) -> L where L::Service::Error = BoxError is certainly useful for other parts of the tower ecosystem in which error types are discarded, however for us feeble Infallible lot an alternative would certainly be welcome - whether that's via impl Layer<S> for Option<L> or otherwise.

[davidpdrsn]

impl Layer<S> for Option<L> I remember @hawkw suggesting that a while ago. Having used tracing which has a similar impl I think I've come around and now think adding such an impl to tower would be nice. Not sure if it can fix this issue though.

[w4]

I suppose the implementation could be replaced with a MapErr on both sides of the Either so option_layer become something along the lines of:

pub fn option_layer<L>(layer: Option<L>) -> Either<MapErrLayer<L, fn(L::Service::Error) -> BoxError>, MapErrLayer<Identity, fn(Identity::Service::Error) -> BoxError> {
    match layer {
        Some(layer) => Either::A(MapErrLayer::new(layer, Into::into)),
        None => Either::B(MapErrLayer::new(Identity, Into::into)),
    }
}

But having both option_layer and Option<Layer> in the API might become a little confusing. Perhaps deprecating option_layer and providing a impl dyn Layer { fn box_err(self) -> MapErrLayer<Self, fn(L::Error) -> BoxError> } would help ease the transition.. So .option_layer(opt) becomes .layer(Layer::from(opt).box_err())