trivial_regex false positive and/or incorrect hint for replace?

[emk]

I love the new lints! But I found what seems to be a false positive:

For the following code:

    pub fn from_bigml<S: Into<String>>(name: S) -> Result<FieldName> {
        // Again, we have a fair bit of internal knowledge about what kinds of
        // field names can actually exist. Don't hesitate to add new types.
        lazy_static! {
            static ref PRIVATE_RE: Regex = Regex::new("^private_").unwrap();
        }
        let name = name.into();
        Self::from_unescaped(PRIVATE_RE.replace(&name, "private/"))
    }

...the trivial_regex lint suggests:

warning: trivial regex
  --> src/types/field_name.rs:80:55
   |
80 |             static ref PRIVATE_RE: Regex = Regex::new("^private_").unwrap();
   |                                                       ^^^^^^^^^^^
   |
   = note: #[warn(trivial_regex)] on by default
   = help: consider using consider using `str::starts_with`
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#trivial_regex

Not all regular expressions are used for matches, and I'm not aware of anything in std that implements "replace this string only at the beginning of another string."

[oli-obk]

hmm... I think the lint itself is correct.

At the minimum we should fix consider using consider using 😆

We can check all use sites of the trival regex and produce suggestions for them.

In this case I'd probably write

let name = name.into();
if name.starts_with("private_") { name.replacen("private_", "private/", 1) } else { name }

[emk]

name.replacen has a couple of drawbacks relative to Regex::replace:

• You need to either duplicate the string private_ (risking bugs if something changes) or pull it out into a let binding.
• Regex::replace returns a Cow::Owned if no replacement occurs (IIRC), which makes it easier to avoid extra memory allocations.

let private_prefix = "private_";
let replaced = if name.starts_with(private_prefix) {
    Cow::Owned(name.replacen(private_prefix, "private/", 1))
} else {
    Cow::Borrowed(name)
};

At this point, I think there's a strong argument to be made for using PRIVATE_RE.replace(&name, "private/"), even if the only regex feature used is ^. This would be even more compelling if regex!("^private_").replace(&name, "private/") worked on stable again. :-/

I think that, ideally, trivial_regex should only trigger when there's a simple, elegant replacement in the standard library, and not for cases where the replacement code is significantly more complex. And I agree that if it's going to suggest a replacement, it shouldn't automatically assume I'm calling is_match.

[oli-obk]

The regexes are much more succinct. I can totally see why they are neat.

Maybe we shouldn't be linting on regex creation, but on the use sites! That would totally make more sense, and if all use sites are gone, then rustc will tell you your regex is unused.

[emk]

Yeah, I do agree that clippy is right to recommend that regex!("^private_").is_match(&name) should be replaced by name.starts_with("private/"). But as for the more complicated cases (including replace), I think that the regex crate offers real advantages over anything in the standard library, and that clippy should be less aggressive by default.

Linting on regex sites would definitely be one good approach, if it's not too complex. The basic idea of this lint is very useful. But I do feel that my original example is arguably a false positive, and that in case, the recommendation to use is_match clearly incorrect. Basically, I think it would be fine to change the wiki to list this as a possible limitation.