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