From 12b269663485955176613a8f2e3f101793f65fda Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 26 Feb 2019 20:55:17 +0100 Subject: [PATCH 01/18] Add lint writing documentation This adds a new documentation page that explains how to write Clippy lints. It guides the reader through creating a `foo` function lint. --- doc/adding_lints.md | 324 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 324 insertions(+) create mode 100644 doc/adding_lints.md diff --git a/doc/adding_lints.md b/doc/adding_lints.md new file mode 100644 index 000000000000..365564ef25d6 --- /dev/null +++ b/doc/adding_lints.md @@ -0,0 +1,324 @@ +## Adding a new lint + +You are probably here because you want to add a new lint to Clippy. If this is +the first time you're contributing to Clippy, this document guides you through +creating an example lint from scratch. + +To get started, we will create a lint that detects functions called `foo`, +because that's clearly a non-descriptive name. + +* [Testing](#Testing) +* [Lint declaration](#Lint-declaration) +* [Lint passes](#Lint-passes) +* [Emitting a lint](#Emitting-a-lint) +* [Adding the lint logic](#Adding-the-lint-logic) +* [Documentation](#Documentation) +* [Debugging](#Debugging) +* [PR Checklist](#PR-Checklist) +* [Cheatsheet](#Cheatsheet) + +### Testing + +Let's write some tests first that we can execute while we iterate on our lint. + +Clippy uses UI tests for testing. UI tests check that the output of Clippy is +exactly as expected. 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. + +Let's create the test file at `tests/ui/foo_functions.rs`. It doesn't really +matter what the file is called, but it's a good convention to name it after the +lint it is testing, so `foo_functions.rs` it is. + +Inside we put 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 { + pub fn fo(&self) {} + pub fn foo(&self) {} + pub 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(); +} + +``` + +Now we can run the test with `TESTNAME=ui/foo_functions cargo uitest`. +Currently this test will fail. If you go through the output you will see that we +have to add some missing imports to our lint file. + +While you are working on implementing your lint, you can keep running the UI +test. That allows you to check if the output is turning into what you want. + +Once you are satisfied with the output, you need to run +`tests/ui/update-all-references.sh` to update the `stderr` file for your lint. +Running `TESTNAME=ui/foo_functions cargo uitest` should pass then. When you +commit your lint, be sure to commit the `*.stderr` files, too. + +Let's have a look at implementing our lint now. + +### Lint declaration + +We start by creating a new file in the `clippy_lints` crate. That's the crate +where all the lint code is. We are going to call the file +`clippy_lints/src/foo_functions.rs` and import some initial things we need: + +```rust +use rustc::lint::{LintArray, LintPass}; +use rustc::{declare_tool_lint, lint_array}; +``` + +The next step is to provide a lint declaration. Lints are declared using the [`declare_clippy_lint!`][declare_clippy_lint] macro: + +```rust +declare_clippy_lint! { + pub FOO_FUNCTIONS, + pedantic, + "function named `foo`, which is not a descriptive name" +} +``` + +* `FOO_FUNCTIONS` is the name of our lint. Be sure to follow the [lint naming +guidelines][lint_naming] here when naming your lint. In short, the name should +state the thing that is being checked for and read well when used with +`allow`/`warn`/`deny`. +* `pedantic` sets the lint level to `Allow`. + The exact mapping can be found [here][category_level_mapping] +* The last part should be a text that explains what exactly is wrong with the + code + +With our lint declaration done, we will now make sure that our lint declaration is assigned to a lint pass: + +```rust +// clippy_lints/src/foo_functions.rs + +// .. imports and lint declaration .. + +#[derive(Copy, Clone)] +pub struct FooFunctionsPass; + +impl LintPass for FooFunctionsPass { + fn get_lints(&self) -> LintArray { + lint_array!( + FOO_FUNCTIONS, + ) + } + + fn name(&self) -> &'static str { + "FooFunctions" + } +} +``` + +Don't worry about the `name` method here. As long as it includes the name of the lint pass it should be fine. + +Next you should run `util/dev update_lints` to register the lint in various +places, mainly in `clippy_lints/src/lib.rs`. + +While `update_lints` automates some things, it doesn't automate everything. We will have to register our lint pass manually in the `register_plugins` function in `clippy_lints/src/lib.rs`: + +```rust +reg.register_early_lint_pass(box foo_functions::FooFunctionsPass); +``` + +Without that, running the UI tests would produce an error like `unknown clippy lint: clippy::foo_functions`. +The next decision we have to make is which lint pass our lint is going to need. + +### Lint passes + +Writing a lint that just checks for the name of a function means that we just +have to deal with the AST and don't have to deal with the type system at all. +This is good, because it makes writing this particular lint less complicated. + +We have to make this decision with every new Clippy lint. It boils down to using either `EarlyLintPass` or `LateLintPass`. This is a result of Rust's compilation process. You can read more about it [in the rustc guide][compilation_stages]. + +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`. The `EarlyLintPass` is also faster. However linting speed hasn't really been a concern with Clippy so far. + +Since we don't need type information for checking the function name, we are going to use the `EarlyLintPass`. It has to be imported as well, changing our imports to: + +```rust +use rustc::lint::{LintArray, LintPass, EarlyLintPass, EarlyContext}; +use rustc::{declare_tool_lint, lint_array}; +``` + +### Emitting a lint + +With UI tests in place, we can start working on the implementation of the lint logic. We can keep executing the tests until we make them pass. + +Let's start by emitting a lint for every function definition. + +```rust +impl EarlyLintPass for Pass { + fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, _: &FnDecl, span: Span, _: NodeId) { + // TODO: Emit lint here + } +} +``` + +We implement the [`check_fn`][check_fn] method from the [`EarlyLintPass`][early_lint_pass] trait. This gives us access to various information about the function that is currently being checked. More on that in the next section. Let's worry about the details later and emit our lint for *every* function definition first. + +Depending on how complex we want our lint message to be, we can choose from a variety of lint emission functions. +They can all be found in [`clippy_lints/src/utils/diagnostics.rs`][diagnostics]. + + +```rust +impl EarlyLintPass for Pass { + fn check_fn(&mut self, cx: &EarlyContext<'_>, _: FnKind<'_>, _: &FnDecl, span: Span, _: NodeId) { + span_help_and_lint( + cx, + FOO_FUNCTIONS, + span, + "function named `foo`", + "consider using a more meaningful name" + ); + } +} +``` + +### Adding the lint logic + +Writing the logic for your lint will most likely be different from this example, +so this section is kept rather short. + +Using the [`check_fn`][check_fn] method gives us access to [`FnKind`][fn_kind] +that has two relevant variants for us `FnKind::ItemFn` and `FnKind::Method`. +Both provide access to the name of the function/method via an [`Ident`][ident]. +and delegate our check to it's own function, passing through only the data we +need. + +In our example, the implementation would look like: + +```rust +fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { + match fn_kind { + FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) => { + return ident.name == "foo" + }, + FnKind::Closure(..) => return false + } +} +``` + + +Now you'll want to also run the full test suite with `cargo test`. Apart from +running all other UI tests, this ensures that our lint implementation is not +violating any Clippy lints itself. If you are still following the example, +you'll see that the `FooFunctionsPass` violates a Clippy lint. So we are going +to rename that struct to just `Pass`: + +```rust +#[derive(Copy, Clone)] +pub struct Pass; + +impl LintPass for Pass { /* .. */ } +``` + + +### Documentation + +The final thing before submitting our PR is to add some documentation to our +lint declaration. + +Please document your lint with a doc comment akin to the following: + +```rust +/// **What it does:** Checks for ... (describe what the lint matches). +/// +/// **Why is this bad?** Supply the reason for linting the code. +/// +/// **Known problems:** None. (Or describe where it could go wrong.) +/// +/// **Example:** +/// +/// ```rust,ignore +/// // Bad +/// Insert a short example of code that triggers the lint +/// +/// // Good +/// Insert a short example of improved code that doesnt trigger the lint +/// ``` +declare_clippy_lint! // ... +``` + +Once your lint is merged, this documentation will show up in the [lint +list][lint_list]. + +### Debugging + +If you want to debug parts of your lint implementation, you can use the `dbg!` +macro anywhere in your code. Running the tests should then include the debug +output in the `stdout` part. + +### PR Checklist + +TODO: Prose + +- [ ] Followed [lint naming conventions][lint_naming] +- [ ] Added passing UI tests (including committed `.stderr` file) +- [ ] `cargo test` passes locally +- [ ] Added lint documentation + +### Cheatsheet + +Here are some pointers to things you are likely going to need for every lint: + +* [The `if_chain` macro][if_chain] +* [`in_macro`][in_macro] and [`in_external_macro`][in_external_macro] +* [`Span`][span] +* [Clippy diagnostics][diagnostics] +* [`Applicability`][applicability] + +For `EarlyLintPass` lints: + +* [`EarlyLintPass`][early_lint_pass] +* [`syntax::ast`][ast] + +For `LateLintPass` lints: + +* [`LateLintPass`][late_lint_pass] +* [`Ty::TyKind`][ty] + + +While most of Clippy's lint utils are documented, most of rustc's internals lack +documentation currently. This is unfortunate, but in most cases you can probably +get away with copying things from existing similar lints. If you are stuck, +don't hesitate to ask on Discord, IRC or in the issue/PR. + +[lint_list]: https://rust-lang.github.io/rust-clippy/master/index.html +[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints +[category_level_mapping]: https://github.com/rust-lang/rust-clippy/blob/bd23cb89ec0ea63403a17d3fc5e50c88e38dd54f/clippy_lints/src/lib.rs#L43 +[declare_clippy_lint]: https://github.com/rust-lang/rust-clippy/blob/a71acac1da7eaf667ab90a1d65d10e5cc4b80191/clippy_lints/src/lib.rs#L39 +[compilation_stages]: https://rust-lang.github.io/rustc-guide/high-level-overview.html#the-main-stages-of-compilation +[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_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/trait.LateLintPass.html +[fn_kind]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/visit/enum.FnKind.html +[diagnostics]: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/utils/diagnostics.rs +[ident]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/source_map/symbol/struct.Ident.html +[span]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax_pos/struct.Span.html +[applicability]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/enum.Applicability.html +[if_chain]: https://docs.rs/if_chain/0.1.2/if_chain/ +[ty]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/sty/index.html +[ast]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/index.html +[in_macro]: https://github.com/rust-lang/rust-clippy/blob/d0717d1f9531a03d154aaeb0cad94c243915a146/clippy_lints/src/utils/mod.rs#L94 +[in_external_macro]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/fn.in_external_macro.html From c83b0298d727ad36dbf630217797d5091e7b7917 Mon Sep 17 00:00:00 2001 From: Wilco Kusee Date: Wed, 27 Feb 2019 22:15:19 +0100 Subject: [PATCH 02/18] Implicit return Co-Authored-By: phansch --- doc/adding_lints.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 365564ef25d6..fa165f2ea5e8 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -214,7 +214,7 @@ fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) => { return ident.name == "foo" }, - FnKind::Closure(..) => return false + FnKind::Closure(..) => false } } ``` From ef97c13f5e5e2bd5ba1aae5602891941a2ce4fc7 Mon Sep 17 00:00:00 2001 From: Wilco Kusee Date: Wed, 27 Feb 2019 22:15:33 +0100 Subject: [PATCH 03/18] Implicit return Co-Authored-By: phansch --- doc/adding_lints.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index fa165f2ea5e8..6cae03a6b0b8 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -212,7 +212,7 @@ In our example, the implementation would look like: fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { match fn_kind { FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) => { - return ident.name == "foo" + ident.name == "foo" }, FnKind::Closure(..) => false } From 2851f9f70795a0735562266a30cf9ad672f1e24b Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 27 Feb 2019 22:34:16 +0100 Subject: [PATCH 04/18] Expand on lint implementation section, wrap lines --- doc/adding_lints.md | 88 +++++++++++++++++++++++++++++++++------------ 1 file changed, 65 insertions(+), 23 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 6cae03a6b0b8..c61e247f9cfa 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -88,7 +88,8 @@ use rustc::lint::{LintArray, LintPass}; use rustc::{declare_tool_lint, lint_array}; ``` -The next step is to provide a lint declaration. Lints are declared using the [`declare_clippy_lint!`][declare_clippy_lint] macro: +The next step is to provide a lint declaration. Lints are declared using the +[`declare_clippy_lint!`][declare_clippy_lint] macro: ```rust declare_clippy_lint! { @@ -107,7 +108,8 @@ state the thing that is being checked for and read well when used with * The last part should be a text that explains what exactly is wrong with the code -With our lint declaration done, we will now make sure that our lint declaration is assigned to a lint pass: +With our lint declaration done, we will now make sure that our lint declaration +is assigned to a lint pass: ```rust // clippy_lints/src/foo_functions.rs @@ -130,19 +132,23 @@ impl LintPass for FooFunctionsPass { } ``` -Don't worry about the `name` method here. As long as it includes the name of the lint pass it should be fine. +Don't worry about the `name` method here. As long as it includes the name of the +lint pass it should be fine. Next you should run `util/dev update_lints` to register the lint in various places, mainly in `clippy_lints/src/lib.rs`. -While `update_lints` automates some things, it doesn't automate everything. We will have to register our lint pass manually in the `register_plugins` function in `clippy_lints/src/lib.rs`: +While `update_lints` automates some things, it doesn't automate everything. We +will have to register our lint pass manually in the `register_plugins` function +in `clippy_lints/src/lib.rs`: ```rust reg.register_early_lint_pass(box foo_functions::FooFunctionsPass); ``` -Without that, running the UI tests would produce an error like `unknown clippy lint: clippy::foo_functions`. -The next decision we have to make is which lint pass our lint is going to need. +Without that, running the UI tests would produce an error like `unknown clippy +lint: clippy::foo_functions`. The next decision we have to make is which lint +pass our lint is going to need. ### Lint passes @@ -150,11 +156,19 @@ Writing a lint that just checks for the name of a function means that we just have to deal with the AST and don't have to deal with the type system at all. This is good, because it makes writing this particular lint less complicated. -We have to make this decision with every new Clippy lint. It boils down to using either `EarlyLintPass` or `LateLintPass`. This is a result of Rust's compilation process. You can read more about it [in the rustc guide][compilation_stages]. +We have to make this decision with every new Clippy lint. It boils down to using +either [`EarlyLintPass`][early_lint_pass] or [`LateLintPass`][late_lint_pass]. +This is a result of Rust's compilation process. You can read more about it [in +the rustc guide][compilation_stages]. -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`. The `EarlyLintPass` is also faster. However linting speed hasn't really been a concern with Clippy so far. +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`. The `EarlyLintPass` is also faster. However linting speed +hasn't really been a concern with Clippy so far. -Since we don't need type information for checking the function name, we are going to use the `EarlyLintPass`. It has to be imported as well, changing our imports to: +Since we don't need type information for checking the function name, we are +going to use the `EarlyLintPass`. It has to be imported as well, changing our +imports to: ```rust use rustc::lint::{LintArray, LintPass, EarlyLintPass, EarlyContext}; @@ -165,20 +179,25 @@ use rustc::{declare_tool_lint, lint_array}; With UI tests in place, we can start working on the implementation of the lint logic. We can keep executing the tests until we make them pass. -Let's start by emitting a lint for every function definition. +Let's start by implementing the `EarlyLintPass` for our `FooFunctionsPass`: ```rust -impl EarlyLintPass for Pass { +impl EarlyLintPass for FooFunctionsPass { fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, _: &FnDecl, span: Span, _: NodeId) { // TODO: Emit lint here } } ``` -We implement the [`check_fn`][check_fn] method from the [`EarlyLintPass`][early_lint_pass] trait. This gives us access to various information about the function that is currently being checked. More on that in the next section. Let's worry about the details later and emit our lint for *every* function definition first. +We implement the [`check_fn`][check_fn] method from the +[`EarlyLintPass`][early_lint_pass] trait. This gives us access to various +information about the function that is currently being checked. More on that in +the next section. Let's worry about the details later and emit our lint for +*every* function definition first. -Depending on how complex we want our lint message to be, we can choose from a variety of lint emission functions. -They can all be found in [`clippy_lints/src/utils/diagnostics.rs`][diagnostics]. +Depending on how complex we want our lint message to be, we can choose from a +variety of lint emission functions. They can all be found in +[`clippy_lints/src/utils/diagnostics.rs`][diagnostics]. ```rust @@ -203,12 +222,33 @@ so this section is kept rather short. Using the [`check_fn`][check_fn] method gives us access to [`FnKind`][fn_kind] that has two relevant variants for us `FnKind::ItemFn` and `FnKind::Method`. Both provide access to the name of the function/method via an [`Ident`][ident]. -and delegate our check to it's own function, passing through only the data we -need. -In our example, the implementation would look like: +With that we can expand our `check_fn` method to: + +```rust +impl EarlyLintPass for Pass { + fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, _: &FnDecl, span: Span, _: NodeId) { + if !is_foo_fn(fn_kind) { return; } + span_help_and_lint( + cx, + FOO_FUNCTIONS, + span, + "function named `foo`", + "consider using a more meaningful name" + ); + } +} +``` + +We separate the lint conditional from the lint emissions because it makes the +code a bit easier to read. In some cases this separation would also allow to +write some unit tests (as opposed to UI tests) for the separate function. + +In our example, `is_foo_fn` looks like: ```rust +// use statements, impl EarlyLintPass, check_fn, .. + fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { match fn_kind { FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) => { @@ -219,12 +259,12 @@ fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { } ``` - Now you'll want to also run the full test suite with `cargo test`. Apart from running all other UI tests, this ensures that our lint implementation is not -violating any Clippy lints itself. If you are still following the example, -you'll see that the `FooFunctionsPass` violates a Clippy lint. So we are going -to rename that struct to just `Pass`: +violating any Clippy lints itself. + +If you are still following the example, you'll see that the `FooFunctionsPass` +violates a Clippy lint. So we are going to rename that struct to just `Pass`: ```rust #[derive(Copy, Clone)] @@ -233,6 +273,8 @@ pub struct Pass; impl LintPass for Pass { /* .. */ } ``` +That should be it for the lint implementation. Running `cargo test` should now +pass and we can finish up our work by adding some documentation. ### Documentation @@ -255,9 +297,9 @@ Please document your lint with a doc comment akin to the following: /// Insert a short example of code that triggers the lint /// /// // Good -/// Insert a short example of improved code that doesnt trigger the lint +/// Insert a short example of improved code that doesn't trigger the lint /// ``` -declare_clippy_lint! // ... +declare_clippy_lint! { /* ... */ } ``` Once your lint is merged, this documentation will show up in the [lint From 9ff1340a698d315040af85523776303d199f42f8 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 28 Feb 2019 07:00:22 +0100 Subject: [PATCH 05/18] Use positive if condition for readability --- doc/adding_lints.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index c61e247f9cfa..852277d8eb2d 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -228,14 +228,15 @@ With that we can expand our `check_fn` method to: ```rust impl EarlyLintPass for Pass { fn check_fn(&mut self, cx: &EarlyContext<'_>, fn_kind: FnKind<'_>, _: &FnDecl, span: Span, _: NodeId) { - if !is_foo_fn(fn_kind) { return; } - span_help_and_lint( - cx, - FOO_FUNCTIONS, - span, - "function named `foo`", - "consider using a more meaningful name" - ); + if is_foo_fn(fn_kind) { + span_help_and_lint( + cx, + FOO_FUNCTIONS, + span, + "function named `foo`", + "consider using a more meaningful name" + ); + } } } ``` From 81ad4427a4f82759e8302b97359917b6b0df8d8c Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 28 Feb 2019 07:06:10 +0100 Subject: [PATCH 06/18] Remind to run update-all-references.sh at the end --- doc/adding_lints.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 852277d8eb2d..c4e84098906f 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -260,9 +260,12 @@ fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { } ``` -Now you'll want to also run the full test suite with `cargo test`. Apart from -running all other UI tests, this ensures that our lint implementation is not -violating any Clippy lints itself. +Now you'll want to also run the full test suite with `cargo test`. At this point +running `cargo test` should produce the expected output. Remember to run +`tests/ui/update-all-references.sh` to update the `.stderr` file. + +`cargo test` (as opposed to `cargo uitest`) will also ensure that our lint +implementation is not violating any Clippy lints itself. If you are still following the example, you'll see that the `FooFunctionsPass` violates a Clippy lint. So we are going to rename that struct to just `Pass`: From ca35e6750e3a3be5d6a5137a16e2d10b964b8185 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 28 Feb 2019 07:10:59 +0100 Subject: [PATCH 07/18] Mention Clippy utils --- doc/adding_lints.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index c4e84098906f..1ca93a44e1cf 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -328,10 +328,12 @@ TODO: Prose Here are some pointers to things you are likely going to need for every lint: +* [Clippy utils][utils] - Various helper functions. Maybe the function you need + is already in here. (`implements_trait`, `match_path`, `snippet`, etc) +* [Clippy diagnostics][diagnostics] * [The `if_chain` macro][if_chain] * [`in_macro`][in_macro] and [`in_external_macro`][in_external_macro] * [`Span`][span] -* [Clippy diagnostics][diagnostics] * [`Applicability`][applicability] For `EarlyLintPass` lints: @@ -360,6 +362,7 @@ don't hesitate to ask on Discord, IRC or in the issue/PR. [late_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/trait.LateLintPass.html [fn_kind]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/visit/enum.FnKind.html [diagnostics]: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/utils/diagnostics.rs +[utils]: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/utils/mod.rs [ident]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/source_map/symbol/struct.Ident.html [span]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax_pos/struct.Span.html [applicability]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/enum.Applicability.html From 68d04eda7b493668d34d5764dcaf46c5c996450a Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 1 Mar 2019 07:30:58 +0100 Subject: [PATCH 08/18] Copy from CONTRIBUTING.md: author lint, rustfix tests, rustfmt And adapt the wording a bit so that it fits in the overall guide. --- doc/adding_lints.md | 85 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 4 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 1ca93a44e1cf..ea53332605bf 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -7,16 +7,25 @@ creating an example lint from scratch. To get started, we will create a lint that detects functions called `foo`, because that's clearly a non-descriptive name. +* [Setup](#Setup) * [Testing](#Testing) +* [Rustfix tests](#Rustfix-tests) * [Lint declaration](#Lint-declaration) * [Lint passes](#Lint-passes) * [Emitting a lint](#Emitting-a-lint) * [Adding the lint logic](#Adding-the-lint-logic) * [Documentation](#Documentation) +* [Running rustfmt](#Running-rustfmt) * [Debugging](#Debugging) * [PR Checklist](#PR-Checklist) * [Cheatsheet](#Cheatsheet) +### Setup + +Clippy depends on the current git master version of rustc, which can change rapidly. Make sure you're +working near rust-clippy's master, and use the `setup-toolchain.sh` script to configure the appropriate +toolchain for this directory. + ### Testing Let's write some tests first that we can execute while we iterate on our lint. @@ -75,7 +84,25 @@ Once you are satisfied with the output, you need to run Running `TESTNAME=ui/foo_functions cargo uitest` should pass then. When you commit your lint, be sure to commit the `*.stderr` files, too. -Let's have a look at implementing our lint now. +### 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. This will +additionally run [rustfix](https://github.com/rust-lang-nursery/rustfix) for +that 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 `tests/ui/update-all-references.sh` to automatically generate the +`.fixed` file after running `cargo test`. + +With tests in place, let's have a look at implementing our lint now. + +### Testing manually + +Manually testing against an example file is useful if you have added some +`println!`s and test suite output becomes unreadable. To try Clippy with your +local modifications, run `env CLIPPY_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug input.rs` +from the working copy root. ### Lint declaration @@ -278,7 +305,44 @@ impl LintPass for Pass { /* .. */ } ``` That should be it for the lint implementation. Running `cargo test` should now -pass and we can finish up our work by adding some documentation. +pass. + +### Author lint + +If you have trouble implementing your lint, there is also the internal `author` +lint to generate Clippy code that detects the offending pattern. It does not +work for all of the Rust syntax, but can give a good starting point. + +First, create a new UI test file in the `tests/ui/` directory with the pattern +you want to match: + +```rust +// ./tests/ui/my_lint.rs +fn main() { + #[clippy::author] + let arr: [i32; 1] = [7]; // Replace line with the code you want to match +} +``` + +Now you run `TESTNAME=ui/my_lint cargo uitest` to produce a `.stdout` file with +the generated code: + +```rust +// ./tests/ui/my_lint.stdout + +if_chain! { + if let ExprKind::Array(ref elements) = stmt.node; + if elements.len() == 1; + if let ExprKind::Lit(ref lit) = elements[0].node; + if let LitKind::Int(7, _) = lit.node; + then { + // report your lint here + } +} +``` + +If the command was executed successfully, you can copy the code over to where +you are implementing your lint. ### Documentation @@ -309,6 +373,19 @@ declare_clippy_lint! { /* ... */ } Once your lint is merged, this documentation will show up in the [lint list][lint_list]. +### Running rustfmt + +[Rustfmt](https://github.com/rust-lang/rustfmt) is a tool for formatting Rust code according +to style guidelines. Your code has to be formatted by `rustfmt` before a PR can be merged. + +It can be installed via `rustup`: + +```bash +rustup component add rustfmt +``` + +Use `cargo fmt --all` to format the whole codebase. + ### Debugging If you want to debug parts of your lint implementation, you can use the `dbg!` @@ -317,7 +394,7 @@ output in the `stdout` part. ### PR Checklist -TODO: Prose +Before submitting your PR make sure you followed all of the basic requirements: - [ ] Followed [lint naming conventions][lint_naming] - [ ] Added passing UI tests (including committed `.stderr` file) @@ -329,7 +406,7 @@ TODO: Prose Here are some pointers to things you are likely going to need for every lint: * [Clippy utils][utils] - Various helper functions. Maybe the function you need - is already in here. (`implements_trait`, `match_path`, `snippet`, etc) + is already in here (`implements_trait`, `match_path`, `snippet`, etc) * [Clippy diagnostics][diagnostics] * [The `if_chain` macro][if_chain] * [`in_macro`][in_macro] and [`in_external_macro`][in_external_macro] From 1eac53b06b9bfdb7c6b5b710818595a0ab29c30f Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 1 Mar 2019 07:32:10 +0100 Subject: [PATCH 09/18] Cleanup 'Writing code' in CONTRIBUTING.md This is now all included in `doc/adding_lints.md` --- CONTRIBUTING.md | 117 ++---------------------------------------------- 1 file changed, 3 insertions(+), 114 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7ae8f3936a48..3eff20cead74 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,11 +14,6 @@ All contributors are expected to follow the [Rust Code of Conduct](http://www.ru * [Getting started](#getting-started) * [Finding something to fix/improve](#finding-something-to-fiximprove) * [Writing code](#writing-code) - * [Author lint](#author-lint) - * [Documentation](#documentation) - * [Running test suite](#running-test-suite) - * [Running rustfmt](#running-rustfmt) - * [Testing manually](#testing-manually) * [How Clippy works](#how-clippy-works) * [Fixing nightly build failures](#fixing-build-failures-caused-by-rust) * [Issue and PR Triage](#issue-and-pr-triage) @@ -73,121 +68,15 @@ an AST expression). `match_def_path()` in Clippy's `utils` module can also be us ## Writing code -Clippy depends on the current git master version of rustc, which can change rapidly. Make sure you're -working near rust-clippy's master, and use the `setup-toolchain.sh` script to configure the appropriate -toolchain for this directory. - -[Llogiq's blog post on lints](https://llogiq.github.io/2015/06/04/workflows.html) is a nice primer -to lint-writing, though it does get into advanced stuff. Most lints consist of an implementation of -`LintPass` with one or more of its default methods overridden. See the existing lints for examples -of this. +Have a look at the [docs for writing lints](doc/adding_lints.md) for more details. [Llogiq's blog post on lints](https://llogiq.github.io/2015/06/04/workflows.html) is also a nice primer +to lint-writing, though it does get into advanced stuff and may be a bit +outdated. If you want to add a new lint or change existing ones apart from bugfixing, it's also a good idea to give the [stability guarantees][rfc_stability] and [lint categories][rfc_lint_cats] sections of the [Clippy 1.0 RFC][clippy_rfc] a quick read. -### Author lint - -There is also the internal `author` lint to generate Clippy code that detects the offending pattern. It does not work for all of the Rust syntax, but can give a good starting point. - -First, create a new UI test file in the `tests/ui/` directory with the pattern you want to match: - -```rust -// ./tests/ui/my_lint.rs -fn main() { - #[clippy::author] - let arr: [i32; 1] = [7]; // Replace line with the code you want to match -} -``` - -Now you run `TESTNAME=ui/my_lint cargo uitest` to produce -a `.stdout` file with the generated code: - -```rust -// ./tests/ui/my_lint.stdout - -if_chain! { - if let ExprKind::Array(ref elements) = stmt.node; - if elements.len() == 1; - if let ExprKind::Lit(ref lit) = elements[0].node; - if let LitKind::Int(7, _) = lit.node; - then { - // report your lint here - } -} -``` - -If the command was executed successfully, you can copy the code over to where you are implementing your lint. - -### Documentation - -Please document your lint with a doc comment akin to the following: - -```rust -/// **What it does:** Checks for ... (describe what the lint matches). -/// -/// **Why is this bad?** Supply the reason for linting the code. -/// -/// **Known problems:** None. (Or describe where it could go wrong.) -/// -/// **Example:** -/// -/// ```rust -/// // Bad -/// Insert a short example of code that triggers the lint -/// -/// // Good -/// Insert a short example of improved code that doesn't trigger the lint -/// ``` -``` - -Once your lint is merged it will show up in the [lint list](https://rust-lang.github.io/rust-clippy/master/index.html) - -### Running test suite - -Use `cargo test` to run the whole testsuite. - -If you don't want to wait for all tests to finish, you can also execute a single test file by using `TESTNAME` to specify the test to run: - -```bash -TESTNAME=ui/empty_line_after_outer_attr cargo uitest -``` - -Clippy uses UI tests. UI tests check that the output of the compiler is exactly as expected. -Of course there's little sense in writing the output yourself or copying it around. -Therefore you should use `tests/ui/update-all-references.sh` (after running -`cargo test`) and check whether the output looks as you expect with `git diff`. Commit all -`*.stderr` files, too. - -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. This will -additionally run [rustfix](https://github.com/rust-lang-nursery/rustfix) for -that 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 `tests/ui/update-all-references.sh` to automatically generate the -`.fixed` file after running `cargo test`. - -### Running rustfmt - -[Rustfmt](https://github.com/rust-lang/rustfmt) is a tool for formatting Rust code according -to style guidelines. The code has to be formatted by `rustfmt` before a PR will be merged. - -It can be installed via `rustup`: -```bash -rustup component add rustfmt -``` - -Use `cargo fmt --all` to format the whole codebase. - -### Testing manually - -Manually testing against an example file is useful if you have added some -`println!`s and test suite output becomes unreadable. To try Clippy with your -local modifications, run `env CLIPPY_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug input.rs` -from the working copy root. - ## How Clippy works Clippy is a [rustc compiler plugin][compiler_plugin]. The main entry point is at [`src/lib.rs`][main_entry]. In there, the lint registration is delegated to the [`clippy_lints`][lint_crate] crate. From 680dc8a1055e9ace9548406a2939670733b53dd4 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 1 Mar 2019 07:32:51 +0100 Subject: [PATCH 10/18] Run remark in `doc` directory, too. --- ci/base-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index d2b959a2d9d1..5a1a3888df4e 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -4,7 +4,7 @@ echo "Running clippy base tests" PATH=$PATH:./node_modules/.bin if [ "$TRAVIS_OS_NAME" == "linux" ]; then - remark -f *.md > /dev/null + remark -f *.md -f doc/*.md > /dev/null fi # build clippy in debug mode and run tests cargo build --features debugging From 81f47e1ffc58de882042cab79f6b929932f98f75 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 2 Mar 2019 16:25:05 +0100 Subject: [PATCH 11/18] Author lint usage via playground --- doc/adding_lints.md | 46 +++++++++++++++------------------------------ 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index ea53332605bf..484e62eb4e98 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -14,6 +14,7 @@ because that's clearly a non-descriptive name. * [Lint passes](#Lint-passes) * [Emitting a lint](#Emitting-a-lint) * [Adding the lint logic](#Adding-the-lint-logic) +* [Author lint](#Author-lint) * [Documentation](#Documentation) * [Running rustfmt](#Running-rustfmt) * [Debugging](#Debugging) @@ -165,7 +166,7 @@ lint pass it should be fine. Next you should run `util/dev update_lints` to register the lint in various places, mainly in `clippy_lints/src/lib.rs`. -While `update_lints` automates some things, it doesn't automate everything. We +While `update_lints` automates some things, it doesn't automate everything. You will have to register our lint pass manually in the `register_plugins` function in `clippy_lints/src/lib.rs`: @@ -179,7 +180,7 @@ pass our lint is going to need. ### Lint passes -Writing a lint that just checks for the name of a function means that we just +Writing a lint that only checks for the name of a function means that we only have to deal with the AST and don't have to deal with the type system at all. This is good, because it makes writing this particular lint less complicated. @@ -204,7 +205,9 @@ use rustc::{declare_tool_lint, lint_array}; ### Emitting a lint -With UI tests in place, we can start working on the implementation of the lint logic. We can keep executing the tests until we make them pass. +With UI tests and the lint declaration in place, we can start working on the +implementation of the lint logic. We can keep executing the tests until we make +them pass. Let's start by implementing the `EarlyLintPass` for our `FooFunctionsPass`: @@ -270,7 +273,7 @@ impl EarlyLintPass for Pass { We separate the lint conditional from the lint emissions because it makes the code a bit easier to read. In some cases this separation would also allow to -write some unit tests (as opposed to UI tests) for the separate function. +write some unit tests (as opposed to only UI tests) for the separate function. In our example, `is_foo_fn` looks like: @@ -294,7 +297,7 @@ running `cargo test` should produce the expected output. Remember to run `cargo test` (as opposed to `cargo uitest`) will also ensure that our lint implementation is not violating any Clippy lints itself. -If you are still following the example, you'll see that the `FooFunctionsPass` +If you are still following the example, you will see that `FooFunctionsPass` violates a Clippy lint. So we are going to rename that struct to just `Pass`: ```rust @@ -313,33 +316,12 @@ If you have trouble implementing your lint, there is also the internal `author` lint to generate Clippy code that detects the offending pattern. It does not work for all of the Rust syntax, but can give a good starting point. -First, create a new UI test file in the `tests/ui/` directory with the pattern -you want to match: +The quickest way to use it, is the [Rust playground][play].rust-lang.org). +Put the code you want to lint into the editor and add the `#[clippy::author]` +attribute above the item. Then run Clippy via `Tools -> Clippy` and you should +see the generated code in the output below. -```rust -// ./tests/ui/my_lint.rs -fn main() { - #[clippy::author] - let arr: [i32; 1] = [7]; // Replace line with the code you want to match -} -``` - -Now you run `TESTNAME=ui/my_lint cargo uitest` to produce a `.stdout` file with -the generated code: - -```rust -// ./tests/ui/my_lint.stdout - -if_chain! { - if let ExprKind::Array(ref elements) = stmt.node; - if elements.len() == 1; - if let ExprKind::Lit(ref lit) = elements[0].node; - if let LitKind::Int(7, _) = lit.node; - then { - // report your lint here - } -} -``` +[Here][author_example] is an example on the playground. If the command was executed successfully, you can copy the code over to where you are implementing your lint. @@ -448,3 +430,5 @@ don't hesitate to ask on Discord, IRC or in the issue/PR. [ast]: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/index.html [in_macro]: https://github.com/rust-lang/rust-clippy/blob/d0717d1f9531a03d154aaeb0cad94c243915a146/clippy_lints/src/utils/mod.rs#L94 [in_external_macro]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/fn.in_external_macro.html +[play]: https://play.rust-lang.org +[author_example]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f093b986e80ad62f3b67a1f24f5e66e2 From b03011480c4b04b043ce3a6b67e1459739a08651 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 2 Mar 2019 16:29:11 +0100 Subject: [PATCH 12/18] Note that stderr files don't have to be created manually --- doc/adding_lints.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 484e62eb4e98..d8edf79714cb 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -34,6 +34,8 @@ Let's write some tests first that we can execute while we iterate on our lint. Clippy uses UI tests for testing. UI tests check that the output of Clippy is exactly as expected. 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 further down. Let's create the test file at `tests/ui/foo_functions.rs`. It doesn't really matter what the file is called, but it's a good convention to name it after the @@ -81,9 +83,9 @@ While you are working on implementing your lint, you can keep running the UI test. That allows you to check if the output is turning into what you want. Once you are satisfied with the output, you need to run -`tests/ui/update-all-references.sh` to update the `stderr` file for your lint. +`tests/ui/update-all-references.sh` to update the `.stderr` file for your lint. Running `TESTNAME=ui/foo_functions cargo uitest` should pass then. When you -commit your lint, be sure to commit the `*.stderr` files, too. +commit your lint, be sure to commit the `.stderr` files, too. ### Rustfix tests From 0da3521eb6161cc855da65e6f0c7308147b22ba7 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 3 Mar 2019 09:48:42 +0100 Subject: [PATCH 13/18] Reword 'Setup' a bit --- README.md | 4 ++-- doc/adding_lints.md | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 98365d228d04..3a56bce4f139 100644 --- a/README.md +++ b/README.md @@ -107,13 +107,13 @@ script: - cargo clippy # if you want the build job to fail when encountering warnings, use - cargo clippy -- -D warnings - # in order to also check tests and none-default crate features, use + # in order to also check tests and non-default crate features, use - cargo clippy --all-targets --all-features -- -D warnings - cargo test # etc. ``` -It might happen that Clippy is not available for a certain nightly release. +If you are on nightly, It might happen that Clippy is not available for a certain nightly release. In this case you can try to conditionally install Clippy from the git repo. ```yaml diff --git a/doc/adding_lints.md b/doc/adding_lints.md index d8edf79714cb..12276b1a4107 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -23,9 +23,10 @@ because that's clearly a non-descriptive name. ### Setup -Clippy depends on the current git master version of rustc, which can change rapidly. Make sure you're -working near rust-clippy's master, and use the `setup-toolchain.sh` script to configure the appropriate -toolchain for this directory. +When working on Clippy, you will need the current git master version of rustc, +which can change rapidly. Make sure you're working near rust-clippy's master, +and use the `setup-toolchain.sh` script to configure the appropriate toolchain +for the Clippy directory. ### Testing From 23f3256e960b4eadcdecda6adea74da033b8ffcb Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 3 Mar 2019 09:50:56 +0100 Subject: [PATCH 14/18] More text fixes after Test section was moved to top --- doc/adding_lints.md | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 12276b1a4107..27381721103e 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -78,7 +78,7 @@ fn main() { Now we can run the test with `TESTNAME=ui/foo_functions cargo uitest`. Currently this test will fail. If you go through the output you will see that we -have to add some missing imports to our lint file. +are told that `clippy::foo_functions` is an unknown lint, which is expected. While you are working on implementing your lint, you can keep running the UI test. That allows you to check if the output is turning into what you want. @@ -97,16 +97,16 @@ that 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 `tests/ui/update-all-references.sh` to automatically generate the -`.fixed` file after running `cargo test`. +`.fixed` file after running the tests. With tests in place, let's have a look at implementing our lint now. ### Testing manually -Manually testing against an example file is useful if you have added some -`println!`s and test suite output becomes unreadable. To try Clippy with your -local modifications, run `env CLIPPY_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug input.rs` -from the working copy root. +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 `env CLIPPY_TESTS=true cargo run --bin +clippy-driver -- -L ./target/debug input.rs` from the working copy root. ### Lint declaration @@ -177,9 +177,9 @@ in `clippy_lints/src/lib.rs`: reg.register_early_lint_pass(box foo_functions::FooFunctionsPass); ``` -Without that, running the UI tests would produce an error like `unknown clippy -lint: clippy::foo_functions`. The next decision we have to make is which lint -pass our lint is going to need. +This should fix the `unknown clippy lint: clippy::foo_functions` error that we +saw when we executed our tests the first time. The next decision we have to make +is which lint pass our lint is going to need. ### Lint passes @@ -189,8 +189,6 @@ This is good, because it makes writing this particular lint less complicated. We have to make this decision with every new Clippy lint. It boils down to using either [`EarlyLintPass`][early_lint_pass] or [`LateLintPass`][late_lint_pass]. -This is a result of Rust's compilation process. You can read more about it [in -the rustc guide][compilation_stages]. 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 @@ -209,8 +207,7 @@ use rustc::{declare_tool_lint, lint_array}; ### Emitting a lint With UI tests and the lint declaration in place, we can start working on the -implementation of the lint logic. We can keep executing the tests until we make -them pass. +implementation of the lint logic. Let's start by implementing the `EarlyLintPass` for our `FooFunctionsPass`: @@ -229,9 +226,12 @@ the next section. Let's worry about the details later and emit our lint for *every* function definition first. Depending on how complex we want our lint message to be, we can choose from a -variety of lint emission functions. They can all be found in +variety of lint emission functions. They can all be found in [`clippy_lints/src/utils/diagnostics.rs`][diagnostics]. +`span_help_and_lint` seems most appropriate in this case. It allows us to +provide an extra help message and we can't really suggest a better name +automatically. This is how it looks: ```rust impl EarlyLintPass for Pass { @@ -247,9 +247,11 @@ impl EarlyLintPass for Pass { } ``` +Running our UI test should now produce output that contains the lint message. + ### Adding the lint logic -Writing the logic for your lint will most likely be different from this example, +Writing the logic for our lint will most likely be different from this example, so this section is kept rather short. Using the [`check_fn`][check_fn] method gives us access to [`FnKind`][fn_kind] From 8d2cbb94d468bb0e354c9a211b5b1c174d76cdb2 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 3 Mar 2019 10:00:05 +0100 Subject: [PATCH 15/18] Add the rustc guide to the cheatsheet --- doc/adding_lints.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 27381721103e..f74b179f7597 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -399,6 +399,7 @@ Here are some pointers to things you are likely going to need for every lint: * [`in_macro`][in_macro] and [`in_external_macro`][in_external_macro] * [`Span`][span] * [`Applicability`][applicability] +* [The rustc guide][rustc_guide] explains a lot of internal compiler concepts. For `EarlyLintPass` lints: @@ -437,3 +438,4 @@ don't hesitate to ask on Discord, IRC or in the issue/PR. [in_external_macro]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/fn.in_external_macro.html [play]: https://play.rust-lang.org [author_example]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f093b986e80ad62f3b67a1f24f5e66e2 +[rustc_guide]: https://rust-lang.github.io/rustc-guide/ From d1caaa47c0a72534f8c417ff28e61b39137dd6b0 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 4 Mar 2019 20:39:28 +0100 Subject: [PATCH 16/18] s/you/we; One more checklist item --- doc/adding_lints.md | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index f74b179f7597..ca285f37bf8d 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -38,11 +38,11 @@ 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 further down. -Let's create the test file at `tests/ui/foo_functions.rs`. It doesn't really -matter what the file is called, but it's a good convention to name it after the -lint it is testing, so `foo_functions.rs` it is. +We start by creating the test file at `tests/ui/foo_functions.rs`. It doesn't +really matter what the file is called, but it's a good convention to name it +after the lint it is testing, so `foo_functions.rs` it is. -Inside we put some examples to get started: +Inside the file we put some examples to get started: ```rust #![warn(clippy::foo_functions)] @@ -80,13 +80,13 @@ Now we can run the test with `TESTNAME=ui/foo_functions cargo uitest`. Currently this test will fail. If you go through the output you will see that we are told that `clippy::foo_functions` is an unknown lint, which is expected. -While you are working on implementing your lint, you can keep running the UI -test. That allows you to check if the output is turning into what you want. +While we are working on implementing our lint, we can keep running the UI +test. That allows us to check if the output is turning into what we want. -Once you are satisfied with the output, you need to run -`tests/ui/update-all-references.sh` to update the `.stderr` file for your lint. -Running `TESTNAME=ui/foo_functions cargo uitest` should pass then. When you -commit your lint, be sure to commit the `.stderr` files, too. +Once we are satisfied with the output, we need to run +`tests/ui/update-all-references.sh` to update the `.stderr` file for our lint. +Running `TESTNAME=ui/foo_functions cargo uitest` should pass then. When we +commit our lint, we need to commit the generated `.stderr` files, too. ### Rustfix tests @@ -139,8 +139,8 @@ state the thing that is being checked for and read well when used with * The last part should be a text that explains what exactly is wrong with the code -With our lint declaration done, we will now make sure that our lint declaration -is assigned to a lint pass: +With our lint declaration done, we will now make sure that it is assigned to a +lint pass: ```rust // clippy_lints/src/foo_functions.rs @@ -166,10 +166,10 @@ impl LintPass for FooFunctionsPass { Don't worry about the `name` method here. As long as it includes the name of the lint pass it should be fine. -Next you should run `util/dev update_lints` to register the lint in various +Next we need to run `util/dev update_lints` to register the lint in various places, mainly in `clippy_lints/src/lib.rs`. -While `update_lints` automates some things, it doesn't automate everything. You +While `update_lints` automates some things, it doesn't automate everything. We will have to register our lint pass manually in the `register_plugins` function in `clippy_lints/src/lib.rs`: @@ -251,7 +251,7 @@ Running our UI test should now produce output that contains the lint message. ### Adding the lint logic -Writing the logic for our lint will most likely be different from this example, +Writing the logic for your lint will most likely be different from our example, so this section is kept rather short. Using the [`check_fn`][check_fn] method gives us access to [`FnKind`][fn_kind] @@ -295,7 +295,7 @@ fn is_foo_fn(fn_kind: FnKind<'_>) -> bool { } ``` -Now you'll want to also run the full test suite with `cargo test`. At this point +Now we should also run the full test suite with `cargo test`. At this point running `cargo test` should produce the expected output. Remember to run `tests/ui/update-all-references.sh` to update the `.stderr` file. @@ -386,6 +386,7 @@ Before submitting your PR make sure you followed all of the basic requirements: - [ ] Followed [lint naming conventions][lint_naming] - [ ] Added passing UI tests (including committed `.stderr` file) - [ ] `cargo test` passes locally +- [ ] Executed `util/dev update_lints` - [ ] Added lint documentation ### Cheatsheet From 3c67c629fdaf3b274a7bbf286e56485742f67d48 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 4 Mar 2019 20:43:11 +0100 Subject: [PATCH 17/18] Add rustc nightly docs to cheatsheet --- doc/adding_lints.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index ca285f37bf8d..688be09542a8 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -400,7 +400,9 @@ Here are some pointers to things you are likely going to need for every lint: * [`in_macro`][in_macro] and [`in_external_macro`][in_external_macro] * [`Span`][span] * [`Applicability`][applicability] -* [The rustc guide][rustc_guide] explains a lot of internal compiler concepts. +* [The rustc guide][rustc_guide] explains a lot of internal compiler concepts +* [The nightly rustc docs][nightly_docs] which has been linked to throughout + this guide For `EarlyLintPass` lints: @@ -440,3 +442,4 @@ don't hesitate to ask on Discord, IRC or in the issue/PR. [play]: https://play.rust-lang.org [author_example]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f093b986e80ad62f3b67a1f24f5e66e2 [rustc_guide]: https://rust-lang.github.io/rustc-guide/ +[nightly_docs]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ From 34685a5f6039ec406962730cf2613044a4d8f31c Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 8 Mar 2019 08:50:13 +0100 Subject: [PATCH 18/18] Update comment location --- doc/adding_lints.md | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 688be09542a8..53e671e17e03 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -339,22 +339,26 @@ lint declaration. Please document your lint with a doc comment akin to the following: ```rust -/// **What it does:** Checks for ... (describe what the lint matches). -/// -/// **Why is this bad?** Supply the reason for linting the code. -/// -/// **Known problems:** None. (Or describe where it could go wrong.) -/// -/// **Example:** -/// -/// ```rust,ignore -/// // Bad -/// Insert a short example of code that triggers the lint -/// -/// // Good -/// Insert a short example of improved code that doesn't trigger the lint -/// ``` -declare_clippy_lint! { /* ... */ } +declare_clippy_lint! { + /// **What it does:** Checks for ... (describe what the lint matches). + /// + /// **Why is this bad?** Supply the reason for linting the code. + /// + /// **Known problems:** None. (Or describe where it could go wrong.) + /// + /// **Example:** + /// + /// ```rust,ignore + /// // Bad + /// Insert a short example of code that triggers the lint + /// + /// // Good + /// Insert a short example of improved code that doesn't trigger the lint + /// ``` + pub FOO_FUNCTIONS, + pedantic, + "function named `foo`, which is not a descriptive name" +} ``` Once your lint is merged, this documentation will show up in the [lint