reduce error boilerplate with thiserror (#980)

The [`thiserror` crate][1] provides a `#[derive(Error)]` macro that can be
used to generate `std::error::Error` implementations. Using this results
in significantly less boilerplate required by most of the error types in
the proxy, replacing almost all of the manual `Error` implementations.

There were a few cases where the `Error` impl either required complex
formatting logic (it has a conditional in it), or complex `source` logic
(the `Arc`ed `SharedError` type). I left these alone, but was able to
replace every other manual implementation of `std::error::Error`.

No functional change. :)

[1]: https://github.com/dtolnay/thiserror#deriveerror

Signed-off-by: Eliza Weisman <[email protected]>

Author: hawkw

Cargo.lock

@@ -591,6 +591,7 @@ version = "0.1.0"
 dependencies = [
  "http",
  "linkerd-dns-name",
+ "thiserror",
 ]
 
 [[package]]
@@ -608,6 +609,7 @@ dependencies = [
  "linkerd-error",
  "linkerd-opencensus",
  "regex",
+ "thiserror",
  "tokio",
  "tonic",
  "tower",
@@ -672,6 +674,7 @@ dependencies = [
  "procinfo",
  "prost-types",
  "regex",
+ "thiserror",
  "tokio",
  "tonic",
  "tower",
@@ -689,6 +692,7 @@ dependencies = [
  "linkerd-app-inbound",
  "linkerd-app-outbound",
  "linkerd-app-test",
+ "thiserror",
  "tokio",
  "tokio-test",
  "tower",
@@ -708,6 +712,7 @@ dependencies = [
  "linkerd-app-core",
  "linkerd-app-test",
  "linkerd-io",
+ "thiserror",
  "tokio",
  "tokio-test",
  "tower",
@@ -760,6 +765,7 @@ dependencies = [
  "linkerd-io",
  "linkerd-retry",
  "pin-project",
+ "thiserror",
  "tokio",
  "tokio-test",
  "tower",
@@ -837,6 +843,7 @@ dependencies = [
  "linkerd-error",
  "linkerd-io",
  "linkerd-stack",
+ "thiserror",
  "tokio",
  "tower",
  "tracing",
@@ -850,6 +857,7 @@ dependencies = [
  "linkerd-dns-name",
  "linkerd-error",
  "pin-project",
+ "thiserror",
  "tokio",
  "tracing",
  "trust-dns-resolver",
@@ -859,6 +867,7 @@ dependencies = [
 name = "linkerd-dns-name"
 version = "0.1.0"
 dependencies = [
+ "thiserror",
  "untrusted",
  "webpki",
 ]
@@ -927,6 +936,7 @@ dependencies = [
  "pin-project",
  "quickcheck",
  "rand",
+ "thiserror",
  "tokio",
 ]
 
@@ -980,6 +990,7 @@ dependencies = [
  "linkerd-dns-name",
  "ring",
  "rustls",
+ "thiserror",
  "tracing",
  "untrusted",
  "webpki",
@@ -1122,6 +1133,7 @@ dependencies = [
  "linkerd-timeout",
  "pin-project",
  "rand",
+ "thiserror",
  "tokio",
  "tokio-test",
  "tower",
@@ -1156,6 +1168,7 @@ dependencies = [
  "linkerd-error",
  "linkerd-proxy-core",
  "pin-project",
+ "thiserror",
  "tower",
  "tracing",
 ]
@@ -1181,6 +1194,7 @@ dependencies = [
  "pin-project",
  "prost-types",
  "rand",
+ "thiserror",
  "tokio",
  "tonic",
  "tower",
@@ -1325,6 +1339,7 @@ dependencies = [
  "linkerd-error",
  "linkerd-stack",
  "pin-project",
+ "thiserror",
  "tokio",
  "tokio-test",
  "tower",
@@ -1347,6 +1362,7 @@ dependencies = [
  "linkerd-proxy-transport",
  "linkerd-stack",
  "rustls",
+ "thiserror",
  "tokio",
  "tokio-rustls",
  "tower",
@@ -1369,6 +1385,7 @@ dependencies = [
  "linkerd-error",
  "linkerd-stack",
  "rand",
+ "thiserror",
  "tower",
  "tracing",
 ]

linkerd/addr/Cargo.toml

@@ -9,3 +9,4 @@ publish = false
 [dependencies]
 http = "0.2"
 linkerd-dns-name = { path = "../dns/name" }
+thiserror = "1.0"

linkerd/addr/src/lib.rs

@@ -1,11 +1,11 @@
 #![deny(warnings, rust_2018_idioms)]
-
 use linkerd_dns_name::Name;
 use std::{
     fmt,
     net::{IpAddr, SocketAddr},
     str::FromStr,
 };
+use thiserror::Error;
 
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub enum Addr {
@@ -19,12 +19,14 @@ pub struct NameAddr {
     port: u16,
 }
 
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Error)]
 pub enum Error {
     /// The host is not a valid DNS name or IP address.
+    #[error("address contains an invalid host")]
     InvalidHost,
 
     /// The port is missing.
+    #[error("address is missing a port")]
     MissingPort,
 }
 
@@ -239,19 +241,6 @@ impl fmt::Display for NameAddr {
     }
 }
 
-// === impl Error ===
-
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Self::InvalidHost => write!(f, "address contains an invalid host"),
-            Self::MissingPort => write!(f, "address is missing a port"),
-        }
-    }
-}
-
-impl std::error::Error for Error {}
-
 #[cfg(test)]
 mod tests {
     use super::*;

linkerd/app/Cargo.toml

@@ -26,6 +26,7 @@ linkerd-channel = { path = "../channel" }
 linkerd-opencensus = { path = "../opencensus" }
 linkerd-error = { path = "../error" }
 regex = "1.0.0"
+thiserror = "1.0"
 tokio = { version = "1", features = ["rt"] }
 tonic = { version = "0.4", default-features = false, features = ["prost"] }
 tower = "0.4"

linkerd/app/core/Cargo.toml

@@ -63,6 +63,7 @@ linkerd-stack-tracing = { path = "../../stack/tracing" }
 linkerd-tls = { path = "../../tls" }
 linkerd-trace-context = { path = "../../trace-context" }
 regex = "1.0.0"
+thiserror = "1.0"
 tokio = { version = "1", features = ["macros", "sync", "parking_lot"]}
 tonic = { version = "0.4", default-features = false, features = ["prost"] }
 tracing = "0.1.23"

linkerd/app/core/src/classify.rs

@@ -3,7 +3,7 @@ use linkerd_error::Error;
 use linkerd_http_classify as classify;
 pub use linkerd_http_classify::{CanClassify, NewClassify};
 use linkerd_proxy_http::HasH2Reason;
-use linkerd_timeout::error::ResponseTimeout;
+use linkerd_timeout::ResponseTimeout;
 use std::borrow::Cow;
 use tonic as grpc;
 use tracing::trace;

linkerd/app/core/src/errors.rs

@@ -5,11 +5,12 @@ use linkerd_error_metrics as metrics;
 use linkerd_error_respond as respond;
 pub use linkerd_error_respond::RespondLayer;
 use linkerd_proxy_http::{ClientHandle, HasH2Reason};
-use linkerd_timeout::{error::ResponseTimeout, FailFastError};
+use linkerd_timeout::{FailFastError, ResponseTimeout};
 use linkerd_tls as tls;
 use pin_project::pin_project;
 use std::pin::Pin;
 use std::task::{Context, Poll};
+use thiserror::Error;
 use tonic::{self as grpc, Code};
 use tracing::{debug, warn};
 
@@ -28,7 +29,8 @@ pub struct LabelError(super::metrics::Direction);
 
 pub type Label = (super::metrics::Direction, Reason);
 
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, Error)]
+#[error("{}", self.message)]
 pub struct HttpError {
     http: http::StatusCode,
     grpc: Code,
@@ -453,11 +455,3 @@ impl HttpError {
         self.http
     }
 }
-
-impl std::fmt::Display for HttpError {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.message.fmt(f)
-    }
-}
-
-impl std::error::Error for HttpError {}

linkerd/app/core/src/http_tracing.rs

@@ -2,7 +2,8 @@ use linkerd_error::Error;
 use linkerd_opencensus::proto::trace::v1 as oc;
 use linkerd_stack::layer;
 use linkerd_trace_context::{self as trace_context, TraceContext};
-use std::{collections::HashMap, error, fmt, sync::Arc};
+use std::{collections::HashMap, sync::Arc};
+use thiserror::Error;
 use tokio::sync::mpsc;
 
 pub type OpenCensusSink = Option<mpsc::Sender<oc::Span>>;
@@ -19,25 +20,14 @@ pub struct SpanConverter {
     labels: Labels,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Error)]
+#[error("ID '{:?} should have {} bytes, but it has {}", self.id, self.expected_size, self.actual_size)]
 pub struct IdLengthError {
     id: Vec<u8>,
     expected_size: usize,
     actual_size: usize,
 }
 
-impl error::Error for IdLengthError {}
-
-impl fmt::Display for IdLengthError {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(
-            f,
-            "Id '{:?}' should have {} bytes but it has {}",
-            self.id, self.expected_size, self.actual_size
-        )
-    }
-}
-
 pub fn server<S>(
     sink: OpenCensusSink,
     labels: impl Into<Labels>,

linkerd/app/gateway/Cargo.toml

@@ -13,6 +13,7 @@ indexmap = "1.0"
 linkerd-app-core = { path = "../core" }
 linkerd-app-inbound = { path = "../inbound" }
 linkerd-app-outbound = { path = "../outbound" }
+thiserror = "1.0"
 tokio = { version = "1", features = ["sync"] }
 tower = { version = "0.4.5", default-features = false }
 tracing = "0.1.23"

linkerd/app/gateway/src/lib.rs

@@ -28,6 +28,7 @@ use std::{
     convert::{TryFrom, TryInto},
     fmt,
 };
+use thiserror::Error;
 use tracing::debug_span;
 
 #[derive(Clone, Debug, Default)]
@@ -57,10 +58,12 @@ struct HttpTarget {
     version: http::Version,
 }
 
-#[derive(Debug, Default)]
+#[derive(Debug, Default, Error)]
+#[error("a named target must be provided on gateway connections")]
 struct RefusedNoTarget(());
 
-#[derive(Debug)]
+#[derive(Debug, Error)]
+#[error("the provided address could not be resolved: {}", self.0)]
 struct RefusedNotResolved(NameAddr);
 
 #[allow(clippy::clippy::too_many_arguments)]
@@ -279,7 +282,7 @@ impl<E: Into<Error>> TryFrom<(Result<Option<http::Version>, E>, ClientInfo)> for
 
     fn try_from(
         (version, client): (Result<Option<http::Version>, E>, ClientInfo),
-    ) -> Result<Self, Error> {
+    ) -> Result<Self, Self::Error> {
         match version {
             Ok(Some(version)) => Ok(Self { version, client }),
             Ok(None) => Err(RefusedNoTarget(()).into()),
@@ -338,23 +341,3 @@ impl<B> svc::stack::RecognizeRoute<http::Request<B>> for RouteHttp<HttpLegacy> {
         Err(RefusedNoTarget(()).into())
     }
 }
-
-// === impl RefusedNoTarget ===
-
-impl fmt::Display for RefusedNoTarget {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "A named target must be provided on gateway connections")
-    }
-}
-
-impl std::error::Error for RefusedNoTarget {}
-
-// === impl RefusedNotResolved ===
-
-impl fmt::Display for RefusedNotResolved {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "The provided address could not be resolved: {}", self.0)
-    }
-}
-
-impl std::error::Error for RefusedNotResolved {}