subscriber: make PartialOrd & Ord impls more correct (#995)

## Motivation

Currently, there are some minor issues with `Ord` and `PartialOrd` impls
in `tracing_subscriber::filter::env`:

- The `Directive` and `StaticDirective` types implement `PartialOrd`
  with an implementation that never returns `None`, and then have `Ord`
  implementations that call `partial_cmp` and `expect` that the returned
  value is `Some`. This isn't necessary.
- `field::Match` implements `PartialOrd` manually but derives `Ord`.
  Since these traits must agree, using the derived implementation for
  one but manually implementing the other is potentially incorrect (see
  #991).

## Solution

This branch fixes these issues. I've moved actual comparison code from
`PartialOrd` impls for `Directive` and `StaticDirective` to their `Ord`
impls, and changed `PartialOrd::partial_cmp` for those types to call
`Ord::cmp` and wrap it in a `Some`, rather than having `Ord::cmp` call
`PartialOrd::partial_cmp` and unwrap it. This way, the fact that these
comparison impls can never return `None` is encoded at the type level,
rather than having an `expect` that will always succeed.

Additionally, I've added a manual impl of `Ord` for `field::Match` and
removed the derived impl. This should make Clippy happier.

Signed-off-by: Eliza Weisman <[email protected]>

Author: hawkw

tracing-subscriber/src/filter/env/directive.rs

@@ -275,6 +275,12 @@ impl Default for Directive {
 
 impl PartialOrd for Directive {
     fn partial_cmp(&self, other: &Directive) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Ord for Directive {
+    fn cmp(&self, other: &Directive) -> Ordering {
         // We attempt to order directives by how "specific" they are. This
         // ensures that we try the most specific directives first when
         // attempting to match a piece of metadata.
@@ -321,14 +327,7 @@ impl PartialOrd for Directive {
             }
         }
 
-        Some(ordering)
-    }
-}
-
-impl Ord for Directive {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.partial_cmp(other)
-            .expect("Directive::partial_cmp should define a total order")
+        ordering
     }
 }
 
@@ -502,8 +501,8 @@ impl Statics {
     }
 }
 
-impl PartialOrd for StaticDirective {
-    fn partial_cmp(&self, other: &StaticDirective) -> Option<Ordering> {
+impl Ord for StaticDirective {
+    fn cmp(&self, other: &StaticDirective) -> Ordering {
         // We attempt to order directives by how "specific" they are. This
         // ensures that we try the most specific directives first when
         // attempting to match a piece of metadata.
@@ -542,14 +541,13 @@ impl PartialOrd for StaticDirective {
             }
         }
 
-        Some(ordering)
+        ordering
     }
 }
 
-impl Ord for StaticDirective {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.partial_cmp(other)
-            .expect("StaticDirective::partial_cmp should define a total order")
+impl PartialOrd for StaticDirective {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }
 

tracing-subscriber/src/filter/env/field.rs

@@ -13,7 +13,7 @@ use std::{
 use super::{FieldMap, LevelFilter};
 use tracing_core::field::{Field, Visit};
 
-#[derive(Debug, Eq, PartialEq, Ord)]
+#[derive(Debug, Eq, PartialEq)]
 pub(crate) struct Match {
     pub(crate) name: String, // TODO: allow match patterns for names?
     pub(crate) value: Option<ValueMatch>,
@@ -95,8 +95,8 @@ impl fmt::Display for Match {
     }
 }
 
-impl PartialOrd for Match {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+impl Ord for Match {
+    fn cmp(&self, other: &Self) -> Ordering {
         // Ordering for `Match` directives is based first on _whether_ a value
         // is matched or not. This is semantically meaningful --- we would
         // prefer to check directives that match values first as they are more
@@ -113,11 +113,15 @@ impl PartialOrd for Match {
         // This ordering is no longer semantically meaningful but is necessary
         // so that the directives can be stored in the `BTreeMap` in a defined
         // order.
-        Some(
-            has_value
-                .then_with(|| self.name.cmp(&other.name))
-                .then_with(|| self.value.cmp(&other.value)),
-        )
+        has_value
+            .then_with(|| self.name.cmp(&other.name))
+            .then_with(|| self.value.cmp(&other.value))
+    }
+}
+
+impl PartialOrd for Match {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }