[Roadmap] Improve Clippy documentation

[flip1995]

Similar to a Clippy Book, which describes how to use Clippy, a book about how to contribute to Clippy might be helpful for new and existing contributors. There's already the doc directory in the Clippy repo, this can be turned into a mdbook.

6011

Steps to completion:

• [x] Write a Clippy book explaining its features and how to use it https://doc.rust-lang.org/nightly/clippy/
• [ ] Convert the contributor documentation to a book (partly done)
• [ ] Update dev part of book with feedback from contributors
• [x] Add book to docs.rs drop down

[flip1995]

From #8579 and the experience of a first-time contributor:

Here's a list of some things I had to gather from context, some might be due to being relatively new to Rust / compilers (some of these might be in the docs and I missed them):

• Naming conventions in the project, e.g. "recv" (recursive? receive? if so, what's recursive or being received?), "ty" (type), "tcx" (T context? the lifetime of the context), "bless" (basically snapshot / approve the current stdout / stderr files)

• The types of contexts and which to use (I recall seeing a general definition in the docs but not why you'd chose one or the other when creating a lint)

• Where things are (rustc_hir / rustc_ast / rustc_middle etc.)

• Method lints aren't normal lints and as such aren't covered by the lint generator directly (might be worth it to make an interactive step there and ask which kind of lint the user wants to create)

• The suggestion in the lint implementation is also what's used for automatic fixing

• Finding which span is the correct one to use (might be easier if there was some sort of automatic debug / pretty print showing the current match for the lint + which spans cover what within the match, would especially be helpful for with_hi / with_lo situations)

  • This point might be related to the fact that debugging mostly involved printing, the tests I ran (I'm assuming) used a binary and didn't stop on breakpoints

• Which fields are what when destructing a node (e.g. ExprKind::MethodCall(_path, [join_self_arg, join_arg], _span))

• Creating a new test (where, what it should include, etc.)

• Not being able to use a raw string in the lint description might be something others run into

• When matching a method in method/mod.rs, the name you're matching is last rather than first (e.g. "join" for .collect().join())

• The fact that a method isn't being handled in methods/mod.rs doesn't mean it shouldn't be there / it's is specifically for iterators rather than vectors / etc. but rather that it wasn't needed until now

• Knowing whether my lint already existed / was relevant

General suggestions:

• Example implementations for common lints / operations

[xFrednet]

While reviewing PRs, I've notices that we don't really have good documentation for our clippy_lints::methods module.

And we don't have any documentation for span_lint_and_sugg here it might be enough to improve the doc comment to include the option to include // run-rustfix to the test

[flip1995]

Yeah, we should extend the cargo dev new_lint tool with a ´--typeflag, where you can choosemethods,transmutes` or other lints that have their own directory. And then also documenting this, of course.

I think we should write a short guide on how good errors and suggestions can be build in Clippy. There's the Errors and Lints chapter in the rustc-dev-guide, but this is really long and I guess no one who contributes to Clippy (for the first time/only once) wants to read through all of that.

[xFrednet]

True, we don't have a shortage of to-dos ^^. Having a --type argument might help, currently I just end up doing everything manually. For the documentation, I'm considering to try something new. Currently, we have most of our documentation in markdown files, it might be interesting to instead implement a simple lint and create commits with explanatory comments for every step. That approach is a bit more hands on, not sure if it's good, though.

I think we should write a short guide on how good errors and suggestions can be build in Clippy.

:+1: for that, but we might want to wait on #7797

[flip1995]

True, we don't have a shortage of to-dos

Nervously looks at triage.rust-lang.org and the 14+/- PRs that are awaiting my review.

it might be interesting to instead implement a simple lint and create commits with explanatory comments

Yes, maybe. Though, if this gets outdated, it doesn't really help anymore or might be confusing. And we can't implement such a lint every few months.

Once I get to the Clippy book (I hope that I can finish the work on 2022-04-09, fingers crossed), we can start improving our documentation and better write guides.

EDIT:

I hope that I can finish the work on 2022-04-09, fingers crossed

It's 2022-07-14 and the book was just released: https://doc.rust-lang.org/nightly/clippy/

[Rqnsom]

Yes, maybe. Though, if this gets outdated, it doesn't really help anymore or might be confusing.

What about picking 2-3 lints that are already implemented in Clippy and then try to document every implementation step for those lints in their respective files within the codebase?

And we can't implement such a lint every few months.

If anything changes, we can accordingly update the code documentation within the same commit that makes a change for those lints. So then those "more-than-well" documented lints are always kept up-to-date. Of course, if that would be sustainable, because I'm not sure how often Clippy lints are changed or refactored.

[flip1995]

The "adding lints" documentation already goes through a step by step instruction of adding lints. Everything that has to be implemented in addition to this is already lint specific.

And documenting something like this in code is not a good solution IMO. This will get outdated over time and we can't enforce updating it. Functions used, rustc types, ... might change over time and only get updated in the code, not the documentation.

It's just bit-rotty by nature and wrong documentation is worse than no documentation IMO.

Also we do want to document the bits and pieces that will help writing lints. But I don't see the need to do this inside a lint implementation.

EDIT:

Looking at the adding lints doc, I must admit that it exploded and now also has many unnecessary additional information, that should live somewhere else. Splitting this up is also on my todo list for the book.

[xFrednet]

The "adding lints" documentation already goes through a step by step instruction of adding lints. Everything that has to be implemented in addition to this is already lint specific.

I would like to see some documentation for our methods and types module. For context, lints which only focus on specific methods or type checks are combined into one lint pass in the modules clippy_lints::methods and clippy_lints::types respectively. Currently, we have no good documentation how new lints are added in those modules and new contributors usually start by implementing them in a separate lint pass.

Also, a thought that came to mind: Do we maybe want to remove the early lint pass documentation, or at least add a note that late lint passes should be preferred? Currently, I can't think of a lint that is better when implemented in an early lint pass (after macro expansion). Our tooling is also better for late lint passes. Encouraging the usage of just late lint passes could save us maintenance work in the future. We also had some contributors that got stuck because they selected an early lint pass but needed a late lint pass.

[flip1995]

Another request what to add to the Clippy book: How to deprecate/uplift a Clippy lint?

would it be possible to write up a document explaining how to uplift a clippy lint? It would be great if people were able to deprecate the lint in clippy without you doing it for them; you'd still be pinged on the PR of course but it would lighten the workload for you and share institutional knowledge.

Posted here: https://github.com/rust-lang/rust/pull/99696#issuecomment-1273285209 Instructions here: https://github.com/rust-lang/rust/pull/99696#pullrequestreview-1134072751

I noticed that cargo dev rename --uplift could be improved and do more things that deprecate does, like deleting tests.