fix Layered's max_level_hint impl

tracing-subscriber/src/fmt/mod.rs

@@ -480,9 +480,9 @@ where
     #[must_use = "you may want to use `try_init` or similar to actually install the collector."]
     pub fn finish(self) -> Collector<N, E, F, W> {
         let collector = self.inner.with_collector(Registry::default());
-        Collector {
-            inner: self.filter.with_collector(collector),
-        }
+        let mut inner = self.filter.with_collector(collector);
+        inner.inner_is_registry = true;
+        Collector { inner }
     }
 
     /// Install this collector as the global default if one is

tracing-subscriber/src/subscribe/layered.rs

@@ -39,9 +39,8 @@ pub struct Layered<S, I, C = I> {
     // level hints when per-subscriber filters are in use.
     /// Is `self.inner` a `Registry`?
     ///
-    /// If so, when combining `Interest`s, we want to "bubble up" its
-    /// `Interest`.
-    inner_is_registry: bool,
+    /// If so, we ignore the subscriber when returning a `max_level_hint`
+    pub(crate) inner_is_registry: bool,
 
     /// Does `self.subscriber` have per-subscriber filters?
     ///
@@ -482,6 +481,8 @@ where
             return outer_hint;
         }
 
+        // This is the same as the fallthrough, but we want it to happen
+        // before we check the individual cases below.
         if self.has_subscriber_filter && self.inner_has_subscriber_filter {
             return Some(cmp::max(outer_hint?, inner_hint?));
         }
@@ -494,7 +495,9 @@ where
             return None;
         }
 
-        cmp::max(outer_hint, inner_hint)
+        // `None` is basically == `Some(TRACE)` so we have to short
+        // circuit, instead of using `Option`s `PartialOrd` impl.
+        Some(cmp::max(outer_hint?, inner_hint?))
     }
 }
 

tracing-subscriber/tests/option.rs

@@ -1,7 +1,22 @@
 #![cfg(feature = "registry")]
-use tracing::level_filters::LevelFilter;
-use tracing::Collect;
-use tracing_subscriber::prelude::*;
+use tracing_core::{collect::Interest, Collect, LevelFilter, Metadata};
+use tracing_subscriber::{prelude::*, subscribe};
+
+// A basic layer that returns its inner for `max_level_hint`
+struct BasicLayer(Option<LevelFilter>);
+impl<C: Collect> tracing_subscriber::Subscribe<C> for BasicLayer {
+    fn register_callsite(&self, _m: &Metadata<'_>) -> Interest {
+        Interest::sometimes()
+    }
+
+    fn enabled(&self, _m: &Metadata<'_>, _: subscribe::Context<'_, C>) -> bool {
+        true
+    }
+
+    fn max_level_hint(&self) -> Option<LevelFilter> {
+        self.0
+    }
+}
 
 // This test is just used to compare to the tests below
 #[test]
@@ -39,3 +54,26 @@ fn just_option_none_subscriber() {
     let collector = tracing_subscriber::registry().with(Some(LevelFilter::ERROR));
     assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR));
 }
+
+/// Test that the `Layered` impl fallthrough the `max_level_hint` and `None` doesn't disable
+/// everything, as well as ensuring the `Some` case also works.
+#[test]
+fn layered_fallthrough() {
+    // None means the other layer takes control
+    let subscriber = tracing_subscriber::registry()
+        .with(BasicLayer(None))
+        .with(None::<LevelFilter>);
+    assert_eq!(subscriber.max_level_hint(), None);
+
+    // The `None`-returning layer still wins
+    let subscriber = tracing_subscriber::registry()
+        .with(BasicLayer(None))
+        .with(Some(LevelFilter::ERROR));
+    assert_eq!(subscriber.max_level_hint(), None);
+
+    // Check that we aren't doing anything truly wrong
+    let subscriber = tracing_subscriber::registry()
+        .with(BasicLayer(Some(LevelFilter::DEBUG)))
+        .with(None::<LevelFilter>);
+    assert_eq!(subscriber.max_level_hint(), Some(LevelFilter::DEBUG));
+}