diff --git a/src/app_error/core.rs b/src/app_error/core.rs index 83c02ed..6f35b68 100644 --- a/src/app_error/core.rs +++ b/src/app_error/core.rs @@ -228,20 +228,111 @@ mod tests { #[cfg(feature = "std")] #[test] - fn error_downcast_mut_returns_none() { + fn error_downcast_mut_returns_source_for_owned_source() { use std::io::Error as IoError; let io_err = IoError::other("test"); let mut err = Error::new(AppErrorKind::Internal, "fail").with_context(io_err); + let source = err.downcast_mut::().expect("owned io source"); + assert_eq!(source.to_string(), "test"); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_mut_mutation_visible_via_downcast_ref() { + use std::io::Error as IoError; + let io_err = IoError::other("before"); + let mut err = Error::new(AppErrorKind::Internal, "fail").with_context(io_err); + let source = err.downcast_mut::().expect("owned io source"); + *source = IoError::other("after"); + let source = err.downcast_ref::().expect("io source"); + assert_eq!(source.to_string(), "after"); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_mut_returns_none_for_wrong_type() { + use std::{fmt::Error as FmtError, io::Error as IoError}; + let io_err = IoError::other("test"); + let mut err = Error::new(AppErrorKind::Internal, "fail").with_context(io_err); + assert!(err.downcast_mut::().is_none()); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_mut_returns_none_without_source() { + use std::io::Error as IoError; + let mut err = Error::new(AppErrorKind::Internal, "fail"); + assert!(err.downcast_mut::().is_none()); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_mut_returns_none_for_shared_arc_source() { + use std::{io::Error as IoError, sync::Arc}; + let shared = Arc::new(IoError::other("shared")); + let source: Arc = shared.clone(); + let mut err = Error::new(AppErrorKind::Internal, "fail").with_source_arc(source); assert!(err.downcast_mut::().is_none()); + assert_eq!(Arc::strong_count(&shared), 2); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_mut_returns_source_for_unique_arc_source() { + use std::{io::Error as IoError, sync::Arc}; + let shared: Arc = + Arc::new(IoError::other("unique")); + let mut err = Error::new(AppErrorKind::Internal, "fail").with_source_arc(shared); + let source = err.downcast_mut::().expect("unique arc source"); + assert_eq!(source.to_string(), "unique"); } #[cfg(feature = "std")] #[test] - fn error_downcast_returns_err() { + fn error_downcast_returns_boxed_source_for_owned_source() { use std::io::Error as IoError; let io_err = IoError::other("test"); let err = Error::new(AppErrorKind::Internal, "fail").with_context(io_err); - assert!(err.downcast::().is_err()); + let source = err.downcast::().expect("owned io source"); + assert_eq!(source.to_string(), "test"); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_returns_err_with_source_intact_for_wrong_type() { + use std::{fmt::Error as FmtError, io::Error as IoError}; + let io_err = IoError::other("test"); + let err = Error::new(AppErrorKind::Internal, "fail").with_context(io_err); + let err = err.downcast::().expect_err("wrong type"); + assert_eq!(err.kind, AppErrorKind::Internal); + assert!(err.is::()); + assert_eq!( + err.downcast_ref::() + .expect("io source") + .to_string(), + "test" + ); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_returns_err_without_source() { + use std::io::Error as IoError; + let err = Error::new(AppErrorKind::Internal, "fail"); + let err = err.downcast::().expect_err("no source"); + assert_eq!(err.kind, AppErrorKind::Internal); + } + + #[cfg(feature = "std")] + #[test] + fn error_downcast_returns_err_with_source_intact_for_shared_arc() { + use std::{io::Error as IoError, sync::Arc}; + let shared = Arc::new(IoError::other("shared")); + let source: Arc = shared.clone(); + let err = Error::new(AppErrorKind::Internal, "fail").with_source_arc(source); + let err = err.downcast::().expect_err("shared source"); + assert!(err.is::()); + assert_eq!(Arc::strong_count(&shared), 2); } #[test] diff --git a/src/app_error/core/builder.rs b/src/app_error/core/builder.rs index 2af4589..08366f4 100644 --- a/src/app_error/core/builder.rs +++ b/src/app_error/core/builder.rs @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: MIT -use alloc::{borrow::Cow, string::String, sync::Arc}; +use alloc::{borrow::Cow, boxed::Box, string::String, sync::Arc}; use core::error::Error as CoreError; #[cfg(feature = "backtrace")] use std::backtrace::Backtrace; @@ -14,7 +14,7 @@ use serde_json::{Value as JsonValue, to_value}; use super::{ error::Error, - types::{CapturedBacktrace, ContextAttachment, MessageEditPolicy} + types::{CapturedBacktrace, ContextAttachment, MessageEditPolicy, StoredSource} }; use crate::{ AppCode, AppErrorKind, RetryAdvice, @@ -240,14 +240,18 @@ impl Error { ContextAttachment::Owned(source) => { match source.downcast::>() { Ok(shared) => self.with_source_arc(*shared), - Err(source) => self.with_source_arc(Arc::from(source)) + Err(source) => self.with_boxed_source(source) } } ContextAttachment::Shared(source) => self.with_source_arc(source) } } - /// Attach a source error for diagnostics. + /// Attach an owned source error for diagnostics. + /// + /// The source stays exclusively owned by this error, so it can later be + /// recovered by value via [`downcast`](Self::downcast) or mutated via + /// [`downcast_mut`](Self::downcast_mut). /// /// Prefer [`with_context`](Self::with_context) when capturing upstream /// diagnostics without additional `Arc` allocations. @@ -264,14 +268,29 @@ impl Error { /// # } /// ``` #[must_use] - pub fn with_source(mut self, source: impl CoreError + Send + Sync + 'static) -> Self { - self.source = Some(Arc::new(source)); + pub fn with_source(self, source: impl CoreError + Send + Sync + 'static) -> Self { + self.with_boxed_source(Box::new(source)) + } + + /// Attach an already boxed source error without re-boxing. + pub(crate) fn with_boxed_source( + mut self, + source: Box + ) -> Self { + self.source = Some(StoredSource::Owned(source)); self.mark_dirty(); self } /// Attach a shared source error without cloning the underlying `Arc`. /// + /// A shared source can be borrowed via + /// [`downcast_ref`](Self::downcast_ref) but never extracted by value: + /// [`downcast`](Self::downcast) returns `Err(self)` for shared sources and + /// [`downcast_mut`](Self::downcast_mut) succeeds only while the `Arc` is + /// uniquely referenced. Use [`with_source`](Self::with_source) when the + /// caller owns the source and may want it back. + /// /// # Examples /// /// ```rust @@ -288,7 +307,7 @@ impl Error { /// ``` #[must_use] pub fn with_source_arc(mut self, source: Arc) -> Self { - self.source = Some(source); + self.source = Some(StoredSource::Shared(source)); self.mark_dirty(); self } diff --git a/src/app_error/core/display.rs b/src/app_error/core/display.rs index 3943c2d..cfe349f 100644 --- a/src/app_error/core/display.rs +++ b/src/app_error/core/display.rs @@ -211,7 +211,7 @@ impl Error { } if let Some(source) = &self.source { writeln!(f)?; - let mut current: &dyn CoreError = source.as_ref(); + let mut current: &dyn CoreError = source.as_dyn(); let mut depth = 0; while depth < 10 { writeln!( @@ -246,7 +246,7 @@ impl Error { } if let Some(source) = &self.source { writeln!(f)?; - let mut current: &dyn CoreError = source.as_ref(); + let mut current: &dyn CoreError = source.as_dyn(); let mut depth = 0; while depth < 10 { writeln!(f, " Caused by: {}", current)?; @@ -300,7 +300,7 @@ impl Error { } if let Some(source) = &self.source { write!(f, r#","source_chain":["#)?; - let mut current: &dyn CoreError = source.as_ref(); + let mut current: &dyn CoreError = source.as_dyn(); let mut depth = 0; let mut first = true; while depth < 5 { diff --git a/src/app_error/core/error.rs b/src/app_error/core/error.rs index 9bb9564..f322aee 100644 --- a/src/app_error/core/error.rs +++ b/src/app_error/core/error.rs @@ -2,7 +2,9 @@ // // SPDX-License-Identifier: MIT -use alloc::{borrow::Cow, boxed::Box, string::String, sync::Arc}; +#[cfg(feature = "backtrace")] +use alloc::sync::Arc; +use alloc::{borrow::Cow, boxed::Box, string::String}; use core::{ error::Error as CoreError, fmt::{Display, Formatter, Result as FmtResult}, @@ -17,7 +19,7 @@ use serde_json::Value as JsonValue; #[cfg(not(feature = "backtrace"))] use super::types::CapturedBacktrace; -use super::types::MessageEditPolicy; +use super::types::{MessageEditPolicy, StoredSource}; use crate::{AppCode, AppErrorKind, RetryAdvice, app_error::metadata::Metadata}; /// Internal representation of error state. @@ -48,7 +50,7 @@ pub struct ErrorInner { /// Optional textual details when JSON is unavailable. #[cfg(not(feature = "serde_json"))] pub details: Option, - pub source: Option>, + pub source: Option, #[cfg(feature = "backtrace")] pub backtrace: Option>, #[cfg(feature = "backtrace")] @@ -107,7 +109,7 @@ impl Display for Error { } if let Some(source) = &self.source { writeln!(f)?; - let mut current: &dyn CoreError = source.as_ref(); + let mut current: &dyn CoreError = source.as_dyn(); let mut depth = 0; while depth < 10 { write!(f, " {}: ", style::source_context("Caused by"))?; @@ -135,8 +137,8 @@ impl Display for Error { impl CoreError for Error { fn source(&self) -> Option<&(dyn CoreError + 'static)> { self.source - .as_deref() - .map(|source| source as &(dyn CoreError + 'static)) + .as_ref() + .map(|source| source.as_dyn() as &(dyn CoreError + 'static)) } } diff --git a/src/app_error/core/introspection.rs b/src/app_error/core/introspection.rs index da6ccec..f58bac0 100644 --- a/src/app_error/core/introspection.rs +++ b/src/app_error/core/introspection.rs @@ -12,7 +12,7 @@ use {alloc::sync::Arc, std::backtrace::Backtrace}; use super::backtrace::capture_backtrace_snapshot; use super::{ error::Error, - types::{CapturedBacktrace, ErrorChain} + types::{CapturedBacktrace, ErrorChain, StoredSource} }; use crate::app_error::metadata::Metadata; @@ -85,7 +85,7 @@ impl Error { /// ``` #[must_use] pub fn source_ref(&self) -> Option<&(dyn CoreError + Send + Sync + 'static)> { - self.source.as_deref() + self.source.as_ref().map(StoredSource::as_dyn) } /// Human-readable message or the kind fallback. @@ -217,10 +217,23 @@ impl Error { /// Attempt to take ownership of the source error as a concrete type. /// - /// **Note:** This method is currently a stub and always returns - /// `Err(Self)`. + /// Succeeds when the immediate source (not this error itself, and not the + /// entire chain) is of type `E` and the error owns it exclusively, i.e. + /// the source was attached from an owned value via + /// [`with_source`](Self::with_source) or + /// [`with_context`](Self::with_context). /// - /// Use [`downcast_ref`](Self::downcast_ref) for inspecting error sources. + /// Sources attached as a shared [`Arc`](alloc::sync::Arc) via + /// [`with_source_arc`](Self::with_source_arc) are never extracted by + /// value, even when the `Arc` is uniquely referenced, because exclusive + /// ownership of an unsized `Arc` cannot be reclaimed without unsafe code. + /// Use [`downcast_ref`](Self::downcast_ref) to borrow such sources. + /// + /// # Errors + /// + /// Returns `Err(self)` with the error (including its source) intact when + /// no source is attached, the source is not of type `E`, or the source is + /// shared. /// /// # Examples /// @@ -233,14 +246,44 @@ impl Error { /// let io_err = IoError::other("disk offline"); /// let err = AppError::internal("boom").with_context(io_err); /// - /// assert!(err.downcast::().is_err()); + /// let io = err.downcast::().expect("owned io source"); + /// assert_eq!(io.to_string(), "disk offline"); /// # } /// ``` - pub fn downcast(self) -> Result, Self> + /// + /// A failed downcast returns the original error unchanged: + /// + /// ```rust + /// # #[cfg(feature = "std")] { + /// use std::{fmt::Error as FmtError, io::Error as IoError}; + /// + /// use masterror::AppError; + /// + /// let err = AppError::internal("boom").with_context(IoError::other("disk offline")); + /// + /// let err = err + /// .downcast::() + /// .expect_err("source is not FmtError"); + /// assert!(err.is::()); + /// # } + /// ``` + pub fn downcast(mut self) -> Result, Self> where E: CoreError + 'static { - Err(self) + match self.source.take() { + Some(StoredSource::Owned(source)) => match source.downcast::() { + Ok(source) => Ok(source), + Err(source) => { + self.source = Some(StoredSource::Owned(source)); + Err(self) + } + }, + other => { + self.source = other; + Err(self) + } + } } /// Attempt to downcast the attached source error to a concrete type by @@ -276,9 +319,15 @@ impl Error { /// Attempt to downcast the attached source error to a concrete type by /// mutable reference. /// - /// **Note:** This method is currently a stub and always returns `None`. + /// Returns `Some(&mut E)` when the immediate source (not this error + /// itself, and not the entire chain) is of type `E` and mutable access is + /// possible: the source was attached from an owned value, or it was + /// attached as a shared [`Arc`](alloc::sync::Arc) that is currently + /// uniquely referenced. /// - /// Use [`downcast_ref`](Self::downcast_ref) for inspecting error sources. + /// Returns `None` when no source is attached, the source is not of type + /// `E`, or the source is a shared `Arc` with other strong or weak + /// references, since handing out `&mut E` would alias the other holders. /// /// # Examples /// @@ -291,7 +340,14 @@ impl Error { /// let io_err = IoError::other("disk offline"); /// let mut err = AppError::internal("boom").with_context(io_err); /// - /// assert!(err.downcast_mut::().is_none()); + /// let io = err.downcast_mut::().expect("owned io source"); + /// *io = IoError::other("disk replaced"); + /// assert_eq!( + /// err.downcast_ref::() + /// .expect("io source") + /// .to_string(), + /// "disk replaced" + /// ); /// # } /// ``` #[must_use] @@ -299,6 +355,6 @@ impl Error { where E: CoreError + 'static { - None + self.source.as_mut()?.as_dyn_mut()?.downcast_mut::() } } diff --git a/src/app_error/core/types.rs b/src/app_error/core/types.rs index 226e781..63ed5d8 100644 --- a/src/app_error/core/types.rs +++ b/src/app_error/core/types.rs @@ -32,6 +32,40 @@ where } } +/// Storage for an attached source error. +/// +/// Sources attached from owned values keep the `Owned` variant so that +/// [`Error::downcast`](super::error::Error::downcast) can recover the concrete +/// error by value. Sources attached from an existing [`Arc`] keep the `Shared` +/// variant, which supports borrowing but never yields exclusive ownership. +#[derive(Debug)] +#[doc(hidden)] +pub enum StoredSource { + Owned(Box), + Shared(Arc) +} + +impl StoredSource { + /// Borrow the stored source as a trait object. + pub(crate) fn as_dyn(&self) -> &(dyn CoreError + Send + Sync + 'static) { + match self { + Self::Owned(source) => source.as_ref(), + Self::Shared(source) => source.as_ref() + } + } + + /// Mutably borrow the stored source when ownership is exclusive. + /// + /// Returns `None` for a `Shared` source whose [`Arc`] has other strong or + /// weak references. + pub(crate) fn as_dyn_mut(&mut self) -> Option<&mut (dyn CoreError + Send + Sync + 'static)> { + match self { + Self::Owned(source) => Some(source.as_mut()), + Self::Shared(source) => Arc::get_mut(source) + } + } +} + #[cfg(feature = "std")] pub type CapturedBacktrace = std::backtrace::Backtrace; diff --git a/src/app_error/tests.rs b/src/app_error/tests.rs index aa413fa..f42714c 100644 --- a/src/app_error/tests.rs +++ b/src/app_error/tests.rs @@ -842,6 +842,56 @@ fn downcast_ref_returns_none_when_no_source() { assert!(err.downcast_ref::().is_none()); } +#[test] +#[cfg(feature = "std")] +fn downcast_extracts_source_attached_via_with_source() { + let io_err = IoError::other("disk offline"); + let app_err = AppError::internal("db down").with_source(io_err); + let extracted = app_err.downcast::().expect("owned source"); + assert_eq!(extracted.to_string(), "disk offline"); +} + +#[test] +#[cfg(feature = "std")] +fn downcast_extracts_source_attached_via_with_context() { + let io_err = IoError::other("disk offline"); + let app_err = AppError::internal("db down").with_context(io_err); + let extracted = app_err.downcast::().expect("owned source"); + assert_eq!(extracted.to_string(), "disk offline"); +} + +#[test] +#[cfg(feature = "std")] +fn downcast_returns_err_for_shared_arc_attached_via_with_context() { + let shared = Arc::new(IoError::other("shared source")); + let context: Arc = shared.clone(); + let app_err = AppError::internal("db down").with_context(context); + let app_err = app_err.downcast::().expect_err("shared source"); + assert!(app_err.is::()); + assert_eq!(Arc::strong_count(&shared), 2); +} + +#[test] +#[cfg(feature = "std")] +fn downcast_mut_mutates_source_attached_via_with_source() { + let io_err = IoError::other("before"); + let mut app_err = AppError::internal("db down").with_source(io_err); + let source = app_err.downcast_mut::().expect("owned source"); + *source = IoError::other("after"); + let source = app_err.downcast_ref::().expect("io source"); + assert_eq!(source.to_string(), "after"); +} + +#[test] +#[cfg(feature = "std")] +fn downcast_mut_returns_none_for_shared_arc_attached_via_with_context() { + let shared = Arc::new(IoError::other("shared source")); + let context: Arc = shared.clone(); + let mut app_err = AppError::internal("db down").with_context(context); + assert!(app_err.downcast_mut::().is_none()); + assert!(app_err.downcast_ref::().is_some()); +} + #[test] #[cfg(feature = "colored")] fn colored_display_bare_error_without_message() { diff --git a/src/result_ext.rs b/src/result_ext.rs index ccc6777..5ecabba 100644 --- a/src/result_ext.rs +++ b/src/result_ext.rs @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: MIT -use alloc::{borrow::Cow, boxed::Box, sync::Arc}; +use alloc::{borrow::Cow, boxed::Box}; use core::error::Error as CoreError; use crate::app_error::{Context, Error}; @@ -110,7 +110,7 @@ impl ResultExt for Result { } enriched.with_context(app_err) } - Err(source) => Error::internal(msg.clone()).with_source_arc(Arc::from(source)) + Err(source) => Error::internal(msg.clone()).with_boxed_source(source) } }) }