[Roadmap] Lint groups

[flip1995]

There are more and more issues about managing lints in Clippy popping up. Lints are hard to implement with a guarantee of no/few false positives (FPs). One way to address this might be to introduce more lint groups to give users the ability to better manage lints, or improve the process of classifying lints, so that disabling lints due to FPs becomes rare. It is important to note, that Clippy lints are less conservative than rustc lints, which won't change in the future.

• 5537 (production lints)

• 6366 (suspicious group)

• Idea: Make it possible to define custom groups (in configuration file)

Steps to completion:

• [x] Add suspicious lint group
• [ ] Start discussion about subdividing allow-by-default groups
• [x] Better define our expectations for lint groups (regarding FPs and strictness)
• [ ] Make it possible to dump a statistics file when running Clippy (allowed lints, ...?)
• [ ] CFP: Ask people to send us their Clippy statistic files, to help evaluate lints
• [x] Implement expect attribute (https://github.com/rust-lang/rust/issues/54503)
• [ ] Go over all lints and reassign lint groups

[flip1995]

Make it possible to dump a statistics file when running Clippy (allowed lints, ...?)

https://github.com/flip1995/rust-clippy/commit/fa539c9d85a8b1ae9e88bc777a495c2cebf554bc is a first attempt at implementing this as a PoC.

Output for Clippy

``` clippy_lints: Global allow attributes: missing_docs_in_private_items: 1 must_use_candidate: 1 Inner allow attributes: enum_glob_use: 3 wildcard_imports: 2 similar_names: 1 float_cmp: 1 Outer allow attributes: too_many_lines: 26 module_name_repetitions: 16 cast_possible_truncation: 14 similar_names: 10 cast_possible_wrap: 4 cast_sign_loss: 4 integer_division: 2 match_same_arms: 2 many_single_char_names: 1 shadow_unrelated: 1 too_many_arguments: 1 unknown_clippy_lints: 1 useless_attribute: 1 wrong_self_convention: 1 needless_pass_by_value: 1 option_if_let_else: 1 clippy_driver: Outer allow attributes: too_many_lines: 1 ```

[dhardy]

Notes on existing groups:

• cargo: 5 lints; seems fine
• complexity: has more lints than I care to count; vague sub-groups are:
  • redundant operations: bool_comparison, borrow_deref_ref, clone_on_copy, deref_addrof
  • simpler alternative: bind_instead_of_map, bytes_count_to_len, char_lit_as_u8, deprecated_cfg_attr, derivable_impls, double_comparisons, duration_subsec
  • more generic alternative: borrowed_box
  • probably wrong: crosspointer_transmute
  • confusing: diverging_sub_expression
  • "simpler alt" and/or "probably wrong": double_parens
• correctness: many lints; again, vague sub-groups:
  • probably wrong: almost_swapped, clone_double_ref (issues list false positives), cast_slice_different_sizes, derive_hash_xor_eq, derive_ord_xor_partial_ord
  • known incorrect (should be promoted to rustc?): bad_bit_mask, cast_ref_to_mut, cmp_nan, deprecated_semver
  • simpler alternative and probably wrong: absurd_extreme_comparisons, approx_constant
  • wrong but harmless ("redundant"?): drop_copy
  • probably wrong or perf?: async_yields_async
• nursery:
  • known incorrect?: as_ptr_cast_mut, debug_assert_with_mut_call
  • more generic alternative: derive_partial_eq_without_eq
  • "simpler alt" or "redundant": branches_sharing_code (but lint has open issues)
  • "over-complex": cognitive_complexity
• pedantic:
  • safer alternative: borrow_as_ptr, checked_conversions
  • possibly intentional: case_sensitive_file_extensions_comparisons, cast_precision_loss, cast_ptr_alignment, cast_sign_loss, cast_possible_truncation, cast_possible_wrap, copy_iterator, doc_link_with_quotes
  • probably wrong but imperfect lint: doc_markdown
  • pedantic (known safe): cast_lossless, cloned_instead_of_copied, default_trait_access
• perf (note: uses same sub-categories as above but with potential perf issues):
  • simpler alt: box_collection, boxed_local, collapsible_str_replace
  • rendundant: box_default, cmp_owned
• restriction (even more pedantic?):
  • more generic alternative: alloc_instead_of_core
  • okay, lacking documentation: allow_attributes_without_reason, assertions_on_result_states
  • input validity check / safer alternative is preferred: arithmetic_side_effects
  • behaviour that possibly shouldn't have been allowed at all in Rust, but is nevertheless commonly used: as_conversions, as_underscore, default_union_representation
  • vaguely suspect: create_dir, dbg_macro, default_numerical_fallback, disallowed_script_idents
  • okay but with a vaguely better alternative: clone_on_ref_ptr, decimal_literal_representation, deref_by_slicing
• style:
  • simpler alternative: assign_op_pattern, bool_assert_comparison, bool_to_int_with_if, chars_last_cmp, chars_next_cmp, default_instead_of_iter_empty
  • redundant: assertions_on_constants, double_must_use
  • redundant, non-Rust style: blocks_in_if_conditions
  • suspicious: borrow_interior_mutable_const (why is this in style?), builtin_type_shadow, declare_interior_mutable_const, double_neg, duplicate_underscore_argument (maybe)
  • perf, "more readable": bytes_nth, comparison_to_empty
  • "more readable": cmp_null
  • "unnecessary complexity": collapsible_else_if, collapsible_if, collapsibile_match
  • possibly simpler alternative: comparison_chain
  • utility: disallowed_macros, disallowed_ methods, disallowed_names, disallowed_types
• suspicious:
  • probable simple mistake: almost_complete_letter_range, crate_in_macro_def
  • probable mistake (await): await_holding_lock, await_holding_refcell_ref
  • probable mistake (other): cast_enum_constructor, duplicate_mod
  • almost definitely wrong: cast_enum_truncation, cast_nan_to_int
  • safer alternative: cast_abs_to_unsigned, cast_slice_from_raw_parts
  • redundant, possible mistake: drop_non_drop
  • meta: blanket_clippy_restriction_lints
  • utility: await_holding_invalid_type

I only looked at lints starting a..=d for the larger groups. Let me know if you want me to go through the rest for re-categorisation purposes.

Notes

The following lint sub-categories occur in multiple groups:

• redundant (harmless but in some cases a possible mistake)
• simpler alternative
• simpler alternative and probably wrong
• probably wrong
• more generic alternative
• over-complex
• safer alternative
• utility (lints only useful given specific configuration)
• perf

Both correctness and suspicious groups both concern probable mistakes. (Possibly I am missing the distinction, beyond that suspicious lints are less certain?) Several other categories also have "probably wrong" lints. A possible distinction is "probably a bug" and "probably incorrect but harmless" (the latter should probably be categorised elsewhere).

"Style" is an odd mix of lints. Possibly simply some lints here are mis-categorised. There is also significant divide between lints which concern redundant stuff and lints which concern complexity, with other lints suggesting "more readable" alternatives.

"Utility" lints (from multiple groups) do nothing unless specific configuration is added, therefore they should be at least "warn" level by default; these could be placed in a new group.

Special sub-categories:

• pedantic / possibly intentional: concerns operations which are explicitly allowed in standard Rust, but some people consider bad practice
• restriction / suspicious: same as above
• pedantic / pedantic: operations known safe, thus use of these lints should be considered stylistic choice
• style / complexity: e.g. collapsible_else_if, comparison_chain lints would possibly be better put in complexity?

I tried to take an un-biased view on these categories, though certainly do have opinions (e.g. collapsible_if's suggestions are often less readable). Ideally it would be easy to disable this type of "style" lint while keeping others such as needless_borrow.

Suggestions and questions

Is a simple list of groups enough? Would hierarchical groups or non-exclusive categories (as in Venn diagram) be better?

Is introducing more categories an issue, e.g. because people use config like #[warn(clippy::pedantic)] which would affect fewer lints than previously? Is allowing lints in multiple groups a satisfactory solution?

Some lints are strongly related, e.g. collapsible_if and collapsible_else_if or chars_last_cmp and chars_next_cmp. Ideally it would be easier to en/dis-able these as a group. Motivation for hierarchical groups?

Potentially new categories:

• "Probably redundant" (sub-cat. of style or new group)
• "Simpler alternative" (sub-cat. of style or new group)
• "More generic alternative"

Possibly mis-categorised lints:

• crosspointer_transmute is in complexity but should be in correctness or suspicious?
• async_yields_async is in correctness but is purely a performance issue?
• borrow_interior_mutable_const and declare_interior_mutable_const are under style but concern possible bugs
• drop_non_drop is under suspicious but is harmless (other than the possibility that the wrong object was dropped)

[llogiq]

My 2¢ on this:

• I struggle to see how drop_copy would come up in correct code, and manual drops are usually only ever issued to trigger some guard and/or uphold some invariants. So the lint is less about the code found, more about the code that's likely missing.
• We moved some lints to pedantic in the past that should probably be in nursery because of false positives. I feel that pedantic as a group is probably the least strongly defined one.
• restriction lints should only be used in specific scenarios. That we have a lint group for them at all is because a) we tie default lint levels to our groups and b) the "restriction" moniker serves as documentation
• borrow_interior_mutable_const´,builtin_type_shadow,declare_interior_mutable_constshould likely be insuspiciousinstead ofstyle. I think their placement is simply due to the fact that thesuspicious` lint group is quite young and the lints probably predate it.
• The disallowed_* lints are probably only in style to warn by default, because conceptually, they are clearly restriction lints.
• Similarly to drop_copy, drop_non_drop is linting something that is not by itself problematic but likely masks an issue because of code that isn't there instead. So suspicious or correctness seem to be the correct group for them.
• We used to have a shadow lint group before, but it was removed when we got our current system. We could in theory reinstate some of the smaller groups of similar lints, but I doubt it makes much of a difference in practical use.
• The current lint groups focus on the why something is linted. Your proposed groups as I understand them focus on the what instead.

[dhardy]

Regarding drop, I have in the past used it with (mutable) references to get around borrow-checker restrictions. None of the examples I have now which require an explicit drop trip these lints, however.

The current lint groups focus on the why something is linted. Your proposed groups as I understand them focus on the what instead.

I guess so, though sometimes the why eludes me. E.g. why are bytes_count_to_len and derivable_impls complexity lints while bool_to_int_with_if and chars_last_cmp are style lints? My best guess is that style is one of the oldest groups and a lot of its contents never got re-assigned. Should they be moved to complexity (or even a new group)?

A few of those from above are a little hard to place:

• diverging_sub_expression: this could be considered a style lint but is probably closest to being a restriction lint. I guess it is not in that category mainly to enable it by default. Maybe there should be a default_restriction category (also including disallowed_* lints)?
• borrowed_box is in complexity; it seems more like a default_restriction lint
• cognitive_complexity
• doc_markdown (possibly only in pedantic due to known issues; I guess this is okay)
• blocks_in_if_conditions could almost be in suspicious
• await_holding_invalid_type is another lint for the hypothetical default_restriction group

[flip1995]

My best guess is that style is one of the oldest groups and a lot of its contents never got re-assigned. Should they be moved to complexity (or even a new group)?

Lint groups are just assigned by whatever the author + reviewer think is the best group. We try to find the best group, but sometimes some lints might slip through into the arguably wrong group. If you think a lint should be moved to a different group, it is as easy as changing the group in the lint definition and running cargo dev update_lints.

---

I don't think we will introduce non-exclusive groups in Clippy. The groups we have need to stay, as they were defined in an RFC. Additional groups that might overlap (with existing groups) will just make configuring lints confusing.

Sub-groups maybe. (I brought this up for pedantic) But now, I don't really see a practical use for this in our warn-by-default lint groups.