implement stricter env filter parsing

tracing-core/src/metadata.rs

@@ -683,7 +683,7 @@ impl FromStr for LevelFilter {
                 _ => None,
             })
             .or_else(|| match from {
-                "" => Some(LevelFilter::ERROR),
+                "" => Some(LevelFilter::ERROR), // ?? this doesn't match EnvFilter empty directive
                 s if s.eq_ignore_ascii_case("error") => Some(LevelFilter::ERROR),
                 s if s.eq_ignore_ascii_case("warn") => Some(LevelFilter::WARN),
                 s if s.eq_ignore_ascii_case("info") => Some(LevelFilter::INFO),

tracing-subscriber/Cargo.toml

@@ -24,7 +24,7 @@ keywords = ["logging", "tracing", "metrics", "subscriber"]
 [features]
 
 default = ["env-filter", "smallvec", "fmt", "ansi", "chrono", "tracing-log", "json"]
-env-filter = ["matchers", "regex", "lazy_static", "tracing"]
+env-filter = ["matchers", "lazy_static", "tracing", "parse-env-filter"]
 fmt = ["registry"]
 ansi = ["fmt", "ansi_term"]
 registry = ["sharded-slab", "thread_local"]
@@ -36,9 +36,9 @@ tracing-core = { path = "../tracing-core", version = "0.2" }
 # only required by the filter feature
 tracing = { optional = true, path = "../tracing", version = "0.2", default-features = false, features = ["std"] }
 matchers = { optional = true, version = "0.1.0" }
-regex = { optional = true, version = "1", default-features = false, features = ["std"] }
 smallvec = { optional = true, version = "1" }
 lazy_static = { optional = true, version = "1" }
+parse-env-filter = { optional = true, version = "0.1" }
 
 # fmt
 tracing-log = { path = "../tracing-log", version = "0.2", optional = true, default-features = false, features = ["log-tracer", "std"] }

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

@@ -1,7 +1,5 @@
 use super::super::level::{self, LevelFilter};
 use super::{field, FieldMap, FilterVec};
-use lazy_static::lazy_static;
-use regex::Regex;
 use std::{cmp::Ordering, error::Error, fmt, iter::FromIterator, str::FromStr};
 use tracing_core::{span, Level, Metadata};
 
@@ -174,94 +172,87 @@ impl Match for Directive {
 
 impl FromStr for Directive {
     type Err = ParseError;
-    fn from_str(from: &str) -> Result<Self, Self::Err> {
-        lazy_static! {
-            static ref DIRECTIVE_RE: Regex = Regex::new(
-                r"(?x)
-                ^(?P<global_level>(?i:trace|debug|info|warn|error|off|[0-5]))$ |
-                 #                 ^^^.
-                 #                     `note: we match log level names case-insensitively
-                ^
-                (?: # target name or span name
-                    (?P<target>[\w:-]+)|(?P<span>\[[^\]]*\])
-                ){1,2}
-                (?: # level or nothing
-                    =(?P<level>(?i:trace|debug|info|warn|error|off|[0-5]))?
-                     #          ^^^.
-                     #              `note: we match log level names case-insensitively
-                )?
-                $
-                "
-            )
-            .unwrap();
-            static ref SPAN_PART_RE: Regex =
-                Regex::new(r#"(?P<name>[^\]\{]+)?(?:\{(?P<fields>[^\}]*)\})?"#).unwrap();
-            static ref FIELD_FILTER_RE: Regex =
-                // TODO(eliza): this doesn't _currently_ handle value matchers that include comma
-                // characters. We should fix that.
-                Regex::new(r#"(?x)
-                    (
-                        # field name
-                        [[:word:]][[[:word:]]\.]*
-                        # value part (optional)
-                        (?:=[^,]+)?
-                    )
-                    # trailing comma or EOS
-                    (?:,\s?|$)
-                "#).unwrap();
-        }
 
-        let caps = DIRECTIVE_RE.captures(from).ok_or_else(ParseError::new)?;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let mut filters = parse_env_filter::filters(s);
 
-        if let Some(level) = caps
-            .name("global_level")
-            .and_then(|s| s.as_str().parse().ok())
-        {
-            return Ok(Directive {
-                level,
-                ..Default::default()
-            });
+        let filter = match (filters.next(), filters.next()) {
+            (Some(Ok(filter)), None) => filter,
+            _ => {
+                return Err(ParseError::new());
+            }
+        };
+
+        if filter.span.is_none() && filter.level.is_none() {
+            return if filter.target.is_empty() {
+                // empty directive
+                Err(ParseError::new())
+            } else if let Ok(level) = filter.target.parse() {
+                // bare level
+                Ok(Directive {
+                    in_span: None,
+                    fields: FilterVec::new(),
+                    target: None,
+                    level,
+                })
+            } else {
+                // bare target
+                Ok(Directive {
+                    in_span: None,
+                    fields: FilterVec::new(),
+                    target: Some(filter.target.into()),
+                    level: LevelFilter::TRACE,
+                })
+            };
         }
 
-        let target = caps.name("target").and_then(|c| {
-            let s = c.as_str();
-            if s.parse::<LevelFilter>().is_ok() {
-                None
-            } else {
-                Some(s.to_owned())
-            }
-        });
-
-        let (in_span, fields) = caps
-            .name("span")
-            .and_then(|cap| {
-                let cap = cap.as_str().trim_matches(|c| c == '[' || c == ']');
-                let caps = SPAN_PART_RE.captures(cap)?;
-                let span = caps.name("name").map(|c| c.as_str().to_owned());
-                let fields = caps
-                    .name("fields")
-                    .map(|c| {
-                        FIELD_FILTER_RE
-                            .find_iter(c.as_str())
-                            .map(|c| c.as_str().parse())
-                            .collect::<Result<FilterVec<_>, _>>()
-                    })
-                    .unwrap_or_else(|| Ok(FilterVec::new()));
-                Some((span, fields))
-            })
-            .unwrap_or_else(|| (None, Ok(FilterVec::new())));
+        let (in_span, fields) = match filter.span {
+            None => (None, FilterVec::new()),
+            Some(mut filters) => match (filters.next(), filters.next()) {
+                (None, None) => (None, FilterVec::new()),
+                (Some(Ok(filter)), None) => {
+                    let in_span = Some(filter.name).filter(|s| !s.is_empty()).map(Into::into);
+                    let fields = match filter.fields {
+                        None => FilterVec::new(),
+                        Some(mut filters) => match (filters.next(), filters.next()) {
+                            (Some(Ok(filter)), None) => std::iter::once(field::Match {
+                                name: filter.name.to_string(),
+                                value: filter.value.map(str::parse).transpose().map_err(|err| {
+                                    ParseError {
+                                        kind: ParseErrorKind::Field(Box::new(err)),
+                                    }
+                                })?,
+                            })
+                            .collect(),
+                            _ => {
+                                return Err(ParseError::new());
+                            }
+                        },
+                    };
+
+                    (in_span, fields)
+                }
+                // NB: we only support one in_span per directive
+                _ => {
+                    return Err(ParseError::new());
+                }
+            },
+        };
 
-        let level = caps
-            .name("level")
-            .and_then(|l| l.as_str().parse().ok())
-            // Setting the target without the level enables every level for that target
-            .unwrap_or(LevelFilter::TRACE);
+        let target = Some(filter.target)
+            .filter(|s| !s.is_empty())
+            .map(Into::into);
+
+        let level = match filter.level {
+            None | Some("") => LevelFilter::TRACE,
+            Some(s) => s.parse()?,
+        };
 
         Ok(Directive {
-            level,
-            target,
             in_span,
-            fields: fields?,
+            fields,
+            target,
+            level,
         })
     }
 }