Add an easy way to run clippy with high FPR / trivial lints disabled

[matklad]

Hi!

It often happens that users submit a "make clippy happier" PR to one of my projects. These PRs often contain quite a bunch of changes, of which a couple are undeniably great, but majority just shuffle code around. An example of good change would be a removal of (now useless) .into() or .clone(). A typical example of not really good change is .unwrap_or(xs.len()) -> .unwrap_or_else(|| xs.len()).

I've written at some length about this here: rust-analyzer/rowan#57 (comment).

I would really love the ability to run cargo clippy --conservative, and only get lints about the code which can be unambiguously improved across all dimensions. (that is, those lints that we probably want to lift eventually into the compiler).

I know that lint categories exists, but:

• they seem to slice the lints across the different axis -- not the FPR range, but the, well, category
• it's unclear how to actually make clippy run only specific category from the command line.

[flip1995]

Implementation wise, this would be really easy to do: Add a lint group conservative and a convenience argument --conservative that will execute cargo clippy -- -Aclippy::all -Wclippy::conservative.

The hard/tedious thing would be to build this group. Someone would have to handpick the lints for this group. I would suggest some heuristic for this like auto-applicability, number of open issues with FPs, ...

[matthiaskrgr]

I feel like the need of a conservative lint group already tells that something is wrong with the choices of the default lints... :/

I think ideally, all the changes suggested by default should make sense in ~95% of the case, warnings that can be argued about should be moved to style (perhaps we could have several style-groups, if there is a need?) or pedantic and lints that have too many false positives should be moved (back to?) the nursery group.

[flip1995]

After sleeping about this, I'm also against introducing a conservative group.

TL;DR: This would involve too much bikeshedding, and personal opinion which lints should be included.

I feel like the need of a conservative lint group already tells that something is wrong with the choices of the default lints...

For this dtolnay opened #5418, where he already moved multiple lints.

I think ideally, all the changes suggested by default should make sense in ~95% of the case

After reading @matklad comment on the rust-analyzer/rowan PR, it is not about that the suggestion doesn't make sense, but that the suggested code doesn't improve something directly, so the change is in his opinion a bigger downside than having stylistic slightly worse code. (That's how I understood his comment, expressed in a very condensed way).

As I commented in rust-analyzer/rowan#57 (comment), I think that for lints that doesn't directly improve code, the lint groups style and complexity exist and disabling those should already lead to only lints that directly improve the code and, therefore, spending time on making a change should be worthwhile.

Now disabling style and complexity might also disable lints you want to have though. In @matklad case specifically the identity_conversion lint. But in the case of identity_conversion this also does not improve the code directly, except that it removes 7 characters.

And now we're at the bikeshedding point. Why should identity_conversion be in the default/conservative list of lints, but for example needless_lifetimes not? Both lints remove (at least) 7 characters from the code, while performance or correctness wise don't have an impact at all.

This is where I think the existing groups give a really good classification. You want Clippy to get you to write more correct code, but don't want to change code style? Allow clippy::style. You really want to enforce that one style lint in your code base, but don't care about the rest? Warn on clippy::style_lint_name.

---

What I could see as an improvement would be to subdivide the pedantic group more. So we have a clippy::pedantic group similar to the clippy::all group, but also have pedantic_style/pedantic_complexity/... . This would enable us to move more lints to pedantic, but then recommend to enable e.g. pedantic_complexity in most projects. Downside would be, that lints for beginner will probably be moved to pedantic_* and are less discover-able for them.

[matklad]

Why should identity_conversion be in the default/conservative list of lints, but for example needless_lifetimes not?

I feel they both should be in the conservative set of lints, as they both strictly improve the code along more dimensions.

I don't mind needless_lifetimes. I mind the other examples and I don't think that needless_lifetimes is important enough to justify the cost of silencing other lints. --concervative would allow me to reap (small, but non-zero) benefit of needless_lifetimes without paying the cost of other noisier lints.

[flip1995]

I would rather move lints to allow-by-default, if they are too noisy, than introducing a new subgroup of clippy::all. If you have specific lints in mind, that you think should be downgraded, please leave them in #5418.

The philosophy of Clippy is (from the README.md) to be annoying helpful and sprinkling of allows is encouraged. We're happy to downgrade lints, if they are too annoying/noisy. In the past months I was careful to only allow new warn-by-default lints, if they are an obvious code improvement. But this was always my own judgement (+ the contributor's) and is only some initial classification, not backed by any analysis.

[matklad]

The philosophy of Clippy is (from the README.md) to be annoying helpful and sprinkling of allows is encouraged.

Yup, this a totally fine philosophy, and I think a good default.

However for me personally, explicitly allowing lints and/or explicitely configuring linter is a big red flag, in the context of Rust, where compiler already handles most important lints. So clippy's philosophy and my philosophy are directly in conflict. At the moment, I solve this by not using clippy. I see --conservative as way to make both approaches coexist.

[camsteffen]

they seem to slice the lints across the different axis -- not the FPR range, but the, well, category

This reminds me of a comment I made.

I think there are 3 orthogonal attributes at play:

1. Is this about code style, performance or is there risk of a logic error (correctness)? (complexity seems like a sort of intersection of style and performance)
2. How much? For style, how picky? For performance, how much impact? For correctness, how much risk?
3. How confidant is the lint? (i.e. could it be a false positive?)

The lint categories are an effective solution for allowing the user to configure what they want to be linted. What we are missing is a way to configure how confident clippy must be when it lints. To solve this, each lint would need to be assigned a confidence level in addition to its category. Probably just a boolean that means "I might be a false positive".

For example, this flag would make sense for or_fun_call. The lint could detect something with a large performance impact, but it also could be a false positive (unwrap_or(x.len(), ..)). So I think it makes sense for that lint to warn-by-default, but not "conservative".

However for me personally, explicitly allowing lints and/or explicitely configuring linter is a big red flag

@matklad I assume you are talking about line-level configuration. You would be okay with configuring clippy at the project level, perhaps disabling a handful of lints, right? I suspect there will be some cases where the lint is confident, but you just don't care for the stylistic preference being asserted.

[matklad]

I’d be semi fine. I probably wouldn’t add a clippy config (even if it is small) to every project, but I can imagine this working for Rust-analyzer, if that’s easy to do, and if only a handful of lints needs to be disabled.

We actually have something like this in ra already: https://github.com/rust-analyzer/rust-analyzer/blob/20a911f3cc2beb0409ab71cc1560648374745f7f/xtask/src/lib.rs#L73

we don’t run this code on CI though, as it never was tweaked to silence all false positives.

[lolbinarycat]

To solve this, each lint would need to be assigned a confidence level in addition to its category

isn't this just applicability?

[flip1995]

Not quite. Applicability is for the suggestion, not the lint triggering. But we could also use it as a heuristic for a confidence level. But the issue here is mostly that applicability is not fine grained enough. There's a rustc issue about improving this. I.e. MaybeIncorrect can mean multiple things:

1. The suggestion/lint itself is incorrect
2. Applying the suggestion might trigger another lint/error (i.e. missing use Trait;), but in itself it is correct
3. applying the suggestion might break the code (e.g. because of macro expansions/shenanigans)

If we could separate this with applicability we could just filter out the 1. variant.