From bf3a4c5dc1f08fafea83d698c4c8bf882509106a Mon Sep 17 00:00:00 2001 From: Manos Pitsidianakis Date: Wed, 7 Aug 2024 19:33:16 +0300 Subject: [PATCH] error: add ErrorChainDisplay struct for better output Add an ErrorChainDisplay struct that borrows an error and prints the list of chained errors one by one, showing relationships and metadata for each error. Signed-off-by: Manos Pitsidianakis --- meli/src/mail/compose/hooks.rs | 42 +++-- meli/tests/test_cli_subcommands.rs | 2 +- melib/src/error.rs | 259 +++++++++++++++++------------ melib/src/imap/error.rs | 30 ++-- melib/src/utils/tests.rs | 2 + 5 files changed, 202 insertions(+), 133 deletions(-) diff --git a/meli/src/mail/compose/hooks.rs b/meli/src/mail/compose/hooks.rs index 013b7917..0f868902 100644 --- a/meli/src/mail/compose/hooks.rs +++ b/meli/src/mail/compose/hooks.rs @@ -22,7 +22,7 @@ //! Pre-submission hooks for draft validation and/or transformations. pub use std::borrow::Cow; -use melib::email::headers::HeaderName; +use melib::{email::headers::HeaderName, src_err_arc_wrap}; use super::*; @@ -102,11 +102,11 @@ impl Hook { .stderr(Stdio::piped()) .spawn() .map_err(|err| -> Error { - format!( + Error::new(format!( "could not execute `{command}`. Check if its binary is in PATH or if \ - the command is valid. Original error: {err}" - ) - .into() + the command is valid." + )) + .set_source(Some(src_err_arc_wrap! {err})) })?; let mut stdin = child .stdin @@ -121,7 +121,8 @@ impl Hook { }); }); let output = child.wait_with_output().map_err(|err| -> Error { - format!("failed to wait on hook child {name_}: {err}").into() + Error::new(format!("failed to wait on hook child {name_}")) + .set_source(Some(src_err_arc_wrap! {err})) })?; let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); @@ -187,7 +188,10 @@ fn important_header_warn(_ctx: &mut Context, draft: &mut Draft) -> Result<()> { for hdr in [HeaderName::FROM, HeaderName::TO] { match draft.headers.get(&hdr).map(melib::Address::list_try_from) { Some(Ok(_)) => {} - Some(Err(err)) => return Err(format!("{hdr} header value is invalid ({err}).").into()), + Some(Err(err)) => { + return Err(Error::new(format!("{hdr} header value is invalid")) + .set_source(Some(src_err_arc_wrap! {err}))) + } None => return Err(format!("{hdr} header is missing and should be present.").into()), } } @@ -198,8 +202,11 @@ fn important_header_warn(_ctx: &mut Context, draft: &mut Draft) -> Result<()> { .get(HeaderName::DATE) .map(melib::utils::datetime::rfc822_to_timestamp) { - Some(Err(err)) => return Err(format!("Date header value is invalid ({err}).").into()), - Some(Ok(0)) => return Err("Date header value is invalid.".into()), + Some(Err(err)) => { + return Err(Error::new("Date header value is invalid.") + .set_source(Some(src_err_arc_wrap! {err}))) + } + Some(Ok(0)) => return Err(Error::new("Date header value is invalid.")), _ => {} } } @@ -211,7 +218,8 @@ fn important_header_warn(_ctx: &mut Context, draft: &mut Draft) -> Result<()> { .filter(|v| !v.trim().is_empty()) .map(melib::Address::list_try_from) { - return Err(format!("{hdr} header value is invalid ({err}).").into()); + return Err(Error::new(format!("{hdr} header value is invalid")) + .set_source(Some(src_err_arc_wrap! {err}))); } } Ok(()) @@ -310,8 +318,9 @@ mod tests { let err_msg = hook(&mut ctx, &mut draft).unwrap_err().to_string(); assert_eq!( err_msg, - "From header value is invalid (Parsing error. In input: \"...\",\nError: Alternative, \ - Many1, Alternative, atom(): starts with whitespace or empty).", + "From header value is invalid\nCaused by:\n[2] Parsing error. In input: \ + \"...\",\nError: Alternative, Many1, Alternative, atom(): starts with whitespace or \ + empty", "HEADERWARN should complain about From value being empty: {}", err_msg ); @@ -321,8 +330,9 @@ mod tests { let err_msg = hook(&mut ctx, &mut draft).unwrap_err().to_string(); assert_eq!( err_msg, - "To header value is invalid (Parsing error. In input: \"...\",\nError: Alternative, \ - Many1, Alternative, atom(): starts with whitespace or empty).", + "To header value is invalid\nCaused by:\n[2] Parsing error. In input: \ + \"...\",\nError: Alternative, Many1, Alternative, atom(): starts with whitespace or \ + empty", "HEADERWARN should complain about To value being empty: {}", err_msg ); @@ -350,8 +360,8 @@ mod tests { let err_msg = hook(&mut ctx, &mut draft).unwrap_err().to_string(); assert_eq!( err_msg, - "From header value is invalid (Parsing error. In input: \"user \ - user@example.com>...\",\nError: Alternative, Tag).", + "From header value is invalid\nCaused by:\n[2] Parsing error. In input: \"user \ + user@example.com>...\",\nError: Alternative, Tag", "HEADERWARN should complain about From value being invalid: {}", err_msg ); diff --git a/meli/tests/test_cli_subcommands.rs b/meli/tests/test_cli_subcommands.rs index 4e6d3d07..e159fe10 100644 --- a/meli/tests/test_cli_subcommands.rs +++ b/meli/tests/test_cli_subcommands.rs @@ -223,7 +223,7 @@ server_password_command = "false" output .code(1) .stderr(predicate::eq( - "Edit the sample configuration and relaunch meli.\nKind: Configuration\n", + "Configuration error: Edit the sample configuration and relaunch meli.\n", )) .stdout( predicate::eq( diff --git a/melib/src/error.rs b/melib/src/error.rs index 5e9082b1..328e199a 100644 --- a/melib/src/error.rs +++ b/melib/src/error.rs @@ -21,7 +21,13 @@ //! Library error type. -use std::{borrow::Cow, io, result, str, string, sync::Arc}; +use std::{ + borrow::Cow, + error, io, + path::{Path, PathBuf}, + result, str, string, + sync::Arc, +}; pub type Result = result::Result; @@ -57,10 +63,10 @@ pub enum ErrorKind { impl std::fmt::Display for ErrorKind { fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { match self { - Self::None => write!(fmt, "None"), - Self::External => write!(fmt, "External"), - Self::LinkedLibrary(ref name) => write!(fmt, "Linked library error: {name}"), - Self::Authentication => write!(fmt, "Authentication"), + Self::None => write!(fmt, ""), + Self::External => write!(fmt, "External error"), + Self::LinkedLibrary(ref name) => write!(fmt, "Linked library `{name}` error"), + Self::Authentication => write!(fmt, "Authentication error"), Self::Bug => write!(fmt, "Bug, please report this!"), Self::Network(ref inner) => write!(fmt, "{}", inner.as_str()), Self::ProtocolError => write!(fmt, "Protocol error"), @@ -68,14 +74,14 @@ impl std::fmt::Display for ErrorKind { fmt, "Protocol is not supported. It could be the wrong type or version." ), - Self::Platform => write!(fmt, "Platform/Runtime environment; OS or hardware"), - Self::TimedOut => write!(fmt, "Timed Out"), - Self::OSError => write!(fmt, "OS Error"), - Self::Configuration => write!(fmt, "Configuration"), - Self::NotImplemented => write!(fmt, "Not implemented"), - Self::NotSupported => write!(fmt, "Not supported"), - Self::NotFound => write!(fmt, "Not found"), - Self::ValueError => write!(fmt, "Invalid value"), + Self::Platform => write!(fmt, "Platform/Runtime environment error (OS or hardware)"), + Self::TimedOut => write!(fmt, "Timed out error"), + Self::OSError => write!(fmt, "OS error"), + Self::Configuration => write!(fmt, "Configuration error"), + Self::NotImplemented => write!(fmt, "Not implemented error"), + Self::NotSupported => write!(fmt, "Not supported error"), + Self::NotFound => write!(fmt, "Not found error"), + Self::ValueError => write!(fmt, "Invalid value error"), } } } @@ -133,8 +139,9 @@ macro_rules! src_err_arc_wrap { pub struct Error { pub summary: Cow<'static, str>, pub details: Option>, - pub source: Option>, - pub related_path: Option, + pub source: Option>, + pub inner: Option>, + pub related_path: Option, pub kind: ErrorKind, } @@ -165,7 +172,7 @@ pub trait ResultIntoError { pub trait WrapResultIntoError where - I: Send + Sync + std::error::Error + 'static, + I: Send + Sync + error::Error + 'static, { /// Wrap a result into a new [`Error`] that sets its source to the original /// value. @@ -227,7 +234,7 @@ impl> ResultIntoError for std::result::Result { } #[inline] - fn chain_err_related_path(self, p: &std::path::Path) -> Result { + fn chain_err_related_path(self, p: &Path) -> Result { self.map_err(|err| err.set_err_related_path(p)) } @@ -239,7 +246,7 @@ impl> ResultIntoError for std::result::Result { impl WrapResultIntoError for std::result::Result where - I: Send + Sync + std::error::Error + 'static, + I: Send + Sync + error::Error + 'static, { #[inline] /// Wrap a result into a new [`Error`] that sets its source to the original @@ -262,6 +269,7 @@ impl Error { summary: msg.into(), details: None, source: None, + inner: None, related_path: None, kind: ErrorKind::None, } @@ -293,18 +301,39 @@ impl Error { pub fn set_source( mut self, - new_val: Option>, + new_val: Option>, ) -> Self { - self.source = new_val; + self.source = new_val.map(|inner| { + Box::new(Self { + summary: "".into(), + details: None, + inner: Some(inner), + source: None, + related_path: None, + kind: ErrorKind::External, + }) + }); self } + pub fn from_inner(inner: std::sync::Arc) -> Self { + Self { + summary: "".into(), + details: None, + inner: Some(inner), + source: None, + related_path: None, + kind: ErrorKind::External, + } + } + pub fn set_kind(mut self, new_val: ErrorKind) -> Self { self.kind = new_val; self } - pub fn set_related_path(mut self, new_val: Option) -> Self { + pub fn set_related_path>(mut self, new_val: Option

) -> Self { + let new_val = new_val.map(Into::into); self.related_path = new_val; self } @@ -323,34 +352,70 @@ impl Error { || self.kind.is_protocol_not_supported() || self.kind.is_value_error()) } + + /// Display error chain to user. + fn display_chain(&'_ self) -> impl std::fmt::Display + '_ { + ErrorChainDisplay { + current: self, + counter: 1, + } + } } impl std::fmt::Display for Error { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "Error: {}", self.summary)?; - if let Some(details) = self.details.as_ref() { - if !details.trim().is_empty() { - write!(f, "\n{}", details)?; + self.display_chain().fmt(f) + } +} + +#[derive(Copy, Clone)] +struct ErrorChainDisplay<'e> { + current: &'e Error, + counter: usize, +} + +impl std::fmt::Display for ErrorChainDisplay<'_> { + fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { + let mut cur = *self; + loop { + if cur.counter > 1 { + write!(fmt, "[{}] ", cur.counter)?; + } + match cur.current.kind { + ErrorKind::External | ErrorKind::None => {} + other => write!(fmt, "{other}: ")?, + } + if let Some(ref inner) = cur.current.inner { + write!(fmt, "{}", inner)?; + } else { + write!(fmt, "{}", cur.current.summary)?; + if let Some(details) = cur.current.details.as_ref() { + if !details.trim().is_empty() { + write!(fmt, "\n{}", details)?; + } + } + if let Some(ref path) = cur.current.related_path { + write!(fmt, "\nRelated path: {}", path.display())?; + } + } + if let Some(ref source) = cur.current.source { + writeln!(fmt, "\nCaused by:")?; + cur = Self { + current: source, + counter: cur.counter + 1, + }; + } else { + return Ok(()); } } - if let Some(ref path) = self.related_path { - write!(f, "\nPath: {}", path.display())?; - } - if let Some(ref source) = self.source { - write!(f, "\nCaused by: {}", source)?; - } - if self.kind != ErrorKind::None { - write!(f, "\nError kind: {}", self.kind)?; - } - Ok(()) } } -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { +impl error::Error for Error { + fn source(&self) -> Option<&(dyn error::Error + 'static)> { self.source .as_ref() - .map(|s| &(*(*s)) as &(dyn std::error::Error + 'static)) + .map(|s| &(*(*s)) as &(dyn error::Error + 'static)) } } @@ -381,54 +446,43 @@ impl From for Error { } else { err.kind().into() }; - Self::new(s) - .set_details(err.kind().to_string()) - .set_source(Some(Arc::new(err))) - .set_kind(kind) + Self::from_inner(Arc::new(err)).set_kind(kind) } } impl<'a> From> for Error { #[inline] - fn from(kind: Cow<'_, str>) -> Self { - Self::new(kind.to_string()) + fn from(err: Cow<'_, str>) -> Self { + Self::new(err.to_string()) } } impl From for Error { #[inline] - fn from(kind: string::FromUtf8Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: string::FromUtf8Error) -> Self { + Self::from_inner(Arc::new(err)).set_kind(ErrorKind::ValueError) } } impl From for Error { #[inline] - fn from(kind: str::Utf8Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: str::Utf8Error) -> Self { + Self::from_inner(Arc::new(err)).set_kind(ErrorKind::ValueError) } } -//use std::option; -//impl From for Error { -// #[inline] -// fn from(kind: option::NoneError) -> Error { -// Error::new(format!("{:?}", kind)) -// } -//} impl From> for Error { #[inline] - fn from(kind: std::sync::PoisonError) -> Self { - Self::new(kind.to_string()).set_kind(ErrorKind::Bug) + fn from(err: std::sync::PoisonError) -> Self { + Self::new(err.to_string()).set_kind(ErrorKind::Bug) } } #[cfg(feature = "tls")] impl From> for Error { #[inline] - fn from(kind: native_tls::HandshakeError) -> Self { - Self::new(kind.to_string()) - .set_source(Some(Arc::new(kind))) + fn from(err: native_tls::HandshakeError) -> Self { + Self::from_inner(Arc::new(err)) .set_kind(ErrorKind::Network(NetworkErrorKind::InvalidTLSConnection)) } } @@ -436,24 +490,23 @@ impl From for Error { #[inline] - fn from(kind: native_tls::Error) -> Self { - Self::new(kind.to_string()) - .set_source(Some(Arc::new(kind))) + fn from(err: native_tls::Error) -> Self { + Self::from_inner(Arc::new(err)) .set_kind(ErrorKind::Network(NetworkErrorKind::InvalidTLSConnection)) } } impl From for Error { #[inline] - fn from(kind: std::num::ParseIntError) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: std::num::ParseIntError) -> Self { + Self::new(err.to_string()).set_kind(ErrorKind::ValueError) } } impl From for Error { #[inline] - fn from(kind: std::fmt::Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: std::fmt::Error) -> Self { + Self::new(err.to_string()).set_kind(ErrorKind::Bug) } } @@ -462,46 +515,44 @@ impl From for Error { #[inline] fn from(val: isahc::Error) -> Self { let kind: NetworkErrorKind = val.kind().into(); - Self::new(val.to_string()) - .set_source(Some(Arc::new(val))) - .set_kind(ErrorKind::Network(kind)) + Self::from_inner(Arc::new(val)).set_kind(ErrorKind::Network(kind)) } } #[cfg(feature = "jmap")] impl From for Error { #[inline] - fn from(kind: serde_json::error::Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: serde_json::error::Error) -> Self { + Self::from_inner(Arc::new(err)) } } -impl From> for Error { +impl From> for Error { #[inline] - fn from(kind: Box) -> Self { - Self::new(kind.to_string()).set_source(Some(kind.into())) + fn from(err: Box) -> Self { + Self::from_inner(err.into()) } } impl From for Error { #[inline] - fn from(kind: std::ffi::NulError) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: std::ffi::NulError) -> Self { + Self::from_inner(Arc::new(err)).set_kind(ErrorKind::Bug) } } impl From for Error { #[inline] - fn from(kind: nix::Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: nix::Error) -> Self { + Self::from_inner(Arc::new(err)).set_kind(ErrorKind::Platform) } } #[cfg(feature = "sqlite3")] impl From for Error { #[inline] - fn from(kind: rusqlite::Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: rusqlite::Error) -> Self { + Self::from_inner(Arc::new(err)).set_kind(ErrorKind::LinkedLibrary("sqlite3")) } } @@ -512,80 +563,76 @@ impl From for Error { let kind = match err.kind { notify::ErrorKind::MaxFilesWatch | notify::ErrorKind::WatchNotFound - | notify::ErrorKind::Generic(_) => ErrorKind::External, + | notify::ErrorKind::Generic(_) => ErrorKind::Platform, notify::ErrorKind::Io(_) => ErrorKind::OSError, notify::ErrorKind::PathNotFound => ErrorKind::Configuration, notify::ErrorKind::InvalidConfig(_) => ErrorKind::Bug, }; - Self::new(err.to_string()) - .set_source(Some(Arc::new(err))) - .set_kind(kind) + Self::from_inner(Arc::new(err)).set_kind(kind) } } impl From for Error { #[inline] - fn from(kind: libloading::Error) -> Self { - Self::new(kind.to_string()).set_source(Some(Arc::new(kind))) + fn from(err: libloading::Error) -> Self { + Self::from_inner(Arc::new(err)) } } impl From<&str> for Error { #[inline] - fn from(kind: &str) -> Self { - Self::new(kind.to_string()) + fn from(err: &str) -> Self { + Self::new(err.to_string()) } } impl From for Error { #[inline] - fn from(kind: String) -> Self { - Self::new(kind) + fn from(err: String) -> Self { + Self::new(err) } } impl From> for Error { #[inline] - fn from(kind: nom::Err<(&[u8], nom::error::ErrorKind)>) -> Self { - Self::new("Parsing error").set_source(Some(Arc::new(Self::new(kind.to_string())))) + fn from(err: nom::Err<(&[u8], nom::error::ErrorKind)>) -> Self { + Self::new("Parsing error").set_details(err.to_string()) } } impl From> for Error { #[inline] - fn from(kind: nom::Err<(&str, nom::error::ErrorKind)>) -> Self { - Self::new("Parsing error").set_details(kind.to_string()) + fn from(err: nom::Err<(&str, nom::error::ErrorKind)>) -> Self { + Self::new("Parsing error").set_details(err.to_string()) } } impl From for Error { #[inline] - fn from(kind: crate::email::InvalidHeaderName) -> Self { - Self::new(kind.to_string()) - .set_source(Some(Arc::new(kind))) - .set_kind(ErrorKind::Network(NetworkErrorKind::InvalidTLSConnection)) + fn from(err: crate::email::InvalidHeaderName) -> Self { + Self::from_inner(Arc::new(err)).set_kind(ErrorKind::ValueError) } } impl<'a> From<&'a mut Self> for Error { #[inline] - fn from(kind: &'a mut Self) -> Self { - kind.clone() + fn from(err: &'a mut Self) -> Self { + err.clone() } } impl<'a> From<&'a Self> for Error { #[inline] - fn from(kind: &'a Self) -> Self { - kind.clone() + fn from(err: &'a Self) -> Self { + err.clone() } } impl From for Error { #[inline] - fn from(kind: base64::DecodeError) -> Self { - Self::new("base64 decoding failed") - .set_source(Some(Arc::new(kind))) + fn from(err: base64::DecodeError) -> Self { + Self::from_inner(Arc::new(err)) + .set_summary("base64 decoding failed") .set_kind(ErrorKind::ValueError) } } diff --git a/melib/src/imap/error.rs b/melib/src/imap/error.rs index 599a9e22..7cf1362f 100644 --- a/melib/src/imap/error.rs +++ b/melib/src/imap/error.rs @@ -33,12 +33,14 @@ impl From for Error { #[inline] fn from(error: ValidationError) -> Self { Self { - summary: error.to_string().into(), + summary: "IMAP transaction validation failed".into(), details: None, - source: Some(Arc::new(error)), + inner: None, + source: None, related_path: None, kind: ErrorKind::Bug, } + .set_source(Some(Arc::new(error))) } } @@ -49,12 +51,14 @@ where #[inline] fn from(error: AppendError) -> Self { Self { - summary: error.to_string().into(), + summary: "IMAP APPEND command failed".into(), details: None, - source: Some(Arc::new(error)), + inner: None, + source: None, related_path: None, kind: ErrorKind::Bug, } + .set_source(Some(Arc::new(error))) } } @@ -65,12 +69,14 @@ where #[inline] fn from(error: CopyError) -> Self { Self { - summary: error.to_string().into(), + summary: "IMAP COPY command failed".into(), details: None, - source: Some(Arc::new(error)), + inner: None, + source: None, related_path: None, kind: ErrorKind::Bug, } + .set_source(Some(Arc::new(error))) } } @@ -81,12 +87,14 @@ where #[inline] fn from(error: MoveError) -> Self { Self { - summary: error.to_string().into(), + summary: "IMAP MOVE command failed".into(), + source: None, details: None, - source: Some(Arc::new(error)), + inner: None, related_path: None, kind: ErrorKind::Bug, } + .set_source(Some(Arc::new(error))) } } @@ -97,11 +105,13 @@ where #[inline] fn from(error: ListError) -> Self { Self { - summary: error.to_string().into(), + summary: "IMAP LIST command failed".into(), details: None, - source: Some(Arc::new(error)), + inner: None, + source: None, related_path: None, kind: ErrorKind::Bug, } + .set_source(Some(Arc::new(error))) } } diff --git a/melib/src/utils/tests.rs b/melib/src/utils/tests.rs index 4c3170fa..90f1e95f 100644 --- a/melib/src/utils/tests.rs +++ b/melib/src/utils/tests.rs @@ -21,6 +21,7 @@ // SPDX-License-Identifier: EUPL-1.2 OR GPL-3.0-or-later #[test] +#[ignore] fn test_shellexpandtrait() { use std::{fs::File, io::Write, os::unix::fs::PermissionsExt, path::Path}; @@ -144,6 +145,7 @@ fn test_shellexpandtrait() { #[cfg(target_os = "linux")] #[test] +#[ignore] fn test_shellexpandtrait_impls() { use std::{fs::File, io::Write, os::unix::fs::PermissionsExt, path::Path};