Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Most commonly ignored lints #5418

Closed
dtolnay opened this issue Apr 4, 2020 · 22 comments
Closed

Most commonly ignored lints #5418

dtolnay opened this issue Apr 4, 2020 · 22 comments
Labels
A-category Area: Categorization of lints C-tracking-issue Category: Tracking Issue S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@dtolnay
Copy link
Member

dtolnay commented Apr 4, 2020

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
@flip1995 flip1995 added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Apr 4, 2020
@matthiaskrgr
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

felix91gr commented Apr 6, 2020

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
Copy link
Member

flip1995 commented Apr 6, 2020

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

@djc
Copy link
Contributor

djc commented Apr 7, 2020

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
Copy link
Member Author

dtolnay commented Apr 7, 2020

👍 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
Copy link

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
Copy link
Contributor

ehuss commented Apr 7, 2020

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
Copy link
Member

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
Copy link
Contributor

llogiq commented Apr 7, 2020

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
Copy link
Contributor

(...) 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
Copy link

dpc commented Apr 7, 2020

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
Copy link
Member

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

flip1995 added a commit to flip1995/rust-clippy that referenced this issue Apr 7, 2020
Downgrade unreadable_literal to pedantic

As motivated by rust-lang#5418. This is the top most commonly suppressed Clippy style lint, which indicates that the community has decided they don't share Clippy's opinion on the best style of this.

I've left the lint in as pedantic, though it could be that "restriction" would be better -- I can see this lint being useful as an opt-in restriction in some codebases.

changelog: Remove unreadable_literal from default set of enabled lints
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Apr 7, 2020
Downgrade new_ret_no_self to pedantic

As motivated by rust-lang#5418. This is the second most widely suppressed Clippy style lint, and [this grep.app search](https://grep.app/search?q=%5C%5Ballow%5C%28.%2Aclippy%3A%3Anew_ret_no_self%5Cb&regexp=true&case=true&filter[lang][0]=Rust) shows a large number of diverse reasonable signatures for a `new` method.

changelog: Remove new_ret_no_self from default set of enabled lints
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Apr 8, 2020
Downgrade unreadable_literal to pedantic

As motivated by rust-lang#5418. This is the top most commonly suppressed Clippy style lint, which indicates that the community has decided they don't share Clippy's opinion on the best style of this.

I've left the lint in as pedantic, though it could be that "restriction" would be better -- I can see this lint being useful as an opt-in restriction in some codebases.

changelog: Remove unreadable_literal from default set of enabled lints
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Apr 8, 2020
Downgrade new_ret_no_self to pedantic

As motivated by rust-lang#5418. This is the second most widely suppressed Clippy style lint, and [this grep.app search](https://grep.app/search?q=%5C%5Ballow%5C%28.%2Aclippy%3A%3Anew_ret_no_self%5Cb&regexp=true&case=true&filter[lang][0]=Rust) shows a large number of diverse reasonable signatures for a `new` method.

changelog: Remove new_ret_no_self from default set of enabled lints
bors added a commit that referenced this issue Apr 8, 2020
Move cognitive_complexity to nursery

As discussed in #5418 (comment); Clippy's current understanding of cognitive complexity is not good enough yet at analyzing code for understandability to have this lint be enabled by default.

changelog: Remove cognitive_complexity from default set of enabled lints
@joshtriplett
Copy link
Member

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

bors added a commit that referenced this issue Apr 10, 2020
compare with the second largest instead of the smallest variant

This should make the lint less noisy for now. See [my comment](#5418 (comment)) to issue #5418.

---

changelog: none
bors added a commit that referenced this issue Apr 10, 2020
compare with the second largest instead of the smallest variant

This should make the lint less noisy for now. See [my comment](#5418 (comment)) to issue #5418.

---

changelog: none
@pickfire
Copy link
Contributor

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
Copy link
Contributor

llogiq commented Apr 12, 2020

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
Copy link
Contributor

llogiq commented Apr 12, 2020

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

@llogiq
Copy link
Contributor

llogiq commented Apr 12, 2020

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
Copy link

fogti commented Apr 12, 2020

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

@flip1995
Copy link
Member

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

@flip1995 flip1995 added the C-tracking-issue Category: Tracking Issue label Apr 28, 2020
Mythra added a commit to Mythra/dev-loop that referenced this issue Jul 10, 2020
* as of the error reworking commit, tasks sometimes didn't erase
  lines when it should have. most specifically at the beginning
  of tasks being executed, and all tasks completing. this is now
  fixed, as the task_inidcator ignores logs that happened before
  it started, and properly counts how many lines it added, that it
  needs to erase.

* we've reworked all of the codebase, so almost all clippy warnings
  that were previously ignored, are no longer ignored, and code
  has been rewritten so the lint step passes.

  * There are some "clippy::too_many_arguments" being ignored,
    I intend to fix these, but these require more of a deft hand,
    and so I don't want to rush it. reworking may also not be needed
    once I try it. (This is currently the most ignored clippy warning
    according too: rust-lang/rust-clippy#5418)
    and reasoning I agree with is there.
  * There is one: `#[allow(unused)]` for pipeline descriptions. This
    is true, it is unused right now, but I want to keep it in the
    schema because I do want to render the pipeline in list one day.
    I just haven't found quite a way to do it yet, but i added it in
    because I know I want to do it (and I don't want to remove it).

* this has also resulted in a giant split up of docker executor,
  so it is no longer all in one gigantic file. This really _really_
  needed to be done, so I'm glad I finally did it.
Mythra added a commit to Mythra/dev-loop that referenced this issue Jul 10, 2020
* as of the error reworking commit, tasks sometimes didn't erase
  lines when it should have. most specifically at the beginning
  of tasks being executed, and all tasks completing. this is now
  fixed, as the task_inidcator ignores logs that happened before
  it started, and properly counts how many lines it added, that it
  needs to erase.

* we've reworked all of the codebase, so almost all clippy warnings
  that were previously ignored, are no longer ignored, and code
  has been rewritten so the lint step passes.

  * There are some "clippy::too_many_arguments" being ignored,
    I intend to fix these, but these require more of a deft hand,
    and so I don't want to rush it. reworking may also not be needed
    once I try it. (This is currently the most ignored clippy warning
    according too: rust-lang/rust-clippy#5418)
    and reasoning I agree with is there.
  * There is one: `#[allow(unused)]` for pipeline descriptions. This
    is true, it is unused right now, but I want to keep it in the
    schema because I do want to render the pipeline in list one day.
    I just haven't found quite a way to do it yet, but i added it in
    because I know I want to do it (and I don't want to remove it).

* this has also resulted in a giant split up of docker executor,
  so it is no longer all in one gigantic file. This really _really_
  needed to be done, so I'm glad I finally did it.
@camsteffen camsteffen added the A-category Area: Categorization of lints label Feb 23, 2021
@webern
Copy link

webern commented Aug 30, 2021

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
Copy link
Member Author

dtolnay commented Sep 13, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-category Area: Categorization of lints C-tracking-issue Category: Tracking Issue S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests