Add unicode-case and unicode-perl features to the regex dependency

[dbidwell94]

Regex parse error

Motivation

tracing-subscriber-0.3.16\src\filter\env\directive.rs:140:10 fails to parse regex in certain cases

Solution

Refactor referenced regex so that unicode-case and unicode-perl is not required.

[BurntSushi]

I don't believe this is quite sufficient. At least, it isn't for me when I run cargo test locally. You also need unicode-perl since y'all are using \w and \s.

With that said, an alternative here is to not add any new features and tweak the regexes so that they don't require huge Unicode tables. There are a couple ways of doing that, but here's one:

diff --git a/tracing-subscriber/src/filter/env/directive.rs b/tracing-subscriber/src/filter/env/directive.rs
index f0ccbd7e..1d332d32 100644
--- a/tracing-subscriber/src/filter/env/directive.rs
+++ b/tracing-subscriber/src/filter/env/directive.rs
@@ -122,15 +122,15 @@ impl Directive {
     pub(super) fn parse(from: &str, regex: bool) -> Result<Self, ParseError> {
         static DIRECTIVE_RE: Lazy<Regex> = Lazy::new(|| Regex::new(
             r"(?x)
-            ^(?P<global_level>(?i:trace|debug|info|warn|error|off|[0-5]))$ |
+            ^(?P<global_level>(?i-u: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>\[[^\]]*\])
+                (?P<target>[[:word:]:-]+)|(?P<span>\[[^\]]*\])
             ){1,2}
             (?: # level or nothing
-                =(?P<level>(?i:trace|debug|info|warn|error|off|[0-5]))?
+                =(?P<level>(?i-u:trace|debug|info|warn|error|off|[0-5]))?
                     #          ^^^.
                     #              `note: we match log level names case-insensitively
             )?
@@ -151,7 +151,7 @@ impl Directive {
                     (?:=[^,]+)?
                 )
                 # trailing comma or EOS
-                (?:,\s?|$)
+                (?:,[[:space:]]?|$)
             "#).unwrap());
 
         let caps = DIRECTIVE_RE.captures(from).ok_or_else(ParseError::new)?;

This patch results in all tests passing locally for me without adding any new regex crate features.

Do note though that this does subtly change the semantics of what the regex matches. My intuition says this is probably fine here, but that's up to y'all to decide. :-)

[dbidwell94]

The force push above is the 3 commits squashed into one

[davidbarsky]

The force push above is the 3 commits squashed into one

you don't need to force-push; we squash on merge.

(I'm running CI and looking at this PR now.)

[dbidwell94]

Yeah those tests were failing on my end as well. Not sure why, untouched by me.

[davidbarsky]

Do note though that this does subtly change the semantics of what the regex matches. My intuition says this is probably fine here, but that's up to y'all to decide. :-)

@BurntSushi Out of curiosity, what is the subtle change? It's disabling unicode case folding?

[davidbarsky]

Yeah those tests were failing on my end as well. Not sure why, untouched by me.

I'm not entirely sure why the UI tests started failing all. Digging into it.

[BurntSushi]

@davidbarsky The change is that your regex matches fewer strings than it did before. (?i:trace|debug|info|warn|error|off) and (?i-u:trace|debug|info|warn|error|off) actually happen to match the same things. But it depends. For example, (?i:s) is equivalent to [Ssſ], but (?i-u:s) is equivalent to [Ss]. Notice the former includes ſ (U+017F).

So you're fine there. But \s is Unicode aware and [[:space:]] is not. Similarly for \w. So \w will match β but [[:word:]] (or equivalently, (?-u:\w)) will not match β. \s also contains some non-ASCII Unicode codepoints for whitespace such as U+2000.

So if you specifically wanted \w to match a whole bunch of stuff other than [0-9A-Za-z_], then yeah, you should keep it and enable the appropriate Unicode features to bring in the requisite Unicode tables.

The sad thing is that you'll still have a dependency on matchers, which in turn depends on an older version of regex-syntax. So you're already bringing in a whole bunch of Unicode tables that you probably aren't using.

[davidbarsky]

@davidbarsky The change is that your regex matches fewer strings than it did before. (?i:trace|debug|info|warn|error|off) and (?i-u:trace|debug|info|warn|error|off) actually happen to match the same things. But it depends. For example, (?i:s) is equivalent to [Ssſ], but (?i-u:s) is equivalent to [Ss]. Notice the former includes ſ (U+017F).

So you're fine there. But \s is Unicode aware and [[:space:]] is not. Similarly for \w. So \w will match β but [[:word:]] (or equivalently, (?-u:\w)) will not match β. \s also contains some non-ASCII Unicode codepoints for whitespace such as U+2000.

So if you specifically wanted \w to match a whole bunch of stuff other than [0-9A-Za-z_], then yeah, you should keep it and enable the appropriate Unicode features to bring in the requisite Unicode tables.

The sad thing is that you'll still have a dependency on matchers, which in turn depends on an older version of regex-syntax. So you're already bringing in a whole bunch of Unicode tables that you probably aren't using.

Gotcha, thanks for explaining this! I'm not especially good at understanding regexes 😅

[BurntSushi]

Aye. If it's any consolation this is less about regexes and more about the intersection of regexes and Unicode. It's a difficult beast to tame!

[dbidwell94]

So it sounds like it might actually be best to enable the features instead of refactor the original regexes?

[BurntSushi]

(@dbidwell94 That question I can't answer. I just wanted to put the tweaked regex out there as an alternative. It's up to y'all which behavior you want.)

[mati865]

From a user perspective I doubt Unicode even makes sense here. In Rust you cannot name functions with non-ASCII characters so there should be no real-world use case where one wants to enable specific log level for some weird path.

[davidbarsky]

We'll end up enabling the feature for now and refactoring the regex later.

[hawkw]

From a user perspective I doubt Unicode even makes sense here. In Rust you cannot name functions with non-ASCII characters so there should be no real-world use case where one wants to enable specific log level for some weird path.

Also, the case-insensitivity is being used for the part of the regex which parses the name of the log level, rather than the module path. The log levels themselves are the strings "trace", "debug", "info", "warn", "error", and "off", which should always be ASCII...

[hawkw]

Let's get a release out with this change, and then revisit refactoring the regex later!

[dbidwell94]

@hawkw Not quite sure why those tests are failing. @davidbarsky said he was going to look into it.

[hawkw]

@hawkw Not quite sure why those tests are failing. @davidbarsky said he was going to look into it.

These are UI tests for proc-macro error output. My guess is that they are failing because the compiler's error output changed somewhat on the latest stable version. @davidbarsky, we can probably fix these by running the UI tests with TRYBUILD=overwrite, to update the expected output to match the current stable compiler's output, and then committing the results.

[dbidwell94]

@hawkw Worked for me locally. Committing and pushing now.

[dbidwell94]

PR which fixes the UI tests (will need to be force merged as the build will fail due to missing regex features)
#2568