From 7612603ca5e7db8eddfc989b60ac6251522611f7 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sun, 4 Sep 2022 17:19:06 +0200 Subject: [PATCH 01/13] Add define_lints chapter --- book/src/SUMMARY.md | 1 + book/src/development/define_lints.md | 189 +++++++++++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 book/src/development/define_lints.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 0b945faf9b78..fdd1b21dd7fe 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -12,6 +12,7 @@ - [Development](development/README.md) - [Basics](development/basics.md) - [Adding Lints](development/adding_lints.md) + - [Define Lints](development/define_lints.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..bf781778d349 --- /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 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 a later chapter. +2. 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 From 7910894368f603b1fea66a25fd5528113054c059 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Tue, 6 Sep 2022 18:55:30 +0200 Subject: [PATCH 02/13] Add write_tests chapter --- book/src/SUMMARY.md | 3 +- book/src/development/define_lints.md | 2 +- book/src/development/write_tests.md | 198 +++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 2 deletions(-) create mode 100644 book/src/development/write_tests.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index fdd1b21dd7fe..c44ab0af62d8 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -12,7 +12,8 @@ - [Development](development/README.md) - [Basics](development/basics.md) - [Adding Lints](development/adding_lints.md) - - [Define Lints](development/define_lints.md) + - [Defining Lints](development/define_lints.md) + - [Writing Tests](development/write_tests.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 index bf781778d349..aa70e6547d9f 100644 --- a/book/src/development/define_lints.md +++ b/book/src/development/define_lints.md @@ -7,7 +7,7 @@ lint involves some boilerplate code. ## Name the Lint -A wise software engineer once said: +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. diff --git a/book/src/development/write_tests.md b/book/src/development/write_tests.md new file mode 100644 index 000000000000..e59785a8e299 --- /dev/null +++ b/book/src/development/write_tests.md @@ -0,0 +1,198 @@ +# Testing + +Uncle Bob Martin once said: +> "Nothing makes a system more flexible than a suite of tests." + +Developing lints for Clippy is a Test-Driven Development (TDD) process because +our first task before implementing any logic for a new lint is to write some test cases. + +## Develop Lint with Tests + +It is not necessarily that every Clippy developer enjoys having the compiler screaming +test failures in their face. Some embrace TDD, others learn to peacefully co-exist with it. + +When we develop Clippy, we enter a complex and chaotic realm full of +programmatic issues, stylistic errors, illogical code and non-adherence to convention. +Tests are the first layer of order we can leverage to define when and where +we want a new lint to trigger or not. + +Moreover, writing tests first help Clippy developers to find a balance for +the first iteration of and further enhancements for a lint. +With test cases on our side, we will not have to worry about over-engineering +a lint on its first version nor missing out some obvious edge cases of the lint. +This approach empowers us to iteratively enhance each lint. + +## Clippy UI Tests + +We use **UI tests** for testing in Clippy. +These UI tests check that the output of Clippy is exactly as we expect it to be. +Each test is just a plain Rust file that contains the code we want to check. + +The output of Clippy is compared against a `.stderr` file. +Note that you don't have to create this file yourself. +We'll get to generating the `.stderr` files with the command `cargo dev bless` later on. + +### Write Test Cases + +Let us now think about some tests for our imaginary `foo_functions` lint, +which we have discussed in the [previous chapter](define_lints.md#name-the-lint). +We start by opening the test file `tests/ui/foo_functions.rs` that was created by +the `cargo dev new_lint` command for adding a new lint. + +Update the file with some examples to get started: + +```rust +#![warn(clippy::foo_functions)] + +// Impl methods +struct A; +impl A { + pub fn fo(&self) {} + pub fn foo(&self) {} + pub fn food(&self) {} +} + +// Default trait methods +trait B { + fn fo(&self) {} + fn foo(&self) {} + fn food(&self) {} +} + +// Plain functions +fn fo() {} +fn foo() {} +fn food() {} + +fn main() { + // We also don't want to lint method calls + foo(); + let a = A; + a.foo(); +} +``` + +Without actual lint logic to emit the lint when we see a `foo` function name, +these tests are still quite meaningless. +However, we can now run the test with the following command: + +```sh +$ TESTNAME=foo_functions cargo uitest +``` + +Clippy will compile and it will conclude with an `ok` for the tests: + +``` +...Clippy warnings and test outputs... + +test compile_test ... ok + +test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.48s +``` + +This is normal. After all, we wrote a bunch of Rust code but we haven't really +implemented any logic for Clippy to detect `foo` functions and emit a lint. + +As we gradually implement our lint logic, we will keep running this UI test command. +Clippy will begin outputting information that allows us to check if the output is +turning into what we want it to be. + +> _Note:_ You can run multiple test files by specifying a comma separated list: +> `TESTNAME=foo_functions,bar_methods,baz_structs`. + +### `cargo dev bless` + +Once we are satisfied with the output, we need to run the following command to +generate or update the `.stderr` file for our lint: + +```sh +$ TESTNAME=foo_functions cargo uitest +$ cargo dev bless +``` + +This will format the `.stderr` file to include the emitted lint suggestions and fixes to the test file, +with the reason for the lint, suggested fixes, and line numbers, etc. + +> _Note:_ we should run `TESTNAME=foo_functions cargo uitest` every time before we run +> `cargo dev bless`. + +Running `TESTNAME=foo_functions cargo uitest` should pass then. When we +commit our lint, we need to commit the generated `.stderr` files, too. + +In general, you should only commit files changed by `cargo dev bless` for the +specific lint you are creating/editing. + +> _Note:_ If the generated `.stderr`, `.txt` and `.fixed` files are empty, +> they should be removed. + +## Cargo Lints + +The process of testing is different for Cargo lints in that now we are +interested in the `Cargo.toml` manifest file. +In this case, we also need a minimal crate associated with that manifest. + +Imagine we have a new lint that is named `foo_categories`, we can run: + +```sh +$ cargo dev new_lint --name=foo_categories --pass=late --category=cargo +``` + +After running `cargo dev new_lint` we will find by default two new crates, +each with its manifest file: + +* `tests/ui-cargo/foo_categories/fail/Cargo.toml`: this file should cause the + new lint to raise an error. +* `tests/ui-cargo/foo_categories/pass/Cargo.toml`: this file should not trigger + the lint. + +If you need more cases, you can copy one of those crates (under `foo_categories`) and rename it. + +The process of generating the `.stderr` file is the same as for other lints +and prepending the `TESTNAME` variable to `cargo uitest` works for Cargo lints too. + +Overall, you should see the following changes when you generate a new Cargo lint: + +```sh +$ git status +On branch foo_categories +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/cargo/mod.rs + modified: clippy_lints/src/lib.register_cargo.rs + modified: clippy_lints/src/lib.register_lints.rs + modified: src/docs.rs + +Untracked files: + (use "git add ..." to include in what will be committed) + clippy_lints/src/cargo/foo_categories.rs + src/docs/foo_categories.txt + tests/ui-cargo/foo_categories/ +``` + +## Rustfix Tests + +If the lint you are working on is making use of structured suggestions, the test +file should include a `// run-rustfix` comment at the top. + +What are structured suggestions? They are suggestions that tell a user how to +fix or re-write certain code that has been linted. + +The `// run-rustfix` comment will additionally run [rustfix] for our test. +Rustfix will apply the suggestions from the lint to the code of the test file and +compare that to the contents of a `.fixed` file. + +Use `cargo dev bless` to automatically generate the `.fixed` file after running the tests. + +[rustfix]: https://github.com/rust-lang/rustfix +## Testing Manually + +Manually testing against an example file can be useful if you have added some +`println!`s and the test suite output becomes unreadable. + +To try Clippy with your local modifications, run from the working copy root. + +```sh +$ cargo dev lint input.rs +``` From 3290f5eca143dc29f9fb30eb33c7793c86a870df Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sat, 10 Sep 2022 21:09:16 +0200 Subject: [PATCH 03/13] Add lint_passes chapter --- book/src/SUMMARY.md | 1 + book/src/development/define_lints.md | 4 +- book/src/development/lint_passes.md | 137 +++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 book/src/development/lint_passes.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index c44ab0af62d8..bd27d946fee3 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -14,6 +14,7 @@ - [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 index aa70e6547d9f..d08be13746be 100644 --- a/book/src/development/define_lints.md +++ b/book/src/development/define_lints.md @@ -47,8 +47,8 @@ $ 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 a later chapter. -2. If not provided, the `category` of this new lint will default to `nursery`. +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` diff --git a/book/src/development/lint_passes.md b/book/src/development/lint_passes.md new file mode 100644 index 000000000000..35cb46a07df1 --- /dev/null +++ b/book/src/development/lint_passes.md @@ -0,0 +1,137 @@ +# 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]. + +It is an either-or kind of decision. +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`. + +Let us expand on these two traits more below. + +## `EarlyLintPass` + +If you examine the documentation on [`EarlyLintPass`][early_lint_pass] closely, +you see that just about every method defined for this trait utilizes a +[`EarlyContext`][early_context]. In `EarlyContext`'s documentation, it states: + +> Context for lint checking of the AST, after expansion, before lowering to HIR. + +Voilà. `EarlyLintPass` works only on the Abstract Syntax Tree (AST) level. +And AST is generated during the [lexing and parsing][lexing_and_parsing] phase +of code compilation. Therefore, this is our trait 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. + +Alternatively, think of the `foo_functions` lint we mentioned in +[define new lints](define_lints.md#name-the-lint) 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. + +If you examine the documentation on [`LateLintPass`][late_lint_pass] closely, +you see 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. + +As Clippy developers, to access such type information, we must implement +`LateLintPass` on our 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. + +Another limitation of the `EarlyLintPass` is that the 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 + +If a dear reader of this documentation 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 From faf52c9ca62e42c6af0e0ea7543f0e92daf5ae7b Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sat, 17 Sep 2022 16:48:13 +0200 Subject: [PATCH 04/13] Add emit lints chapter --- book/src/SUMMARY.md | 1 + book/src/development/emit_lints.md | 151 ++++++++++++++++++++++++++++ book/src/development/lint_passes.md | 4 +- 3 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 book/src/development/emit_lints.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index bd27d946fee3..24af2870d32f 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -15,6 +15,7 @@ - [Defining Lints](development/define_lints.md) - [Writing Tests](development/write_tests.md) - [Lint Passes](development/lint_passes.md) + - [Emitting Lints](development/emit_lints.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/emit_lints.md b/book/src/development/emit_lints.md new file mode 100644 index 000000000000..335332f40aa5 --- /dev/null +++ b/book/src/development/emit_lints.md @@ -0,0 +1,151 @@ +# Emitting a lint + +Once we have [defined a lint](define_lints.md), written [UI tests](write_tests.md) +and chosen [the lint pass](lint_passes.md) for the lint, we can begin the +implementation of the lint logic so that we can emit the lint and gradually work +towards a lint that behaves as expected. + +We will look into how we can emit alint for both `LateLintPass` and `EarlyLintPass`. +Note that we will not go into concrete implementation of a lint logic in this +chapter. We will go into details in later chapters as well as in two examples +of real Clippy lints. + +## `LateLintPass` + +To emit a lint with `LateLintPass`, we must implement it for the lint that we have +declared. Take a look at the [LateLintPass][late_lint_pass] documentation, which +provides an abundance of methods that we can implement for our lint. + +```rust +pub trait LateLintPass<'tcx>: LintPass { + // Trait methods +} +``` + +By far the most common method used for Clippy lints is [`check_expr` method][late_check_expr], +this is likely because Rust is an expression language and, more often than not, +the lint we want to work on must examine expressions. + +> _Note:_ If you don't fully understand what expressions are in Rust, +> take a look at the official documentation on [expressions][rust_expressions] + +Other common ones include [`check_fn` method][late_check_fn] and +[`check_item` method][late_check_item]. We choose to implement whichever trait +method based on what we need for the lint at hand. + +### Implement Trait Method in `LateLintPass` + +Assume that we have added and defined a `BarExpressions` lint, we could write +down a skeleton for this lint as the following: + +```rust +impl<'tcx> LateLintPass<'tcx> for BarExpressions { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {} +} +``` + +### Emitting Lint in `LateLintPass` + +Inside the trait method that we implement, we can write down the lint logic +and the emit the lint with suggestions. + +Clippy's [diagnostics] provides quite a few diagnositc functions that we can +use to emit lints. Take a look at the documentation to pick one that suits +your lint's needs the best. Some common ones you will encounter in the Clippy +repository includes: + +- [span_lint_and_help]: emits lint and provides a helpful message +- [span_lint_and_sugg]: emits lint and provides a suggestion to fix the code + +```rust +impl<'tcx> LateLintPass<'tcx> for BarExpressions { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + // Imagine a `some_bar_expr_logic` that checks for requirements for emitting the lint + if some_bar_expr_logic(expr) { + span_lint_and_help( + cx, + FOO_FUNCTIONS, + expr.span, + "message on why the lint is emitted", + None, + "message that provides a helpful suggestion", + ); + } + } +} +``` + +> Note: According to [the rustc-dev-guide], the message should be matter of fact and avoid +> capitalization and periods, unless multiple sentences are needed. When code or +> an identifier must appear in a message or label, it should be surrounded with +> single grave accents \`. + +## `EarlyLintPass` + +Emitting a lint with `EarlyLintPass` follows the same logic like `LateLintPass`. +Take a look at the [EarlyLintPass][early_lint_pass] documentation, which +provides an abundance of methods that we can implement for our lint. + +```rust +pub trait EarlyLintPass: LintPass { + // Trait methods +} +``` + +### Implement Trait Method in `EarlyLintPass` + +Similar to `LateLintPass`, we pick a trait method to implement if we choose +`EarlyLintPass` for a specific lint we want to design. Using `FooFunctions` +as an example, we will use [`check_fn`][early_check_fn] since it gives us +access to various information about the function that is currently being checked. + +```rust +impl EarlyLintPass for FooFunctions { + fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, span: Span, _: NodeId) { + // TODO: Emit lint here + } +} +``` + +### Emitting Lint in `EarlyLintPass` + +To emit the lint with `EarlyLintPass`, use `span_lint_and_help` +to provide an extra help message. This is how it looks: + +```rust +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) { + span_lint_and_help( + cx, + FOO_FUNCTIONS, + span, + "function named `foo`", + None, + "consider using a more meaningful name" + ); + } + } +} +``` + +## Run UI Tests to Emit the Lint + +Now, if we run our [UI test](write_tests.md), we should see that the compiler now +produce output that contains the lint message we designed. + +The next step is to implement the logic properly, which is a detail that we will +cover in the next chapters. + +[diagnostics]: https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/diagnostics/index.html +[early_check_fn]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html#method.check_fn +[early_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html +[late_check_expr]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_expr +[late_check_fn]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_fn +[late_check_item]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_item +[late_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html +[rust_expressions]: https://doc.rust-lang.org/reference/expressions.html +[span_lint_and_help]: https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_help.html +[span_lint_and_sugg]: https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html +[the rustc-dev-guide]: https://rustc-dev-guide.rust-lang.org/diagnostics.html diff --git a/book/src/development/lint_passes.md b/book/src/development/lint_passes.md index 35cb46a07df1..7071eb1df749 100644 --- a/book/src/development/lint_passes.md +++ b/book/src/development/lint_passes.md @@ -35,7 +35,7 @@ using `EarlyLintPass`: $ cargo dev new_lint --name= --pass=early --category= ``` -### Example +### Example for `EarlyLintPass` Take a look at the following code: @@ -72,7 +72,7 @@ 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 +### Example for `LateLintPass` Let us take a look with the following example: From 3e126161c136806a41accb9c335f10ef3ab01ec7 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Thu, 22 Sep 2022 21:11:58 +0200 Subject: [PATCH 05/13] Add type_checks chapter --- book/src/SUMMARY.md | 1 + book/src/development/type_checks.md | 126 ++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 book/src/development/type_checks.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 24af2870d32f..19d5adc6e985 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -16,6 +16,7 @@ - [Writing Tests](development/write_tests.md) - [Lint Passes](development/lint_passes.md) - [Emitting Lints](development/emit_lints.md) + - [Type Checking](development/type_checks.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/type_checks.md b/book/src/development/type_checks.md new file mode 100644 index 000000000000..7a317da14276 --- /dev/null +++ b/book/src/development/type_checks.md @@ -0,0 +1,126 @@ +# Type Checking + +When we work on a new lint or improve an existing lint, we might want +to retrieve the type `Ty` of an expression `Expr` for a variety of +reasons. This can be achieved by utilizing the [`LateContext`][LateContext] +that is available for [`LateLintPass`][LateLintPass]. + +## `LateContext` and `TypeckResults` + +The lint context [`LateContext`][LateContext] and [`TypeckResults`][TypeckResults] +(returned by `LateContext::typeck_results`) are the two most useful data structures +in `LateLintPass`. They allow us to jump to type definitions and other compilation +stages such as HIR. + +> Note: `LateContext.typeck_results`'s return value is [`TypeckResults`][TypeckResults] +> and is created by type checking step, it includes useful information such as types of +> expressions, ways to resolve methods and so on. + +`TypeckResults` contains useful methods such as [`expr_ty`][expr_ty], +which gives us access to the underlying structure [`Ty`][Ty] of a given expression. + +```rust +pub fn expr_ty(&self, expr: &Expr<'_>) -> Ty<'tcx> +``` + +A side note, besides `expr_ty`, [`TypeckResults`][TypeckResults] contains a +[`pat_ty()`][pat_ty] method that is useful for retrieving a type from a pattern. + +## `Ty` + +`Ty` struct contains the type information of an expression. +Let us take a look at `rustc_middle`'s [`Ty`][Ty] struct to examine this struct: + +```rust +pub struct Ty<'tcx>(Interned<'tcx, WithStableHash>>); +``` + +At a first glance, this struct looks quite esoteric. But at a closer look, +we will see that this struct contains many useful methods for type checking. + +For instance, [`is_char`][is_char] checks if the given `Ty` struct corresponds +to the primitive character type. + +### `is_*` Usage + +In some scenarios, all we need to do is check if the `Ty` of an expression +is a specific type, such as `char` type, so we could write the following: + +```rust +impl LateLintPass<'_> for MyStructLint { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + // Get type of `expr` + let ty = cx.typeck_results().expr_ty(expr); + + // Check if the `Ty` of this expression is of character type + if ty.is_char() { + println!("Our expression is a char!"); + } + } +} +``` + +Furthermore, if we examine the [source code][is_char_source] for `is_char`, +we find something very interesting: + +```rust +#[inline] +pub fn is_char(self) -> bool { + matches!(self.kind(), Char) +} +``` + +Indeed, we just discovered `Ty`'s [`kind` method][kind], which provides us +with [`TyKind`][TyKind] of a `Ty`. + +## `TyKind` + +`TyKind` defines the kinds of types in Rust's type system. +Peeking into [`TyKind` documentation][TyKind], we will see that it is an +enum of 27 variants, including items such as `Bool`, `Int`, `Adt`, etc. + +### `kind` Usage + +The `TyKind` of `Ty` can be returned by calling [`Ty.kind` method][kind]. +We often use this method to perform pattern matching in Clippy. + +For instance, if we want to check for a `struct`, we could examine if the +`ty.kind` corresponds to an [`Adt`][Adt] (algebraic data type) and if its +[`AdtDef`][AdtDef] is a struct: + +```rust +impl LateLintPass<'_> for MyStructLint { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + // Get type of `expr` + let ty = cx.typeck_results().expr_ty(expr); + // Match its kind to enter its type + match ty.kind { + ty::Adt(adt_def, _) if adt_def.is_struct() => println!("Our `expr` is a struct!"), + _ => () + } + } +} +``` + +## Useful Links + +Below are some useful links to further explore the concepts covered +in this chapter: + +- [Stages of compilation](https://rustc-dev-guide.rust-lang.org/compiler-src.html#the-main-stages-of-compilation) +- [Diagnostic items](https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html) +- [Type checking](https://rustc-dev-guide.rust-lang.org/type-checking.html) +- [Ty module](https://rustc-dev-guide.rust-lang.org/ty.html) + +[Adt]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.TyKind.html#variant.Adt +[AdtDef]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/adt/struct.AdtDef.html +[expr_ty]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TypeckResults.html#method.expr_ty +[is_char]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.is_char +[is_char_source]: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_middle/ty/sty.rs.html#1831-1834 +[kind]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.kind +[LateContext]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/struct.LateContext.html +[LateLintPass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html +[pat_ty]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TypeckResults.html#method.pat_ty +[Ty]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html +[TyKind]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.TyKind.html +[TypeckResults]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TypeckResults.html From 5948c6a2d2cf179af30f6957e2efb3188f626616 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sat, 24 Sep 2022 23:50:04 +0200 Subject: [PATCH 06/13] Add trait_checks chapter --- book/src/SUMMARY.md | 1 + book/src/development/trait_checks.md | 112 +++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 book/src/development/trait_checks.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 19d5adc6e985..a1a28486b613 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -17,6 +17,7 @@ - [Lint Passes](development/lint_passes.md) - [Emitting Lints](development/emit_lints.md) - [Type Checking](development/type_checks.md) + - [Trait Checking](development/trait_checks.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/trait_checks.md b/book/src/development/trait_checks.md new file mode 100644 index 000000000000..4c22d0ea21f6 --- /dev/null +++ b/book/src/development/trait_checks.md @@ -0,0 +1,112 @@ +# Trait Checking + +Besides [type checking](type_checks.md), we might want to examine if +a specific type `Ty` implements certain trait when implementing a lint. +There are three approaches to achieve this, depending on if the target trait +that we want to examine has a [diagnostic item][diagnostic_items], +[lang items][lang_items], or neither. + +## Using Diagnostic Items + +As explained in [rustc dev guide][rustc_dev_guide], diagnostic items +are introduced for identifying types via [Symbols][symbol]. + +While rustc dev guide has [a section][using_diagnostic_items] on +how to check for a specific trait on a type `Ty`, Clippy provides +a utils function `is_trait_method`, which simplifies the process for us. + +For instance, if we want to examine whether an expression implements +the `Iterator` trait, we could simply write the following code, +providing the `LateContext` (`cx`), our expression at hand, and +the symbol of the trait in question: + +```rust +use clippy_utils::is_trait_method; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::symbol::sym; + +impl LateLintPass<'_> for CheckIteratorTraitLint { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if is_trait_method(cx, expr, sym::Iterator) { + println!("This expression implements `Iterator` trait!"); + } + } +} +``` + +> Note: Refer to [this index][symbol_index] for all the defined `Symbol`s. + +## Using Lang Items + +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 +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 +to many Rustaceans. + +For instance, if we want to examine whether an expression `expr` implements +`Drop` trait, we could access `LanguageItems` via our `LateContext`'s +[TyCtxt], which provides a `lang_items` method that will return the id of +`Drop` trait to us. Then, by calling Clippy utils function `implements_trait` +we can check that the `Ty` of the `expr` implements the trait: + +```rust +use clippy_utils::implements_trait; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; + +impl LateLintPass<'_> for CheckDropTraitLint { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + let ty = cx.typeck_results().expr_ty(expr); + if cx.tcx.lang_items() + .drop_trait() + .map_or(false, |id| implements_trait(cx, ty, id, &[])) { + println!("`expr` implements `Drop` trait!"); + } + } +} +``` + +## Using Type Path + +If neither diagnostic item or lang item is available, we can use +Clippy utils [paths] with the `match_trait_method` to determine trait +implementation. + +> Note: This approach should be avoided if possible. + +Below, we check if the given `expr` implements `tokio`'s +[`AsyncReadExt`][AsyncReadExt] trait: + +```rust +use clippy_utils::{match_trait_method, paths}; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; + +impl LateLintPass<'_> for CheckTokioAsyncReadExtTrait { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if match_trait_method(cx, expr, &paths::TOKIO_IO_ASYNCREADEXT) { + println!("`expr` implements `TOKIO_IO_ASYNCREADEXT` trait!"); + } + } +} +``` + +> Note: Even though all the `clippy_utils` methods we have seen in this +> chapter takes `expr` as a parameter, these methods are actually using +> each expression's `HirId` under the hood. + +[AsyncReadExt]: https://docs.rs/tokio/latest/tokio/io/trait.AsyncReadExt.html +[DefId]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def_id/struct.DefId.html +[diagnostic_items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html +[lang_items]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/lang_items/struct.LanguageItems.html +[paths]: https://github.com/rust-lang/rust-clippy/blob/master/clippy_utils/src/paths.rs +[rustc_dev_guide]: https://rustc-dev-guide.rust-lang.org/ +[symbol]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/symbol/struct.Symbol.html +[symbol_index]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_span/symbol/sym/index.html +[TyCtxt]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html +[using_diagnostic_items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html#using-diagnostic-items From ea136e59b7a2e9c867c9c5ebcaea301519de819c Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sun, 25 Sep 2022 13:26:46 +0200 Subject: [PATCH 07/13] Add method_checks chapter --- book/src/SUMMARY.md | 1 + book/src/development/method_checks.md | 92 +++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 book/src/development/method_checks.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index a1a28486b613..1530d9f241f5 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -18,6 +18,7 @@ - [Emitting Lints](development/emit_lints.md) - [Type Checking](development/type_checks.md) - [Trait Checking](development/trait_checks.md) + - [Method Checking](development/method_checks.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/method_checks.md b/book/src/development/method_checks.md new file mode 100644 index 000000000000..56f198db4913 --- /dev/null +++ b/book/src/development/method_checks.md @@ -0,0 +1,92 @@ +# Method Checks + +In some scenarios we might want to check for methods when developing +a lint. There are two kinds of questions that we might be curious about: + +- Invocation: Does an expression call a specific method? +- Definition: Does the type `Ty` of an expression define a method? + +## Checking if an expr is calling a specific method + +Suppose we have an `expr`, we can check whether it calls a specific +method, e.g. `our_fancy_method`, by performing a pattern match on +the [ExprKind] that we can access from `expr.kind`: + +```rust +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::sym; + +impl<'tcx> LateLintPass<'tcx> for OurFancyMethodLint { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { + // Check our expr is calling a method with pattern matching + if let hir::ExprKind::MethodCall(path, _, [_self_arg, ..]) = &expr.kind + // Check the name of this method is `some_method` + && path.ident.name == sym!(our_fancy_method) + // Optionally, we can check the type of the self argument whenever necessary. + // See "Type Checking" chapter of the Clippy book for more information. + { + println!("`expr` is a method call for `our_fancy_method`"); + } + } +} +``` + +Take a closer look at the `ExprKind` enum variant [MethodCall] for more +information on the pattern matching. +As mentioned in [Define Lints](define_lints.md#lint-groups), +the `methods` module is full of pattern matching with `Methodcall` +in case the reader wishes to explore more. + +Additionally, we use Clippy utils [sym] macro to conveniently compare +an input `our_fancy_method` into a `Symbol` since the `ident` [Ident] +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 + +While sometimes we want to check whether a method is being called or not, +other times we want to know if our type `Ty` defines a method. + +To check if our type defines a method called `our_fancy_method`, +we will utilize the [check_impl_item] method that is available +in our beloved [LateLintPass] (for more information, refer to the +[lint pass](lint_passes.md) chapter in Clippy book). +This method provides us with an [ImplItem] struct, which represents +anything within an `impl` block. + +Let us take a look at how we might check for the implementation of +`our_fancy_method` on a type: + +```rust +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::return_ty; +use rustc_hir::{ImplItem, ImplItemKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::symbol::sym; + +impl<'tcx> LateLintPass<'tcx> for MyTypeImpl { + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { + // Check if item is a method/function + if let ImplItemKind::Fn(ref signature, _) = impl_item.kind + // Check the method is named `our_fancy_method` + && impl_item.ident.name == sym!(our_fancy_method) + // We can also check it has a parameter `self` + && signature.decl.implicit_self.has_implicit_self() + // We can go further and even check if its return type is `String` + && is_type_diagnostic_item(cx, return_ty(cx, impl_item.hir_id), sym::String) + { + println!("`our_fancy_method` is implemented!"); + } + } +} +``` + +[check_impl_item]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_impl_item +[ExprKind]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_hir/hir/enum.ExprKind.html +[Ident]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_span/symbol/struct.Ident.html +[ImplItem]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/hir/struct.ImplItem.html +[LateLintPass]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html +[MethodCall]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_hir/hir/enum.ExprKind.html#variant.MethodCall +[PathSegment]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_hir/hir/struct.PathSegment.html +[sym]: https://doc.rust-lang.org/stable/nightly-rustc/clippy_utils/macro.sym.html From daa84dcbe0bac17c716fbadc7be70f706a8cb74b Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Mon, 26 Sep 2022 21:19:35 +0200 Subject: [PATCH 08/13] Add macro_expansions chapter --- book/src/SUMMARY.md | 1 + book/src/development/macro_expansions.md | 163 +++++++++++++++++++++++ 2 files changed, 164 insertions(+) create mode 100644 book/src/development/macro_expansions.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 1530d9f241f5..8f16d139cae7 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -19,6 +19,7 @@ - [Type Checking](development/type_checks.md) - [Trait Checking](development/trait_checks.md) - [Method Checking](development/method_checks.md) + - [Macro Expansions](development/macro_expansions.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/macro_expansions.md b/book/src/development/macro_expansions.md new file mode 100644 index 000000000000..90874b4f912c --- /dev/null +++ b/book/src/development/macro_expansions.md @@ -0,0 +1,163 @@ +# Dealing with macros and expansions + +> "In an expanding universe, order is not really order, +> but merely the difference between the actual entropy exhibited and +> the maximum entropy possible." +> -- Kim Stanley Robinson + +Sometimes we might encounter Rust macro expansions while working with Clippy. +While macro expansions are not as dramatic and profound as the expansion +of our universe, they can certainly bring chaos to the orderly world +of code and logic. + +The general rule of thumb is that we should ignore code with macro +expansions when working with Clippy because the code can be dynamic +in ways that are difficult or impossible for us to foresee. + +## False Positives + +What exactly do we mean by _dynamic in ways that are difficult to foresee_? + +Macros are [expanded][expansion] by the `EarlyLintPass` level, +so the Abstract Syntax Trees (AST) are generated in place of macros. +This means the code which we work with in Clippy is already desugared. + +If we wrote a new lint, there is a possibility that the lint is +triggered in macro-generated code. Since this expanded macro code +is not written by the macro's user but really by the macro's author, +the user cannot and should not be responsible for fixing the issue +that triggers the lint. + +Besides, a [Span] in a macro can be changed by the macro author. +Therefore, any lint check related to lines or columns should be +avoided since they might be changed at any time and become unreliable +or incorrect information. + +Because of these unforeseeable or unstable behaviors, macro expansion +should often not be regarded as a part of the stable API. +This is also why most lints check if they are inside a macro or not +before emitting suggestions to the end user to avoid false positives. + +## How to Work with Macros + +Several functions are available for working with macros. + +### `Span.from_expansion` Method + +We could utilize a `span`'s [from_expansion] method, which +detects if the `span` is from a macro expansion/desugaring. +This is a very common first step in a lint: + +```rust +if expr.span.from_expansion() { + // We most likely want to ignore it. + return; +} +``` + +### `Span.ctxt` method + +The `span`'s context, given by the method [ctxt] and returning [SpanContext], +represents if the span is from a macro expansion nad, if it is, which +macro call expanded this span. + +Sometimes, it is useful to check if the context of two spans are equal. +For instance, suppose we have the following line of code that would +expand into `1 + 0`: + +```rust +// The following code expands to `1 + 0` for both `EarlyLintPass` and `lateLintPass` +1 + mac!() +``` + +Assuming that we'd collect the `1` expression as a variable `left` +and the `0` expression as a variable `right`, we can simply compare +their contexts. If the context is different, then we most likely +are dealing with a macro expansion and should just ignore it: + +```rust +if left.span.ctxt() != right.span.ctxt() { + // The code author most likely cannot modify this expression + return; +} +``` + +> Note: Code that is not from expansion is in the "root" context. +> So any spans whose `from_expansion` returns `true` can be assumed +> to have the same context. Because of this, using `span.from_expansion()` +> is often sufficient. + +Going a bit deeper, in a simple expression such as `a == b`, +`a` and `b` have the same context. +However, in a `macro_rules!` with `a == $b`, `$b` is expanded to +an expression that contains a different context from `a`. + +Take a look at the following macro `m`: + +```rust +macro_rules! m { + ($a:expr, $b:expr) => { + if $a.is_some() { + $b; + } + } +} + +let x: Option = Some(42); +m!(x, x.unwrap()); +``` + +If the `m!(x, x.unwrapp());` line is expanded, we would get two desugarized +code: + +- `x.is_some()` (from the `$a.is_some()` line in the `m` macro) +- `x.unwrap()` (corresponding to `$b` in the `m` macro) + +Suppose `x.is_some()` expression's span is associated with `x_is_some_span` variable +and `x.unwrap()` expression's span is associated with `x_unwrap_span` variable, +we could ascertain that these two spans do not share the same context: + +```rust +// x.is_some() is from inside the macro +// x.unwrap() is from outside the macro +assert_eq!(x_is_some_span.ctxt(), x_unwrap_span.ctxt()); +``` + +### `in_external_macro` Function + +`rustc_middle::lint` provides a function [in_external_macro] that can +detect if the given span is from a macro defined in a foreign crate. + +Therefore, if we really want a new lint to work with macro-generated code, +this is the next line of defense to avoid macros not defined inside +the current crate since it is unfair to the user if Clippy lints code +which the user cannot change. + +For example, assume we have the following code that is being examined +by Clippy: + +```rust +#[macro_use] +extern crate a_foreign_crate_with_macros; + +// `foo` macro is defined in `a_foreign_crate_with_macros` +foo!("bar"); +``` + +Also assume that we get the corresponding variable `foo_span` for the +`foo` macro call, we could decide not to lint if `in_external_macro` +results in `true` (note that `cx` can be `EarlyContext` or `LateContext`): + +```rust +if in_external_macro(cx.sess(), foo_span) { + // We should ignore macro from a foreign crate. + return; +} +``` + +[ctxt]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_span/struct.Span.html#method.ctxt +[expansion]: https://rustc-dev-guide.rust-lang.org/macro-expansion.html#expansion-and-ast-integration +[from_expansion]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_span/struct.Span.html#method.from_expansion +[in_external_macro]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_middle/lint/fn.in_external_macro.html +[Span]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_span/struct.Span.html +[SpanContext]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_span/hygiene/struct.SyntaxContext.html From 3be525ba2bbc2f0ac58c3411480e10e134af5986 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Tue, 4 Oct 2022 20:07:09 +0200 Subject: [PATCH 09/13] Add hir_ids chapter --- book/src/SUMMARY.md | 1 + book/src/development/hir_ids.md | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 book/src/development/hir_ids.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 8f16d139cae7..8d1b5d422f52 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -20,6 +20,7 @@ - [Trait Checking](development/trait_checks.md) - [Method Checking](development/method_checks.md) - [Macro Expansions](development/macro_expansions.md) + - [Using HirId](development/hir_ids.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/hir_ids.md b/book/src/development/hir_ids.md new file mode 100644 index 000000000000..6594bedfc944 --- /dev/null +++ b/book/src/development/hir_ids.md @@ -0,0 +1,78 @@ +# `HirId` in Clippy + +`HirId`s are identifiers for [fine-grained entities][fine_grained_entities] +in the [HIR Map][hir_map] of the high-level intermediate representation (HIR), +which is what we work with during a [LateLintPass]. + +`HirId`s are everywhere in `rustc`. If we open up the Clippy +codebase in a text editor and search `hir_id`, +we will get numerous search results across hundreds of files. + +For instance, variables in `rustc` are identified using the `HirId`. +The variable name alone is not enough, since variables can be shadowed. + +> Note: Besides `HirId`, the HIR has other identifiers such as +> `DefId`, `LocalDefId` and `BodyId`. +> Read more about [identifiers in the HIR][identifiers_in_hir]. + +## Working with `HirId`s in Clippy + +A curious and observant reader of Clippy book might have noticed +that `hir_id`s are hiding right under our noses: the [Expr][hir_expr] +in `LateLintPass`'s [check_expr][late_lint_pass_check_expr] +method contains a `hir_id` field: + +```rust +pub struct Expr<'hir> { + pub hir_id: HirId, + pub kind: ExprKind<'hir>, + pub span: Span, +} +``` + +Alas, `HirId`s are very useful to Clippy developers. Below are some +examples for when we might need `HirId`s. + +## `find_parent_node` method + +One example of working with `HirId`s is the [find_parent_node] method +on `rustc_middle::hir::map::Map` that retrieves a node's parent node. +Assuming that we have a `LateContext` variable `cx` and an `expr` variable, +we could perform: + +```rust +// `LateContext.tcx.hir` returns a `rustc_middle::hir::map::Map` +let map = cx.tcx.hir(); +if let Some(parent_id) = map.find_parent_node(expr.hir_id) { + println!("This node has a parent!"); +} +``` + +## `attrs` method + +Another example could be retrieving all the attributes attached to a node +with the [attrs] method on `rustc_middle::hir::map::Map`. + +Assuming that we have an [Item] variable `item` via a [check_item] method, +we could do the following to retrive all attributes for further usage: + +```rust +fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + let attrs = cx.tcx.hir().attrs(item.hir_id()); + // Do something with the attributes of the `item` node +} +``` + +> Note: Read more on [The HIR Map][hir_map] section about how to convert +> `LocalDefId` to `HirId`, etc. + +[attrs]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/hir/map/struct.Map.html#method.attrs +[check_item]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_item +[find_parent_node]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/hir/map/struct.Map.html#method.find_parent_node +[fine_grained_entities]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/hir/enum.Node.html +[hir_expr]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/hir/struct.Expr.html +[hir_map]: https://rustc-dev-guide.rust-lang.org/hir.html#the-hir-map +[identifiers_in_hir]: https://rustc-dev-guide.rust-lang.org/identifiers.html#in-the-hir +[Item]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/hir/struct.Item.html +[LateLintPass]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html +[late_lint_pass_check_expr]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_expr From 851fd7e26ed37be97412c113e511939474dd6482 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Tue, 4 Oct 2022 20:19:37 +0200 Subject: [PATCH 10/13] Add add_documentation chapter --- book/src/SUMMARY.md | 1 + book/src/development/add_documentation.md | 46 +++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 book/src/development/add_documentation.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 8d1b5d422f52..76d6c450b0b7 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -21,6 +21,7 @@ - [Method Checking](development/method_checks.md) - [Macro Expansions](development/macro_expansions.md) - [Using HirId](development/hir_ids.md) + - [Adding Documentation](development/add_documentation.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/add_documentation.md b/book/src/development/add_documentation.md new file mode 100644 index 000000000000..a288702acf59 --- /dev/null +++ b/book/src/development/add_documentation.md @@ -0,0 +1,46 @@ +# Adding Documentation for New Lint + +Assuming that our dear reader has read all the previous chapters +on development in Clippy, the final thing to do when authoring a +new lint or updating an existing lint is to write or edit the +lint decalaration's documentation. + +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: + +```rust +declare_clippy_lint! { + /// ### What it does + /// Checks for ... (describe what the lint matches). + /// + /// ### Why is this bad? + /// Supply the reason for linting the code. + /// + /// ### Example + /// + /// ```rust,ignore + /// // A short example of code that triggers the lint + /// ``` + /// + /// Use instead: + /// ```rust,ignore + /// // A short example of improved code that doesn't trigger the lint + /// ``` + #[clippy::version = "1.64.0"] + pub FOO_FUNCTIONS, + pedantic, + "function named `foo`, which is not a descriptive name" +} +``` + +Make sure you put some love into writing a good documentation so that +other Clippy users can understand the awesome lint that you just created. + +Once the PR to your lint is merged, this documentation will show up in the +[lint list][lint_list]. Pat yourself on the shoulder and be proud that +you have made Rust that extra little bit more awesome for the world! + +[lint_list]: https://rust-lang.github.io/rust-clippy/master/index.html From 99fe34bb825510011286e57a37a02bcf0ed938db Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Mon, 10 Oct 2022 23:01:36 +0200 Subject: [PATCH 11/13] Add example_early chapter for early lint pass example --- book/src/SUMMARY.md | 1 + book/src/development/example_early.md | 406 ++++++++++++++++++++++++++ 2 files changed, 407 insertions(+) create mode 100644 book/src/development/example_early.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 76d6c450b0b7..9c8b14c58c58 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -21,6 +21,7 @@ - [Method Checking](development/method_checks.md) - [Macro Expansions](development/macro_expansions.md) - [Using HirId](development/hir_ids.md) + - [Example: Early Lint](development/example_early.md) - [Adding Documentation](development/add_documentation.md) - [Common Tools](development/common_tools_writing_lints.md) - [Infrastructure](development/infrastructure/README.md) diff --git a/book/src/development/example_early.md b/book/src/development/example_early.md new file mode 100644 index 000000000000..413823d4ccae --- /dev/null +++ b/book/src/development/example_early.md @@ -0,0 +1,406 @@ +# Example with `EarlyLintPass` + +Let us create a Clippy lint that implements `EarlyLintPass` +so that we can put the knowledge from all previous chapters into practice. + +> Note: This lint is actually implemented in Clippy. +> If you are curious, feel free to check out the actual implementation +> of this lint example here: [pub_use]. + +- [Example with `EarlyLintPass`](#example-with-earlylintpass) + - [The Problem: Pub Use](#the-problem-pub-use) + - [Define the Lint](#define-the-lint) + - [Write UI Tests](#write-ui-tests) + - [Implement the Lint](#implement-the-lint) + - [Implement `check_item` Method](#implement-check_item-method) + - [Emit the Lint](#emit-the-lint) + - [Run `cargo dev bless` to Generate `.stderr` file](#run-cargo-dev-bless-to-generate-stderr-file) + - [Document the New Lint](#document-the-new-lint) + +## The Problem: Pub Use + +The usage of `pub use ...` is extremely common in Rust projects. +However, for certain projects we might want to restrict writing +`pub use ...` to prevent unintentional exports or to encourage +the developers to place exported items explicitly in public modules. + +For instance, we might often write the following module export: + +```rust +pub mod outer { + mod inner { + pub struct Test {} + } + pub use inner::Test; // <- `pub use` instance +} + +use outer::Test; +``` + +Instead, for some very specific projects, we want to encourage +the following approach instead: + +```rust +pub mod outer { + pub struct Test {} +} + +use outer::Test; +``` + +## Define the Lint + +Since we only need to detect the usage of `pub use` and do not need +any type information, we could implement `EarlyLintPass` for this lint. + +Let us name it `pub_use`, which suggests explicitly what this lint +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` +group, which contains lints that, when enabled, disallow Rustaceans +from writing certain code. + +With these decisions at hand, we could generate the new lint by running +the following command inside our Clippy directory: + +```sh +$ git checkout -b new_lint_pub_use + +$ cargo dev new_lint --name=pub_use --pass=early --category=restriction + + Finished dev [unoptimized + debuginfo] target(s) in 0.11s + Running `target/debug/clippy_dev new_lint --name=pub_use --pass=early --category=restriction` +Generated lint file: `clippy_lints/src/pub_use.rs` +Generated test file: `tests/ui/pub_use.rs` +``` + +Let us take a look at all the changes that are happening: + +```sh +$ git status +On branch new_lint_pub_use + +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_restriction.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/pub_use.rs + src/docs/pub_use.txt + tests/ui/pub_use.rs + +no changes added to commit (use "git add" and/or "git commit -a") +``` + +Feel free to experiment with the command and check what the `cargo dev new_lint` +command did for us. Most importantly, it has done quite a bit of heavy-lifting +and we have a `clippy_lints/src/pub_use.rs` file to implement the lint logic +as well as a `tests/ui/pub_use.rs` file to implement the UI tests in. + +## Write UI Tests + +Let us safely remove the stub content of the newly created UI test file: + +```rust +// tests/ui/pub_use.rs + +#![warn(clippy::pub_use)] + +fn main() { + // test code goes here +} + +``` + +Since this is a relatively simple and straightforward lint, let us +simply put some undesirable code that we want to lint and some code +which we do not want to lint inside this file so that we have some +positive and negative test cases: + +```rust +// tests/ui/pub_use.rs + +#![warn(clippy::pub_use)] +#![allow(unused_imports)] +#![no_main] + +pub mod outer { + mod inner { + pub struct Test {} + } + // should be linted + pub use inner::Test; +} + +// should not be linted +use std::fmt; +``` + +## Implement the Lint + +We also have a stub lint file: + +```rust +use rustc_ast::ast::*; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```rust + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```rust + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.65.0"] + pub PUB_USE, + restriction, + "default lint description" +} +declare_lint_pass!(PubUse => [PUB_USE]); + +impl EarlyLintPass for PubUse {} +``` + +### Implement `check_item` Method + +Since we want to check the usage of `pub use`, we could utilize +`EarlyLintPass`'s [check_item] method, which gives us an item of [Item]: + +```rust +pub struct Item { + pub attrs: AttrVec, + pub id: NodeId, + pub span: Span, + pub vis: Visibility, + pub ident: Ident, + pub kind: K, + pub tokens: Option, +} +``` + +Note that this `Item` contains information on the item's [ItemKind], +which is an enum that includes a variant `Use`: + +```rust +pub enum ItemKind { + ExternCrate(Option), + Use(UseTree), + // Other item kind variants... +} +``` + +Moreover, the `Item` struct includes a `vis` field of [Visibility] +which contains `kind` for [VisibilityKind], which includes the +`VisibilityKind::Public` variant for indicating the usage of `pub use`. + +Let us write some code down: + +```rust +// ...Code above ignored + +impl EarlyLintPass for PubUse { + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + // We only check for an item if its kind is `ItemKind::Use` + // and if its visibility is `VisibilityKind::Public` + if let ItemKind::Use(_) = item.kind && + let VisibilityKind::Public = item.vis.kind { + // Let's just print the line out + println!("We found a `pub use` item!"); + } + } +} +``` + +At this point, if we run the UI test, Clippy will compile and throw +us some test failures: + +```sh +$ TESTNAME=pub_use cargo uitest + +running 1 test +test [ui] ui/pub_use.rs ... FAILED + +...error messages... + +test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.39s + +...error messages... + +failures: + +---- compile_test stdout ---- +normalized stdout: +We found a `pub use` item! + +...error messages... + +error: test failed, to rerun pass '--test compile-test' +``` + +But we see that our `println!` message is in the `stdout`! + +### Emit the Lint + +To emit a meaningful lint, we could use use `span_lint_and_help`: + +```rust +// ...Code above ignored + +impl EarlyLintPass for PubUse { + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + if let ItemKind::Use(_) = item.kind && + let VisibilityKind::Public = item.vis.kind { + // Emit the lint with `span_lint_and_help` + span_lint_and_help( + cx, + PUB_USE, + item.span, + "using `pub use`", + None, + "move the exported item to a public module instead", + ); + } + } +} +``` + +If we run the UI test again, we will observe some output like: + +```sh +...error messages... + +failures: + +---- compile_test stdout ---- +normalized stderr: +error: using `pub use` + --> $DIR/pub_use.rs:10:5 + | +LL | pub use inner::Test; + | ^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::pub-use` implied by `-D warnings` + = help: move the exported item to a public module instead + +error: aborting due to previous error + +...error messages... +``` + +### Run `cargo dev bless` to Generate `.stderr` file + +This looks very much like how we would want the lint to behave! +So we can run `cargo dev bless`: + +```sh +$ cargo dev bless + Finished dev [unoptimized + debuginfo] target(s) in 0.02s + Running `target/debug/clippy_dev bless` +updating tests/ui/pub_use.stderr +``` + +Peeking into this newly generated `stderr` file we will see: + +```txt +error: using `pub use` + --> $DIR/pub_use.rs:10:5 + | +LL | pub use inner::Test; + | ^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::pub-use` implied by `-D warnings` + = help: move the exported item to a public module instead + +error: aborting due to previous error +``` + +And running `TESTNAME=pub_use cargo uitest` command again will give us +some `ok` messages: + +```sh +$ TESTNAME=pub_use cargo uitest + Finished test [unoptimized + debuginfo] target(s) in 0.06s + Running tests/compile-test.rs (target/debug/deps/compile_test-d827993ccb35780b) + +running 3 tests +test rustfix_coverage_known_exceptions_accuracy ... ok +test ui_cargo_toml_metadata ... ok + +running 1 test +test [ui] ui/pub_use.rs ... ok + +...Lots of ok messages... +``` + +## Document the New Lint + +Don't forget that we should document this new `pub_use` lint +so that other Rustaceans can easily understand what it does and if +it fits their needs: + +```rust +// clippy_lints/src/pub_use.rs + +// ...Code above ignored + +declare_clippy_lint! { + /// ### What it does + /// + /// Restricts the usage of `pub use ...` + /// + /// ### Why is this bad? + /// + /// `pub use` is usually fine, but a project may wish to limit `pub use` instances to prevent + /// unintentional exports or to encourage placing exported items directly in public modules + /// + /// ### Example + /// ```rust + /// pub mod outer { + /// mod inner { + /// pub struct Test {} + /// } + /// pub use inner::Test; + /// } + /// + /// use outer::Test; + /// ``` + /// Use instead: + /// ```rust + /// pub mod outer { + /// pub struct Test {} + /// } + /// + /// use outer::Test; + /// ``` + #[clippy::version = "1.62.0"] + pub PUB_USE, + restriction, + "restricts the usage of `pub use`" +} + +// Code below ignored... +``` + +This looks about right! Now we can commit the code and push to our Github +branch and create a pull request for Clippy's `master` branch. + +[check_item]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html#method.check_item +[Item]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/struct.Item.html +[ItemKind]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/enum.ItemKind.html +[pub_use]: https://github.com/rust-lang/rust-clippy/blob/cf72565a12c982f577ca4394c3b80edb89f6c6d3/clippy_lints/src/pub_use.rs +[Visibility]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/struct.Visibility.html +[VisibilityKind]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/struct.Visibility.html From 2f32508eb031e72c30bca18e76d137df3d5a9137 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Sat, 15 Oct 2022 17:07:33 +0200 Subject: [PATCH 12/13] WIP: example_late chapter draft --- book/src/development/example_late.md | 285 +++++++++++++++++++++++++++ 1 file changed, 285 insertions(+) create mode 100644 book/src/development/example_late.md diff --git a/book/src/development/example_late.md b/book/src/development/example_late.md new file mode 100644 index 000000000000..2257aa9217f3 --- /dev/null +++ b/book/src/development/example_late.md @@ -0,0 +1,285 @@ +# Example with `LateLintPass` + +Let us create a Clippy lint that implements `LateLintPass` +so that we can put the knowledge from all previous chapters into practice. + +> Note: This lint is actually implemented in Clippy. +> If you are curious, feel free to check out the actual implementation +> of this lint example here: [trim_split_whitespace]. + +- [Example with `LateLintPass`](#example-with-latelintpass) + - [The Problem: Unnecessary Whitespace Trimming](#the-problem-unnecessary-whitespace-trimming) + - [Define the Lint](#define-the-lint) + - [Diagnostic Items](#diagnostic-items) + - [Generate New Lint Files with `LateLintPass`](#generate-new-lint-files-with-latelintpass) + - [Understand HIR with `#[clippy::dump]`](#understand-hir-with-clippydump) + - [Write UI Tests](#write-ui-tests) + - [Match for Method Calls](#match-for-method-calls) + - [Edge Cases for UI Tests](#edge-cases-for-ui-tests) + +## The Problem: Unnecessary Whitespace Trimming + +Have you met a cautious Rustacean who are obsessed with leading +and trailing whitespaces in their `String`/`str` that they would +perform [trim]/[trim_start]/[trim_end] before [split_whitespace]: + +```rust +" A B C ".trim().split_whitespace(); +``` + +No judgement! It is okay to be extra careful. But as it turns out, +`split_whitespace` would already ignore leading and trailing whitespaces. +So our careful Rustacean could have written instead the following code +(go to this [playground] for an example): + +```rust +" A B C ".split_whitespace(); +``` + +This presents a perfect opportunity for a lint that could inform +other cautious Rustaceans that they only need to perform one method +call if that call is `split_whitespace`. + +Let us get started! + +## Define the Lint + +As the reader might intuitively sense, we are not just interacting +with Rust "grammar" if we want to implement this lint. + +### Diagnostic Items + +We need to somehow ascertain that we are sequentially calling the +`trim`/`trim_start`/`trim_end` and `split_whitespace` methods and +this would require us to access [diagnostic items][diagnostic_items]. + +Diagnostic items are our way to check for specific types, traits and, +in our present case, functions/methods. + +> Note: Take a quick look at [Using Diagnostic Items][using_diagnostic_items] +> if you want to have an overview of how they work. + +### Generate New Lint Files with `LateLintPass` + +Because of this, we are going to use [LateLintPass] for this new lint. +Since this lint will examine chained method calls, it is quite convenient +to implement the [check_expr] method for `LateLintPass` when working on +this new lint. + +Additionally, we could name our lint `trim_split_whitespace`, which is +an accurate description of the unnecessary method call. + +Moreover, since this lint is more related to writing idiomatic Rust, +we can consider putting it in the `clippy::style` group. + +Let us run our beloved `cargo dev new_lint` command to generate some files: + +```sh +$ git checkout -b new_lint_trim_split_whitespace + +$ cargo dev new_lint --name=trim_split_whitespace --pass=late --category=style + Finished dev [unoptimized + debuginfo] target(s) in 0.07s + Running `target/debug/clippy_dev new_lint --name=trim_split_whitespace --pass=late --category=style` +Generated lint file: `clippy_lints/src/trim_split_whitespace.rs` +Generated test file: `tests/ui/trim_split_whitespace.rs` +``` + +Examine the changes with `git status` carefully, we see that most of the important +files have been created by the command so that we can focus on developing the +new lint: + +```sh +$ git status +On branch new_lint_trim_split_whitespace + +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_all.rs + modified: clippy_lints/src/lib.register_lints.rs + modified: clippy_lints/src/lib.register_style.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/trim_split_whitespace.rs + src/docs/trim_split_whitespace.txt + tests/ui/trim_split_whitespace.rs + +no changes added to commit (use "git add" and/or "git commit -a") +``` + +Feel free to experiment with the command and check what the `cargo dev new_lint` +command did for us. Most importantly, it has done quite a bit of heavy-lifting +and we have a `clippy_lints/src/trim_split_whitespace.rs` file to implement the lint logic as well as a `tests/ui/trim_split_whitespace.rs` file to implement the UI tests in. + +## Understand HIR with `#[clippy::dump]` + +Before we move further with our UI test cases as well as the lint implementation, +it is important to note that it is helpful to first understand the internal +representation that rustc uses. Clippy has the `#[clippy::dump]` attribute that +prints the [_High-Level Intermediate Representation (HIR)_] of the item, +statement, or expression that the attribute is attached to. + +> Note: If you have not read other chapters of the Clippy book and wonder +> what HIR is in the context of `rustc`, feel free to read [_Rustc Overview_] +> as well as [_High-Level Intermediate Representation (HIR)_] for a quick review. + +To attach the attribute to expressions you often need to enable +`#![feature(stmt_expr_attributes)]`. + +[Here][print_hir_example] you can find an example. To see how an expression like +`" A ".trim().split_whitespace()` looks like in HIR, +select _TOOLS_ from the upper right corner of Rust Playground and click _Clippy_. +Overall, you should see a structure that resembles the following: + +```rust +Expr { + hir_id: HirId { // fields }, + kind: MethodCall( + PathSegment { + ident: split_whitespace#0, + hir_id: HirId { // fields }, + // fields + }, + Expr { + hir_id: HirId { // fields }, + kind: MethodCall( + PathSegment { + ident: trim#0, + hir_id: HirId { // fields }, + // fields + }, + Expr { + hir_id: HirId { // fields }, + kind: Lit( + Spanned { + node: Str( + " A ", + Cooked, + ), + span: src/main.rs:5:13: 5:18 (#0), + }, + ), + span: src/main.rs:5:13: 5:18 (#0), + }, + [], + src/main.rs:5:19: 5:25 (#0), + ), + span: src/main.rs:5:13: 5:25 (#0), + }, + [], + src/main.rs:5:26: 5:44 (#0), + ), + span: src/main.rs:5:13: 5:44 (#0), +} +``` + +It might take some getting used to but, if we observe carefully, we +see that there are 2 [MethodCall]s, one for `split_whitespace`, where +the `PathSegment.ident` is `split_whitespace#0`, and one for `trim`, +where the `PathSegment.ident` is `trim#0`. +We might also notice an expression `Expr` whose `kind` is [Lit] with +a `Spanned { node: Str(" A "), ..}`. + +In essence, when an expression has chained method calls like: + +```rust +" A ".trim().split_whitespace() +``` + +Its HIR representation would look like the following, with the last +method `split_whitespace` being the most outer element, the method +`trim` being the second most outer element, and the method call receiver +`" A "` being the most inner element. + +```rust +// Note that this is an extremely simplified pseudo-HIR structure +Expr { + kind: MethodCall( + split_whitespace, // This is the last method call + Expr { + kind: MethodCall( + trim, // This is the first method call + Expr { + kind: Lit(" A ") // This is the receiver of the first method call + } + ) + } + ) +} +``` + +Let us keep this information in our head as we move on to the next steps +in writing a new Clippy lint. + +## Write UI Tests + +With some knowledge of the kind of expression we are working with, +let us write down some cases which we expect the lint to warn. +The following could be a reasonable starting point for us: + +```rust +// tests/ui/trim_split_whitespace.rs + +#![warn(clippy::trim_split_whitespace)] + +fn main() { + // &str + let _ = " A B C ".trim().split_whitespace(); // should trigger lint + let _ = " A B C ".trim_start().split_whitespace(); // should trigger lint + let _ = " A B C ".trim_end().split_whitespace(); // should trigger lint + + // String + let _ = (" A B C ").to_string().trim().split_whitespace(); // should trigger lint + let _ = (" A B C ").to_string().trim_start().split_whitespace(); // should trigger lint + let _ = (" A B C ").to_string().trim_end().split_whitespace(); // should trigger lint + +} +``` + +### Match for Method Calls + +Assuming that we will implement [check_expr] +for `LateLintPass` when implementing this lint and assuming that +we will examine an expression `expr` for `" A ".trim().split_whitespace()`, +we can check for the usage of `trim` and `split_whitespace` like the following: + +```rust +// We first match for +if let ExprKind::MethodCall(path, [split_recv], split_ws_span) = expr.kind + && path.ident.name == sym!(split_whitespace) + && let ExprKind::MethodCall(path, [_trim_recv], trim_span) = split_recv.kind + && let trim_fn_name @ ("trim" | "trim_start" | "trim_end") = path.ident.name.as_str() + && let Some(trim_def_id) = tyckres.type_dependent_def_id(split_recv.hir_id) { + span_lint_and_sugg( + cx, + TRIM_SPLIT_WHITESPACES, + trim_span.with_hi(split_ws_span.lo()), + &format!("found call to `str::{}` before `str::split_whitespace`", trim_fn_name), + &format!("remove `{}()`", trim_fn_name), + String::new(), + Applicability::MachineApplicable, + ); +} +``` + +### Edge Cases for UI Tests + +[check_expr]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_expr +[diagnostic_items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html +[_Rustc Overview_]: https://rustc-dev-guide.rust-lang.org/overview.html +[_High-Level Intermediate Representation (HIR)_]: https://rustc-dev-guide.rust-lang.org/hir.html +[LateLintPass]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/trait.LateLintPass.html +[Lit]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/hir/enum.ExprKind.html#variant.Lit +[MethodCall]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/hir/enum.ExprKind.html#variant.MethodCall +[playground]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=62c07194aada06aecb7c600123fd5786 +[print_hir_example]: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=a32d1fa3bfc35607db75e226e5fe475b +[split_whitespace]: https://doc.rust-lang.org/std/primitive.str.html#method.split_whitespace +[trim]: https://doc.rust-lang.org/std/primitive.str.html#method.trim +[trim_end]: https://doc.rust-lang.org/std/primitive.str.html#method.trim_end +[trim_start]: https://doc.rust-lang.org/std/primitive.str.html#method.trim_start +[trim_split_whitespace]: https://github.com/rust-lang/rust-clippy/blob/9a6eca5f852830cb5e9a520f79ce02e6aae9a1b1/clippy_lints/src/strings.rs#L464-L517 +[using_diagnostic_items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html#using-diagnostic-items From 02330c877d16fe89b03afe95873016bbeb236301 Mon Sep 17 00:00:00 2001 From: Nahua Kang Date: Thu, 20 Oct 2022 17:27:28 +0200 Subject: [PATCH 13/13] Draft duplicate trim_split_whitespaces lint for late lint example --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_style.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/trim_split_whitespaces.rs | 72 +++++++++++++++++ src/docs.rs | 1 + src/docs/trim_split_whitespaces.txt | 12 +++ tests/ui/trim_split_whitespaces.rs | 91 ++++++++++++++++++++++ 9 files changed, 182 insertions(+) create mode 100644 clippy_lints/src/trim_split_whitespaces.rs create mode 100644 src/docs/trim_split_whitespaces.txt create mode 100644 tests/ui/trim_split_whitespaces.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 257add86b6e5..576a301e56bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4112,6 +4112,7 @@ Released 2018-09-13 [`transmutes_expressible_as_ptr_casts`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmutes_expressible_as_ptr_casts [`transmuting_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmuting_null [`trim_split_whitespace`]: https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespace +[`trim_split_whitespaces`]: https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespaces [`trivial_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_regex [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 1f85382347aa..1e4cb9feb654 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -328,6 +328,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(transmute::UNSOUND_COLLECTION_TRANSMUTE), LintId::of(transmute::USELESS_TRANSMUTE), LintId::of(transmute::WRONG_TRANSMUTE), + LintId::of(trim_split_whitespaces::TRIM_SPLIT_WHITESPACES), LintId::of(types::BORROWED_BOX), LintId::of(types::BOX_COLLECTION), LintId::of(types::REDUNDANT_ALLOCATION), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 13b573beea1b..a027423eef7f 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -554,6 +554,7 @@ store.register_lints(&[ transmute::UNSOUND_COLLECTION_TRANSMUTE, transmute::USELESS_TRANSMUTE, transmute::WRONG_TRANSMUTE, + trim_split_whitespaces::TRIM_SPLIT_WHITESPACES, types::BORROWED_BOX, types::BOX_COLLECTION, types::LINKEDLIST, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index 05d2ec2e9e1e..dc5a19713f75 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -115,6 +115,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(strings::TRIM_SPLIT_WHITESPACE), LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME), + LintId::of(trim_split_whitespaces::TRIM_SPLIT_WHITESPACES), LintId::of(unit_types::LET_UNIT_VALUE), LintId::of(unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS), LintId::of(unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a26e129f094e..65093ad61804 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -369,6 +369,7 @@ mod to_digit_is_some; mod trailing_empty_array; mod trait_bounds; mod transmute; +mod trim_split_whitespaces; mod types; mod undocumented_unsafe_blocks; mod unicode; @@ -902,6 +903,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable)); store.register_early_pass(|| Box::new(multi_assignments::MultiAssignments)); store.register_late_pass(|| Box::new(bool_to_int_with_if::BoolToIntWithIf)); + store.register_late_pass(|| Box::new(trim_split_whitespaces::TrimSplitWhitespaces)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/trim_split_whitespaces.rs b/clippy_lints/src/trim_split_whitespaces.rs new file mode 100644 index 000000000000..f9bcdffaacf4 --- /dev/null +++ b/clippy_lints/src/trim_split_whitespaces.rs @@ -0,0 +1,72 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use rustc_errors::Applicability; +use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```rust + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```rust + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.65.0"] + pub TRIM_SPLIT_WHITESPACES, + style, + "default lint description" +} +declare_lint_pass!(TrimSplitWhitespaces => [TRIM_SPLIT_WHITESPACES]); + +impl<'tcx> LateLintPass<'tcx> for TrimSplitWhitespaces { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'_>) { + let tyckres = cx.typeck_results(); + + if let ExprKind::MethodCall(path, [split_recv], split_ws_span) = expr.kind + && path.ident.name == sym!(split_whitespace) + && let Some(split_ws_def_id) = tyckres.type_dependent_def_id(expr.hir_id) + { + if cx.tcx.is_diagnostic_item(sym::str_split_whitespace, split_ws_def_id) { + println!("This passed. split_ws_def_id ==> {:?}", split_ws_def_id); + if let ExprKind::MethodCall(path, [_trim_recv], trim_span) = split_recv.kind + && let trim_fn_name @ ("trim" | "trim_start" | "trim_end") = path.ident.name.as_str() + && let Some(trim_def_id) = tyckres.type_dependent_def_id(split_recv.hir_id) { + if is_one_of_trim_diagnostic_items(cx, trim_def_id) { + println!("This passed. trim_def_id ==> {:?}", trim_def_id); + println!("YYY {:?} got linted!", expr.span); + span_lint_and_sugg( + cx, + TRIM_SPLIT_WHITESPACES, + trim_span.with_hi(split_ws_span.lo()), + &format!("found call to `str::{}` before `str::split_whitespace`", trim_fn_name), + &format!("remove `{}()`", trim_fn_name), + String::new(), + Applicability::MachineApplicable, + ); + } else { + println!("This didn't pass. trim_def_id ==> {:?}", trim_def_id); + println!("XXX {:?} did not linted!", expr.span); + } + } + } else { + println!("This didn't pass. split_ws_def_id ==> {:?}", split_ws_def_id); + println!("XXX {:?} did not linted!", expr.span); + } + } + println!(); + } +} + +fn is_one_of_trim_diagnostic_items(cx: &LateContext<'_>, trim_def_id: DefId) -> bool { + cx.tcx.is_diagnostic_item(sym::str_trim, trim_def_id) + || cx.tcx.is_diagnostic_item(sym::str_trim_start, trim_def_id) + || cx.tcx.is_diagnostic_item(sym::str_trim_end, trim_def_id) +} diff --git a/src/docs.rs b/src/docs.rs index 69243bf4d9c9..2baeac7ecb9b 100644 --- a/src/docs.rs +++ b/src/docs.rs @@ -510,6 +510,7 @@ docs! { "transmutes_expressible_as_ptr_casts", "transmuting_null", "trim_split_whitespace", + "trim_split_whitespaces", "trivial_regex", "trivially_copy_pass_by_ref", "try_err", diff --git a/src/docs/trim_split_whitespaces.txt b/src/docs/trim_split_whitespaces.txt new file mode 100644 index 000000000000..92a0504a7dde --- /dev/null +++ b/src/docs/trim_split_whitespaces.txt @@ -0,0 +1,12 @@ +### What it does + +### Why is this bad? + +### Example +``` +// example code where clippy issues a warning +``` +Use instead: +``` +// example code which does not raise clippy warning +``` \ No newline at end of file diff --git a/tests/ui/trim_split_whitespaces.rs b/tests/ui/trim_split_whitespaces.rs new file mode 100644 index 000000000000..eaf0f707965e --- /dev/null +++ b/tests/ui/trim_split_whitespaces.rs @@ -0,0 +1,91 @@ +// run-rustfix +#![warn(clippy::trim_split_whitespaces)] +#![allow(clippy::let_unit_value)] + +struct Custom; +impl Custom { + fn trim(self) -> Self { + self + } + fn split_whitespace(self) {} +} + +struct DerefStr(&'static str); +impl std::ops::Deref for DerefStr { + type Target = str; + fn deref(&self) -> &Self::Target { + self.0 + } +} + +struct DerefStrAndCustom(&'static str); +impl std::ops::Deref for DerefStrAndCustom { + type Target = str; + fn deref(&self) -> &Self::Target { + self.0 + } +} +impl DerefStrAndCustom { + fn trim(self) -> Self { + self + } + fn split_whitespace(self) {} +} + +struct DerefStrAndCustomSplit(&'static str); +impl std::ops::Deref for DerefStrAndCustomSplit { + type Target = str; + fn deref(&self) -> &Self::Target { + self.0 + } +} +impl DerefStrAndCustomSplit { + #[allow(dead_code)] + fn split_whitespace(self) {} +} + +struct DerefStrAndCustomTrim(&'static str); +impl std::ops::Deref for DerefStrAndCustomTrim { + type Target = str; + fn deref(&self) -> &Self::Target { + self.0 + } +} +impl DerefStrAndCustomTrim { + fn trim(self) -> Self { + self + } +} + +fn main() { + // &str + let _ = " A B C ".trim().split_whitespace(); // should trigger lint + let _ = " A B C ".trim_start().split_whitespace(); // should trigger lint + let _ = " A B C ".trim_end().split_whitespace(); // should trigger lint + + // String + let _ = (" A B C ").to_string().trim().split_whitespace(); // should trigger lint + let _ = (" A B C ").to_string().trim_start().split_whitespace(); // should trigger lint + let _ = (" A B C ").to_string().trim_end().split_whitespace(); // should trigger lint + + // Custom + let _ = Custom.trim().split_whitespace(); // should not trigger lint + + // Deref + let s = DerefStr(" A B C "); + let _ = s.trim().split_whitespace(); // should trigger lint + + // Deref + custom impl + let s = DerefStrAndCustom(" A B C "); + let _ = s.trim().split_whitespace(); // should not trigger lint + + // Deref + only custom split_ws() impl + let s = DerefStrAndCustomSplit(" A B C "); + let _ = s.trim().split_whitespace(); // should trigger lint + // Expl: trim() is called on str (deref) and returns &str. + // Thus split_ws() is called on str as well and the custom impl on S is unused + + // Deref + only custom trim() impl + let s = DerefStrAndCustomTrim(" A B C "); + let _ = s.trim().split_whitespace(); // should not trigger lint +}