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

Clippy 1.0 #2476

Merged
merged 18 commits into from Oct 20, 2018
Merged

Clippy 1.0 #2476

merged 18 commits into from Oct 20, 2018

Conversation

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jun 14, 2018

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

@Manishearth Manishearth commented Jun 14, 2018

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.

Loading

Clippy, the linter for Rust, has been a nightly-only plugin to Rust for many years.
In that time, it's grown big, but it's nightly-only nature makes it pretty hard to use.

The eventual plan is to integrate it in Rustup á la Rustfmt/RLS so that you can simply fetch prebuilt binaries
Copy link
Member

@mcarton mcarton Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's à 😄

Loading

Clippy aims to follow the general Rust style. It may be somewhat opiniated in some situations.

In general clippy is intended to be used with a liberal sprinkling of `#[allow()]` and `#[warn()]`; _it is okay_ to
disagree with Clippy's choices. This is a weaker philosophy than that behind rustc's lints, where usually flipping

This comment has been hidden.

Loading

This comment has been hidden.

Loading

This comment has been hidden.

Loading

This comment has been hidden.

Loading

This comment has been hidden.

Loading

# Rationale and alternatives
[alternatives]: #alternatives

We don't particularly _need_ a 1.0, however it's good to have a milestone here, and a general idea of stability as we move forward in this process.
Copy link
Member

@mcarton mcarton Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@mcarton mcarton Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

* [cmp_owned](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#cmp_owned) (creating owned instances for comparing with others, e.g. `x == "foo".to_string()`)
* [mutex_atomic](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mutex_atomic) (using a mutex where an atomic value could be used instead)
* [box_vec](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#box_vec) (usage of `Box<Vec<T>>`, vector elements are already on the heap)
* [useless_vec](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#useless_vec) (useless `vec!`)
Copy link
Member

@mcarton mcarton Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@oli-obk oli-obk Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 14, 2018

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

Loading

@clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jun 14, 2018

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.

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 14, 2018

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

Loading

@Diggsey

This comment has been hidden.

* [boxed_local](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#boxed_local) (using `Box<T>` where unnecessary)
* [large_enum_variant](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#large_enum_variant) (large size difference between variants on an enum)
* [manual_memcpy](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#manual_memcpy) (manually copying items between slices)
* [unused_collect](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_collect) (`collect()`ing an iterator without using the result; this is usually better written as a for loop)
Copy link
Contributor

@Centril Centril Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Loading

Copy link
Member Author

@Manishearth Manishearth Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Loading

Copy link
Contributor

@Centril Centril Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. 😆

Loading

@F001

This comment has been hidden.

@Centril
Copy link
Contributor

@Centril Centril commented Jun 15, 2018

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.

Loading

@Centril Centril added the T-lang label Jun 15, 2018
@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 15, 2018

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.

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 15, 2018

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

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

Loading

@Centril
Copy link
Contributor

@Centril Centril commented Jun 15, 2018

@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.

Loading


[online]: http://rust-lang-nursery.github.io/rust-clippy/current/

Please leave comments on thoughts about these lints -- if their categorization is correct, if they should exist at all, and if we should be uplifting them to the compiler.

This comment has been hidden.

Loading

This comment has been hidden.

Loading

* [useless_vec](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#useless_vec) (useless `vec!`)

### perf (Warn)
* [naive_bytecount](https://rust-lang-nursery.github.io/rust-clippy/master/index.html#naive_bytecount) (use of naive `<slice>.filter(|&x| x == y).count()` to count byte values)
Copy link
Member

@scottmcm scottmcm Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Loading

Copy link
Contributor

@oli-obk oli-obk Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 15, 2018

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.

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Oct 9, 2018

@rfcbot resolve clippy-toml-stability

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

Loading

@fitzgen
Copy link
Member

@fitzgen fitzgen commented Oct 9, 2018

@rfcbot reviewed

Loading

@rfcbot
Copy link

@rfcbot rfcbot commented Oct 9, 2018

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

Loading

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Oct 9, 2018

@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.

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Oct 9, 2018

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.

Loading

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Oct 10, 2018

@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.

Loading

@rfcbot
Copy link

@rfcbot rfcbot commented Oct 10, 2018

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

Loading

@U007D
Copy link

@U007D U007D commented Oct 18, 2018

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,})]
...

Loading

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Oct 18, 2018

@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}].)

Loading

@rfcbot
Copy link

@rfcbot rfcbot commented Oct 20, 2018

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

Loading

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Oct 20, 2018

🎉 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)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment