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.

    #[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: #3793 (comment)

Thank you for this investigation, @dtolnay ❤️

[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]

👍 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
}

[fogti]

@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

[webern]

I have an HTTP client trait/object where set_foo sends a PUT/POST and self is immutable. I will probably disable wrong_self_convention for this. I guess I could use send_* or put_*, but set_* seems most natural here.

[dtolnay]

Closing in favor of #7666, which reproduces a similar table using all published crates on crates.io as the data source, rather than repos indexed by grep.app. The code is available in https://github.com/dtolnay/noisy-clippy.