Llogiq on stuff

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: Gankro 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