From 7ecaedf9a61f56c97aba65e557fa31762baa6271 Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Tue, 19 Jul 2022 18:36:59 -0400 Subject: [PATCH] core: implement `PartialEq`, `Eq` for `Metadata`, `FieldSet` (#2229) A `FieldSet` is equal to another `FieldSet` if they share the same callsite and fields (provided in the same order). This ensures that a `Field` applicable to one `FieldSet` is applicable to any equal `FieldSet`. A `Metadata` is equal to another `Metadata` if all of their contained metadata is exactly equal. This change manually re-implements `PartialEq` and `Eq` for `Metadata` and `FieldSet` to define their equality strictly in terms of callsite equality. In debug builds, the equality of these types' other fields is also checked. Documentation is added to both `Metadata` and `FieldSet` explaining this behavior. The expectation that, in a well-behaving application, `Metadata` and `FieldSet`s with equal callsites will be otherwise equal is documented on `Callsite::metadata`. This is not a breaking change, as previous releases did not define equality for `Metadata` or `FieldSet`. The `Callsite` trait remains safe, as this expectation is not (yet) a safety-critical property. --- tracing-core/src/callsite.rs | 9 +++++ tracing-core/src/field.rs | 44 ++++++++++++++++++++++ tracing-core/src/metadata.rs | 73 +++++++++++++++++++++++++++++++----- 3 files changed, 116 insertions(+), 10 deletions(-) diff --git a/tracing-core/src/callsite.rs b/tracing-core/src/callsite.rs index 8f7e100f1d..9d2018d047 100644 --- a/tracing-core/src/callsite.rs +++ b/tracing-core/src/callsite.rs @@ -113,6 +113,15 @@ pub trait Callsite: Sync { /// Returns the [metadata] associated with the callsite. /// + ///
+ /// 
+    ///
+    /// **Note:** Implementations of this method should not produce [`Metadata`]
+    /// that share the same callsite [`Identifier`] but otherwise differ in any
+    /// way (e.g., have different `name`s).
+    ///
+    ///
+ /// /// [metadata]: super::metadata::Metadata fn metadata(&self) -> &Metadata<'_>; } diff --git a/tracing-core/src/field.rs b/tracing-core/src/field.rs index cb02eb7e01..7b6e2cc6e7 100644 --- a/tracing-core/src/field.rs +++ b/tracing-core/src/field.rs @@ -70,6 +70,16 @@ pub struct Field { pub struct Empty; /// Describes the fields present on a span. +/// +/// ## Equality +/// +/// In well-behaved applications, two `FieldSet`s [initialized] with equal +/// [callsite identifiers] will have identical fields. Consequently, in release +/// builds, [`FieldSet::eq`] *only* checks that its arguments have equal +/// callsites. However, the equality of field names is checked in debug builds. +/// +/// [initialized]: Self::new +/// [callsite identifiers]: callsite::Identifier pub struct FieldSet { /// The names of each field on the described span. names: &'static [&'static str], @@ -788,6 +798,40 @@ impl fmt::Display for FieldSet { } } +impl Eq for FieldSet {} + +impl PartialEq for FieldSet { + fn eq(&self, other: &Self) -> bool { + if core::ptr::eq(&self, &other) { + true + } else if cfg!(not(debug_assertions)) { + // In a well-behaving application, two `FieldSet`s can be assumed to + // be totally equal so long as they share the same callsite. + self.callsite == other.callsite + } else { + // However, when debug-assertions are enabled, do NOT assume that + // the application is well-behaving; check every the field names of + // each `FieldSet` for equality. + + // `FieldSet` is destructured here to ensure a compile-error if the + // fields of `FieldSet` change. + let Self { + names: lhs_names, + callsite: lhs_callsite, + } = self; + + let Self { + names: rhs_names, + callsite: rhs_callsite, + } = &other; + + // Check callsite equality first, as it is probably cheaper to do + // than str equality. + lhs_callsite == rhs_callsite && lhs_names == rhs_names + } + } +} + // ===== impl Iter ===== impl Iterator for Iter { diff --git a/tracing-core/src/metadata.rs b/tracing-core/src/metadata.rs index 7b848b4a20..73fdc03a22 100644 --- a/tracing-core/src/metadata.rs +++ b/tracing-core/src/metadata.rs @@ -35,16 +35,13 @@ use core::{ /// _significantly_ lower than that of creating the actual span. Therefore, /// filtering is based on metadata, rather than on the constructed span. /// -///
-/// 
+/// ## Equality
 ///
-/// **Note**: Although instances of `Metadata` cannot
-/// be compared directly, they provide a method [`callsite`][Metadata::callsite],
-/// returning an opaque [callsite identifier] which uniquely identifies the
-/// callsite where the metadata originated. This can be used to determine if two
-/// `Metadata` correspond to the same callsite.
-///
-///
+/// In well-behaved applications, two `Metadata` with equal +/// [callsite identifiers] will be equal in all other ways (i.e., have the same +/// `name`, `target`, etc.). Consequently, in release builds, [`Metadata::eq`] +/// *only* checks that its arguments have equal callsites. However, the equality +/// of `Metadata`'s other fields is checked in debug builds. /// /// [span]: super::span /// [event]: super::event @@ -56,7 +53,7 @@ use core::{ /// [line number]: Self::line /// [module path]: Self::module_path /// [collector]: super::collect::Collect -/// [callsite identifier]: super::callsite::Identifier +/// [callsite identifiers]: Self::callsite pub struct Metadata<'a> { /// The name of the span described by this metadata. name: &'static str, @@ -444,6 +441,62 @@ impl fmt::Debug for Kind { } } +impl<'a> Eq for Metadata<'a> {} + +impl<'a> PartialEq for Metadata<'a> { + #[inline] + fn eq(&self, other: &Self) -> bool { + if core::ptr::eq(&self, &other) { + true + } else if cfg!(not(debug_assertions)) { + // In a well-behaving application, two `Metadata` can be assumed to + // be totally equal so long as they share the same callsite. + self.callsite() == other.callsite() + } else { + // However, when debug-assertions are enabled, do not assume that + // the application is well-behaving; check every field of `Metadata` + // for equality. + + // `Metadata` is destructured here to ensure a compile-error if the + // fields of `Metadata` change. + let Metadata { + name: lhs_name, + target: lhs_target, + level: lhs_level, + module_path: lhs_module_path, + file: lhs_file, + line: lhs_line, + fields: lhs_fields, + kind: lhs_kind, + } = self; + + let Metadata { + name: rhs_name, + target: rhs_target, + level: rhs_level, + module_path: rhs_module_path, + file: rhs_file, + line: rhs_line, + fields: rhs_fields, + kind: rhs_kind, + } = &other; + + // The initial comparison of callsites is purely an optimization; + // it can be removed without affecting the overall semantics of the + // expression. + self.callsite() == other.callsite() + && lhs_name == rhs_name + && lhs_target == rhs_target + && lhs_level == rhs_level + && lhs_module_path == rhs_module_path + && lhs_file == rhs_file + && lhs_line == rhs_line + && lhs_fields == rhs_fields + && lhs_kind == rhs_kind + } + } +} + // ===== impl Level ===== impl Level {