Rust-clippy: Most commonly ignored lints

[dtolnay]

I collected some data from https://grep.app on which Clippy lints are needing to be suppressed most commonly in the wild. This is based on number of results for this regex:

\[allow\(.*clippy::LINT_NAME\b

which is an approximation but useful. Example query

Clippy's most severe flaw in my experience has been low-signal lints that are enabled by default, aren't worth resolving and commonly need to be suppressed. Hopefully this gives some data to support deleting or downgrading many of those lints to pedantic opt-in lints.

In the table below, I would recommend paying attention to the style and perf lints. On style, highly suppressed style lints indicate that the community has consciously decided that Clippy's opinion on style is wrong. On perf, highly suppressed lints indicate that the community does not consider it valuable to make their code more obtuse for the sake of questionable alleged performance. I think it would be wise to delete or downgrade many of these.

Here is the distribution by lint of how many times lints are suppressed. As expected, most lints are rarely suppressed, but there are a dozen or so lints that are very commonly suppressed.

And here are the top most commonly ignored lints. I'll cross off lints as I delete or downgrade them.

ignored | lint name | category
--- | --- | ---
~432~ | ~cognitive_complexity~ | ~complexity~
421 | too_many_arguments | complexity
312 | type_complexity | complexity
259 | large_enum_variant | perf
~201~ | ~unreadable_literal~ | ~style~
~193~ | ~new_ret_no_self~ | ~style~
184 | cast_ptr_alignment | correctness
~178~ | ~trivially_copy_pass_by_ref~ | ~perf~
148 | new_without_default | style
~141~ | ~needless_pass_by_value~ | ~style~
128 | float_cmp | correctness
125 | many_single_char_names | style
102 | redundant_closure | style
85 | identity_op | complexity
84 | should_implement_trait | style
83 | module_inception | style
74 | wrong_self_convention | style
65 | needless_range_loop | style
63 | len_without_is_empty | style
63 | mutex_atomic | perf
59 | missing_safety_doc | style
57 | needless_doctest_main | style
56 | ptr_arg | style
51 | let_and_return | style
50 | mut_from_ref | correctness
47 | single_match | style
46 | unnecessary_operation | complexity
45 | enum_variant_names | style
~38~ | ~let_unit_value~ | ~style~
38 | suspicious_arithmetic_impl | correctness
37 | never_loop | correctness
37 | unit_arg | complexity
36 | derive_hash_xor_eq | correctness
36 | needless_return | style
36 | no_effect | complexity
~36~ | ~option_option~ | ~complexity~
34 | match_ref_pats | style
32 | if_same_then_else | correctness
32 | redundant_clone | perf
31 | collapsible_if | style
~31~ | ~implicit_hasher~ | ~style~
31 | needless_lifetimes | complexity
~28~ | ~identity_conversion~ | ~complexity~
26 | blacklisted_name | style
26 | borrowed_box | complexity
25 | eq_op | correctness
~25~ | ~useless_let_if_seq~ | ~style~
24 | excessive_precision | style
23 | comparison_chain | style
22 | map_entry | perf
~22~ | ~match_wild_err_arm~ | ~style~
22 | transmute_ptr_to_ptr | complexity
21 | redundant_field_names | style
20 | inconsistent_digit_grouping | style
~20~ | ~match_bool~ | ~style~
20 | not_unsafe_ptr_arg_deref | correctness

[matthiaskrgr]

This is very interesting, thank you!

I'm wondering how skewed the data might be since the regex does not differentiate between disabling a lint on crate level #![allow(clippy::useless_format)] a single time
or having it enabled globally and suppressing five false positives manually.
rust #[allow(clippy::useless_format)] let _ = format!("{}", "a");

We might also want to look the same way at #[allow()]ed lints to check if there are allowed-by-default lints that people are interested in. :)

[matthiaskrgr]

I had a look at the top 10 items and looked at suppressing warnings on single items [^\!]\[allow\(.*clippy::cognitive_complexity\b vs the entire file: \!\[allow\(.*clippy::cognitive_complexity\b :

| lint | allow on file | allow on item |
| --- | --- | --- |
cognitive_complexity | 94 | 352
too_many_arguments | 64 | 362
type_complexity | 84 | 244
large_enum_variant | 27 | 356
unreadable_literal | 117| 95
new_ret_no_self | 70 | 125
cast_ptr_alignment | 33 | 153
trivially_copy_pass_by_ref | 55 | 124
new_without_default | 84 | 63
needless_pass_by_value | 26 | 118

[felix91gr]

This makes a lot of sense to me. I still think we should deprecate the Cognitive Complexity lint, if not moving it to "Pedantic". I was the last one to touch it, and tried re-implementing it only to find out while researching it, the reality of our current understanding of Cognitive Complexity: we're not good enough yet at analyzing code for understandability. To see the discussion that was had on the topic, you can go to the issue itself and start here: https://github.com/rust-lang/rust-clippy/issues/3793#issuecomment-542457771

Thank you for this investigation, @dtolnay :heart:

[flip1995]

Yeah, we should move cognitive complexity to nursery I think.

[djc]

Given that new_without_default is enabled more at the file level than on the item level, that also seems like a strong indicator that people don't value it much.

[dtolnay]

:+1: anecdotally I haven't found new_without_default to be valuable. It's a good observation that new_without_default is one of the two in the top ten for which globally disabling is more common than one-off exceptions, the other being unreadable_literal which we know is controversial. I agree that we can take this to mean people don't find it valuable.

[CensoredUsername]

I'm not very surprised to see both cognitive_complexity and too_many_arguments right next too each other at the top. In my experience those two tend to interact badly.

If you have a problem which just involves a lot of complex logic with a lot of edge cases, it can become hard to factor it out into multiple functions that do not just trigger too_many_arguments itself. And wrapping those args in a struct to reduce the too_many_arguments warning then just causes more clutter in the original function making the whole thing more complex than it already was (unless you can reuse that struct multiple times but complex logic and borrowing rules tend to inhibit that).

[ehuss]

I did something similar in November. I counted each level (forbid/deny/warn/allow) of all lints, counting each lint only once per crate across crates.io. You can filter on clippy:: for clippy-only.
https://gist.github.com/ehuss/f7cdee6b622ff4a219cdebb3645ad198

[Manishearth]

I think seeing a difference between global and local disables is useful here. Clippy is meant to be used with a liberal sprinkling of allow, both to pick your personal style, and to deal with cases where you actually need to write code the way clippy doesn't want you to. If a lot of folks are globally disabling style lints that's a good indicator that Clippy style has diverged from the community. Local disables are harder to grok, because to some extent they can be a signal. I agree cognitive complexity should be off by default, but it's a good example here: a legit way of using it is to get an early warning when your functions get complicated, and turn it off whenever you feel like they have to be that way. It's still valuable as a signal in this case!

[llogiq]

For large_enum_variant, would it help to compare not the smallest and largest variant, but the largest and second largest one instead?

We also probably should document that this lint can by definition not look at the actual variant distribution in your program execution, so it may be that the smallest variant is only used in a very small percentage of cases, in which case boxing the largest variant would likely not bring much benefit.

[felix91gr]

(...) so it may be that the smallest variant is only used in a very small percentage of cases, in which case boxing the largest variant would likely not bring much benefit.

Oh, good point. You reminded me of Huffman encoding, where you pick the most probable variants and encode them with less bits, so that in average you get the best performance. In many cases, the large enum variant might be a result of applying the same idea to the particular usage of that enum. That seems very interesting. Also smart.

[dpc]

I did disable complexity ones many times myself. Usually there's just no way to fix these. Some complexity is just essential, and shuffling it around only obscures it. Though I usually disable it on case-by-case basis, not globally. I don't really mind it on it's own. It's a a decent annotation that "yes, the authors realize that this is complexgly and have reasons to keep it".

[Manishearth]

Clippy is meant to be used with a liberal sprinkling of allow,

Oh, another good example of this is lints that want you to make a choice, where one of the choice is to allow() -- the rustc missing_copy_implementations lint is like this, sometimes you don't want to commit to it being Copy, but in that case that allow() actually helps make it clearer that that choice was made.

In general I'd be wary of viewing allow()s as an intrinsically bad thing, however this list is still super useful as a starting point, and stuff at the very top probably should be flipped

[joshtriplett]

Please recheck the PR thread on new_ret_no_self: I don't think that one should have been merged.

[pickfire]

For large_enum_variant, would it help to compare not the smallest and largest variant, but the largest and second largest one instead?

What if the second largest one is as large as the largest but the third largest deviates a lot? For example, 8000, 8000, 64, 64, 64? Shouldn't it look into standard deviation or variance in this sort of case?

[llogiq]

What if the second largest one is as large as the largest but the third largest deviates a lot?

I also thought about that, but for now decided against it as a) this would make the lint much more complex and b) as I wrote in the Problems section, we cannot know the distribution of variants in running code, so it seems best to be conservative for now.

[llogiq]

Regarding missing_safety_doc, I think most #[allow]s are due to the fact that the lint triggers on non-exported functions.

[llogiq]

Which is really strange, because the lint has the following code:

if !cx.access_levels.is_exported(hir_id) {
    return; // Private functions do not require doc comment
}

[zserik]

@llogiq Maybe that code was added later... or does the lint not ignore #[doc(hidden)] functions?

[flip1995]

Which is really strange, because the lint has the following code:

It doesn't lint private functions. Also #[doc(hidden)] functions won't get linted. I think that was a bug in an early version of the lint.

Playground