convert several Stack unit errors into Never

Since this stack pieces will never error, we can mark their
`Error`s with a type that can "never" be created. When seeing an `Error
= ()`, it can either mean the error never happens, or that the detailed
error is dealt with elsewhere and only a unit is passed on. When seeing
`Error = Never`, it is clearer that the error case never happens.
Besides helping humans, LLVM can also remove the error branchs entirely.

Signed-off-by: Sean McArthur <sean@buoyant.io>

Cargo.lock

@@ -505,6 +505,10 @@ dependencies = [
  "quickcheck 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
+[[package]]
+name = "linkerd2-never"
+version = "0.1.0"
+
 [[package]]
 name = "linkerd2-proxy"
 version = "0.1.0"
@@ -525,6 +529,7 @@ dependencies = [
  "libc 0.2.40 (registry+https://github.com/rust-lang/crates.io-index)",
  "linkerd2-fs-watch 0.1.0",
  "linkerd2-metrics 0.1.0",
+ "linkerd2-never 0.1.0",
  "linkerd2-proxy-api 0.1.3 (git+https://github.com/linkerd/linkerd2-proxy-api?tag=v0.1.3)",
  "linkerd2-router 0.1.0",
  "linkerd2-stack 0.1.0",
@@ -597,6 +602,7 @@ version = "0.1.0"
 dependencies = [
  "futures 0.1.23 (registry+https://github.com/rust-lang/crates.io-index)",
  "futures-watch 0.1.0 (git+https://github.com/carllerche/better-future)",
+ "linkerd2-never 0.1.0",
  "linkerd2-task 0.1.0",
  "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "tokio 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)",

Cargo.toml

@@ -25,6 +25,7 @@ flaky_tests = []
 futures-mpsc-lossy = { path = "lib/futures-mpsc-lossy" }
 linkerd2-fs-watch  = { path = "lib/fs-watch" }
 linkerd2-metrics   = { path = "lib/metrics" }
+linkerd2-never     = { path = "lib/never" }
 linkerd2-router    = { path = "lib/router" }
 linkerd2-stack     = { path = "lib/stack" }
 linkerd2-task      = { path = "lib/task" }

lib/never/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "linkerd2-never"
+version = "0.1.0"
+authors = ["Sean McArthur <sean@buoyant.io>"]
+publish = false
+
+[dependencies]

lib/never/src/lib.rs

@@ -0,0 +1,15 @@
+use std::{error::Error, fmt};
+
+/// A type representing a value that can never materialize.
+///
+/// This would be `!`, but it isn't stable yet.
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum Never {}
+
+impl fmt::Display for Never {
+    fn fmt(&self, _: &mut fmt::Formatter) -> fmt::Result {
+        match *self {}
+    }
+}
+
+impl Error for Never {}

lib/stack/Cargo.toml

@@ -8,6 +8,7 @@ publish = false
 log = "0.4.1"
 futures = "0.1"
 futures-watch = { git = "https://github.com/carllerche/better-future" }
+linkerd2-never = { path = "../never" }
 tower-service = { git = "https://github.com/tower-rs/tower" }
 
 [dev-dependencies]

lib/stack/src/lib.rs

@@ -1,6 +1,7 @@
 extern crate futures;
 #[macro_use]
 extern crate log;
+extern crate linkerd2_never as never;
 extern crate tower_service as svc;
 
 pub mod either;
@@ -53,7 +54,7 @@ pub trait Stack<T> {
 
 /// Implements `Stack<T>` for any `T` by cloning a `V`-typed value.
 pub mod shared {
-    use std::{error, fmt};
+    use never::Never;
 
     pub fn stack<V: Clone>(v: V) -> Stack<V> {
         Stack(v)
@@ -62,23 +63,12 @@ pub mod shared {
     #[derive(Clone, Debug)]
     pub struct Stack<V: Clone>(V);
 
-    #[derive(Debug)]
-    pub enum Error {}
-
     impl<T, V: Clone> super::Stack<T> for Stack<V> {
         type Value = V;
-        type Error = Error;
+        type Error = Never;
 
-        fn make(&self, _: &T) -> Result<V, Error> {
+        fn make(&self, _: &T) -> Result<V, Never> {
             Ok(self.0.clone())
         }
     }
-
-    impl fmt::Display for Error {
-        fn fmt(&self, _: &mut fmt::Formatter) -> fmt::Result {
-            unreachable!()
-        }
-    }
-
-    impl error::Error for Error {}
 }

src/app/main.rs

@@ -18,6 +18,7 @@ use dns;
 use drain;
 use logging;
 use metrics::{self, FmtMetrics};
+use never::Never;
 use proxy::{
     self, buffer,
     http::{client, insert_target, metrics::timestamp_request_open, normalize_uri, router},
@@ -341,7 +342,7 @@ where
                     outbound_listener,
                     accept,
                     connect,
-                    server_stack.map_err(|_| {}),
+                    server_stack,
                     config.outbound_ports_disable_protocol_detection,
                     get_original_dst.clone(),
                     drain_rx.clone(),
@@ -411,7 +412,7 @@ where
                     inbound_listener,
                     accept,
                     connect,
-                    source_stack.map_err(|_| {}),
+                    source_stack,
                     config.inbound_ports_disable_protocol_detection,
                     get_original_dst.clone(),
                     drain_rx.clone(),
@@ -484,16 +485,15 @@ fn serve<A, C, R, B, G>(
     drain_rx: drain::Watch,
 ) -> impl Future<Item = (), Error = io::Error> + Send + 'static
 where
-    A: svc::Stack<proxy::server::Source, Error = ()> + Send + Clone + 'static,
+    A: svc::Stack<proxy::server::Source, Error = Never> + Send + Clone + 'static,
     A::Value: proxy::Accept<Connection>,
     <A::Value as proxy::Accept<Connection>>::Io: Send + transport::Peek + 'static,
-    C: svc::Stack<connect::Target> + Send + Clone + 'static,
-    C::Error: error::Error + Send + 'static,
+    C: svc::Stack<connect::Target, Error = Never> + Send + Clone + 'static,
     C::Value: connect::Connect + Send,
     <C::Value as connect::Connect>::Connected: Send + 'static,
     <C::Value as connect::Connect>::Future: Send + 'static,
     <C::Value as connect::Connect>::Error: fmt::Debug + 'static,
-    R: svc::Stack<proxy::server::Source, Error = ()> + Send + Clone + 'static,
+    R: svc::Stack<proxy::server::Source, Error = Never> + Send + Clone + 'static,
     R::Value:
         svc::Service<Request = http::Request<proxy::http::Body>, Response = http::Response<B>>,
     R::Value: Send + 'static,

src/drain.rs

@@ -4,6 +4,8 @@ use futures::{Async, Future, Poll, Stream};
 use futures::future::Shared;
 use futures::sync::{mpsc, oneshot};
 
+use never::Never;
+
 /// Creates a drain channel.
 ///
 /// The `Signal` is used to start a drain, and the `Watch` will be notified
@@ -56,10 +58,6 @@ enum State<F> {
     Draining,
 }
 
-//TODO: in Rust 1.26, replace this with `!`.
-#[derive(Debug)]
-enum Never {}
-
 /// A future that resolves when all `Watch`ers have been dropped (drained).
 pub struct Drained {
     drained_rx: mpsc::Receiver<Never>,

src/lib.rs

@@ -44,6 +44,7 @@ extern crate try_lock;
 
 #[macro_use]
 extern crate linkerd2_metrics;
+extern crate linkerd2_never as never;
 extern crate linkerd2_proxy_api as api;
 extern crate linkerd2_task as task;
 extern crate linkerd2_timeout as timeout;

src/proxy/http/router.rs

@@ -6,6 +6,7 @@ use std::marker::PhantomData;
 use std::time::Duration;
 use std::{error, fmt};
 
+use never::Never;
 use svc;
 
 extern crate linkerd2_router;
@@ -117,7 +118,7 @@ where
     B: Default + Send + 'static,
 {
     type Value = Service<Req, Rec, Stk>;
-    type Error = ();
+    type Error = Never;
 
     fn make(&self, config: &Config) -> Result<Self::Value, Self::Error> {
         let inner = Router::new(

src/proxy/server.rs

@@ -1,4 +1,4 @@
-use futures::{future::{self, Either}, Future};
+use futures::{future::Either, Future};
 use h2;
 use http;
 use hyper;
@@ -9,6 +9,7 @@ use tower_h2;
 
 use Conditional;
 use drain;
+use never::Never;
 use svc::{Stack, Service, stack::StackNewService};
 use transport::{connect, tls, Connection, GetOriginalDst, Peek};
 use proxy::http::glue::{HttpBody, HttpBodyNewSvc, HyperServerSvc};
@@ -44,14 +45,13 @@ use super::Accept;
 pub struct Server<A, C, R, B, G>
 where
     // Prepares a server transport, e.g. with telemetry.
-    A: Stack<Source, Error = ()> + Clone,
+    A: Stack<Source, Error = Never> + Clone,
     A::Value: Accept<Connection>,
     // Used when forwarding a TCP stream (e.g. with telemetry, timeouts).
-    C: Stack<connect::Target> + Clone,
-    C::Error: error::Error,
+    C: Stack<connect::Target, Error = Never> + Clone,
     C::Value: connect::Connect,
     // Prepares a route for each accepted HTTP connection.
-    R: Stack<Source, Error = ()> + Clone,
+    R: Stack<Source, Error = Never> + Clone,
     R::Value: Service<
         Request = http::Request<HttpBody>,
         Response = http::Response<B>,
@@ -144,8 +144,7 @@ impl fmt::Display for Source {
 
 impl<C> Stack<Source> for ForwardConnect<C>
 where
-    C: Stack<connect::Target>,
-    C::Error: error::Error,
+    C: Stack<connect::Target, Error = Never>,
 {
     type Value = C::Value;
     type Error = NoOriginalDst;
@@ -157,10 +156,11 @@ where
         };
 
         let tls = Conditional::None(tls::ReasonForNoIdentity::NotHttp.into());
-        let c = self.0.make(&connect::Target::new(addr, tls))
-            .expect("target must be valid");
-
-        Ok(c)
+        match self.0.make(&connect::Target::new(addr, tls)) {
+            Ok(c) => Ok(c),
+            // Matching never allows LLVM to eliminate this entirely.
+            Err(never) => match never {},
+        }
     }
 }
 
@@ -175,24 +175,23 @@ impl fmt::Display for NoOriginalDst {
 // Allows `()` to be used for `Accept`.
 impl Stack<Source> for () {
     type Value = ();
-    type Error = ();
-    fn make(&self, _: &Source) -> Result<(), ()> {
+    type Error = Never;
+    fn make(&self, _: &Source) -> Result<(), Never> {
         Ok(())
     }
 }
 
 impl<A, C, R, B, G> Server<A, C, R, B, G>
 where
-    A: Stack<Source, Error = ()> + Clone,
+    A: Stack<Source, Error = Never> + Clone,
     A::Value: Accept<Connection>,
     <A::Value as Accept<Connection>>::Io: Send + Peek + 'static,
-    C: Stack<connect::Target> + Clone,
-    C::Error: error::Error,
+    C: Stack<connect::Target, Error = Never> + Clone,
     C::Value: connect::Connect,
     <C::Value as connect::Connect>::Connected: Send + 'static,
     <C::Value as connect::Connect>::Future: Send + 'static,
     <C::Value as connect::Connect>::Error: fmt::Debug + 'static,
-    R: Stack<Source, Error = ()> + Clone,
+    R: Stack<Source, Error = Never> + Clone,
     R::Value: Service<
         Request = http::Request<HttpBody>,
         Response = http::Response<B>,
@@ -259,9 +258,11 @@ where
             _p: (),
         };
 
-        let io = self.accept.make(&source)
-            .expect("source must be acceptable")
-            .accept(connection);
+        let io = match self.accept.make(&source) {
+            Ok(accept) => accept.accept(connection),
+            // Matching never allows LLVM to eliminate this entirely.
+            Err(never) => match never {},
+        };
 
         // We are using the port from the connection's SO_ORIGINAL_DST to
         // determine whether to skip protocol detection, not any port that
@@ -304,11 +305,8 @@ where
                     Protocol::Http1 => Either::A({
                         trace!("detected HTTP/1");
                         match route.make(&source) {
-                            Err(()) => Either::A({
-                                error!("failed to build HTTP/1 client");
-                                future::err(())
-                            }),
-                            Ok(s) => Either::B({
+                            Err(never) => match never {},
+                            Ok(s) => {
                                 let svc = HyperServerSvc::new(
                                     s,
                                     drain_signal.clone(),
@@ -324,7 +322,7 @@ where
                                     })
                                     .map(|_| ())
                                     .map_err(|e| trace!("http1 server error: {:?}", e))
-                            }),
+                            },
                         }
                     }),
                     Protocol::Http2 => Either::B({

src/transport/connect.rs

@@ -2,9 +2,10 @@ extern crate tokio_connect;
 
 pub use self::tokio_connect::Connect;
 
-use std::{error, fmt, io};
+use std::io;
 use std::net::SocketAddr;
 
+use never::Never;
 use svc;
 use transport::{connection, tls};
 
@@ -18,10 +19,6 @@ pub struct Target {
     _p: (),
 }
 
-/// Note: this isn't actually used, but is needed to satisfy Error.
-#[derive(Debug)]
-pub struct InvalidTarget;
-
 // ===== impl Target =====
 
 impl Target {
@@ -61,17 +58,9 @@ where
     Target: From<T>,
 {
     type Value = Target;
-    type Error = InvalidTarget;
+    type Error = Never;
 
     fn make(&self, t: &T) -> Result<Self::Value, Self::Error> {
         Ok(t.clone().into())
     }
 }
-
-impl fmt::Display for InvalidTarget {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "Invalid target")
-    }
-}
-
-impl error::Error for InvalidTarget {}