Clippy 1.0

[Manishearth]

This RFC proposes a Clippy 1.0 in preparation for being shipped via Rustup. It aims to get community consensus on the current state of Clippy lints, as well as on what lints we should uplift to the compiler directly if any.

See also: The Future of Clippy

Co-written by @oli-obk

Rendered
Tracking issue

[Manishearth]

cc @rust-lang/dev-tools @rust-lang/compiler

This isn't tagged T-Compiler (maybe it should be?) but the uplift stuff does impact the compiler team.

[mcarton]

It's à 😄

[mcarton]

I once gave a talk about clippy and the first question asked by the audience was "Why the fuck is your version 0.0.97? Are you ever going to increase from 0.0?". This was more than 100 versions ago.
We need to move away from 0.0 at some point.

[Victor-Savu]

Such an angry audience you had :) I hope they are not representative for the rust community.

[mcarton]

They weren't particularly angry, just really astonished by such a weird version. I also mentioned semver in a previous talk about Rust at the same conference, and then here I am, presenting a tool that seems like it doesn't want to commit to a proper versioning. Kind of ironic for a tool that aims to help you with Rust good practises.

[mcarton]

I don't see temporary_cstring_as_ptr which is definitely something I would like uplifted.
IIRC at the time of the creation of this lint we had found a quite few instances of this, for which we opened PRs, but new instances might have appeared by now.

[oli-obk]

It's not generally wrong if used as a function argument, it's "just" a very common source of dangling pointers because a simple refactoring can easily turn correct code into UB. Any use of CString::new(...).unwrap().as_ptr() outside a function argument is definitely wrong.

More detailed discussion can be found here

[Manishearth]

cc the other clippy maintainers @llogiq @mcarton @phansch @flip1995 @birkenfeld

[clarfonthey]

A few thoughts:

1. How should clippy deal with stability of lints that recommend using an external crate? For example, the naive_bytecount lint will redirect the user to the bytecount crate. If this crate updates to a new major version (not likely in this case, but in general), how should the clippy lint respond?
2. I recall somewhere there being a style guide for lint names for rustc, and I think that such a style should be adopted for clippy too. The names of the lints seem very inconsistent and all over the place, and it makes sense to standardise them before 1.0.
3. There are a few lints that abbreviate words that I think should not. For example, cmp_owned might be better as owned_comparison rather than deferring to the cmp abbreviation used by Ord and PartialOrd. Similarly, manual_memcpy should be manual_slice_copy IMO because it talks about what's done, rather than deferring to the term memcpy which is again a function name.

Also:

• unused_collect may be replaceable with a #[must_use] on collect? Not 100% sure but that seems to make sense to me.

[Manishearth]

If this crate updates to a new major version (not likely in this case, but in general), how should the clippy lint respond?

Still redirect them to the crate if the crate still works the same way. If not, remove the lint.

The names of the lints seem very inconsistent and all over the place, and it makes sense to standardise them before 1.0.
...
There are a few lints that abbreviate words that I think should not

Would you like to come up with a list of potential renames? 😄 It seems like you have a rough idea of a consistent system here. We can make this happen; and it doesn't need to happen in-band in this RFC (we can cross link a PR). It's in scope for this RFC but I want to break out discussions as much as possible.

Edit: Discussion at rust-lang/rust-clippy#2845

unused_collect may be replaceable with a #[must_use] on collect? Not 100% sure but that seems to make sense to me.

That's a fair point, could you open a rustc PR for that? We can deprecate the lint if it ends up landing. But that's out of scope for this RFC so I don't want to discuss it here too much 😄

Edit: Moved to rust-lang/rust-clippy#2846

[Centril]

It's a good lint; For the suggestion part, I think using .for_each(|elt| { .. }) is also idiomatic (if you don't need to break and stuff).

[Manishearth]

I think specifics like this should be raised as issues on the repo, not here 😄

We're dealing with the broad idea of the lints here, not the specific ones.

(This RFC can become huge in scope otherwise)

[Centril]

Yeah that makes sense. 😆

[Centril]

This RFC does not yet propose lints to be uplifted, but the intention is that the RFC discussion will bring up lints that the community feels should be uplifted and we can list them here.

Apologies in advance if this is a major land-grab, but since uplifting to rustc is being discussed here and that compiler lints are under the purview of the lang team, I'm also adding T-lang.

@F001

I have a question that why it is named as "clippy". Why not just "rustlint" or something like that?

In honour of Clippy, the Office Assistant. I think it is pretty cute.

[Manishearth]

I feel like lints fall under the purview of the compiler team; at least this discussion has been ongoing with the compiler team, not the lang team. (I didn't include the compiler team myself because I wanted them to make the decision of adding themselves)

Lints are a UI feature, not a language feature. Alternative implementations need not use the same set of lints.

[Manishearth]

The lint categorisation section appears to have the same set of lints for all categories?

Cc @oli-obk bug in your script? :)

[Centril]

@Manishearth Hmm well; It could be that the RFC policy - language design document is outdated, but just today we did discuss #2471 in the lang team meeting.

My rationale for continued T-lang purview (in addition to T-dev-tools) is mainly that while lints are a UI feature, as you say, compiler lints are important wrt. the feel of the language itself, so they factor into the design of the language. Of course, by that logic, major swaths of T-libs responsibilities (and of other teams..) could suddenly become T-lang, but you have to draw the line somewhere, so I draw the line at compiler lints.

Anyways, if the language team agrees this is not under their purview they can remove themselves... ;)
If that happens, we should update the lang_changes.md file in some way.

EDIT: nominating this for the next lang meeting so the question of purview itself can be dealt with quickly.

[scottmcm]

It feels weird to me for an official rust-lang-organization tool to ever recommend a non-rust-lang crate in a lint, no matter how good it is. I never want to see an issue for "my crate for this is better than that one, so you should be recommending mine instead".

[oli-obk]

If there is an ambiguity, we can just recommend all the crates anyone wants recommended. We don't rate crates.

[Manishearth]

I think some lints are lang team things, but not in general. I consider that document outdated because Rust has had a semi official "no new lints" policy for ages which effectively means it never got exercised, and historically we've added lints along with language changes to support them, or without an RFC at all in case of things like soundness deprecations.

Compiler lints are as important as diagnostics in the feel of the language imo, but diagnostics are squarely a compiler team concern. Docs are also in a similar space here. Ultimately everything we do here impacts Rust the language 😄

---

I basically want to avoid this becoming a three team RFC 😄

And also, this is something we've been discussing with the compiler team for a while which is why I want to avoid additional churn and rehashing things we've discussed.

[Manishearth]

@rfcbot resolve clippy-toml-stability

discussed a bit in the meeting, clippy.toml is explicitly unstable

[fitzgen]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[withoutboats]

@rfcbot concern cargo-clippy

Following from the discussion of rust-lang/rust-clippy#1358, I'm not certain that cargo clippy is the interface we want to settle around for accessing this set of lints. Other than a few references at the beginning of the RFC, I notice that the interface of the cargo clippy subcommand is essentially not discussed in this RFC.

I think the most expedient thing would be to edit the RFC to avoid asserting that the interface to clippy will be a cargo clippy command, and to just say that we intend to ship clippy with rustup, providing the lints described in the RFC, expanding further to new lints along the guidelines established in the RFC.

---

I'm also concerned that the rules for uplifting lints into rustc are too vague, but I think we're managing that discussion well enough on rust-lang/rust#53224 and don't think I need to register a concern here regarding that.

[Manishearth]

rules for uplifting lints into rustc are too vague

This is intentional, we don't want to legislate too strongly what gets uplifted, more of give some guidelines and mention that uplifts should happen. Actual uplifts should be discussed case by case, as they are in rust-lang/rust#53224

---

Regarding cargo clippy, I personally like having cargo clippy but having clippy Just Work in Rust (via a lint key in cargo.toml, or just cargo lint) would be pretty neat, too.

Amended RFC to punt on cargo-clippy till later.

[withoutboats]

@rfcbot resolve cargo-clippy

I think we have a shared understanding of what we still don't have consensus on.

This is intentional, we don't want to legislate too strongly what gets uplifted, more of give some guidelines and mention that uplifts should happen. Actual uplifts should be discussed case by case, as they are in rust-lang/rust#53224

I've raised a concern about that on the issue linked, which we should continue discussing there.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[U007D]

Request to allow {} to be used with clippy:: prefix to make using multiple clippy lints DRY.

For exmaple,

#![warn(clippy::clone_on_ref_ptr, clippy::decimal_literal_representation, clippy::default_trait_access,
        clippy::doc_markdown, clippy::else_if_without_else, clippy::empty_enum, clippy::enum_glob_use,)]

would be allowed to become:

#![warn(clippy::{clone_on_ref_ptr, decimal_literal_representation, default_trait_access,
        doc_markdown, else_if_without_else, empty_enum, enum_glob_use,})]
...

[joshtriplett]

@U007D That seems completely sensible, but I don't think it should be part of this RFC.

Would you consider proposing a general RFC to allow scoped attributes and warning names like foo::bar, foo::baz to always be abbreviated as foo::{bar, baz}? (This should apply to warning names, and to attribute lists like #[foo::{bar, baz}].)

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Manishearth]

🎉 This RFC has merged!

Tracking issue at rust-lang/rust-clippy#3343

(the tracking issue is a bit weird since there's no stabilization step)