]>
Commit | Line | Data |
---|---|---|
f20569fa XL |
1 | # Contributing to Clippy |
2 | ||
3 | Hello fellow Rustacean! Great to see your interest in compiler internals and lints! | |
4 | ||
5 | **First**: if you're unsure or afraid of _anything_, just ask or submit the issue or pull request anyway. You won't be | |
6 | yelled at for giving it your best effort. The worst that can happen is that you'll be politely asked to change | |
7 | something. We appreciate any sort of contributions, and don't want a wall of rules to get in the way of that. | |
8 | ||
9 | Clippy welcomes contributions from everyone. There are many ways to contribute to Clippy and the following document | |
10 | explains how you can contribute and how to get started. If you have any questions about contributing or need help with | |
11 | anything, feel free to ask questions on issues or visit the `#clippy` on [Zulip]. | |
12 | ||
13 | All contributors are expected to follow the [Rust Code of Conduct]. | |
14 | ||
15 | - [Contributing to Clippy](#contributing-to-clippy) | |
16 | - [Getting started](#getting-started) | |
17 | - [High level approach](#high-level-approach) | |
18 | - [Finding something to fix/improve](#finding-something-to-fiximprove) | |
19 | - [Writing code](#writing-code) | |
20 | - [Getting code-completion for rustc internals to work](#getting-code-completion-for-rustc-internals-to-work) | |
cdc7bbd5 XL |
21 | - [IntelliJ Rust](#intellij-rust) |
22 | - [Rust Analyzer](#rust-analyzer) | |
f20569fa XL |
23 | - [How Clippy works](#how-clippy-works) |
24 | - [Syncing changes between Clippy and `rust-lang/rust`](#syncing-changes-between-clippy-and-rust-langrust) | |
25 | - [Patching git-subtree to work with big repos](#patching-git-subtree-to-work-with-big-repos) | |
26 | - [Performing the sync from `rust-lang/rust` to Clippy](#performing-the-sync-from-rust-langrust-to-clippy) | |
27 | - [Performing the sync from Clippy to `rust-lang/rust`](#performing-the-sync-from-clippy-to-rust-langrust) | |
28 | - [Defining remotes](#defining-remotes) | |
29 | - [Issue and PR triage](#issue-and-pr-triage) | |
30 | - [Bors and Homu](#bors-and-homu) | |
31 | - [Contributions](#contributions) | |
32 | ||
33 | [Zulip]: https://rust-lang.zulipchat.com/#narrow/stream/clippy | |
34 | [Rust Code of Conduct]: https://www.rust-lang.org/policies/code-of-conduct | |
35 | ||
36 | ## Getting started | |
37 | ||
38 | **Note: If this is your first time contributing to Clippy, you should | |
39 | first read the [Basics docs](doc/basics.md).** | |
40 | ||
41 | ### High level approach | |
42 | ||
43 | 1. Find something to fix/improve | |
44 | 2. Change code (likely some file in `clippy_lints/src/`) | |
45 | 3. Follow the instructions in the [Basics docs](doc/basics.md) to get set up | |
46 | 4. Run `cargo test` in the root directory and wiggle code until it passes | |
47 | 5. Open a PR (also can be done after 2. if you run into problems) | |
48 | ||
49 | ### Finding something to fix/improve | |
50 | ||
51 | All issues on Clippy are mentored, if you want help simply ask @Manishearth, @flip1995, @phansch | |
52 | or @llogiq directly by mentioning them in the issue or over on [Zulip]. This list may be out of date. | |
53 | All currently active mentors can be found [here](https://github.com/rust-lang/highfive/blob/master/highfive/configs/rust-lang/rust-clippy.json#L3) | |
54 | ||
55 | Some issues are easier than others. The [`good-first-issue`] label can be used to find the easy | |
56 | issues. You can use `@rustbot claim` to assign the issue to yourself. | |
57 | ||
58 | There are also some abandoned PRs, marked with [`S-inactive-closed`]. | |
59 | Pretty often these PRs are nearly completed and just need some extra steps | |
60 | (formatting, addressing review comments, ...) to be merged. If you want to | |
61 | complete such a PR, please leave a comment in the PR and open a new one based | |
62 | on it. | |
63 | ||
64 | Issues marked [`T-AST`] involve simple matching of the syntax tree structure, | |
65 | and are generally easier than [`T-middle`] issues, which involve types | |
66 | and resolved paths. | |
67 | ||
68 | [`T-AST`] issues will generally need you to match against a predefined syntax structure. | |
69 | To figure out how this syntax structure is encoded in the AST, it is recommended to run | |
70 | `rustc -Z ast-json` on an example of the structure and compare with the [nodes in the AST docs]. | |
71 | Usually the lint will end up to be a nested series of matches and ifs, [like so][deep-nesting]. | |
72 | But we can make it nest-less by using [if_chain] macro, [like this][nest-less]. | |
73 | ||
74 | [`E-medium`] issues are generally pretty easy too, though it's recommended you work on an [`good-first-issue`] | |
75 | first. Sometimes they are only somewhat involved code wise, but not difficult per-se. | |
76 | Note that [`E-medium`] issues may require some knowledge of Clippy internals or some | |
77 | debugging to find the actual problem behind the issue. | |
78 | ||
79 | [`T-middle`] issues can be more involved and require verifying types. The [`ty`] module contains a | |
80 | lot of methods that are useful, though one of the most useful would be `expr_ty` (gives the type of | |
81 | an AST expression). `match_def_path()` in Clippy's `utils` module can also be useful. | |
82 | ||
83 | [`good-first-issue`]: https://github.com/rust-lang/rust-clippy/labels/good-first-issue | |
84 | [`S-inactive-closed`]: https://github.com/rust-lang/rust-clippy/pulls?q=is%3Aclosed+label%3AS-inactive-closed | |
85 | [`T-AST`]: https://github.com/rust-lang/rust-clippy/labels/T-AST | |
86 | [`T-middle`]: https://github.com/rust-lang/rust-clippy/labels/T-middle | |
87 | [`E-medium`]: https://github.com/rust-lang/rust-clippy/labels/E-medium | |
88 | [`ty`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty | |
89 | [nodes in the AST docs]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/ | |
90 | [deep-nesting]: https://github.com/rust-lang/rust-clippy/blob/557f6848bd5b7183f55c1e1522a326e9e1df6030/clippy_lints/src/mem_forget.rs#L29-L43 | |
91 | [if_chain]: https://docs.rs/if_chain/*/if_chain | |
92 | [nest-less]: https://github.com/rust-lang/rust-clippy/blob/557f6848bd5b7183f55c1e1522a326e9e1df6030/clippy_lints/src/bit_mask.rs#L124-L150 | |
93 | ||
94 | ## Writing code | |
95 | ||
96 | Have a look at the [docs for writing lints][adding_lints] for more details. | |
97 | ||
98 | If you want to add a new lint or change existing ones apart from bugfixing, it's | |
99 | also a good idea to give the [stability guarantees][rfc_stability] and | |
100 | [lint categories][rfc_lint_cats] sections of the [Clippy 1.0 RFC][clippy_rfc] a | |
101 | quick read. | |
102 | ||
103 | [adding_lints]: https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md | |
104 | [clippy_rfc]: https://github.com/rust-lang/rfcs/blob/master/text/2476-clippy-uno.md | |
105 | [rfc_stability]: https://github.com/rust-lang/rfcs/blob/master/text/2476-clippy-uno.md#stability-guarantees | |
106 | [rfc_lint_cats]: https://github.com/rust-lang/rfcs/blob/master/text/2476-clippy-uno.md#lint-audit-and-categories | |
107 | ||
108 | ## Getting code-completion for rustc internals to work | |
109 | ||
cdc7bbd5 XL |
110 | ### IntelliJ Rust |
111 | Unfortunately, [`IntelliJ Rust`][IntelliJ_rust_homepage] does not (yet?) understand how Clippy uses compiler-internals | |
f20569fa XL |
112 | using `extern crate` and it also needs to be able to read the source files of the rustc-compiler which are not |
113 | available via a `rustup` component at the time of writing. | |
114 | To work around this, you need to have a copy of the [rustc-repo][rustc_repo] available which can be obtained via | |
115 | `git clone https://github.com/rust-lang/rust/`. | |
116 | Then you can run a `cargo dev` command to automatically make Clippy use the rustc-repo via path-dependencies | |
cdc7bbd5 | 117 | which `IntelliJ Rust` will be able to understand. |
136023e0 | 118 | Run `cargo dev setup intellij --repo-path <repo-path>` where `<repo-path>` is a path to the rustc repo |
f20569fa XL |
119 | you just cloned. |
120 | The command will add path-dependencies pointing towards rustc-crates inside the rustc repo to | |
a2a8927a | 121 | Clippy's `Cargo.toml`s and should allow `IntelliJ Rust` to understand most of the types that Clippy uses. |
f20569fa XL |
122 | Just make sure to remove the dependencies again before finally making a pull request! |
123 | ||
f20569fa | 124 | [rustc_repo]: https://github.com/rust-lang/rust/ |
cdc7bbd5 XL |
125 | [IntelliJ_rust_homepage]: https://intellij-rust.github.io/ |
126 | ||
127 | ### Rust Analyzer | |
128 | As of [#6869][6869], [`rust-analyzer`][ra_homepage] can understand that Clippy uses compiler-internals | |
a2a8927a XL |
129 | using `extern crate` when `package.metadata.rust-analyzer.rustc_private` is set to `true` in Clippy's `Cargo.toml.` |
130 | You will require a `nightly` toolchain with the `rustc-dev` component installed. | |
cdc7bbd5 XL |
131 | Make sure that in the `rust-analyzer` configuration, you set |
132 | ``` | |
133 | { "rust-analyzer.rustcSource": "discover" } | |
134 | ``` | |
135 | and | |
136 | ``` | |
137 | { "rust-analyzer.updates.channel": "nightly" } | |
138 | ``` | |
139 | You should be able to see information on things like `Expr` or `EarlyContext` now if you hover them, also | |
140 | a lot more type hints. | |
141 | This will work with `rust-analyzer 2021-03-15` shipped in nightly `1.52.0-nightly (107896c32 2021-03-15)` or later. | |
142 | ||
143 | [ra_homepage]: https://rust-analyzer.github.io/ | |
144 | [6869]: https://github.com/rust-lang/rust-clippy/pull/6869 | |
f20569fa XL |
145 | |
146 | ## How Clippy works | |
147 | ||
148 | [`clippy_lints/src/lib.rs`][lint_crate_entry] imports all the different lint modules and registers in the [`LintStore`]. | |
149 | For example, the [`else_if_without_else`][else_if_without_else] lint is registered like this: | |
150 | ||
151 | ```rust | |
152 | // ./clippy_lints/src/lib.rs | |
153 | ||
154 | // ... | |
155 | pub mod else_if_without_else; | |
156 | // ... | |
157 | ||
158 | pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &Conf) { | |
159 | // ... | |
160 | store.register_early_pass(|| box else_if_without_else::ElseIfWithoutElse); | |
161 | // ... | |
162 | ||
163 | store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ | |
164 | // ... | |
165 | LintId::of(&else_if_without_else::ELSE_IF_WITHOUT_ELSE), | |
166 | // ... | |
167 | ]); | |
168 | } | |
169 | ``` | |
170 | ||
171 | The [`rustc_lint::LintStore`][`LintStore`] provides two methods to register lints: | |
172 | [register_early_pass][reg_early_pass] and [register_late_pass][reg_late_pass]. Both take an object | |
173 | that implements an [`EarlyLintPass`][early_lint_pass] or [`LateLintPass`][late_lint_pass] respectively. This is done in | |
174 | every single lint. It's worth noting that the majority of `clippy_lints/src/lib.rs` is autogenerated by `cargo dev | |
175 | update_lints`. When you are writing your own lint, you can use that script to save you some time. | |
176 | ||
177 | ```rust | |
178 | // ./clippy_lints/src/else_if_without_else.rs | |
179 | ||
180 | use rustc_lint::{EarlyLintPass, EarlyContext}; | |
181 | ||
182 | // ... | |
183 | ||
184 | pub struct ElseIfWithoutElse; | |
185 | ||
186 | // ... | |
187 | ||
188 | impl EarlyLintPass for ElseIfWithoutElse { | |
189 | // ... the functions needed, to make the lint work | |
190 | } | |
191 | ``` | |
192 | ||
193 | The difference between `EarlyLintPass` and `LateLintPass` is that the methods of the `EarlyLintPass` trait only provide | |
194 | AST information. The methods of the `LateLintPass` trait are executed after type checking and contain type information | |
195 | via the `LateContext` parameter. | |
196 | ||
197 | That's why the `else_if_without_else` example uses the `register_early_pass` function. Because the | |
198 | [actual lint logic][else_if_without_else] does not depend on any type information. | |
199 | ||
200 | [lint_crate_entry]: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/lib.rs | |
201 | [else_if_without_else]: https://github.com/rust-lang/rust-clippy/blob/4253aa7137cb7378acc96133c787e49a345c2b3c/clippy_lints/src/else_if_without_else.rs | |
202 | [`LintStore`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/struct.LintStore.html | |
203 | [reg_early_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/struct.LintStore.html#method.register_early_pass | |
204 | [reg_late_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/struct.LintStore.html#method.register_late_pass | |
205 | [early_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html | |
206 | [late_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html | |
207 | ||
208 | ## Syncing changes between Clippy and [`rust-lang/rust`] | |
209 | ||
210 | Clippy currently gets built with a pinned nightly version. | |
211 | ||
212 | In the `rust-lang/rust` repository, where rustc resides, there's a copy of Clippy | |
213 | that compiler hackers modify from time to time to adapt to changes in the unstable | |
214 | API of the compiler. | |
215 | ||
216 | We need to sync these changes back to this repository periodically, and the changes | |
217 | made to this repository in the meantime also need to be synced to the `rust-lang/rust` repository. | |
218 | ||
219 | To avoid flooding the `rust-lang/rust` PR queue, this two-way sync process is done | |
220 | in a bi-weekly basis if there's no urgent changes. This is done starting on the day of | |
221 | the Rust stable release and then every other week. That way we guarantee that we keep | |
222 | this repo up to date with the latest compiler API, and every feature in Clippy is available | |
223 | for 2 weeks in nightly, before it can get to beta. For reference, the first sync | |
224 | following this cadence was performed the 2020-08-27. | |
225 | ||
226 | This process is described in detail in the following sections. For general information | |
227 | about `subtree`s in the Rust repository see [Rust's `CONTRIBUTING.md`][subtree]. | |
228 | ||
229 | ### Patching git-subtree to work with big repos | |
230 | ||
a2a8927a | 231 | Currently, there's a bug in `git-subtree` that prevents it from working properly |
f20569fa XL |
232 | with the [`rust-lang/rust`] repo. There's an open PR to fix that, but it's stale. |
233 | Before continuing with the following steps, we need to manually apply that fix to | |
234 | our local copy of `git-subtree`. | |
235 | ||
236 | You can get the patched version of `git-subtree` from [here][gitgitgadget-pr]. | |
237 | Put this file under `/usr/lib/git-core` (taking a backup of the previous file) | |
238 | and make sure it has the proper permissions: | |
239 | ||
240 | ```bash | |
241 | sudo cp --backup /path/to/patched/git-subtree.sh /usr/lib/git-core/git-subtree | |
242 | sudo chmod --reference=/usr/lib/git-core/git-subtree~ /usr/lib/git-core/git-subtree | |
243 | sudo chown --reference=/usr/lib/git-core/git-subtree~ /usr/lib/git-core/git-subtree | |
244 | ``` | |
245 | ||
246 | _Note:_ The first time running `git subtree push` a cache has to be built. This | |
247 | involves going through the complete Clippy history once. For this you have to | |
248 | increase the stack limit though, which you can do with `ulimit -s 60000`. | |
249 | Make sure to run the `ulimit` command from the same session you call git subtree. | |
250 | ||
251 | _Note:_ If you are a Debian user, `dash` is the shell used by default for scripts instead of `sh`. | |
252 | This shell has a hardcoded recursion limit set to 1000. In order to make this process work, | |
253 | you need to force the script to run `bash` instead. You can do this by editing the first | |
254 | line of the `git-subtree` script and changing `sh` to `bash`. | |
255 | ||
256 | ### Performing the sync from [`rust-lang/rust`] to Clippy | |
257 | ||
258 | Here is a TL;DR version of the sync process (all of the following commands have | |
259 | to be run inside the `rust` directory): | |
260 | ||
261 | 1. Clone the [`rust-lang/rust`] repository or make sure it is up to date. | |
262 | 2. Checkout the commit from the latest available nightly. You can get it using `rustup check`. | |
263 | 3. Sync the changes to the rust-copy of Clippy to your Clippy fork: | |
264 | ```bash | |
3c0e092e XL |
265 | # Make sure to change `your-github-name` to your github name in the following command. Also be |
266 | # sure to either use a net-new branch, e.g. `sync-from-rust`, or delete the branch beforehand | |
267 | # because changes cannot be fast forwarded | |
f20569fa XL |
268 | git subtree push -P src/tools/clippy git@github.com:your-github-name/rust-clippy sync-from-rust |
269 | ``` | |
270 | ||
271 | _Note:_ This will directly push to the remote repository. You can also push | |
272 | to your local copy by replacing the remote address with `/path/to/rust-clippy` | |
273 | directory. | |
274 | ||
275 | _Note:_ Most of the time you have to create a merge commit in the | |
276 | `rust-clippy` repo (this has to be done in the Clippy repo, not in the | |
277 | rust-copy of Clippy): | |
278 | ```bash | |
279 | git fetch origin && git fetch upstream | |
280 | git checkout sync-from-rust | |
281 | git merge upstream/master | |
282 | ``` | |
283 | 4. Open a PR to `rust-lang/rust-clippy` and wait for it to get merged (to | |
284 | accelerate the process ping the `@rust-lang/clippy` team in your PR and/or | |
285 | ~~annoy~~ ask them in the [Zulip] stream.) | |
286 | ||
287 | ### Performing the sync from Clippy to [`rust-lang/rust`] | |
288 | ||
289 | All of the following commands have to be run inside the `rust` directory. | |
290 | ||
291 | 1. Make sure Clippy itself is up-to-date by following the steps outlined in the previous | |
292 | section if necessary. | |
293 | ||
294 | 2. Sync the `rust-lang/rust-clippy` master to the rust-copy of Clippy: | |
295 | ```bash | |
296 | git checkout -b sync-from-clippy | |
297 | git subtree pull -P src/tools/clippy https://github.com/rust-lang/rust-clippy master | |
298 | ``` | |
299 | 3. Open a PR to [`rust-lang/rust`] | |
300 | ||
301 | ### Defining remotes | |
302 | ||
303 | You may want to define remotes, so you don't have to type out the remote | |
304 | addresses on every sync. You can do this with the following commands (these | |
305 | commands still have to be run inside the `rust` directory): | |
306 | ||
307 | ```bash | |
308 | # Set clippy-upstream remote for pulls | |
309 | $ git remote add clippy-upstream https://github.com/rust-lang/rust-clippy | |
310 | # Make sure to not push to the upstream repo | |
311 | $ git remote set-url --push clippy-upstream DISABLED | |
312 | # Set clippy-origin remote to your fork for pushes | |
313 | $ git remote add clippy-origin git@github.com:your-github-name/rust-clippy | |
314 | # Set a local remote | |
315 | $ git remote add clippy-local /path/to/rust-clippy | |
316 | ``` | |
317 | ||
318 | You can then sync with the remote names from above, e.g.: | |
319 | ||
320 | ```bash | |
321 | $ git subtree push -P src/tools/clippy clippy-local sync-from-rust | |
322 | ``` | |
323 | ||
324 | [gitgitgadget-pr]: https://github.com/gitgitgadget/git/pull/493 | |
325 | [subtree]: https://rustc-dev-guide.rust-lang.org/contributing.html#external-dependencies-subtree | |
326 | [`rust-lang/rust`]: https://github.com/rust-lang/rust | |
327 | ||
328 | ## Issue and PR triage | |
329 | ||
330 | Clippy is following the [Rust triage procedure][triage] for issues and pull | |
331 | requests. | |
332 | ||
333 | However, we are a smaller project with all contributors being volunteers | |
334 | currently. Between writing new lints, fixing issues, reviewing pull requests and | |
335 | responding to issues there may not always be enough time to stay on top of it | |
336 | all. | |
337 | ||
338 | Our highest priority is fixing [crashes][l-crash] and [bugs][l-bug], for example | |
339 | an ICE in a popular crate that many other crates depend on. We don't | |
340 | want Clippy to crash on your code and we want it to be as reliable as the | |
341 | suggestions from Rust compiler errors. | |
342 | ||
343 | We have prioritization labels and a sync-blocker label, which are described below. | |
344 | - [P-low][p-low]: Requires attention (fix/response/evaluation) by a team member but isn't urgent. | |
345 | - [P-medium][p-medium]: Should be addressed by a team member until the next sync. | |
346 | - [P-high][p-high]: Should be immediately addressed and will require an out-of-cycle sync or a backport. | |
136023e0 | 347 | - [L-sync-blocker][l-sync-blocker]: An issue that "blocks" a sync. |
f20569fa XL |
348 | Or rather: before the sync this should be addressed, |
349 | e.g. by removing a lint again, so it doesn't hit beta/stable. | |
350 | ||
351 | ## Bors and Homu | |
352 | ||
353 | We use a bot powered by [Homu][homu] to help automate testing and landing of pull | |
354 | requests in Clippy. The bot's username is @bors. | |
355 | ||
356 | You can find the Clippy bors queue [here][homu_queue]. | |
357 | ||
358 | If you have @bors permissions, you can find an overview of the available | |
359 | commands [here][homu_instructions]. | |
360 | ||
361 | [triage]: https://forge.rust-lang.org/release/triage-procedure.html | |
362 | [l-crash]: https://github.com/rust-lang/rust-clippy/labels/L-crash | |
363 | [l-bug]: https://github.com/rust-lang/rust-clippy/labels/L-bug | |
364 | [p-low]: https://github.com/rust-lang/rust-clippy/labels/P-low | |
365 | [p-medium]: https://github.com/rust-lang/rust-clippy/labels/P-medium | |
366 | [p-high]: https://github.com/rust-lang/rust-clippy/labels/P-high | |
367 | [l-sync-blocker]: https://github.com/rust-lang/rust-clippy/labels/L-sync-blocker | |
368 | [homu]: https://github.com/rust-lang/homu | |
369 | [homu_instructions]: https://bors.rust-lang.org/ | |
370 | [homu_queue]: https://bors.rust-lang.org/queue/clippy | |
371 | ||
372 | ## Contributions | |
373 | ||
374 | Contributions to Clippy should be made in the form of GitHub pull requests. Each pull request will | |
375 | be reviewed by a core contributor (someone with permission to land patches) and either landed in the | |
376 | main tree or given feedback for changes that would be required. | |
377 | ||
378 | All code in this repository is under the [Apache-2.0] or the [MIT] license. | |
379 | ||
380 | <!-- adapted from https://github.com/servo/servo/blob/master/CONTRIBUTING.md --> | |
381 | ||
382 | [Apache-2.0]: https://www.apache.org/licenses/LICENSE-2.0 | |
383 | [MIT]: https://opensource.org/licenses/MIT |