From 506a482a11f58eb160758076dd4c5a6667718835 Mon Sep 17 00:00:00 2001
From: Eliza Weisman <eliza@buoyant.io>
Date: Mon, 3 Feb 2020 12:32:34 -0800
Subject: [PATCH] subscriber: make ANSI and not-ANSI formatting consistent

Currently, the `tracing-subscriber` crate's `fmt` module has two
separate `fmt::Display` impls for its' `FmtCtx` and `FullCtx` types,
depending on whether the feature flag that enables ANSI colors is set.
Since a lot of the `fmt::Display` implementations doesn't have anything
to do with ANSI colors, this means that we have a lot of repeated code,
and in order to keep the formatting consistent, we have to keep these
implementations in sync manually. There are currently some
inconsistencies in formatting with ANSI colors on and off, implying that
we have failed to do that.

This commit simplifies the situation significantly by consolidating the
`fmt::Display` impls into one implementation, and only feature flagging
the code that actually needs to be different. We do this by introducing
a `Style` type which exposes no-op versions of methods with the same
names as the `ansi-term` crate's `Style` when ANSI formatting is
disabled. Now, the conditional logic is hidden in the function that
returns a `Style`, and the rest of the `fmt::Display` implementations
can be agnostic of ANSI colors. In release mode, `rustc` should be able
to optimize out the no-op methods and empty struct entirely.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
---
 tracing-subscriber/src/fmt/format/mod.rs | 96 +++++++++---------------
 1 file changed, 34 insertions(+), 62 deletions(-)

diff --git a/tracing-subscriber/src/fmt/format/mod.rs b/tracing-subscriber/src/fmt/format/mod.rs
index 6a5f76e472..b68639b2b2 100644
--- a/tracing-subscriber/src/fmt/format/mod.rs
+++ b/tracing-subscriber/src/fmt/format/mod.rs
@@ -488,53 +488,33 @@ where
     pub(crate) fn new(ctx: &'a FmtContext<'_, S, N>) -> Self {
         Self { ctx }
     }
-}
-
-#[cfg(feature = "ansi")]
-impl<'a, S, N: 'a> fmt::Display for FmtCtx<'a, S, N>
-where
-    S: Subscriber + for<'lookup> LookupSpan<'lookup>,
-    N: for<'writer> FormatFields<'writer> + 'static,
-{
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let mut seen = false;
-        self.ctx.visit_spans(|span| {
-            if seen {
-                f.pad(":")?;
-            }
-            seen = true;
 
-            let metadata = span.metadata();
+    fn bold(&self) -> Style {
+        #[cfg(feature = "ansi")]
+        {
             if self.ansi {
-                write!(f, "{}", Style::new().bold().paint(metadata.name()))?;
-            } else {
-                write!(f, "{}", metadata.name())?;
+                return Style::new().bold();
             }
-
-            Ok(()) as fmt::Result
-        })?;
-        if seen {
-            f.pad(" ")?;
         }
-        Ok(())
+
+        Style::new()
     }
 }
 
-#[cfg(not(feature = "ansi"))]
-impl<'a, S, N> fmt::Display for FmtCtx<'a, S, N>
+impl<'a, S, N: 'a> fmt::Display for FmtCtx<'a, S, N>
 where
     S: Subscriber + for<'lookup> LookupSpan<'lookup>,
     N: for<'writer> FormatFields<'writer> + 'static,
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let bold = self.bold();
         let mut seen = false;
+
         self.ctx.visit_spans(|span| {
-            if seen {
-                f.pad(":")?;
-            }
             seen = true;
-            write!(f, "{}", span.name())
+            write!(f, "{}:", bold.paint(span.metadata().name()))
         })?;
+
         if seen {
             f.pad(" ")?;
         }
@@ -566,33 +546,39 @@ where
     pub(crate) fn new(ctx: &'a FmtContext<'a, S, N>) -> Self {
         Self { ctx }
     }
+
+    fn bold(&self) -> Style {
+        #[cfg(feature = "ansi")] {
+            if self.ansi {
+                return Style::new().bold();
+            }
+        }
+
+
+        Style::new()
+    }
 }
 
-#[cfg(feature = "ansi")]
 impl<'a, S, N> fmt::Display for FullCtx<'a, S, N>
 where
     S: Subscriber + for<'lookup> LookupSpan<'lookup>,
     N: for<'writer> FormatFields<'writer> + 'static,
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let bold = self.bold();
         let mut seen = false;
-        let style = if self.ansi {
-            Style::new().bold()
-        } else {
-            Style::new()
-        };
+
         self.ctx.visit_spans(|span| {
-            let metadata = span.metadata();
-            write!(f, "{}", style.paint(metadata.name()))?;
+            write!(f, "{}", bold.paint(span.metadata().name()))?;
             seen = true;
 
             let ext = span.extensions();
-            let data = &ext
+            let fields = &ext
                 .get::<FormattedFields<N>>()
                 .expect("Unable to find FormattedFields in extensions; this is a bug");
-            write!(f, "{}{}{}", style.paint("{"), data, style.paint("}"))?;
-            ":".fmt(f)
+            write!(f, "{}{}{}:", bold.paint("{"), fields, bold.paint("}"))
         })?;
+
         if seen {
             f.pad(" ")?;
         }
@@ -601,27 +587,13 @@ where
 }
 
 #[cfg(not(feature = "ansi"))]
-impl<'a, S, N> fmt::Display for FullCtx<'a, S, N>
-where
-    S: Subscriber + for<'lookup> LookupSpan<'lookup>,
-    N: for<'writer> FormatFields<'writer> + 'static,
-{
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let mut seen = false;
-        self.ctx.visit_spans(|span| {
-            write!(f, "{}", span.name())?;
-            seen = true;
+struct Style;
 
-            let fields = span.fields();
-            if !fields.is_empty() {
-                write!(f, "{{{}}}", fields)?;
-            }
-            ":".fmt(f)
-        })?;
-        if seen {
-            f.pad(" ")?;
-        }
-        Ok(())
+#[cfg(not(feature = "ansi"))]
+impl Style {
+    fn new() -> Self { Style }
+    fn paint(&self, d: impl fmt::Display) -> impl fmt::Display {
+        d
     }
 }