From f4da668d0545f5271769d3a2af2469663da9bcee Mon Sep 17 00:00:00 2001
From: Jack Wrenn <jack@wrenn.fyi>
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 | 89 ++++++++++++++++++++++++++++--------
 3 files changed, 124 insertions(+), 18 deletions(-)

diff --git a/tracing-core/src/callsite.rs b/tracing-core/src/callsite.rs
index d0fe1a3d6c..bc7405ffa0 100644
--- a/tracing-core/src/callsite.rs
+++ b/tracing-core/src/callsite.rs
@@ -137,6 +137,15 @@ pub trait Callsite: Sync {
 
     /// Returns the [metadata] associated with the callsite.
     ///
+    /// <div class="example-wrap" style="display:inline-block">
+    /// <pre class="ignore" style="white-space:normal;font:inherit;">
+    ///
+    /// **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).
+    ///
+    /// </pre></div>
+    ///
     /// [metadata]: super::metadata::Metadata
     fn metadata(&self) -> &Metadata<'_>;
 
diff --git a/tracing-core/src/field.rs b/tracing-core/src/field.rs
index c9869ce2ca..e103c75a9d 100644
--- a/tracing-core/src/field.rs
+++ b/tracing-core/src/field.rs
@@ -145,6 +145,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],
@@ -911,6 +921,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 47b9388a41..a154419a74 100644
--- a/tracing-core/src/metadata.rs
+++ b/tracing-core/src/metadata.rs
@@ -35,28 +35,25 @@ use crate::stdlib::{
 /// _significantly_ lower than that of creating the actual span. Therefore,
 /// filtering is based on metadata, rather than on the constructed span.
 ///
-/// <pre class="ignore" style="white-space:normal;font:inherit;">
-///     <strong>Note</strong>: Although instances of <code>Metadata</code>
-///     cannot be compared directly, they provide a method
-///     <a href="struct.Metadata.html#method.id"><code>id</code></a>, returning
-///     an opaque <a href="../callsite/struct.Identifier.html">callsite
-///     identifier</a>which uniquely identifies the callsite where the metadata
-///     originated. This can be used to determine if two <code>Metadata</code>
-///     correspond to the same callsite.
-/// </pre>
+/// ## Equality
+///
+/// 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
-/// [name]: Metadata::name()
-/// [target]: Metadata::target()
-/// [fields]: Metadata::fields()
-/// [verbosity level]: Metadata::level()
-/// [file name]: Metadata::file()
-/// [line number]: Metadata::line()
-/// [module path]: Metadata::module_path()
+/// [name]: Self::name
+/// [target]: Self::target
+/// [fields]: Self::fields
+/// [verbosity level]: Self::level
+/// [file name]: Self::file
+/// [line number]: Self::line
+/// [module path]: Self::module_path
 /// [`Subscriber`]: super::subscriber::Subscriber
-/// [`id`]: Metadata::id
-/// [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,
@@ -443,6 +440,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 {