lint single byte char regex count

[llogiq]

Something like Regex::new("x").find_iter().count(), suggest to use bytecount instead, which is much faster.

[clarfonthey]

Counterexample: Regex::new("あ")

Definitely do this if it's only one byte. We could lint for multi-byte characters if bytecount allowed u32s though.

[clarfonthey]

Note that this also includes str::matches and others.

[KasMA1990]

I just saw now that this lint was included in clippy, but I couldn't find any discussion surrounding its inclusion.

While I like the idea of suggesting performance improvements to users like this, I'm a bit cautious about the precedent this might be setting. Do we want to "advertise" third party crates in clippy? What if somebody makes something that's faster than bytecount? Is bytecount the fastest code on all processors? Do we not want users to measure their performance to avoid premature optimizations? Is bytecount documented well enough to be suggested like this (looking at the docs, I don't know why I should prefer naive_count over naive_count_32 or indeed count)? If these questions are answered elsewhere, then I'd love a link or a recap :)

Now, I know bytecount is quite a benign case here; it's owned by a Rust mainstay, it's likely to remain updated, it is undoubtedly very fast, and the code doesn't suffer any noticable hit to clarity for introducing bytecount for these solutions. But I'm still not a fan of including this in clippy, since clippy is for static analysis, when this kind of lint goes way beyond static analysis; you should do performance measurements, you should consider what form your data has, etc.
Maybe this should be part of a different tool in the ideal world? A tool that could suggest alternatives to what you're doing maybe. "We have detected you're using hash maps, have you considered trying ordermap?"

I know such a tool doesn't exist (and I don't expect anyone here to go build it), but I'd like to consider the implications of introducing a lint like this at least :)

[llogiq]

As the 'rust mainstay' in question, I've thought long and hard whether to a) include this lint in clippy and b) make it warn by default. My reasoning is as follows:

• I want Rust code to be the fastest possible. We should all benefit from this. However, I don't want to search through all of crates.io – I'd rather let clippy do that job.
• generally people don't expect that counting bytes can be done faster. So they won't look at bytecount by themselves.
• bytecount really is faster on all CPUs I've tested it with (and much faster on x86_64 CPUs that have SSE4.2). I've added it in multiple projects with very satisfactory results.
• If you have a strange CPU where that isn't the case, you are likely on an embedded environment and know better anyway, so you'll just slap an #![allow(bytecount)] on your code & go your merry way.
• the dependency is quite small and has no further sub-dependency. So the cost should be acceptable to most projects.

[KasMA1990]

• I want Rust code to be the fastest possible. We should all benefit from this. However, I don't want to search through all of crates.io – I'd rather let clippy do that job.
• generally people don't expect that counting bytes can be done faster. So they won't look at bytecount by themselves.
• bytecount really is faster on all CPUs I've tested it with (and much faster on x86_64 CPUs that have SSE4.2). I've added it in multiple projects with very satisfactory results.

Which are excellent reasons for including this lint!

• If you have a strange CPU where that isn't the case, you are likely on an embedded environment and know better anyway, so you'll just slap an #![allow(bytecount)] on your code & go your merry way.

Well, since bytecount is no_std and Rust is all about lowering the barrier of entry to systems programming, I'm not sure prospective programmers will know better. Though it's probably not a big deal for this.

• the dependency is quite small and has no further sub-dependency. So the cost should be acceptable to most projects.

That is very good :)

---

Just to make it clear, I think making crates like bytecount obvious to users is a big win for us all; as you say, many people will not even know this is something worth looking for, and the performance boost is quite real.
But I'm curious what the next step of this is; should we start adding more suggestions for crates that solve a given problem in a more performant or succinct fashion? Surely there must be a bunch of crates that can be advertised like this. E.g. we could detect if people are defining their own Either and point them at https://crates.io/crates/either and so on.

Now, maybe this is totally fine and it actually belongs in clippy. But it's also a move towards 'blessing' certain crates by warning people that they're not using them. Which feels to me like something that is quite distinct from what clippy should be doing.

[llogiq]

Clippy is as much a testbed for lint development as it is the go-to project for extended static analysis in Rust. This is by design – unless a lint crashes, we'll always have room for it, and allow by default if necessary.

[clarfonthey]

Also I should add that various crates are being "blessed" by the Rust project in the Rust cookbook, and I think that anything that meets the necessary guidelines and gets included there is worthwhile for inclusion as a lint.

[Manishearth]

since clippy is for static analysis, when this kind of lint goes way beyond static analysis;

I don't understand this point. You seem to mean something different by "static analysis"; what clippy does is use static analysis to suggest improvements. This lint is static analysis, followed by a suggestion for an improvement which advertises a third party crate.

I'm fine with blessing crates in clippy provided we are confident they will continue to exist and continue to be good. And if they ever stop doing so we can un-bless them relatively easily.

[KasMA1990]

@Manishearth I agree that this is static analysis; my point is that performance is something where we usually say you should measure before you make changes (premature optimization and all), and we're skipping that step with this lint. I realize there are other lints that look for inefficient code, but given that this lint has a known problem with being slower in some cases, it feels like a dynamic component is missing.

Otherwise the point is taken; I'm just happy to have the discussion and know it's a conscious decision that's made :)

[Manishearth]

my point is that performance is something where we usually say you should measure before you make changes

In some cases (like this one) it's always a clear cut win.

Whether or not that perf improvement actually matters in the big picture is a different question. But this leads to both cleaner code and potentially better perf, and should be a lint just for the "cleaner code" part.

[Manishearth]

this lint has a known problem with being slower in some cases

It doesn't? It has the problem of being wrong in some cases because the bytecount API doesn't allow for multibyte characters so the suggestion can't be applied.

That's something you exclude whilst linting. This is standard procedure for all lints.

[KasMA1990]

The lint says it has a known problem where using the count method can be slower, so you have to measure to find out whether you should use it or one of the other methods. That seems like an issue to me.

[Manishearth]

Oh, you're talking about naive_bytecount, my bad. This regex one is more clear cut to me afaict.

Yeah that known problem is an issue.

[Manishearth]

In general if a lint will be correct 90% of the time we still include it (and sometimes are more explicit about the false positive in the lint message). This (naive_bytecount, not regex_bytecount) seems like one of those cases, the lint could mention a threshold and let folks make the decision.

Or the bytecount crate could fallback to naive for small slices.