Missing lint categories?

[llogiq]

With my recent return to reviewing clippy PRs, I found that it is often hard to categorize lints.

For example, #6086 would warn about something that is likely incorrect, but doesn't warrant a correctness lint, because it is just a heuristic and could have false positives. For now, we chose style, but that seems wrong. Perhaps consistency?

[flip1995]

I want to write a MCP about this for a long time now. My plan is basically to introduce more pedantic groups, e.g. pedantic_correctness, where we can put correctness lints, that might have FPs, we can't really avoid (this is a good example for this).

The problem right now is, that when we put lints in an allow-by-default category, discovering those lints is kind of hard. Enabling the complete pedantic group is not really feasible for most projects and going through the whole pedantic list to find lints is really annoying.

If we had more allow by default lints, we would solve 3 problems/annoyances at once:

1. If a lint has to many FPs and we would currently move it to nursery (where it gets lost forever, usually), we could consider moving it to the equivalent pedantic_* group.
2. We can better guarantee, that our warn/deny-by-default set of lints is (nearly) FPs free (as far as we know, that is). Production lint category #5755
3. Enabling only a (recommended) subgroup of pedantic would be feasible. We could recommend to enable pedantic_correctness and pedantic_perf, to get most out of Clippy, but FPs should be expected out of these groups.

The lint in #6086 would then go in pedantic_correctness.

The downside of this is, that lints like #6086 may warrant to be at least warn-by-default, but would then be allow instead.

[llogiq]

I do want this to be warn by default. It's about suspicious code. Really suspicious. If that code was in Among Us, it'd be ejected immediately, it's so suspect. Still, there's nothing explicitly wrong, so deny by default / correctness is not what we want here. Also people will likely agree that even if there are false positives, those look fishy enough to warrant an explanation.

Furthermore, I think we have enough allow-by-default categories. I'd rather have a process to get lints out of nursery, but again, many lints lack a suitable category even if we can somehow get the false positive rate under control.

[flip1995]

I do want this to be warn by default. It's about suspicious code. Really suspicious. If that code was in Among Us, it'd be ejected immediately, it's so suspect.

So a clippy::sus group? 😄

A "warn-by-default correctness group" came up now or then in the past. We could add such a group. The question is about naming.

Furthermore, I think we have enough allow-by-default categories.

I disagree. We have 3 groups, where one is basically for broken/unusable lints (nursery, 17 lints), and one for lints that are only useful for really specific use cases (restriction, 44 lints). And then there is pedantic, which is just a huge pile of allow-by-default lints (77 lints = 18.4% of all lints), that address completely different issues.

I'd rather have a process to get lints out of nursery

The process would be, that someone needs to fix the issues with those lints. And if a lint is in nursery, there are usually many issues.

[llogiq]

clippy::sus is genius! I want it!

And I fully agree with you on nursery. I still think that allow-by-default lints are probably underused, so there's less value in adding more categories there (though I may reconsider if someone shows me a suitable category) than in filling holes in our warnings categories.

[flip1995]

clippy::sus is genius! I want it!

Hmm... @rust-lang/clippy Do you have ideas how to name a warn-by-default correctness group? (Or any objections, why we shouldn't introduce such a group?)

---

so there's less value in adding more categories there (...) than in filling holes in our warnings categories.

I mean, those are not mutably exclusive. I don't want to introduce completely new allow-by-default categories, but use the categories we already have to bring structure to the pedantic group.

[ebroto]

Do you have ideas how to name a warn-by-default correctness group?

I've always liked suspicious, but sus would be OK I guess, although I'm not sure if that will age well :) Another possibility inspired by clang-tidy would be bugprone, although it might not be a good fit for all cases. dubious is another option.

I think we really want that warn-by-default category, regardless of the name, as the need for it has popped up multiple times.

---

IMO splitting pedantic into different new categories is a great idea! I'm not thrilled about the naming scheme though, I think I would prefer finding more specific names for them, where the fact that they are allow-by-default is not expressed in the name necessarily. For example security, safety, readability, concurrency, ...

About the FP rate, I think it's fine if a currently pedantic level has some FPs, but lately I've come to think that we are maybe underusing the nursery. FP can be two kinds of issues:

1. Some cases were not considered, but fixing those is possible. Many of these => nursery
2. Some stuff is incredibly hard or impossible to check in Clippy => maybe we shouldn't be linting.

My point is that it would be great if warn/deny-by-default lints were almost FP free, and lints in the allow-by-default categories we are defining here had a couple FPs at most.

[ebroto]

To be more clear about my last point: we should be more strict about FP rate, pedantic should not be about FP rate, that's the nursery's job.

[llogiq]

@ebroto agreed, but let's be clear here that we will allow some FPs if and only if those are only triggered in a niche inhabited by experts who know when to #[allow(..)] and it's unduly hard to rule them out and the majority of users reaps sufficient benefits to tip the scale.

Otherwise we'd run the risk of making those lints allow by default and only the few who bother to activate them will benefit.

[Manishearth]

Hmm... @rust-lang/clippy Do you have ideas how to name a warn-by-default correctness group? (Or any objections, why we shouldn't introduce such a group?)

"suspicious" seems okay to me actually. Alternatively we can have the correctness group be a mix of levels, there's nothing wrong with that i think.

[llogiq]

I think keeping the lint groups distinct makes things easier for the user. Especially when configuring lint levels per group.

[matthiaskrgr]

I also think that a suspicious lint group makes a ton of sense.
There are probably a bunch of cases where clippy wants to express something, "Hey, please have a look here, this might be a bug but please don't blame me if I'm wrong because static analysis can only know so much...".

[flip1995]

I'm also good with suspicious. So should we go ahead and implement this?

[camsteffen]

Perhaps name the group suspect as in "this code is suspect".

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?)

pedantic is high in pickiness and/or low in confidence. suspicious is high in risk but moderate in confidence. Etc.

[matthiaskrgr]

I wonder if we should have some kind of "simplification" group.
Currently we have the style group which is very generic and has a lot of lints a la "this pattern could be used instead of that pattern" and a lot of "this pattern is bad" lints.

We could have a simplification lint group that only has "replace long pattern with shorter pattern" lints.
For example op_ref where we suggest to change &x == y to x == *y would remain in the style group but something like redundant_field_names which shortens S { a: a, b: b } to S {a, b} would (also?) be part of the simplification group.

[kornelski]

+1 for suspicious category.

I'm reviewing code of Cargo creates and looking for potential security issues, like backdoors and malicious code. I've realized that there are several ways to obfuscate Rust code to mislead code reviewers, so I'd like to flag such suspicious code for extra scrutiny. I like @llogiq description that "If that code was in Among Us, it'd be ejected immediately".

I've started writing my own parser to detect these, but soon realized I'm just reinventing Clippy's internals.

macro_rules! totally_innocent_debug {
  ($e:expr) => unsafe { $e }
}

use std::mem::transmute as into;

#[no_mangle]
fn fopen() {}

include_str!("/etc/passwd")

fn never_mind_me() {
   #[path="../../../cargo/registry/other-crate/hidden.rs"]
   mod sneaky;
}