Merge pull request #365 from shepmaster/clean-report

Author: shepmaster

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<Backtrace> {
 
 #[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")]

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<F: FnOnce() -> bool>(&self, f: F) -> bool {
+        self.start.call_once(|| {
+            let enabled = f();
+            self.enabled.store(enabled, Ordering::SeqCst);
+        });
+
+        self.enabled.load(Ordering::SeqCst)
+    }
+}

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::<crate::Backtrace>(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::<crate::Backtrace>(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;

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]