Clippy vs. Rust
(This blog post was written with some Rust 1.4.0-nightly)
Recently, we’ve begun checking just about all code with clippy, both to see if we can improve something (we could) and to see if we find problems in clippy (we did). Manish started our bout with Servo, and found a few places we could improve. I started with compiletest-rs and produced a pull request, then Manish reminded me that the code comes from rust itself, and we should apply the changes there first, to avoid having the code drift off in different directions. This brought me upon the quest to run clippy on the Rust distribution.
But how do I run clippy on Rust? It doesn’t use Cargo, for one, and the build is not quite simple. To make matters more interesting, as Rust has no stable ABI, plugins compiled with one version of Rust will not work when linked to a different version. To solve this issue, I needed multirust.
Enter Multirust
Multirust is a set of shell scripts that wrap around the rust, cargo, etc. executables that allows to switch rust versions with one short command. Before one can install it, one needs to uninstall Rust. This can be done using /usr/local/lib/rustlib/uninstall.sh, which comes with every rust installation. Installing multirust is as simple as:
$ curl -sf https://raw.githubusercontent.com/brson/multirust/master/blastoff.sh | sh
This will download and install current stable. Because I work on clippy, which needs nightly, I also tell multirust to download and install it and use it as default – this will also compile faster compared to stable:
$ multirust default nightly
Multirust downloads a nightly Rust toolchain and will use it from now on until I tell it otherwise. Neat. I already have cloned the rust git repository, so it’s time to build it:
$ cd /path/to/rust # paths changed to protect the innocent ;-)
$ configure --enable-compiler-docs
$ make -j 4 rustc-stage1-rpass-full
As you can see, I habitually build the compiler docs. When working with internals, they are indispensible, and since I build the stuff anyway, there’s no harm in having them here, too. The make incantation builds the stage1 compiler and takes some time, during which I can write this blog.
Set the stage
Rustc is build in multiple “stages”. Stage zero is a downloaded known good version of rustc that is used to build stage one (that is a complete rustc + standard library build on the local machine). Stage one is used to build stage two, and stage two builds stage three, which is the final stage. It can be checked that the latest two stages are the same. This cannot really save rust against possible malicious attackers intent on adding a backdoor to the compiler, but it at least ensures that rustc and the standard library enjoy all the optimizations of the latest and greatest Rust.
However, we don’t need all this staging here, because thanks to multirust, we already have a current nightly. What we need however, is a locally built version to compile clippy with and this is our stage1 we just built. If you look in your rust directory, you will find a directory that is named after your target triple, something like i286-unknown-linux-toaster or ppc969-unknown-darwin-alienmothership (Jeff Goldblum wouldn’t have stood a chance had they used Rust). Within this directory is another stage1 directory (among others that we don’t look at). Therein is the Rust compiler we want to build clippy with.
Again, multirust to the rescue. We go to our clippy directory and tell multirust to build with the stage1 compiler. Let’s call our new toolchain “stage1”. We also need to link a current cargo into the toolchain, because clippy doesn’t build without it.
$ cd /path/to/rust-clippy
$ multirust update stage1 --link-local /path/to/rust/i286-unknown-linux-toaster/stage1
$ ln -s ~/.multirust/toolchains/nightly/bin/cargo ~/.multirust/toolchains/stage1/bin/
(Of course you need to change /path/to/rust to your actual rust repo on your system, /path/to/rust-clippy to your clippy repo and i286-unknown-linux-toaster to your target triple – this is easy to find with ls, it’s usually the longest named directory) Now we can build clippy. We want the release version, because that will be a bit faster when checking Rust.
$ multirust run stage1 cargo build --release
Staging a Rebellion
Ok, I’m overly dramatic here, but this is really cool – I’m actually using clippy to improve Rust itself! We can now use our stage1 compiler together with our stage1-compiled clippy to compile and check the stage2 compiler. Before I do this, I have to comment out the line RUST_LIB_FLAGS_ST2 += -D warnings to stop Rust from throwing errors because of clippy warnings. For the actual build, the Rust Makefile allows the following command:
VERBOSE=1 RUSTFLAGS_STAGE2='-L ~/projects/rust-clippy/target/release/ -Z extra-plugins=clippy -A shadow_unrelated -A inline_always' RUST_BACKTRACE=1 make rustc-stage2
This spits out a lot of diagnostics, many of which can be deemed false positives. As you can see I disable the shadow_unrelated and inline_always lint, because the Rust codebase uses a lot of shadowing and #[inline(always)], and we’re probably not going to change this.
Manish also created a small script to automatically apply lifetime-related lints, since a lot of Rustc code was written before lifetime elision. If you try to replicate this with a current rust, you won’t find them, however; we’ve fixed them during a number of pull requests. I was also able to remove a few unneeded closures from the iterator libraries, so that the respective code will at worst compile faster (because there’s less of it) and at best run faster (should LLVM fail to inline the closure, which it may if the code is complex).
Bonus: Gankra asked nicely, so here’s a table with the results:
| # matches | LInt name | Comment |
|---|---|---|
| 167 | str_to_string | very common elsewhere |
| 102 | needless_lifetimes | probably old code |
| 71 | needless_return | quite common elsewhere |
| 32 | approx_constant | false positives |
| 23 | mut_mut | may be false positives |
| 19 | float_cmp | may be false positives |
| 16 | unit_cmp | auto-derived PartialEq |
| 13 | type_complexity | needs some fine-tuning |
| 10 | should_implement_trait | no longer fixable |
| 10 | match_ref_pats | matter of taste |
| 9 | needless_range_loop | some false positives |
| 9 | eq_op | may be false positives |
| 8 | len_zero | false positive |
| 7 | bad_bit_mask | false positive, fixed |
| 6 | wrong_self_convention | no longer fixable |
| 6 | len_without_is_empty | false positive |
| 6 | identity_op | probably follows spec |
| 5 | precedence | readability |
| 4 | single_match | matter of taste |
| 4 | redundant_closure | yay for eta-reduction! |
| 3 | linkedlist | may be false positive |
| 3 | let_and_return | readability |
| 3 | collapsible_if | matter of taste |
| 2 | while_let_loop | probably old code |
| 2 | string_to_string | who’d have thought? |
I’ll try to periodically run clippy on Rust, improving code wherever it finds something. This is much better than manually inspecting everything, and gives us a lot of bang for the buck, even if it’s so complicated to run clippy on a codebase. Also if recent history is a guide, clippy will become more powerful over time, detecting more things with hopefully less false positives. If your project uses cargo, you can simply compile clippy with cargo build --release and point it at your code using cargo rustc -- -L /path/to/clippy/target/release -Z extra-plugins=clippy.
What is your success story with clippy? Discuss on reddit or rust-users