Skip to content

Commit 3f856a6

Browse files
kaffarellkaffarell
committed
fmt: fix layers with different ANSI settings sharing the same TypeId
Previously, the FormattedFields struct stored the ANSI state as a boolean field, which could lead to inconsistencies when different layers used different ANSI settings (true/false). This happened because we store the FormattedFields in a HashMap using the TypeId as a key which obviously means that ansi=true and ansi=false would have the same key and the same formatted fields. This meant that whichever layer stored its fields first would determine the ANSI formatting for all subsequent layers. This change refactors FormattedFields to use a const generic parameter for the ANSI state, allowing us to store two different FormattedFields (one with and one without ansi). Each layer can now store its fields with the correct ANSI setting without interfering with other layers. Fixes: tokio-rs#1310 Fixes: tokio-rs#1817 Fixes: tokio-rs#3065 Fixes: tokio-rs#3116 Signed-off-by: Gabriel Goller <[email protected]>
1 parent b486867 commit 3f856a6

File tree

4 files changed

+95
-40
lines changed

4 files changed

+95
-40
lines changed

‎tracing-subscriber/src/fmt/fmt_subscriber.rs‎

Lines changed: 56 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -753,22 +753,22 @@ where
753753
/// the [`Extensions`][extensions] type-map. This means that when multiple
754754
/// formatters are in use, each can store its own formatted representation
755755
/// without conflicting.
756+
/// The `ANSI` const parameter determines whether ANSI escape codes should be used
757+
/// for formatting the fields (i.e. colors, font styles).
756758
///
757759
/// [extensions]: crate::registry::Extensions
758760
#[derive(Default)]
759-
pub struct FormattedFields<E: ?Sized> {
761+
pub struct FormattedFields<E: ?Sized, const ANSI: bool = false> {
760762
_format_fields: PhantomData<fn(E)>,
761-
was_ansi: bool,
762763
/// The formatted fields of a span.
763764
pub fields: String,
764765
}
765766

766-
impl<E: ?Sized> FormattedFields<E> {
767+
impl<E: ?Sized, const ANSI: bool> FormattedFields<E, ANSI> {
767768
/// Returns a new `FormattedFields`.
768769
pub fn new(fields: String) -> Self {
769770
Self {
770771
fields,
771-
was_ansi: false,
772772
_format_fields: PhantomData,
773773
}
774774
}
@@ -778,27 +778,27 @@ impl<E: ?Sized> FormattedFields<E> {
778778
/// The returned [`format::Writer`] can be used with the
779779
/// [`FormatFields::format_fields`] method.
780780
pub fn as_writer(&mut self) -> format::Writer<'_> {
781-
format::Writer::new(&mut self.fields).with_ansi(self.was_ansi)
781+
format::Writer::new(&mut self.fields).with_ansi(ANSI)
782782
}
783783
}
784784

785-
impl<E: ?Sized> fmt::Debug for FormattedFields<E> {
785+
impl<E: ?Sized, const ANSI: bool> fmt::Debug for FormattedFields<E, ANSI> {
786786
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
787787
f.debug_struct("FormattedFields")
788788
.field("fields", &self.fields)
789789
.field("formatter", &format_args!("{}", std::any::type_name::<E>()))
790-
.field("was_ansi", &self.was_ansi)
790+
.field("is_ansi", &format_args!("{}", ANSI))
791791
.finish()
792792
}
793793
}
794794

795-
impl<E: ?Sized> fmt::Display for FormattedFields<E> {
795+
impl<E: ?Sized, const ANSI: bool> fmt::Display for FormattedFields<E, ANSI> {
796796
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
797797
fmt::Display::fmt(&self.fields, f)
798798
}
799799
}
800800

801-
impl<E: ?Sized> Deref for FormattedFields<E> {
801+
impl<E: ?Sized, const ANSI: bool> Deref for FormattedFields<E, ANSI> {
802802
type Target = String;
803803
fn deref(&self) -> &Self::Target {
804804
&self.fields
@@ -834,14 +834,29 @@ where
834834
let span = ctx.span(id).expect("Span not found, this is a bug");
835835
let mut extensions = span.extensions_mut();
836836

837-
if extensions.get_mut::<FormattedFields<N>>().is_none() {
838-
let mut fields = FormattedFields::<N>::new(String::new());
837+
if self.is_ansi {
838+
if extensions.get_mut::<FormattedFields<N, true>>().is_none() {
839+
let mut fields = FormattedFields::<N, true>::new(String::new());
840+
if self
841+
.fmt_fields
842+
.format_fields(fields.as_writer().with_ansi(true), attrs)
843+
.is_ok()
844+
{
845+
extensions.insert(fields);
846+
} else {
847+
eprintln!(
848+
"[tracing-subscriber] Unable to format the following event, ignoring: {:?}",
849+
attrs
850+
);
851+
}
852+
}
853+
} else if extensions.get_mut::<FormattedFields<N, false>>().is_none() {
854+
let mut fields = FormattedFields::<N, false>::new(String::new());
839855
if self
840856
.fmt_fields
841-
.format_fields(fields.as_writer().with_ansi(self.is_ansi), attrs)
857+
.format_fields(fields.as_writer().with_ansi(false), attrs)
842858
.is_ok()
843859
{
844-
fields.was_ansi = self.is_ansi;
845860
extensions.insert(fields);
846861
} else {
847862
eprintln!(
@@ -870,19 +885,35 @@ where
870885
fn on_record(&self, id: &Id, values: &Record<'_>, ctx: Context<'_, C>) {
871886
let span = ctx.span(id).expect("Span not found, this is a bug");
872887
let mut extensions = span.extensions_mut();
873-
if let Some(fields) = extensions.get_mut::<FormattedFields<N>>() {
874-
let _ = self.fmt_fields.add_fields(fields, values);
875-
return;
876-
}
877888

878-
let mut fields = FormattedFields::<N>::new(String::new());
879-
if self
880-
.fmt_fields
881-
.format_fields(fields.as_writer().with_ansi(self.is_ansi), values)
882-
.is_ok()
883-
{
884-
fields.was_ansi = self.is_ansi;
885-
extensions.insert(fields);
889+
if self.is_ansi {
890+
if let Some(fields) = extensions.get_mut::<FormattedFields<N, true>>() {
891+
let _ = self.fmt_fields.add_fields::<true>(fields, values);
892+
return;
893+
}
894+
895+
let mut fields = FormattedFields::<N, true>::new(String::new());
896+
if self
897+
.fmt_fields
898+
.format_fields(fields.as_writer().with_ansi(true), values)
899+
.is_ok()
900+
{
901+
extensions.insert(fields);
902+
}
903+
} else {
904+
if let Some(fields) = extensions.get_mut::<FormattedFields<N, false>>() {
905+
let _ = self.fmt_fields.add_fields::<false>(fields, values);
906+
return;
907+
}
908+
909+
let mut fields = FormattedFields::<N, false>::new(String::new());
910+
if self
911+
.fmt_fields
912+
.format_fields(fields.as_writer().with_ansi(false), values)
913+
.is_ok()
914+
{
915+
extensions.insert(fields);
916+
}
886917
}
887918
}
888919

‎tracing-subscriber/src/fmt/format/json.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ where
145145

146146
let ext = self.0.extensions();
147147
let data = ext
148-
.get::<FormattedFields<N>>()
148+
.get::<FormattedFields<N, false>>()
149149
.expect("Unable to find FormattedFields in extensions; this is a bug");
150150

151151
// TODO: let's _not_ do this, but this resolves
@@ -358,9 +358,9 @@ impl<'a> FormatFields<'a> for JsonFields {
358358
/// By default, this appends a space to the current set of fields if it is
359359
/// non-empty, and then calls `self.format_fields`. If different behavior is
360360
/// required, the default implementation of this method can be overridden.
361-
fn add_fields(
361+
fn add_fields<const ANSI: bool>(
362362
&self,
363-
current: &'a mut FormattedFields<Self>,
363+
current: &'a mut FormattedFields<Self, ANSI>,
364364
fields: &Record<'_>,
365365
) -> fmt::Result {
366366
if current.is_empty() {

‎tracing-subscriber/src/fmt/format/mod.rs‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ use fmt::{Debug, Display};
152152
/// // formatter in the `FmtContext`.
153153
/// let ext = span.extensions();
154154
/// let fields = &ext
155-
/// .get::<FormattedFields<N>>()
155+
/// .get::<FormattedFields<N, true>>()
156156
/// .expect("will never be `None`");
157157
///
158158
/// // Skip formatting the fields if the span had no fields.
@@ -240,9 +240,12 @@ pub trait FormatFields<'writer> {
240240
/// By default, this appends a space to the current set of fields if it is
241241
/// non-empty, and then calls `self.format_fields`. If different behavior is
242242
/// required, the default implementation of this method can be overridden.
243-
fn add_fields(
243+
///
244+
/// The `ANSI` const parameter determines whether ANSI escape codes should be used
245+
/// for formatting the fields (i.e. colors and font styles).
246+
fn add_fields<const ANSI: bool>(
244247
&self,
245-
current: &'writer mut FormattedFields<Self>,
248+
current: &'writer mut FormattedFields<Self, ANSI>,
246249
fields: &span::Record<'_>,
247250
) -> fmt::Result {
248251
if !current.fields.is_empty() {
@@ -980,7 +983,13 @@ where
980983
seen = true;
981984

982985
let ext = span.extensions();
983-
if let Some(fields) = &ext.get::<FormattedFields<N>>() {
986+
if let Some(true) = self.ansi {
987+
if let Some(fields) = &ext.get::<FormattedFields<N, true>>() {
988+
if !fields.is_empty() {
989+
write!(writer, "{}{}{}", bold.paint("{"), fields, bold.paint("}"))?;
990+
}
991+
}
992+
}else if let Some(fields) = &ext.get::<FormattedFields<N, false>>() {
984993
if !fields.is_empty() {
985994
write!(writer, "{}{}{}", bold.paint("{"), fields, bold.paint("}"))?;
986995
}
@@ -1108,7 +1117,13 @@ where
11081117

11091118
for span in ctx.event_scope().into_iter().flat_map(Scope::from_root) {
11101119
let exts = span.extensions();
1111-
if let Some(fields) = exts.get::<FormattedFields<N>>() {
1120+
if let Some(true) = self.ansi {
1121+
if let Some(fields) = exts.get::<FormattedFields<N, true>>() {
1122+
if !fields.is_empty() {
1123+
write!(writer, " {}", dimmed.paint(&fields.fields))?;
1124+
}
1125+
}
1126+
} else if let Some(fields) = exts.get::<FormattedFields<N, false>>() {
11121127
if !fields.is_empty() {
11131128
write!(writer, " {}", dimmed.paint(&fields.fields))?;
11141129
}

‎tracing-subscriber/src/fmt/format/pretty.rs‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -309,11 +309,20 @@ where
309309
}
310310

311311
let ext = span.extensions();
312-
let fields = &ext
313-
.get::<FormattedFields<N>>()
314-
.expect("Unable to find FormattedFields in extensions; this is a bug");
315-
if !fields.is_empty() {
316-
write!(writer, " {} {}", dimmed.paint("with"), fields)?;
312+
if let Some(true) = self.ansi {
313+
let fields = &ext
314+
.get::<FormattedFields<N, true>>()
315+
.expect("Unable to find FormattedFields in extensions; this is a bug");
316+
if !fields.is_empty() {
317+
write!(writer, " {} {}", dimmed.paint("with"), fields)?;
318+
}
319+
} else {
320+
let fields = &ext
321+
.get::<FormattedFields<N, false>>()
322+
.expect("Unable to find FormattedFields in extensions; this is a bug");
323+
if !fields.is_empty() {
324+
write!(writer, " {} {}", dimmed.paint("with"), fields)?;
325+
}
317326
}
318327
writer.write_char('\n')?;
319328
}
@@ -329,9 +338,9 @@ impl<'writer> FormatFields<'writer> for Pretty {
329338
v.finish()
330339
}
331340

332-
fn add_fields(
341+
fn add_fields<const ANSI: bool>(
333342
&self,
334-
current: &'writer mut FormattedFields<Self>,
343+
current: &'writer mut FormattedFields<Self, ANSI>,
335344
fields: &span::Record<'_>,
336345
) -> fmt::Result {
337346
let empty = current.is_empty();

0 commit comments

Comments
 (0)