Most commonly ignored lints on crates.io #7666
Description
This is a similar methodology to #5418, but using all published crates on crates.io as the data source, rather than repos indexed by grep.app. The old table is a year and a half out of date at this point and hard to reproduce, so I've gone ahead and closed that issue. The code to generate the table below is in https://github.com/dtolnay/noisy-clippy.
The global column is counting #![allow(...)] attributes indicating that a lint has been suppressed at the whole crate or module level, and local is counting #[allow(...)] where a lint has been suppressed at only one place it is being triggered. The table is sorted by the sum.
@rustbot label +A-category +C-tracking-issue +S-needs-discussion
Metadata
Metadata
Assignees
Labels
Type
Projects
Milestone
Relationships
Development
No branches or pull requests
Participants
Activity
camsteffen commentedon Sep 13, 2021
This is an extremely helpful metric and it certainly lends itself to some specific changes. Thank you @dtolnay for all the work you've put in to making Clippy less annoying. 😃
flip1995 commentedon Sep 14, 2021
I filtered out the allow-by-default lints and sorted this table by number of global and by local allows each:
Global
correctnessstylecorrectnessstyleLocal
correctnessstylecorrectnessstyleI think the lints that are globally allowed very often are the first we should look at. The allow-by-default lints don't really need immediate action.
dtolnay commentedon Sep 14, 2021
The top 2 most widely suppressed lints, whether you look at global or local, are
too_many_argumentsandtype_complexity. Both of these happen to be configurable and it's possible Clippy has gotten the default thresholds wrong. I filed dtolnay/noisy-clippy#3 to look into collecting a histogram of the actual number of arguments and actual complexity among functions/types where those lints are being suppressed.We should also collect info on the
upper_case_acronymscases. It's possible there is a relatively small number of heuristics Clippy is missing which account for the majority of cases.many_single_char_namesI would be amenable to bumping topedanticwithout further scrutiny. This is just too domain specific for Clippy to have reliable visibility into. For example in the context of colors, a function usingh,s,l,r,g,bis 100% sensible and spelling all those out is silly, but it's blown way past the default lint threshold.needless_doctest_mainis one that I am personally opposed to and would bump torestrictionbut I don't know how others feel. Module-level example code that shows anything more than a single line of code, and especially if it shows imports or trait impls, is just better with the statements collected into a function andmainis as good as any.flip1995 commentedon Sep 14, 2021
When I looked at the list, I also thought that by today's standards we'd probably place this lint in
pedantic, so I'd be fine with moving it.I don't have a strong opinion about
needless_doctest_main, I think @llogiq is the best person to talk about this lint to. I agree with everything else you wrote.dtolnay commentedon Sep 14, 2021
For the rest, I think dtolnay/noisy-clippy#1 is gonna be a blocker for further action on a lot of them. We need a way to browse what is going on at each of these suppression sites.
I'd call out that
not_unsafe_ptr_arg_derefis shockingly high. The lint seems airtight to me but I wonder if there is something we are missing or if people are just willingly writing unsound code.I'm guessing
from_over_intoare all just left over from old rustc before the coherence rule rebalance but it's something we should check.large_enum_varianthas never been useful to me as aperflint but I defer to the Clippy team whether to keep that one.needless_return, assuming it isn't false positives somehow, is just people being wrong so we can ignore that.module_inceptionhas never been useful to me and I have disabled it several times.wrong_self_conventionneeds to get all the suppression sites categorized. Clippy is likely missing some heuristics there judging by the massive number of local suppressed.match_single_bindingis documented to have a severe known false positive sonurserymight be justified.identity_opprobably needs to be relaxed a bit. @oli-obk commented on this in #3866 (comment): "we should maybe just whitelist multiplying with constants".Auto merge of #7671 - dtolnay-contrib:singlecharnames, r=xFrednet
Jarcho commentedon Sep 15, 2021
I'm curious when you're doing this. Is there some kind of pattern that could be allowed here? I'm also curious if this is actually a common beginner mistake, or if that's just an baseless assertion in the docs.
This one seems to not work properly.
It's currently claiming
Xis the second largest variant on the playground.I've seen people do that for private helper functions. It's not unsound as a whole assuming they're careful, but the individual function definitely is.
new_ret_no_self is probably due to false positives with generics.
len_without_is_empty is probably also due to false positives. At least partially.
new_without_default should probably only trigger on exported types. I've disabled this lint before on private types.
clone_on_copy is almost definitely false positives which are fixed now. (suggestion didn't account for derive having the wrong trait bounds)
It would probably be a good idea to filter on when the last update on the crate was. I imagine that would give a better idea into the current state of ignored lints.
workingjubilee commentedon Sep 19, 2021
excessive_precision is too severe: decimals intended as f32s with less than 9 significant digits are linted against, e.g.
This is 8 significant digits. It should permit at least 9, even if some are considered "excessive", because f32s can include up to 9 significant digits meaningfully, and there is various verbiage in the IEEE754 standard that provides guarantees around only float strings with at least 9 decimal digits. That little extra padding, means float parsing should always get the result and round it correctly. Likewise with f64, it should allow at least 17 before throwing a warning for the same reason.
The reasoning around float_cmp may be overly strict... Correct handling of floats is hard to address via a simple lint. It seems particularly puzzling as a correctness lint, as most correctness lints are Almost Certainly Wrong, as opposed to merely Likely. And there are many cases, if one "knows what one is doing" that exact float comparison is quite appropriate. A warning would be fine but even that might be a bit much.
Both of these are high on the frequent-allow list.
workingjubilee commentedon Sep 19, 2021
Several instances of
#[allow(too_many_arguments)]on grep.app are for anewfunction. Since these are just building a complex struct, clippy should probably ignore these entirely.For a typical procedure call, the System V AMD64 ABI allows passing up to 14 arguments in registers if 8 of them belong to the {x,y,z}mm registers (e.g. floats). The AAPCS64 is slightly more lenient: 16 arguments, for similar reasons. Actually, slightly more for both, but it gets increasingly conditional. It is common to have to pass multiple floats as arguments to e.g. OpenGL APIs, which are beyond the control of the Rust programmer, but for which additional abstraction improves nothing (and the functions are likely perf-sensitive, so may be bad, as occasionally useless code is generated by building and dismantling a struct). While I do not believe clippy should become aware of ABI details, and it's not clippy's fault that compilers are suboptimal, it may be justified to skip floats, or to separately count them, or to just raise the argument threshold by several increments to account for this.
37 remaining items