Update Documentation to Focus on `LateLintPass` #9426

nahuakang · 2022-09-04T16:05:49Z

This PR fixes #9311 by flattening the chapter structure and enriching the Clippy book documentation on how to work on lints.

Below is the checklist for all the chapters that should be edited/written:

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

Followed [lint naming conventions][lint_naming]
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

changelog: Update Clippy book

rust-highfive · 2022-09-04T16:05:52Z

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

nahuakang · 2022-09-04T16:07:05Z

@xFrednet @flip1995 Here's a draft PR. Will just keep working on this branch after getting initial feedback on the style/tone of the sample chapter :)

xFrednet · 2022-09-04T16:12:57Z

r? @xFrednet @flip1995

llogiq

This looks like a good start! Let me know if you need anything.

book/src/development/define_lints.md

xFrednet

I like the general tone :)

I've added some comments which may be a bit nitpicky, feel free to ignore them in that case :)

book/src/development/define_lints.md

xFrednet

The lint pass chapter looks good to me and also correct. Still, it would be good if someone else could double-check the differentiation, as I helped with the content of these docs 🙃

Maybe @flip1995 🙃

book/src/development/lint_passes.md

flip1995 · 2022-09-19T08:46:36Z

Sorry about not commenting on this. It's on my radar, but I currently really struggle to find time for Clippy, besides the plumbing stuff for releases/syncs/... 😐 I hope that will get better soon, but with how $day_job is currently going, I can't promise anything.

nahuakang · 2022-09-19T09:42:45Z

@flip1995 No worries my day job is also quite stressful so my progress is a bit slow here as well 💪 Take care of yourself (@xFrednet that includes you, too 😄)

xFrednet

This is just a partial review, but I like what I've read so far. Thank you for all the work, you've already put into this!

xFrednet · 2022-10-12T18:52:45Z

book/src/development/add_documentation.md

+    ///
+    /// ### Example
+    ///
+    /// ```rust,ignore


Here and below, I would remove the ,ignore as these should be tested as part of the CI in most cases :)

xFrednet · 2022-10-12T18:59:38Z

book/src/development/add_documentation.md

+}
+```
+
+Make sure you put some love into writing a good documentation so that


Here we could also add something about the special magic {{produces}}. Our metadata collection actually supports the automated linting of blocks and adding the output to the website. This was added in #8947 . All that's required is to add {{produces}} below a code block, and the rest is done by black magic. Sadly, we don't have any lints yet using this. (Only because nobody got to adding them 😅)

Besides this, another awesome chapter!

xFrednet · 2022-10-12T19:10:06Z

book/src/development/method_checks.md

+            // Optionally, we can check the type of the self argument whenever necessary.
+            // See "Type Checking" chapter of the Clippy book for more information.


It's good that you mentioned this, since this is an easy thing to overlook and already came up a few times in my reviews. But, I think this should be formulated stronger. It's almost never correct to only check for the name. For example, just because the method is called map doesn't mean that it actually belongs to an Iterator. It could be an option or a user-defined type and method. Maybe we can directly add the type check, even if that overloads the example a bit 🤔

xFrednet · 2022-10-12T19:14:20Z

book/src/development/method_checks.md

+in [PathSegment], which is a value of the `MethodCall` enum variant,
+contains a field `name` that is defined as a `Symbol`.
+
+## Checking if a type defines a specific method


You could also add a reference to the section about checking if a type implements a certain trait. I can imagine this part could also be confused with that.

Otherwise, this is solid and a fun read. I'm looking forward to merging this :)

xFrednet · 2022-10-12T19:16:39Z

book/src/development/trait_checks.md

+dependencies.
+
+Using one of its `*_trait` method, we could obtain the [DefId] of any
+specific item, such as `clone`, `copy`, `drop`, `eq`, which are familiar


Suggested change

specific item, such as `clone`, `copy`, `drop`, `eq`, which are familiar

specific item, such as `Clone`, `Copy`, `Drop`, `Eq`, which are familiar

xFrednet · 2022-10-12T19:18:38Z

book/src/development/trait_checks.md

+
+Besides diagnostic items, we can also use [`lang_items`][lang_items].
+Take a look at the documentation and we find that `LanguageItems` contains
+all defined or undefined language items both from the current crate or its


all defined or undefined language items

I believe this could be confusing and doesn't affect the lint implementation, IIRC. Therefore, I would leave this out :)

flip1995

Thanks so so much for writing this and your patience on my review. I haven't done any detailed reading yet, but just looked at this from a high level perspective.

It will be nearly impossible for me to review this as a whole. Or rather: this will take months to get it merge ready (because of me). I would suggest to take one chapter out of this PR and put it in its own PR. In this PR we can refine this chapter, focus on it and then merge it.

I would suggest to start with define_lints.

May general feedback: I like your writing style and the story telling you are doing in parts of the documentation. However, some parts of the documentation shouldn't contain any story telling, but just hard facts. The example, which goes through a whole lint implementation can contain story telling and should have a nice flow when reading. Technical chapters that explain how to do something should just contain hard facts though. Short concise documentation that you don't have to read in depth to understand what's going on.

flip1995 · 2022-10-15T13:54:12Z

book/src/development/add_documentation.md

+You might have come across [Clippy lints list][lint_list] and
+read about a few lints that you want to use, restrict, or understand.
+
+These lints's documentation is generated by using the `declare_clippy_lint`
+macro and by writing the documentation inside:


I would write this something like

Clippy's lint documentation, that is available on the Clippy lints list is generated from the `declare_clippy_lint` macro.

book/src/development/define_lints.md

flip1995 · 2022-10-15T14:06:18Z

book/src/development/add_documentation.md

@@ -0,0 +1,46 @@
+# Adding Documentation for New Lint


There shouldn't be any story telling in here. Just the code snippet how the declaration/documentation looks like and a short explanation for each section in the documentation.

book/src/development/define_lints.md

flip1995 · 2022-10-15T14:32:03Z

book/src/development/emit_lints.md

+impl EarlyLintPass for FooFunctions {
+    fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, span: Span, _: NodeId) {
+        // Imagine a `is_foo_function` that checks for functions named `foo`
+        if is_foo_function(fn_kind) {


I wouldn't put examples in here, but rather explain lint passes and how they work in general.

I think my issue with this file is, that it thows together lint emission and lint passes. I think both things deserve their own chapter

Lint passes: What are those? How does rustc call them? How to implement them (high level)? Difference Early/Late? EDIT: Just saw that this chapter already exists. So I would focus here on the next point.

Lint emission: How to emit a lint in Clippy? How to get code snippets? How to produce suggestions? Applicability? Difference between suggestion/help/note?

flip1995 · 2022-10-15T14:34:06Z

book/src/development/example_early.md

+is aimed to do.
+
+Additionally, since restricting `pub use ...` is for very specific cases,
+we probably want to create a lint but put it into `clippy::restriction`


I wouldn't chose an restriction lint here. Those lints are for really special use cases only and people might not relate to it and won't see the motivation. There is probably a simple early lint that is warn-by-default?

flip1995 · 2022-10-15T14:38:07Z

book/src/development/hir_ids.md

+Alas, `HirId`s are very useful to Clippy developers. Below are some
+examples for when we might need `HirId`s.
+
+## `find_parent_node` method


Wait, this chapter is not at all about hir_ids but rather about util functions using hir_ids. I'm really unsure if this chapter is necessary. We should have a list with abbreviations and terms, that lists the HirId with a lint to the rustc HIR identifier documentation. This should be enough.

A chapter with a list of useful functions might be good. But this can also be really bitrotty.

flip1995 · 2022-10-15T14:41:12Z

book/src/development/lint_passes.md

+As a reminder, run the following command to generate boilerplates for lints
+using `LateLintPass`:
+
+```sh
+$ cargo dev new_lint --name=<your_new_lint> --pass=late --category=<your_category_choice>
+```


This is weirdly placed. I would remove it.

flip1995 · 2022-10-15T14:42:15Z

book/src/development/lint_passes.md

@@ -0,0 +1,137 @@
+# Lint passes


So this chapter already exists. I think there is a little documentation missing on what a lint pass is in general and how it works together with rustc.

nahuakang · 2022-10-15T15:04:07Z

@flip1995 @xFrednet Thank you and I will read through your comments and address them!

And yes, I will move define_lints chapter into a PR and submit it after addressing relevant comments.

1 chapter or several short chapters per PR sounds like a good strategy to keep things flowing! 😇

xFrednet · 2023-02-28T15:08:50Z

Hey @flip1995 a small ping from triage. What is the status on this PR/the sub PRs? I still really like the changes and believe that they'll improve the docs and approachability for newcomers. My understanding was that you wanted to review the sub PRs. Is that correct?

flip1995 · 2023-02-28T15:10:58Z

I don't think that this PR will ever get merged as-is. I'll review and merge the sub PRs and then we'll continue pulling the next things out of this PR.

nahuakang · 2023-02-28T15:14:21Z

Sounds good :) I'll continue on this thread soon. Still getting used to my new job (also using 🦀)

blyxyas · 2023-04-02T22:54:16Z

@nahuakang Any thoughts on continuing with the project, is this abandoned? It's a very cool project and very sad that it has been stale for 211 days. If you do not mind someone else taking the lead partitioning this and making the whole PR process for each file, let me know.

nahuakang · 2023-04-03T06:39:32Z

@blyxyas Are you interested in taking the lead? Feel free to do so since I'll focus on some other stuff this year :) This branch contains everything I wrote. Feel free to cherry pick.

blyxyas · 2023-04-03T10:00:31Z

@nahuakang Yeah, the Clippy documentation feels pretty outdated and definitely needs some updates, but I was worried that you wanted the PRs to your name for some reason. Thanks for giving the green light to continue the project!

nahuakang · 2023-04-03T10:02:32Z

@blyxyas It's open source. No need for name attached to PRs haha 😄

flip1995 · 2023-04-03T10:04:45Z

@blyxyas You can use Co-authored-by in the commit message, and the commits will also be attributed to Nahua.

Damn, I really need to get to reviews... I try to sort things out over Easter.

blyxyas · 2023-04-03T13:25:26Z

I'll push this over the finish line then.

@nahuakang Do you want to still manage #9659, #9668 and #9740 or do I open new PRs of those chapters and you close those?

nahuakang · 2023-04-03T14:17:18Z

@blyxyas They are closed now :)

flip1995 · 2023-04-03T14:19:55Z

@blyxyas Please also split out smaller PRs, instead of trying to rebase this one.

@nahuakang I'm really sorry I never got to review those...

nahuakang · 2023-04-03T14:26:57Z

@flip1995 No apology needed 🙌 @blyxyas good luck :)

blyxyas · 2023-04-03T18:42:01Z

@flip1995 Would you like to review the new PRs about Book Chapter Updates Reborn or are you busy (or don't want to read)? In honor of the old PRs

flip1995 · 2023-04-04T08:16:26Z

I would really like to review them. So please assign me to them. If I won't get to them within 2-3 weeks, I'll reassign them.

Clippy Book Chapter Updates Reborn: Defining Lints Revival of #9659, I've made a few changes (mainly ones from reviews from #9426, like removing mid-section storytelling) and some re-writes. The goal of the project would be to slowly re-write the Clippy book (only chapters that need it) to more up-to-date style. ## Notes - Things like `git status` commands have changed, we need to make sure that they're correct. - As this is a team-wide effort, I would please ask anyone and everyone to read it and give your opinion. - To talk about the whole project, please use the tracking issue for the project #10597 (It also contains a timeline, discussions and more information) --- changelog: Add a new "Defining lints" chapter to the book r? `@flip1995`

Clippy Book Chapter Updates Reborn: Writing tests This PR adds a new chapter to the book: "Writing tests". The changes have been mainly done from reviews from #9426 and some minor re-writes. ## Notes - We still need to check that the `git status`es are correct, as `cargo dev new_lint` changed a lot since 2022. - Requires #10598: Link to "Emitting Lints" where I flagged with `FIXME:`. - To talk about the whole project, please use the tracking issue for the project #10597 (It also contains a timeline, discussions and more information) changelog: Add a new "Writing tests" chapter to the book r? `@flip1995`

xFrednet · 2024-03-21T15:32:31Z

I believe this PR has been split into sub PRs, which have been partially reviewed. I'll close this PR to clean up our backlog a bit. You're welcome to open a new PR or comment here to have this one reopened.

Thank you for the time, you put into this update :D

rust-highfive assigned llogiq Sep 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 4, 2022

rust-highfive assigned xFrednet and unassigned llogiq Sep 4, 2022

xFrednet assigned flip1995 Sep 4, 2022

llogiq reviewed Sep 4, 2022

View reviewed changes

book/src/development/define_lints.md Show resolved Hide resolved

book/src/development/define_lints.md Outdated Show resolved Hide resolved

xFrednet reviewed Sep 4, 2022

View reviewed changes

nahuakang added 2 commits September 5, 2022 18:52

Add define_lints chapter

7612603

Add write_tests chapter

7910894

xFrednet reviewed Sep 10, 2022

View reviewed changes

book/src/development/lint_passes.md Outdated Show resolved Hide resolved

book/src/development/lint_passes.md Show resolved Hide resolved

xFrednet reviewed Sep 11, 2022

View reviewed changes

book/src/development/lint_passes.md Outdated Show resolved Hide resolved

nahuakang added 2 commits September 11, 2022 10:53

Add lint_passes chapter

3290f5e

Add emit lints chapter

faf52c9

nahuakang added 6 commits September 22, 2022 21:18

Add type_checks chapter

3e12616

Add trait_checks chapter

5948c6a

Add method_checks chapter

ea136e5

Add macro_expansions chapter

daa84dc

Add hir_ids chapter

3be525b

Add add_documentation chapter

851fd7e

xFrednet reviewed Oct 12, 2022

View reviewed changes

Add example_early chapter for early lint pass example

99fe34b

flip1995 reviewed Oct 15, 2022

View reviewed changes

WIP: example_late chapter draft

2f32508

nahuakang mentioned this pull request Oct 16, 2022

Clippy Book Updates: Add define_lints chapter #9659

Closed

nahuakang mentioned this pull request Oct 17, 2022

Clippy Book Updates: Add write_tests chapter #9668

Closed

Draft duplicate trim_split_whitespaces lint for late lint example

02330c8

nahuakang mentioned this pull request Oct 28, 2022

Clippy Book Updates: Add lint_passes chapter #9740

Closed

This was referenced Apr 4, 2023

Clippy Book Chapter Updates Reborn: Defining Lints #10595

Merged

Clippy Book Chapter Updates Reborn: Writing tests #10596

Merged

[Tracking issue] Project: Clippy Book Chapter Updates Reborn #10597

Open

xFrednet removed their assignment May 16, 2023

This was referenced Jun 8, 2023

Update docs and tooling to focus on LateLintPasses #9311

Open

Direct towards late passes in cargo dev new_lint #10907

Merged

xFrednet closed this Mar 21, 2024

		// Optionally, we can check the type of the self argument whenever necessary.
		// See "Type Checking" chapter of the Clippy book for more information.

	specific item, such as `clone`, `copy`, `drop`, `eq`, which are familiar
	specific item, such as `Clone`, `Copy`, `Drop`, `Eq`, which are familiar

Update Documentation to Focus on LateLintPass #9426

Update Documentation to Focus on LateLintPass #9426

Conversation

nahuakang commented Sep 4, 2022 • edited Loading

rust-highfive commented Sep 4, 2022

nahuakang commented Sep 4, 2022

xFrednet commented Sep 4, 2022

llogiq left a comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

flip1995 commented Sep 19, 2022

nahuakang commented Sep 19, 2022

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahuakang commented Oct 15, 2022

xFrednet commented Feb 28, 2023

flip1995 commented Feb 28, 2023

nahuakang commented Feb 28, 2023

blyxyas commented Apr 2, 2023

nahuakang commented Apr 3, 2023 • edited Loading

blyxyas commented Apr 3, 2023

nahuakang commented Apr 3, 2023

flip1995 commented Apr 3, 2023

blyxyas commented Apr 3, 2023

nahuakang commented Apr 3, 2023

flip1995 commented Apr 3, 2023

nahuakang commented Apr 3, 2023

blyxyas commented Apr 3, 2023 • edited Loading

flip1995 commented Apr 4, 2023

xFrednet commented Mar 21, 2024

Update Documentation to Focus on `LateLintPass` #9426

Update Documentation to Focus on `LateLintPass` #9426

nahuakang commented Sep 4, 2022 •

edited

Loading

flip1995 Oct 15, 2022 •

edited

Loading

nahuakang commented Apr 3, 2023 •

edited

Loading

blyxyas commented Apr 3, 2023 •

edited

Loading