Skip to content

Update docs and tooling to focus on LateLintPasses #9311

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

Open
xFrednet opened this issue Aug 9, 2022 · 15 comments
Open

Update docs and tooling to focus on LateLintPasses #9311

xFrednet opened this issue Aug 9, 2022 · 15 comments
Assignees
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@xFrednet
Copy link
Member

xFrednet commented Aug 9, 2022

Our documentation, how to add a lint, uses an EarlyLintPass for the example lint. However, most new lints use the LateLintPass since they need type information from rustc. Since the LateLintPass is the main one used for new lint, I suggest updating the documentation to focus on that pass and to update the cargo dev new_lint tool to use the LateLintPass by default.

Related discussion on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202022-08-09/near/292596176

I'll wait a bit until I do these changes, to potentially get additional feedback, but also because I don't have that much time rn 😅 🙃

@xFrednet xFrednet self-assigned this Aug 9, 2022
@xFrednet xFrednet added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started A-documentation Area: Adding or improving documentation labels Aug 9, 2022
@nahuakang
Copy link
Contributor

I missed that Zulip chat but I think this update would make perfect sense.

On the documentation side, could we maybe keep the EarlyLintPass as a chapter and have another LateLintPass as a chapter. Then newcomers can read whichever one that suits the ticket they take up?

Fully agree that cargo dev new_lint could use LateLintPass by default (or have --early and --late flags)?

@xFrednet
Copy link
Member Author

xFrednet commented Aug 11, 2022

On the documentation side, could we maybe keep the EarlyLintPass as a chapter and have another LateLintPass as a chapter. Then newcomers can read whichever one that suits the ticket they take up?

That would also be an option, i think the important part is, that we clearly indicate when which version is better. And in the big majority the LateLintPass is even reauired. My goal with this suggestions is to simplify contributing a bit and prevent information overload. so if we find a way to simplify everything and keep both chapters that would be even better :)

or have --early and --late flags)?

I also like that. Either way, this should be a simple change 🙃

@nahuakang
Copy link
Contributor

@xFrednet @flip1995 👋 Hey everyone, I'm happy to help with iterating over the docs aspect of this issue, i.e. editing and revamping the existing documentation for adding lints, especially switching to emphasis on LateLintPass.

Below is a draft outline I organized. Please review it and give your suggestions:

- **Adding lints**
    - **Overview: How to define a lint**
        - Standalone
        - Location for lint tests
        - How to add specific type of lint (`methods`, `function`, etc.)
        - Lint passes (late vs early)
    - **Write a lint with `LateLintPass` (elaborative)**
        - Define the lint
        - Lint registration
        - Writing tests
        - Emitting the lint
        - Add lint logic
            - (TBD) Teach how to check for type
            - (TBD) Teach how to check for certain trait
            - (TBD) Teach how to access items by IDs
        - Documentation
    - **Write a lint with `EarlyLintPass` (copy & condense existing content)**
        - Define the lint
        - Lint registration
        - Writing tests
        - Emitting the lint
        - Add lint logic
        - Documentation
    - **Checklist for PR**
        - Display PR Checklist items
        - How to install `rustfmt` and run `cargo dev fmt`
    - **Useful tips & Cheatsheet**
        - Specifying the lint's minimum supported Rust version (MSRV)
        - Author lint
        - Print HIR lint (or using`#[clippy::dump`)
        - Debugging
        - Cheatsheet
    - **Adding configuration to a lint**

Note: Per suggestion from @xFrednet, we should also update CONTRIBUTING.md and emphasize and make it extremely clear that newcomers should not hesitate to reach out to maintainers for help and no question is too stupid to ask. Additionally, CONTRIBUTING.md should be edited along with the Clippy book so that we strike a balance between the information displayed in CONTRIBUTING.md (which should welcome newcomers and lead them to sections in the Clippy book that's helpful while the Clippy book should be the main reference for all things related to developing lints.

Let me know what you think? :)

@flip1995
Copy link
Member

I would make the adding lints section as short as possible. Instead of a big chapter about adding lints, I would introduce multiple chapters on how to do certain things and then link there. E.g. One chapter about testing, one about type checking, one about lint passes, and so on.

The adding lints chapter should then only consist of the shortest "happy path" on how to add lints. All details and in depth information will be in the other chapters.

My motivation for this approach is, that everything should be easy to find and not hidden in the adding lints chapter, like it currently is.


Regarding the contributing.md file: I wrote a short Todo list in the dev Readme of the book about things that still have to be moved from there into the book. Doing this would be a great first step. Then we can focus on the contributing.md file on just encouraging contributions.

@nahuakang
Copy link
Contributor

nahuakang commented Aug 20, 2022

@flip1995 I like your suggestion :) How does this look? Did I miss any important bullets?

(Edited to remove the Adding lints bullet and fixed some typos)

- Define and Register a Lint
    - Location for lint tests
    - How to add specific type of lint (`methods`, `function`, etc.)
- Write Lint Tests
    - UI Tests
    - Rustfix Tests
    - UI Cargo
    - UI Toml
    - `cargo dev` commands
- Lint Passes: `EarlyLintPass` & `LateLintPass`
- Emit a Lint
- Type Checking in `LateLintPass`
- Trait Checking in `LateLintPass`
- Access Items by Id in `LateLintPass`
- Ådd Documentation to Your New Lint
- Example: (Concise) Write a lint with `LateLintPass`
    - Define the lint
    - Lint registration
    - Writing tests
    - Emitting the lint
    - Add lint logic
    - Documentation
- Example: (Concise) Write a lint with `EarlyLintPass`
    - Define the lint
    - Lint registration
    - Writing tests
    - Emitting the lint
    - Add lint logic
    - Documentation
- Checklist for Lint PRs
    - Display PR Checklist items
    - How to install `rustfmt` and run `cargo dev fmt`
- Useful Tips & Cheatsheet
    - Specifying the lint's minimum supported Rust version (MSRV)
    - Author lint
    - Print HIR lint (or using`#[clippy::dump`)
    - Debugging
    - Cheatsheet
- Adding Configuration to a Lint

@flip1995
Copy link
Member

flip1995 commented Aug 20, 2022

I didn't really have complaints about the first ToC you s'ent.

My issue is that this is grouped under the adding lints chapter. Maybe my plan becomes clearer with a concrete example:

Let's look at testing: there should be one chapter about all that exists for testing:

  • ui tests
  • rust fix
  • ui cargo
  • ui toml
  • cargo dev commands
  • ...

(not necessarily in that order)

This chapter should be on the same level in the book as the adding lints chapter.

Then in the testing section in adding lints it should only mention how to add and run ui tests and maybe the bless command.

And each section in adding lints should be as short. it should be a brief cooking recipe on how to bake bread and not also include instructions on how to make a cake.

@nahuakang
Copy link
Contributor

Ah yes we're on the same page: I actually wanted to eliminate that Adding lints umbrella and flatten it out but forgot to do so 😆 Updated the message above now.

@xFrednet
Copy link
Member Author

I like @flip1995's suggestion as well. Thank you for working on the documentation ❤️

@xFrednet xFrednet assigned nahuakang and unassigned xFrednet Aug 20, 2022
@flip1995
Copy link
Member

Ah ok, some of the things are still a bit too fine grained I think. But we can figure this out once the actual documentation is written. I wouldn't write two examples for both lint passes though. Just one for late lint passes and then link to the early vs late documentation.

Content LGTM. Though, I would be careful not to duplicate things that already exist in the rustc-dev-guide. We can also just link to there instead of rewriting it ourselves. So before you start writing documentation for something, I would check the dev-guide.

@xFrednet
Copy link
Member Author

xFrednet commented Sep 1, 2022

Our documentation currently still references the if_chain! macro, while we slowly want to move towards if let chains. @nahuakang If possible, it would be cool to also use the new let chains in the documentation. Keeping a reference to the if_chain! macro might still be good for bug fixes etc. 🙃

@nahuakang
Copy link
Contributor

Yes great point :) @flip1995 has already corrected me twice on using if_chain! and I edited the if_chain! into the let_chain here so it's great to document it and save people some time (since if_chain! exists everywhere and people might think it's the standard way).

@nahuakang
Copy link
Contributor

@flip1995 @xFrednet I edited/wrote a define_lints.md chapter here: https://github.com/nahuakang/rust-clippy/blob/book-latelint-updates/book/src/development/define_lints.md. Would like to know if you think the tone and the writing style is within your expectation/vision? Please give some feedback so I can continue the updates with a consistent style 🙏 😄 Cheers!

@xFrednet
Copy link
Member Author

xFrednet commented Sep 4, 2022

Hey @nahuakang awesome, looking forward to reading it! Do you maybe want to create a draft PR, so we can point out specific things, or do you just want feedback on the general style? 🙃

@nahuakang
Copy link
Contributor

Just did :) It's just a short chapter because I want to know if the writing is in line with your preference before I write more 👍

@nahuakang
Copy link
Contributor

@flip1995 @xFrednet 👋 Hope weekend is going well! I just pushed the late vs early lint pass chapter and I think it could use some feedback early on because my knowledge is limited and maybe you can come up with feedback/examples to make it great :) Thanks!

bors added a commit that referenced this issue Jun 10, 2023
Direct towards late passes in `cargo dev new_lint`

changelog: none

This would be the tooling part of #9311

- `--pass late` is now the default
- It prints a message recommending the use of a late pass if you choose `--pass early`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants