stuart crust

a constant work in progress

25 Nov 2018

I See You Are Writing Some Rust

under open sourcerustclippy

Would You Like Some Help With That?

In my last post I wrote a bit about code linting and code formatting, particularly in more modern programming languages like Rust and Go where such tools come first-class as part of the language’s toolchain. In addition to Rust’s rustfmt tool which formats Rust code according to style guidelines, Rust’s ecosystem also has a tool called clippy which is a much more opinionated tool “to catch common mistakes and improve your Rust code.”

Introducing clippy

 _____________________________________
/ I see you are writing some Rust.    \
\ Would you like some help with that? /
 -------------------------------------
 \
  \
    /  \
    |  |
    @  @
    |  |
    || |/
    || ||
    |\_/|
    \___/

clippy currently has 288 lints to help developers write better Rust code. The lints are broken down into various categories such as:

  • style: code that should be written in a more idiomatic way (e.g. if x.len() == 0 {...} could be re-written as if x.is_empty() {...})
  • correctness: code that it outright wrong of very useless (e.g. ensuring syntax when creating regexes)
  • complexity: code that is more complex than necessary (e.g. if x == true {...} could be re-written as if x {...})

as well as many others.

I’ve been interested in contributing to a Rust tooling project since I did my initial Rust project overview as part of Hacktoberfest 2018. Good tooling is a force multiplier in software development, and improving tooling - especially tooling that is “blessed” and supported by the core language team - can reach so many more people than small purpose-built tools set up for individual projects.

During Hacktoberfest, I ran across rust-lang/rust-clippy#1673 but because of the time constraints on Hacktoberfest contributions along with my other coursework I didn’t have time to claim the issue.

The full issue is as follows:

It is not idiomatic in Rust to have setters and getters. Make the field public instead. If the type only has a get_foo method but not a set_foo method, suggest naming it foo instead.

This seemed like a relatively simple lint to implement which would hopefully introduce me to a number of features clippy uses when analyzing Rust code and inspecting the representation that the rustc compiler sees before it generates build artifacts. Once Hacktoberfest was over and I cleared some work off my plate, I went back and asked if the lint was still up for grabs before I invested the time to attempt an implementation. Manish Goregaokar of the Rust dev tools team got back to me almost immediately:

Actually, I’m not sure if this really is valid rust style – setters and getters may be added to future proof an API, for example.

Manish raised the excellent point that getters and setters are in fact valid Rust style and I agreed, so I thought I was going to have to find another issue to work on and moved to close the issue.

I was worried that I would run into a similar situation with other issues I was interested in working on, so I reached out to Manish directly on the wg-clippy Discord channel and asked about another issue I was interested in working on:

@manishearth i was interested in picking up https://github.com/rust-lang/rust-clippy/issues/1673 but i agree with your comment that it may not be a desirable lint to have

i’m looking at https://github.com/rust-lang/rust-clippy/issues/2144 now, or if there’s another good first issue that’s up for grabs i’d definitely be interested in taking a look!

I got a response pretty quickly:

that seems fine!

However, activity on the original issue I was interested in had clearly caught some attention, and I got into a discussion with user hcpl about other use cases for the lint, specifically:

If the type only has a get_foo method but not a set_foo method, suggest naming it foo instead.

It turned out that there was already some precedence for this style in the Rust standard library, and the Rust API Guidelines has an entire section about Rust conventions for getter names. Except for the cases of:

  • get
  • get_mut
  • get_unchecked
  • get_unchecked_mut
  • get_ref

which have some special meanings in Rust related to data mutability, references, or unsafe code, the get_ prefix is not generally used in Rust. Searching for these exceptions also turned up an unstable feature relating to TypeId to support reflection that does include the get_ prefix even though it’s not supposed to, which goes to show that implementing this lint could be very valuable to help maintain style even in core Rust projects and the compiler.

After some good back-and-forth discussion with hcpl and Philipp Krones, I summarized the proposed refinements to the filed issue:

To summarize, if the type has a get_foo method we should suggest naming it foo instead to follow the API Guidelines for Rust getter name conventions except for cases of:

  • get
  • get_mut
  • get_unchecked
  • get_unchecked_mut
  • get_ref

This should be enough to get me started on a style lint for this convention, I should have some time over the next couple days to start digging into this.

With better clarity on what the lint should implement as well as some known exceptions, I was in a position to start getting the project set up and go through all the steps of onboarding onto a new project.

Working on clippy

To start implementing the lint, I had to go through all the usual steps of forking and cloning clippy and making sure I could build the project locally before I could start digging into code. After cloning the project, I went ahead and tried to build clippy locally:

$ cargo --version
cargo 1.32.0-nightly (1fa308820 2018-10-31)

$ cargo build

[snip]

error[E0050]: method `check_pat` has 4 parameters but the declaration in trait `rustc::lint::EarlyLintPass::check_pat` has 3
   --> clippy_lints/src/misc_early.rs:244:66
    |
244 |     fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat, _: &mut bool) {
    |                                                                  ^^^^^^^^^ expected 3 parameters, found 4
    |
    = note: `check_pat` from trait: `fn(&mut Self, &rustc::lint::EarlyContext<'_>, &syntax::ast::Pat)`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0050`.
error: Could not compile `clippy_lints`.
warning: build failed, waiting for other jobs to finish...
error: build failed

Oops, that’s not good. I knew clippy relied heavily on features from the nightly release channel, and, as the name implies, the nightly channel is released every night with new changes and improvements. clippy must be making use of some new feature here and my nightly Rust is out of date. I updated Rust with rustup and then tried again to build clippy locally:

$ rustup update

[snip]

nightly-x86_64-apple-darwin updated - rustc 1.32.0-nightly (5aff30734 2018-11-19)

$ cargo --version
cargo 1.32.0-nightly (b3d0b2e54 2018-11-15)

$ cargo build

[snip]

Finished dev [unoptimized + debuginfo] target(s) in 2m 03s

Now that I knew I really had to watch the versions and make sure everything was up to date, I was ready to start thinking about implementing the lint.

A Few Days Later…

A challenge that I’ve been running into working on clippy is that because it relies so heavily on nightly compiler features, and both clippy and nightly are moving targets, my local clippy checkout can very quickly get out of date not only from upstream but also from the nightly release channel.

For example, I updated everything recently and got:

$ cargo build

   Compiling clippy_lints v0.0.212 (/Users/azure/devel/rust-clippy/clippy_lints)
error[E0615]: attempted to take value of method `abi` on type `rustc_target::abi::Align`
    --> clippy_lints/src/types.rs:1067:93
     |
1067 |                 if let Some(from_align) = cx.layout_of(from_ptr_ty.ty).ok().map(|a| a.align.abi);
     |                                                                                             ^^^
     |
     = help: maybe a `()` to call it is missing?

error[E0615]: attempted to take value of method `abi` on type `rustc_target::abi::Align`
    --> clippy_lints/src/types.rs:1068:89
     |
1068 |                 if let Some(to_align) = cx.layout_of(to_ptr_ty.ty).ok().map(|a| a.align.abi);
     |                                                                                         ^^^
     |
     = help: maybe a `()` to call it is missing?

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0615`.
error: Could not compile `clippy_lints`.

which is the change introduced in rust-lang/rust-clippy#3452.

For whatever reason, even after updating Rust and using the same tool versions as in this passing test for the above pull request the project does not build locally. Luckily reverting back to 61501b2810d887367d360025398dd9280c4bcd8b lets me compile the project so I can continue working without getting too out of date with upstream, but there’s a lot of churn and things often break for unexplained reasons.

I reached out to Matthias Krüger, the original author of #3453, on the wg-rust Discord channel to find out what was going on. It turns out that sometimes even the nightly release channel isn’t bleeding-edge enough to work on clippy lints and a tool called rustup-toolchain-install-master is necessary to install compiler artifacts directly from Rust’s continuous integration pipeline that haven’t even been published to the nightly channel yet. This information is also documented in clippy’s CONTRIBUTING.MD file, but it’s located at almost the bottom of the document which is why I hadn’t run across the information earlier. It is very true that it pays to read the documentation, and in many other projects asking a question about why my local build was failing in this way would receive comments to “RTFM”. However, my experiences in the Rust community have been nothing but positive, everyone I have interacted with has been very helpful, and even “big names” in the community are accessible and directly engaged in projects and contributor mentorship.

To Be Continued…

This week was all about finding my footing and getting my local environment set up to actually do development work on clippy. In the coming weeks I’ll tackle actually implementing the lint now that the requirements and goals have been fleshed out, and I hope to have something up for code review soon to get community feedback on improving the lint and catching anything I’ve missed.

First up: reading llogiq’s blogpost on writing clippy lints. Then I’ll create a starting point for my lint with clippy’s internal author lint as well as reading some existing lints to get a general idea of lint structure.