diff --git a/src/lib.rs b/src/lib.rs index 4f6f9b61..0f0ec81a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -256,6 +256,9 @@ mod backtrace_shim; ))] pub use crate::backtrace_shim::*; +#[cfg(any(feature = "std", test))] +mod once_bool; + #[cfg(feature = "backtraces-impl-backtrace-crate")] pub use backtrace::Backtrace; @@ -1216,26 +1219,17 @@ impl AsBacktrace for Option { #[cfg(any(feature = "std", test))] fn backtrace_collection_enabled() -> bool { - use std::{ - env, - sync::{ - atomic::{AtomicBool, Ordering}, - Once, - }, - }; + use crate::once_bool::OnceBool; + use std::env; - static START: Once = Once::new(); - static ENABLED: AtomicBool = AtomicBool::new(false); + static ENABLED: OnceBool = OnceBool::new(); - START.call_once(|| { + ENABLED.get(|| { // TODO: What values count as "true"? - let enabled = env::var_os("RUST_LIB_BACKTRACE") + env::var_os("RUST_LIB_BACKTRACE") .or_else(|| env::var_os("RUST_BACKTRACE")) - .map_or(false, |v| v == "1"); - ENABLED.store(enabled, Ordering::SeqCst); - }); - - ENABLED.load(Ordering::SeqCst) + .map_or(false, |v| v == "1") + }) } #[cfg(feature = "backtraces-impl-backtrace-crate")] diff --git a/src/once_bool.rs b/src/once_bool.rs new file mode 100644 index 00000000..1a858c84 --- /dev/null +++ b/src/once_bool.rs @@ -0,0 +1,27 @@ +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Once, +}; + +pub struct OnceBool { + start: Once, + enabled: AtomicBool, +} + +impl OnceBool { + pub const fn new() -> Self { + Self { + start: Once::new(), + enabled: AtomicBool::new(false), + } + } + + pub fn get bool>(&self, f: F) -> bool { + self.start.call_once(|| { + let enabled = f(); + self.enabled.store(enabled, Ordering::SeqCst); + }); + + self.enabled.load(Ordering::SeqCst) + } +} diff --git a/src/report.rs b/src/report.rs index 6fe2962b..ac277e87 100644 --- a/src/report.rs +++ b/src/report.rs @@ -219,6 +219,35 @@ struct ReportFormatter<'a>(&'a dyn crate::Error); impl<'a> fmt::Display for ReportFormatter<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + #[cfg(feature = "std")] + { + if trace_cleaning_enabled() { + self.cleaned_error_trace(f)?; + } else { + self.error_trace(f)?; + } + } + + #[cfg(not(feature = "std"))] + { + self.error_trace(f)?; + } + + #[cfg(feature = "unstable-provider-api")] + { + use core::any; + + if let Some(bt) = any::request_ref::(self.0) { + writeln!(f, "\nBacktrace:\n{}", bt)?; + } + } + + Ok(()) + } +} + +impl<'a> ReportFormatter<'a> { + fn error_trace(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { writeln!(f, "{}", self.0)?; let sources = ChainCompat::new(self.0).skip(1); @@ -236,19 +265,98 @@ impl<'a> fmt::Display for ReportFormatter<'a> { writeln!(f, "{:3}: {}", i, source)?; } - #[cfg(feature = "unstable-provider-api")] - { - use core::any; + Ok(()) + } - if let Some(bt) = any::request_ref::(self.0) { - writeln!(f, "\nBacktrace:\n{}", bt)?; + #[cfg(feature = "std")] + fn cleaned_error_trace(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { + const NOTE: char = '*'; + + let mut original_messages = ChainCompat::new(self.0).map(ToString::to_string); + let mut prev = original_messages.next(); + + let mut cleaned_messages = vec![]; + let mut any_cleaned = false; + let mut any_removed = false; + for msg in original_messages { + if let Some(mut prev) = prev { + let cleaned = prev.trim_end_matches(&msg).trim_end().trim_end_matches(':'); + if cleaned.is_empty() { + any_removed = true; + // Do not add this to the output list + } else if cleaned != prev { + any_cleaned = true; + let cleaned_len = cleaned.len(); + prev.truncate(cleaned_len); + prev.push(' '); + prev.push(NOTE); + cleaned_messages.push(prev); + } else { + cleaned_messages.push(prev); + } } + + prev = Some(msg); + } + cleaned_messages.extend(prev); + + let mut visible_messages = cleaned_messages.iter(); + + let head = match visible_messages.next() { + Some(v) => v, + None => return Ok(()), + }; + + writeln!(f, "{}", head)?; + + match cleaned_messages.len() { + 0 | 1 => {} + 2 => writeln!(f, "\nCaused by this error:")?, + _ => writeln!(f, "\nCaused by these errors (recent errors listed first):")?, + } + + for (i, msg) in visible_messages.enumerate() { + // Let's use 1-based indexing for presentation + let i = i + 1; + writeln!(f, "{:3}: {}", i, msg)?; + } + + if any_cleaned || any_removed { + write!(f, "\nNOTE: ")?; + + if any_cleaned { + write!( + f, + "Some redundant information has been removed from the lines marked with {}. ", + NOTE, + )?; + } else { + write!(f, "Some redundant information has been removed. ")?; + } + + writeln!( + f, + "Set {}=1 to disable this behavior.", + SNAFU_RAW_ERROR_MESSAGES, + )?; } Ok(()) } } +#[cfg(feature = "std")] +const SNAFU_RAW_ERROR_MESSAGES: &str = "SNAFU_RAW_ERROR_MESSAGES"; + +#[cfg(feature = "std")] +fn trace_cleaning_enabled() -> bool { + use crate::once_bool::OnceBool; + use std::env; + + static DISABLED: OnceBool = OnceBool::new(); + !DISABLED.get(|| env::var_os(SNAFU_RAW_ERROR_MESSAGES).map_or(false, |v| v == "1")) +} + #[doc(hidden)] pub trait __InternalExtractErrorType { type Err; diff --git a/tests/report.rs b/tests/report.rs index 476fc624..7a1cd7bb 100644 --- a/tests/report.rs +++ b/tests/report.rs @@ -1,5 +1,27 @@ use snafu::{prelude::*, IntoError, Report}; +macro_rules! assert_contains { + (needle: $needle:expr, haystack: $haystack:expr) => { + assert!( + $haystack.contains($needle), + "Expected {:?} to include {:?}", + $haystack, + $needle, + ) + }; +} + +macro_rules! assert_not_contains { + (needle: $needle:expr, haystack: $haystack:expr) => { + assert!( + !$haystack.contains($needle), + "Expected {:?} to not include {:?}", + $haystack, + $needle, + ) + }; +} + #[test] fn includes_the_error_display_text() { #[derive(Debug, Snafu)] @@ -10,12 +32,7 @@ fn includes_the_error_display_text() { let msg = r.to_string(); let expected = "This is my Display text!"; - assert!( - msg.contains(expected), - "Expected {:?} to include {:?}", - msg, - expected, - ); + assert_contains!(needle: expected, haystack: msg); } #[test] @@ -35,12 +52,72 @@ fn includes_the_source_display_text() { let msg = r.to_string(); let expected = "This is my inner Display"; - assert!( - msg.contains(expected), - "Expected {:?} to include {:?}", - msg, - expected, - ); + assert_contains!(needle: expected, haystack: msg); +} + +#[test] +fn reduces_duplication_of_the_source_display_text() { + // Including the source in the Display message is discouraged but + // quite common. + + #[derive(Debug, Snafu)] + #[snafu(display("Level 0"))] + struct Level0Error; + + #[derive(Debug, Snafu)] + #[snafu(display("Level 1: {source}"))] + struct Level1Error { + source: Level0Error, + } + + #[derive(Debug, Snafu)] + #[snafu(display("Level 2: {source}"))] + struct Level2Error { + source: Level1Error, + } + + let e = Level2Snafu.into_error(Level1Snafu.into_error(Level0Error)); + let raw_msg = e.to_string(); + + let expected = "Level 2: Level 1"; + assert_contains!(needle: expected, haystack: raw_msg); + + let r = Report::from_error(e); + let msg = r.to_string(); + + assert_not_contains!(needle: expected, haystack: msg); +} + +#[test] +fn removes_complete_duplication_in_the_source_display_text() { + // Including **only** the source in the Display message is also + // discouraged but occurs. + + #[derive(Debug, Snafu)] + #[snafu(display("Level 0"))] + struct Level0Error; + + #[derive(Debug, Snafu)] + #[snafu(display("{source}"))] + struct Level1Error { + source: Level0Error, + } + + #[derive(Debug, Snafu)] + #[snafu(display("{source}"))] + struct Level2Error { + source: Level1Error, + } + + let e = Level2Snafu.into_error(Level1Snafu.into_error(Level0Error)); + let raw_msg = e.to_string(); + + assert_contains!(needle: "Level 0", haystack: raw_msg); + + let r = Report::from_error(e); + let msg = r.to_string(); + + assert_not_contains!(needle: "Caused by", haystack: msg); } #[test]