From af87f96856d701fc27e39ebe88c58e008cba67c6 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sat, 10 Sep 2022 21:09:16 +0200 Subject: [PATCH] Add lint_passes chapter --- book/src/SUMMARY.md | 3 + book/src/development/define_lints.md | 189 +++++++++++++++++++++++++++ book/src/development/lint_passes.md | 139 ++++++++++++++++++++ 3 files changed, 331 insertions(+) create mode 100644 book/src/development/define_lints.md create mode 100644 book/src/development/lint_passes.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 0b945faf9b78..bd27d946fee3 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -12,6 +12,9 @@ - [Development](development/README.md) - [Basics](development/basics.md) - [Adding Lints](development/adding_lints.md) + - [Defining Lints](development/define_lints.md) + - [Writing Tests](development/write_tests.md) + - [Lint Passes](development/lint_passes.md) - [Common Tools](development/common_tools_writing_lints.md) - [Infrastructure](development/infrastructure/README.md) - [Syncing changes between Clippy and rust-lang/rust](development/infrastructure/sync.md) diff --git a/book/src/development/define_lints.md b/book/src/development/define_lints.md new file mode 100644 index 000000000000..d08be13746be --- /dev/null +++ b/book/src/development/define_lints.md @@ -0,0 +1,189 @@ +# Define New Lints + +The first step in the journey of a new lint is to define the definition +and registration of the lint in Clippy's codebase. +We can use the Clippy dev tools to handle this step since setting up the +lint involves some boilerplate code. + +## Name the Lint + +A wise software engineer Phil Karlton once said: +> There are only two hard things in Computer Science: cache invalidation and naming things. + +Naming a lint is no exception. +Therefore, in case of uncertainty about if the name you chose fits the lint, +please do the following: + +1. Consult our [lint naming guidelines][lint_naming] +2. Ping a [Clippy team member][clippy_team_members] in our [Zulip] chat +3. Comment on the corresponding Github issue (less preferrable as comments might be overlooked) + +For now, let's imagine that your fellow developers at work tend to define some of their +functions with the name `foo` and forget to re-name it to a more meaningful name +when they submit a pull request. + +`foo` is a highly non-descriptive name for a function, so we want to detect this +bad naming and fix it early on in the development process. + +For this, we will create a lint that detects these `foo` functions and +help our fellow developers correct this bad practice. Note that in Clippy, +lints are generally written in snake cases. +We can name this new lint `foo_functions`. + +## Add and Register the Lint + +Now that a name is chosen, we shall register `foo_functions` as a lint to the codebase. +There are two ways to register a lint. + +### Standalone + +If you believe that this new lint is a standalone lint, you can run the following +command in your Clippy project: + +```sh +$ cargo dev new_lint --name=foo_functions --pass=late --category=pedantic +``` + +There are two things to note here: + +1. We set `--pass=late` in this command to do a late lint pass. The alternative +is an `early` lint pass. We will discuss this difference in [Lint Passes](lint_passes.md). +1. If not provided, the `category` of this new lint will default to `nursery`. +See Clippy's [lint groups](../lints.md) for more information on categories. + +The `cargo dev new_lint` command will create a new file: `clippy_lints/src/foo_functions.rs` +as well as [register the lint](#lint-registration). + +Overall, you should notice that the following files are modified or created: + +```sh +$ git status +On branch foo_functions +Changes not staged for commit: + (use "git add ..." to update what will be committed) + (use "git restore ..." to discard changes in working directory) + modified: CHANGELOG.md + modified: clippy_lints/src/lib.register_lints.rs + modified: clippy_lints/src/lib.register_pedantic.rs + modified: clippy_lints/src/lib.rs + modified: src/docs.rs + +Untracked files: + (use "git add ..." to include in what will be committed) + clippy_lints/src/foo_functions.rs + src/docs/foo_functions.txt + tests/ui/foo_functions.rs +``` + +### Specific Type + +If you believe that this new lint belong to a specific type of lints, +you can run `cargo dev new_lint` with a `--type` option. + +Since our `foo_functions` lint is related to function calls, one could +argue that we should put it into a group of lints that detect some behaviors +of functions, we can put it in the `functions` group. + +Let's run the following command in your Clippy project: + +```sh +$ cargo dev new_lint --name=foo_functions --type=functions --category=pedantic +``` + +This command will create, among other things, a new file: +`clippy_lints/src/{type}/foo_functions.rs`. +In our case, the path will be `clippy_lints/src/functions/foo_functions.rs`. + +Notice how this command has a `--type` flag instead of `--pass`. Unlike a standalone +definition, this lint won't be registered in the traditional sense. Instead, you will +call your lint from within the type's lint pass, found in `clippy_lints/src/{type}/mod.rs`. + +A _type_ is just the name of a directory in `clippy_lints/src`, like `functions` in +the example command. Clippy groups together some lints that share common behaviors, +so if your lint falls into one, it would be best to add it to that type. +Read more about [lint groups](#lint-groups) below. + +Overall, you should notice that the following files are modified or created: + +```sh +$ git status +On branch foo_functions +Changes not staged for commit: + (use "git add ..." to update what will be committed) + (use "git restore ..." to discard changes in working directory) + modified: CHANGELOG.md + modified: clippy_lints/src/functions/mod.rs + modified: clippy_lints/src/lib.register_lints.rs + modified: clippy_lints/src/lib.register_pedantic.rs + modified: src/docs.rs + +Untracked files: + (use "git add ..." to include in what will be committed) + clippy_lints/src/functions/foo_functions.rs + src/docs/foo_functions.txt + tests/ui/foo_functions.rs +``` + +## Lint registration + +If we run the `cargo dev new_lint` command for a new lint, +the lint will be automatically registered and there is nothing more to do. + +However, sometimes we might want to declare a new lint by hand. +In this case, we'd use `cargo dev update_lints` command. + +When `cargo dev update_lints` is used, we might need to register the lint pass +manually in the `register_plugins` function in `clippy_lints/src/lib.rs`: + +```rust +store.register_early_pass(|| Box::new(foo_functions::FooFunctions)); +``` + +As you might have guessed, where there's something early, there is something late: +in Clippy there is a `register_late_pass` method as well. +More on early vs. late passes in a later chapter. + +Without a call to one of `register_early_pass` or `register_late_pass`, +the lint pass in question will not be run. + +One reason that `cargo dev update_lints` does not automate this step is that +multiple lints might use the same lint pass, so registering the lint pass may +have already been done when adding a new lint. + +Another reason for not automating this step is that the order +that the passes are registered determines the order the passes actually run, +which in turn affects the order that any emitted lints are output in. + +## Lint groups + +As of the writing of this documentation update, there are 11 categories (a.k.a. _types_) +of lints besides the numerous standalone lints living under `clippy_lints/src/`: + +- `cargo` +- `casts` +- `functions` +- `loops` +- `matches` +- `methods` +- `misc_early` +- `operators` +- `transmute` +- `types` +- `unit_types` + +These categories group together lints that share some common behaviors. +For instance, as we have mentioned earlier, `functions` groups together lints +that deal with some aspects of function calls in Rust. + +Some other common categories are `loops` and `methods`. `loops` group is for +lints that involve `for` loops, `while` loops, `range` loops, etc. +`methods` group is for lints that target method calls. + +For more information, feel free to compare the lint files under any category +with [All Clippy lints][all_lints] or +ask one of the maintainers. + +[all_lints]: https://rust-lang.github.io/rust-clippy/master/ +[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints +[clippy_team_members]: https://www.rust-lang.org/governance/teams/dev-tools#Clippy%20team +[Zulip]: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy diff --git a/book/src/development/lint_passes.md b/book/src/development/lint_passes.md new file mode 100644 index 000000000000..d40858670c2a --- /dev/null +++ b/book/src/development/lint_passes.md @@ -0,0 +1,139 @@ +# Lint passes + +Before working on the logic of a new lint, there is an important decision +that every Clippy developers must make: to use +[`EarlyLintPass`][early_lint_pass] or [`LateLintPass`][late_lint_pass]. + +In short, the `LateLintPass` has access to type information while the +`EarlyLintPass` doesn't. If you don't need access to type information, use the +`EarlyLintPass`. + +- [Lint passes](#lint-passes) + - [`EarlyLintPass`](#earlylintpass) + - [Example](#example) + - [`LateLintPass`](#latelintpass) + - [Example](#example-1) + - [Additional Readings for Beginners](#additional-readings-for-beginners) + +## `EarlyLintPass` + +Inside the documentation of [`EarlyLintPass`][early_lint_pass], we can see that +most methods defined for this trait utilizes [`EarlyContext`][early_context]. +In `EarlyContext`'s documentation, it states: + +> Context for lint checking of the AST, after expansion, before lowering to HIR. + +As entailed, `EarlyLintPass` works only on the Abstract Syntax Tree (AST) level. +Since AST is generated during the [lexing and parsing][lexing_and_parsing] phase +of code compilation, so `EarlyLintPass` is a good choice for a new lint if +the lint only deals with syntax-related issues. + +While linting speed has not been a concern for Clippy, the `EarlyLintPass` is +faster and it should be your choice if you know for sure a lint does not need +type information. + +As a reminder, run the following command to generate boilerplates for lints +using `EarlyLintPass`: + +```sh +$ cargo dev new_lint --name= --pass=early --category= +``` + +### Example + +Take a look at the following code: + +```rust +let x = OurUndefinedType; +x.non_existing_method(); +``` + +From the AST perspective, both lines are "gramatically" correct. +The assignment uses a `let` and ends with a semicolon. The invocation +of a method looks fine, too. As programmers, we might raise a few +questions already, but the parser is okay with it. This is what we +mean when we say `EarlyLintPass` deals with only syntax on the AST level. + +Think about the `foo_functions` lint we mentioned in +[define new lints](define_lints.md#lint-name) chapter. + +We want the `foo_functions` lint to detect functions with `foo` as their name. +Writing a lint that only checks for the name of a function means that we only +work with the AST and don't have to access the type system at all (that is where +`LateLintPass` comes into the picture). + +## `LateLintPass` + +In contrast to `EarlyLintPass`, `LateLintPass` contains type information. + +Examining the documentation on [`LateLintPass`][late_lint_pass] closely, +we can observe that just about every method defined for this trait utilizes a +[`LateContext`][late_context]. + +In `LateContext`'s documentation we will find methods that +deal with type-checking which do not exist in `EarlyContext`, such as: + +- [`maybe_typeck_results`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/context/struct.LateContext.html#method.maybe_typeck_results) +- [`typeck_results`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/context/struct.LateContext.html#method.typeck_results) + +### Example + +Let us take a look with the following example: + +```rust +let x = OurUndefinedType; +x.non_existing_method(); +``` + +These two lines of code are syntactically correct code from the perspective +of the AST. We have an assignment and invoke a method on the variable that +is of a type. Gramatically, everything is in order for the parser. + +However, going down a level and looking at the type information, +the compiler will notice that both `OurUndefinedType` and `non_existing_method()` +are undefined. + +To access such type information means to implement `LateLintPass` on a lint. +When you browse through Clippy's lints, you will notice that almost every lint +is implemented in a `LateLintPass`, specifically because we often need to check +not only for syntactic issues but also type information. + +Moreover, in `EarlyLintPass`, nodes are only identified by their position in the AST. +This means that you can't just get an `id` and request a certain node. +For most lints that is fine, but we have some lints that require the inspection of other nodes, +which is easier at the HIR level. In these cases, `LateLintPass` is the better choice. + +As a reminder, run the following command to generate boilerplates for lints +using `LateLintPass`: + +```sh +$ cargo dev new_lint --name= --pass=late --category= +``` + +## Additional Readings for Beginners + +Should a reader be relatively new to the linting world or has never taken a class on compilers +and interpreters, it might be confusing as to why AST level deals with only +the language's syntax. And some readers might not even understand what lexing, +parsing, and AST mean. + +This documentation serves by no means as a crash course on compilers or language design. +And for details specifically related to Rust, the [Rustc Development Guide][rustc_dev_guide] +is a far better choice to peruse. + +The [Syntax and AST][ast] chapter and the [High-Level IR][hir] chapter are +great introduction to the concepts mentioned in this chapter. + +Some readers might also find the [introductory chapter][map_of_territory] of +Robert Nystrom's _Crafting Interpreters_ a helpful overview of compiled and +interpreted languages before jumping back to the Rustc guide. + +[ast]: https://rustc-dev-guide.rust-lang.org/syntax-intro.html +[early_context]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/context/struct.EarlyContext.html +[early_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html +[hir]: https://rustc-dev-guide.rust-lang.org/hir.html +[late_context]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/context/struct.LateContext.html +[late_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html +[lexing_and_parsing]: https://rustc-dev-guide.rust-lang.org/overview.html#lexing-and-parsing +[rustc_dev_guide]: https://rustc-dev-guide.rust-lang.org/ +[map_of_territory]: https://craftinginterpreters.com/a-map-of-the-territory.html