03 Jan 2019

Lints, Syntax Parsing, and You

In my previous post I ran into a number of issues and confusion around clippy’s #[clippy::author] annotation and autogenerated code. Instead of continuing with clippy’s documentation, I’m going to jump over to llogiq’s blogpost on writing clippy lints and see what I can learn about lint implementation and the necessary datatypes.

Because clippy is a Rust toolchain component, and the clippy version on https://crates.io is no longer maintained, documentation tools like docs.rs are unavailable for browsing clippy’s types. Fortunately, Rust has a very strong offline documentation story, and generating documentation for a project is as simple as cargo doc --open. This can be a little tricky to get set up when you’re halfway through implementing a feature and looking for information in the documentation though, because the project has to successfully compile as part of generating the offline documentation.

Since clippy uses Rust compiler types to implement lints, we also need to have access to the documentation for rustc, Rust’s compiler, on the nightly toolchain; this can be accessed at https://doc.rust-lang.org/nightly/nightly-rustc/rustc/index.html. Unfortunately this set of compiler documentation is not available offline which seems like a gap in Rust’s documentation story.

Steps to Writing a Clippy Lint

I’ve broken the process I went through to implement this lint into steps to more clearly separate the different parts of a lint, the tools and processes involved at each stage, and problems and errors I encountered along the way as well as solutions I found where applicable.

For quick reference, here is a list of the steps involved:

Step One: Write an Example

The first step in writing a lint, as with most code, is to write the piece that uses the code you’re going to write. If we were writing regular code this might take the form of one or more test cases, but because we’re writing a lint we want to instead write an example of the code we’re trying to lint against. I’ve already done this earlier when I tried to use the #[clippy::author] directive, so I’m going to reuse the same example while using llogiq’s blogpost as a guide for my own lint.

Here’s the small code example we want to lint against:

pub struct MyStruct {
    id: usize
}

impl MyStruct {
    pub fn get_id(&self) -> usize {
        self.id
    }
}

fn main() {
   let s = MyStruct { id: 42 };
   s.get_id();
}

Step Two: Declare the Lint

After writing the example to lint against, we need to actually define the lint; this is done using the declare_clippy_lint! macro from the clippy_lints crate. I tried compiling clippy after declaring the lint and ran into the following error:

cargo check
   Compiling clippy_lints v0.0.212
error: cannot find macro `declare_tool_lint!` in this scope
  --> clippy_lints/src/lib.rs:53:9
   |
53 |           declare_tool_lint! { pub clippy::$name, Warn, $description, report_in_external_macro: true }
   |           ^^^^^^^^^^^^^^^^^
   |
  ::: clippy_lints/src/getter_prefix.rs:3:1
   |
3  | / declare_clippy_lint! {
4  | |     pub GETTER_PREFIX,
5  | |     style,
6  | |     "prefixing a getter with `get_`, which does not follow convention"
7  | | }
   | |_- in this macro invocation

error: aborting due to previous error

error: Could not compile `clippy_lints`.
warning: build failed, waiting for other jobs to finish...
error: cannot find macro `declare_tool_lint!` in this scope
  --> clippy_lints/src/lib.rs:53:9
   |
53 |           declare_tool_lint! { pub clippy::$name, Warn, $description, report_in_external_macro: true }
   |           ^^^^^^^^^^^^^^^^^
   |
  ::: clippy_lints/src/getter_prefix.rs:3:1
   |
3  | / declare_clippy_lint! {
4  | |     pub GETTER_PREFIX,
5  | |     style,
6  | |     "prefixing a getter with `get_`, which does not follow convention"
7  | | }
   | |_- in this macro invocation

error: aborting due to previous error

error: Could not compile `clippy_lints`.

To learn more, run the command again with --verbose.

cargo can’t find the declare_tool_lint! macro in the current scope, which is not an error I was expecting to see because I didn’t think I was using that macro in my lint definition. It turns out that the definition of the declare_clippy_lint! macro uses the declare_tool_lint! macro, so this second macro must be brought into scope before the first can be used. I’m not very familiar with macros or metaprogramming in Rust, but after looking at a number of other lints it seems like all of them use the declare_tool_lint! macro so I will too.

Here is the lint code so far, including the first line which imports the dependent lint:

use crate::rustc::declare_tool_lint;

declare_clippy_lint! {
    pub GETTER_PREFIX,
    style,
    "prefixing a getter with `get_`, which does not follow convention"
}

As part of declaring the lint, we also want to document the lint by describing what it does and include some examples of code that will not pass the lint and alternatives that will pass the lint rules. By following a standard convention, documentation written for each lint can be extracted and added to the lint list which provides a filterable list of all of clippy’s lints.

Rust provides a lot of tools for documenting Rust code, the cornerstone of which is documentation comments. Documentation comments support Markdown syntax for formatting and are used to generate browsable HTML pages, without requiring the author to maintain separate written documentation. Documentation comments also drive other tools such as docs.rs.

Here is a (very rough) initial documentation comment for the lint, including examples of bad and good code samples:

/// **What it does:** Checks for the `get_` prefix on getters.
///
/// **Why is this bad?** The Rust API Guidelines section on naming
/// [specifies](https://rust-lang-nursery.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter)
/// that the `get_` prefix is not used for getters in Rust code unless
/// there is a single and obvious thing that could reasonably be gotten by
/// a getter.
///
/// **Known problems:** Exceptions not yet implemented.
///
/// **Example:**
///
/// ```rust
/// // Bad
/// impl B {
///     fn get_id(&self) -> usize {
///         ..
///     }
///}
///
/// // Good
/// impl G {
///     fn id(&self) -> usize {
///         ..
///     }
/// }
/// ```

Step Three: Register the Lint with Clippy

This is a largely automated process thanks to some clippy tooling as described in the How Clippy Works section of the contribution documentation. Simply (re-)run util/dev update_lints as necessary, which autogenerates the majority of clippy_lints/src/lib.rs to declare lints and lint groups.

Alternatively, this can be done manually by adding the lint to the correct lint group inside the register_plugins function.

Regardless of how the lint is added to the lint groups, the lint must be registered with the lint registry by introducing the lint as either an early or late lint pass. In the case of this lint, which will be implemented as an early lint pass, the following line adds it to the lint registry as declared inside the register_plugins function:

reg.register_early_lint_pass(box naming::GetterPrefix);

Step Four: Start Implementing Lint Passes

Now that the lint is defined and registered with clippy, we can start implementing the logic to lint against the example we implemented in Step One.

For most lints there are two traits that need to be implemented so the lint can be registered with rustc_plugin::registry::Registry: the LintPass trait and one of either the EarlyLintPass or the LateLintPass trait. LintPass is apparently necessary to provide descriptions of the possible lints the lint can emit, but in all of the lints I have looked at, the LintPass implementation always takes the form:

impl LintPass for Pass {
    fn get_lints(&self) -> LintArray {
        lint_array!(LINT_NAME)
    }
}

so I’m surprised there hasn’t been a #[derive] annotation written for it or some kind of macro that would reduce the repetition.

As for EarlyLintPass or LateLintPass, the choice of which trait to implement comes down to the kind of information a given lint needs about the code it is linting: EarlyLintPass methods only provide abstract syntax tree (AST) information, whereas LateLintPass methods are, as the name implies, executed later in the compilation process and contain type information. Since this lint is only interested in function names and checking them against known patterns, I’ve decided to implement the EarlyLintPass trait. There seems to be a lot of overlap in method signatures between the two types of lint pass, so if I need to switch to LateLintPass to get access to additional type information it should not be too difficult to transition over.

Step Five: Inspect and Interpret the AST

Most everything up until now has been boilerplate for getting the lint set up and correctly registered with clippy’s infrastructure. Now that we have everything set up, we need to figure out how to tell the lint to match against the undesired function names we’ve already written in our test. To do this, we need to first understand how the rustc compiler sees the code we have written, which for rustc is represented as an abstract syntax tree (AST), and then parse that tree to match against undesired nodes within the tree that correspond to the Rust source code. To retrieve this representation, we can use rustc to generate the AST and then inspect it manually to get a sense of the program structure; rustc has the -Z option for controlling various debug options (see rustc --help), and one of these options tells rustc to print the AST as JSON and halt compilation: -Z ast-json.

The full command to print the AST as JSON, specifying the individual test for this lint whose code we want to inspect, is:

$ rustc tests/ui/naming.rs -L target/debug -Z ast-json

The initial output of this command is utterly unreadable because rustc prints it as a single line of JSON, so I’m going to use jq to pretty-print the AST so it’s easier to read:

{
    "module": {
        "inner": {
            "lo": 426,
            "hi": 611
        },
        "items": [
            [snip 1604 lines]
        ],
        "inline": true
    },
    "attrs": [],
    "span": {
        "lo": 426,
        "hi": 611
    }
}

In all, the AST for my 12-line test file ended up expanding to a 1627-line (pretty-printed) JSON file. The items key and its 1604 lines are where the actually interesting AST information is in the data structure, containing information about every identifier, implementation, attribute, etc in the code. Within the items array, the name of the function in the test, get_id, appears 5 times in various contexts such as "variant": "Impl" and "variant": "MethodCall". The following is what I believe to be the beginning of the AST for the get_id function:

{
    "id": 20,
    "ident": "get_id",
    "vis": {
        "node": "Public",
        "span": {
        "lo": 485,
        "hi": 488
        }
    },
    "defaultness": "Final",
    "attrs": [],
    "generics": {
        "params": [],
        "where_clause": {
            "id": 21,
            "predicates": [],
            "span": {
                "lo": 0,
                "hi": 0
            }
        },
        "span": {
            "lo": 0,
            "hi": 0
        }
    },
    ...
}

Specifically, the following are the nodes that describe the pub fn get_id text literals of the function signature:

"tokens": [
    {
        "variant": "Token",
        "fields": [
            {
                "lo": 485,
                "hi": 488
            },
            {
                "variant": "Ident",
                "fields": [
                    "pub",
                    false
                ]
            }
        ]
    },
    {
        "variant": "Token",
        "fields": [
            {
                "lo": 489,
                "hi": 491
            },
            {
                "variant": "Ident",
                "fields": [
                    "fn",
                    false
                ]
            }
        ]
    },
    {
        "variant": "Token",
        "fields": [
            {
                "lo": 492,
                "hi": 498
            },
            {
                "variant": "Ident",
                "fields": [
                    "get_id",
                    false
                ]
            }
        ]
    },
]

Now that the relevant sections of the AST have been identified, we can use this information to get a better understanding of how rustc (and thus tools like clippy) understand the written Rust code in the source file. This information can be useful at many different stages of lint implementation, particularly if we need to debug out lint or we are not matching the expected nodes in our lint code.

Step Six: Implement EarlyLintPass

The EarlyLintPass trait requires the implementation of one of its provided methods to perform the lint work. Looking at the list of provided methods, however, I’m not really sure where to start or which method to implement. A lot of rustc’s internals seem to be chronically under-documented, which makes it very difficult to understand how different internals are used (what’s the difference between check_item and check_item_post, for example?) or the subtle (or not so subtle) differences between various types or methods.

My strategy in learning about the rustc internals so far has been a combination of looking at existing lints and trying to identify what the internals do based on the lint’s goals and existing code, as well as inspecting not the EarlyLintPass methods themselves but rather the associated types such as syntax::ast::Item or syntax::ast::Local which are generally better documented than the methods that use them. These associated types also match very closely if not exactly to the AST structure produced by rustc, so by identifying the structure of the information in the AST as above, it is easier to choose the correct function to implement based on its associated type which contains the same information as the already-identified AST subtree(s).

Based on my understanding, it looks like the method that I want to implement is check_item which will yield syntax::ast::Item instances whose syntax::ast::ItemKind we can then match on for function declarations.

My initial implementation of the EarlyLintPass trait based on the above yielded the following:

impl EarlyLintPass for GetterPrefix {
    fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
        if let ast::ItemKind::Fn(..) = item.node {
            let name = item.ident.name;
            if name.as_str().starts_with("get_") {
                span_lint(cx, GETTER_PREFIX, item.span, "prefixing a getter with `get_` does not follow naming conventions");
            }
        }
    }
}

This implementation checks each Item in the AST and looks for nodes identified as ItemKind::Fn; that is, functions declared in the source. For matching nodes, the name is extracted from the node identifier and is checked to see if it starts with the get_ prefix. The span_lint function comes from clippy’s lint utils and is what actually contextualizes lint warnings and errors around the offending code and outputs the lint message.

However, it turns out that if let ast::ItemKind::Fn(..) only matches the main function name of the test case, rather than my expectation that it would match all functions declared within the file.

Instead, since we are interested in functions implemented for a type, we can use the check_impl_item method which will yield syntax::ast::ImplItem instances whose syntax::ast::ImplItemKind we can then match on for methods like so:

impl EarlyLintPass for GetterPrefix {
    fn check_impl_item(&mut self, cx: &EarlyContext<'_>, implitem: &ast::ImplItem) {
        if let ast::ImplItemKind::Method(..) = implitem.node {
            let name = implitem.ident.name;
            if name.as_str().starts_with("get_") {
                span_lint(
                    cx,
                    GETTER_PREFIX,
                    implitem.span,
                    "prefixing a getter with `get_` does not follow naming conventions"
                );
            }
        }
    }
}

The above implementation works very similarly to the initial implementation, but instead of checking Items looking for ItemKind::Fn, it instead looks at ImplItems in the AST and matches ImplItemKind::Method nodes (since implementation nodes also include const declarations, types and type aliases, traits, and macros in addition to methods).

To test an individual lint without the full clippy test harness (or to see println!s or other debugging statements more clearly), we can use the following clippy-driver incantation and specify a single UI test file, tests/ui/naming.rs:

$ CLIPPY_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug tests/ui/naming.rs

This lint pass implementation results in the following (successful) lint warning:

warning: prefixing a getter with `get_` does not follow naming conventions
  --> tests/ui/naming.rs:15:5
   |
15 | /     pub fn get_id(&self) -> usize {
16 | |         self.id
17 | |     }
   | |_____^
   |
   = note: #[warn(clippy::getter_prefix)] on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#getter_prefix

Step Seven: Generate the .stderr File for the New Lint

To test its behaviour, clippy uses UI tests to check that the output of the compiler is exactly as expected. The .stderr file clippy will use to check the newly implemented lint is automatically generated whenever the tests are run using cargo test, but rather than running all of the tests at this point we can run just the test for the individual lint by specifying TESTNAME=ui/naming where ui/naming is the UI test to run:

$ TESTNAME=ui/naming cargo test --test compile-test

To update the .stderr (and .stdout, if applicable) files in tests/ui/, we use the provided update script (the correct incantation for an individual lint can be found in the output of cargo test with a specified TESTNAME as we did above):

$ tests/ui/update-references.sh 'target/debug/test_build_base' 'naming.rs'

This will create the tests/ui/naming.stderr file for the lint.

Step Eight: Iterate

Now that the bare bones of the lint is implemented and it has stderr output as expected, the lint can be iterated on to add more functionality and identify possible false positives that need to be mitigated.

In the case of this lint, I added test cases that cover the exceptions to the naming rule and then introduced an additional check in the lint to see if the matched function name appeared in this exceptions list:

const ALLOWED_METHOD_NAMES: [&'static str; 5] = [
    "get",
    "get_mut",
    "get_unchecked",
    "get_unchecked_mut",
    "get_ref"
];

impl EarlyLintPass for GetterPrefix {
    fn check_impl_item(&mut self, cx: &EarlyContext<'_>, implitem: &ast::ImplItem) {
        if let ast::ImplItemKind::Method(..) = implitem.node {
            let name = implitem.ident.name.as_str().get();
            if name.starts_with("get_") && !ALLOWED_METHOD_NAMES.contains(&name) {
                span_lint(
                    cx,
                    GETTER_PREFIX,
                    implitem.span,
                    "prefixing a getter with `get_` does not follow naming conventions",
                );
            }
        }
    }
}

Step Nine: Run the Full Test Suite

Up until now I have only been running tests for the lint I have been working on to speed up the feedback cycle. Before moving on, I want to run the complete test suite to make sure that the tests still pass with my additions, and if any existing lints need to be changed to conform to the new lint.

In doing so I discovered a get_unit function defined in the unused_unit lint, but since this definition was localized to one lint file I was able to change the function name to pass the getter prefix lint and still maintain the lint’s original functionality. Changing this function name also meant that the tests/ui/unused_unit.stderr file was out of date, which was updated using the provided tests/ui/update-all-references.sh script.

Step Ten: Linting Clippy with Local Changes

The last step before submitting a pull request to rust-lang/rust-clippy is to make sure that all lints that have already been defined pass clippy (that is, there are no suggestions reported by clippy for its own codebase). Running clippy locally and addressing any issues found ahead of submitting a pull request will cut down on the feedback cycle and speed up the pull request review process. The recommended way to do this is by building clippy and then running it with all lint groups (including internal and pedantic) turned on:

$ cargo build
$ `pwd`/target/debug/cargo-clippy clippy --all-targets --all-features -- -D clippy::all -D clippy::internal -D clippy::pedantic

I was not able to run this command successfully, as it resulted in what appear to be dynamic linker errors on my machine:

$ clippy/target/debug/cargo-clippy --all-targets --all-features -- -D clippy::all -D clippy::internal -D clippy::pedantic
error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `clippy/target/debug/clippy-driver rustc - --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (signal: 6, SIGABRT: process abort signal)
--- stderr
dyld: Library not loaded: @rpath/librustc_driver-b630426988dbbdb0.dylib
  Referenced from: clippy/target/debug/clippy-driver
  Reason: image not found

However, even without running clippy locally, in the case of this getter prefix lint clippy would not pass local clippy. It turns out that there a number of rustc methods that do not follow the API naming convention, and I will need to consult with other clippy developers to find out if we will need to add more exceptions to the lint than we had originally identified. The one method that is particularly problematic is LintPass::get_lints because it appears in every lint that has already been defined in clippy.

Submitting the Lint

Here’s the implemented lint in its entirety:

The UI test file tests/ui/naming.rs:

// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

pub struct MyStruct {
    id: usize
}

impl MyStruct {
    pub fn get_id(&self) -> usize {
        self.id
    }

    pub fn get(&self) -> usize {
        self.id
    }

    pub fn get_mut(&mut self) -> usize {
        self.id
    }

    pub fn get_unchecked(&self) -> usize {
        self.id
    }

    pub fn get_unchecked_mut(&mut self) -> usize {
        self.id
    }

    pub fn get_ref(&self) -> usize {
        self.id
    }
}

fn main() {
   let mut s = MyStruct { id: 42 };
   s.get_id();
   s.get();
   s.get_mut();
   s.get_unchecked();
   s.get_unchecked_mut();
   s.get_ref();
}

The lint implementation clippy_lints/src/naming.rs:

// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use crate::rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass};
use crate::rustc::{declare_tool_lint, lint_array};
use crate::syntax::ast;
use crate::utils::span_lint;

/// **What it does:** Checks for the `get_` prefix on getters.
///
/// **Why is this bad?** The Rust API Guidelines section on naming
/// [specifies](https://rust-lang-nursery.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter)
/// that the `get_` prefix is not used for getters in Rust code unless
/// there is a single and obvious thing that could reasonably be gotten by
/// a getter.
///
/// The exceptions to this naming convention are as follows:
/// - `get` (such as in
///   [`std::cell::Cell::get`](https://doc.rust-lang.org/std/cell/struct.Cell.html#method.get))
/// - `get_mut`
/// - `get_unchecked`
/// - `get_unchecked_mut`
/// - `get_ref`
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust
/// // Bad
/// impl B {
///     fn get_id(&self) -> usize {
///         ..
///     }
/// }
///
/// // Good
/// impl G {
///     fn id(&self) -> usize {
///         ..
///     }
/// }
///
/// // Also allowed
/// impl A {
///     fn get(&self) -> usize {
///         ..
///     }
/// }
/// ```
declare_clippy_lint! {
    pub GETTER_PREFIX,
    style,
    "prefixing a getter with `get_`, which does not follow convention"
}

#[derive(Copy, Clone)]
pub struct GetterPrefix;

#[rustfmt::skip]
const ALLOWED_METHOD_NAMES: [&'static str; 5] = [
    "get",
    "get_mut",
    "get_unchecked",
    "get_unchecked_mut",
    "get_ref"
];

impl LintPass for GetterPrefix {
    fn get_lints(&self) -> LintArray {
        lint_array!(GETTER_PREFIX)
    }
}

impl EarlyLintPass for GetterPrefix {
    fn check_impl_item(&mut self, cx: &EarlyContext<'_>, implitem: &ast::ImplItem) {
        if let ast::ImplItemKind::Method(..) = implitem.node {
            let name = implitem.ident.name.as_str().get();
            if name.starts_with("get_") && !ALLOWED_METHOD_NAMES.contains(&name) {
                span_lint(
                    cx,
                    GETTER_PREFIX,
                    implitem.span,
                    "prefixing a getter with `get_` does not follow naming conventions",
                );
            }
        }
    }
}

Once the basic lint functionality was completed, I wanted to open a pull request against rust-lang/rust-clippy as soon as possible. This is my first time writing a lint for clippy, so I wanted to start getting feedback from other clippy developers and incorporate their suggestions to improve the lint code as well as my understanding of how clippy works. Once I got the lint working I also realized there were a number of decisions to be made where I did not have enough Rust ecosystem or clippy-specific knowledge to answer, so opening a pull request would be a great way to get those questions answered within the context of the code I had written for the lint.

My pull request for this getter prefix name lint can be found at rust-lang/rust-clippy#3616 which details the current state of the lint implementation and its progress getting merged into clippy.

There is still some work to be done before I feel confident this lint can be merged. First, I need to go through clippy more thoroughly and identify existing lints that need to be changed to pass the newly introduced lint; resolving the error I identified in Step Ten should get me well on the way to achieving this.

I would also like to implement some machine-applicable renaming suggestions that would remove the get_ prefix from method names so that functions that fail to meet this Rust API naming convention can be automatically fixed by rustfix. I’m looking into the mechanisms clippy provides to make these kinds of suggestions, and span_lint_and_sugg looks like a likely candidate for teaching rustfix about these renaming rules.

Lastly I will of course need to implement the feedback on the pull request from other clippy developers so the lint will be accepted for inclusion in clippy.

Conclusion

I’m really glad I was able to write this lint for clippy. I had found the original issue, rust-lang/rust-clippy#1673, back in October 2018 and thought it was the perfect size for getting my feet wet writing a lint. Some parts of implementing this lint turned out to be more difficult than I had anticipated, most notably around the sparseness or complete lack of rustc internals documentation which was surprising given the Rust community’s focus on writing documentation and the generally high quality documentation available across the ecosystem, but a little trial and error and some println! debugging pointed me in the right direction in the end.

A special thanks to Manish Goregaokar, Philipp Krones, Matthias Krüger, and hcpl for their input, feedback, and help at various points throughout this process, and llogiq for their blog post on writing clippy lints which was very helpful in finding my way to a working lint implementation.

There is a particular quote by Charles H. Spurgeon that I tend to associate with new years and new beginnings: “Begin as you mean to go on, and go on as you began”. The way that I began 2019 was writing Rust code, and I fully intend to go on writing Rust code for the rest of 2019 and beyond. I think that’s a worth-while New Year’s resolution, don’t you?

06 Dec 2018

Yak Shaving in F♭

Following on from my introduction to clippy lints, this week I am beginning my journey of actually implementing a clippy lint.

As a refresher, I am implementing rust-lang/rust-clippy#1673:

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.

clippy’s Author Lint

In typical TDD fashion, I want to start with the test case, the code I want to lint against, so I can test my implementation and drive design. I’ve started off with the simple case of detecting the invalid style rather than worrying about whitelisting the exceptions identified.

pub struct MyStruct {
    id: u32
}

impl MyStruct {
    pub fn get_id(&self) -> u32 {
        self.id
    }
}

fn main() {
   let s = MyStruct { id: 42 };

   #[clippy::author]
   let id = s.get_id();
}

I’ve also added the #[clippy::author] annotation as suggested by clippy’s contributing documentation to generate a starting point for the lint.

Next, I have to run the test to produce a .stdout file with the code generated by the #[clippy::author] lint. The instructions say:

If the command was executed successfully, you can copy the code over to where you are implementing your lint.

Let’s try it out:

$ TESTNAME=ui/getter_prefix cargo test --test compile-test
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
     Running target/debug/deps/compile_test-f89d0316ceade355

running 1 test

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 29 filtered out


running 1 test
test [ui] ui/getter_prefix.rs ... FAILED

failures:

---- [ui] ui/getter_prefix.rs stdout ----
normalized stdout:
if_chain! {
    if let StmtKind::Decl(ref decl, _) = stmt.node
    if let DeclKind::Local(ref local) = decl.node;
    if let Some(ref init) = local.init
    if let ExprKind::MethodCall(ref method_name, ref generics, ref args) = init.node;
    // unimplemented: `ExprKind::MethodCall` is not further destructured at the moment
    if let PatKind::Binding(BindingAnnotation::Unannotated, _, name, None) = local.pat.node;
    if name.node.as_str() == "id";
    then {
        // report your lint here
    }
}


expected stdout:


diff of stdout:

+if_chain! {
+    if let StmtKind::Decl(ref decl, _) = stmt.node
+    if let DeclKind::Local(ref local) = decl.node;
+    if let Some(ref init) = local.init
+    if let ExprKind::MethodCall(ref method_name, ref generics, ref args) = init.node;
+    // unimplemented: `ExprKind::MethodCall` is not further destructured at the moment
+    if let PatKind::Binding(BindingAnnotation::Unannotated, _, name, None) = local.pat.node;
+    if name.node.as_str() == "id";
+    then {
+        // report your lint here
+    }
+}
+

The actual stdout differed from the expected stdout.
Actual stdout saved to /Users/scrust/devel/rust-clippy/target/debug/test_build_base/getter_prefix.stdout
To update references, run this command from build directory:
tests/ui/update-references.sh '/Users/scrust/devel/rust-clippy/target/debug/test_build_base' 'getter_prefix.rs'

error: 1 errors occurred comparing output.
status: exit code: 0
command: "target/debug/clippy-driver" "tests/ui/getter_prefix.rs" "-L" "/Users/scrust/devel/rust-clippy/target/debug/test_build_base" "--target=x86_64-apple-darwin" "-C" "prefer-dynamic" "-o" "/Users/scrust/devel/rust-clippy/target/debug/test_build_base/getter_prefix.stage-id" "-L" "target/debug" "-L" "target/debug/deps" "-Dwarnings" "-L" "/Users/scrust/devel/rust-clippy/target/debug/test_build_base/getter_prefix.stage-id.aux" "-A" "unused"
stdout:
------------------------------------------
if_chain! {
    if let StmtKind::Decl(ref decl, _) = stmt.node
    if let DeclKind::Local(ref local) = decl.node;
    if let Some(ref init) = local.init
    if let ExprKind::MethodCall(ref method_name, ref generics, ref args) = init.node;
    // unimplemented: `ExprKind::MethodCall` is not further destructured at the moment
    if let PatKind::Binding(BindingAnnotation::Unannotated, _, name, None) = local.pat.node;
    if name.node.as_str() == "id";
    then {
        // report your lint here
    }
}

------------------------------------------
stderr:
------------------------------------------

------------------------------------------

thread '[ui] ui/getter_prefix.rs' panicked at 'explicit panic', /Users/scrust/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.17/src/runtest.rs:2553:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    [ui] ui/getter_prefix.rs

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 225 filtered out

test compile_test ... FAILED

failures:

---- compile_test stdout ----
thread 'compile_test' panicked at 'Some tests failed', /Users/scrust/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.17/src/lib.rs:89:22


failures:
    compile_test

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test compile-test'

Wow that’s a lot out output. More importantly, I don’t actually know if it worked. I see

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 29 filtered out

but then I see

The actual stdout differed from the expected stdout.
Actual stdout saved to /Users/scrust/devel/rust-clippy/target/debug/test_build_base/getter_prefix.stdout

and

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 225 filtered out

test compile_test ... FAILED

I also don’t see any generated .stdout file, so I’m going to assume there’s something wrong with my test case.

If I remove the author lint tag, I get a different result:

$ TESTNAME=ui/getter_prefix cargo test --test compile-test
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s
     Running target/debug/deps/compile_test-f89d0316ceade355

running 1 test

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 29 filtered out


running 1 test
test [ui] ui/getter_prefix.rs ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 225 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out

test compile_test ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

This all seems to pass, but I still don’t see a generated .stdout file, and I’ve also lost any output the #[clippy:author] annotation would have given me.

I noticed a line from the earlier, possibly failed, test run:

To update references, run this command from build directory: tests/ui/update-references.sh ‘/Users/scrust/devel/rust-clippy/target/debug/test_build_base’ ‘getter_prefix.rs’

What does this do? What’s a “reference”? clippy’s documentation doesn’t seem to mention tests/ui/update-references.sh at all, though there is a mention of tests/ui/update-all-references.sh which seems to update all of the existing .stderr files that drive the UI tests.

It turns out that running tests/ui/update-references.sh is necessary to actually write the .stdout file. I wasn’t expecting this extra step because the way the instructions are phrased I though running the test would generate the .stdout file automatically. For the test I wrote, #[clippy::author] generated a .stdout file with the following code:

if_chain! {
    if let StmtKind::Decl(ref decl, _) = stmt.node
    if let DeclKind::Local(ref local) = decl.node;
    if let Some(ref init) = local.init
    if let ExprKind::MethodCall(ref method_name, ref generics, ref args) = init.node;
    // unimplemented: `ExprKind::MethodCall` is not further destructured at the moment
    if let PatKind::Binding(BindingAnnotation::Unannotated, _, name, None) = local.pat.node;
    if name.node.as_str() == "id";
    then {
        // report your lint here
    }
}

I’m not at all familiar with the if_chain! macro or any of these datatypes, so I definitely have some reading to do so I can understand what this snippet actually does. I do see if name.node.as_str() == "id"; which seems to match the id field on MyStruct which is about the only piece I understand without delving deeper.

Out of curiosity after reading a number of other lint tests, I decided to update main and added an assert statement:

fn main() {
   let s = MyStruct { id: 42 };

    #[clippy::author]
    let id = s.get_id();

    assert_eq!(id, 42);
}

and got yet another different result:

$ TESTNAME=ui/getter_prefix cargo test --test compile-test
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s
     Running target/debug/deps/compile_test-f89d0316ceade355

running 1 test

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 29 filtered out


running 1 test
test [ui] ui/getter_prefix.rs ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 225 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out

test compile_test ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

I’m not sure the #[clippy::author] annotation is going to help me too much in implementing this lint. Right now I don’t know enough about clippy or the datatypes it uses to make heads or tails of the generated code, and the results of my test are inconsistent depending on various combinations of assert_eq! and the author annotation. This definitely calls for more research, so until next week it looks like I’ll be getting to grips with some of rustc’s compiler internals.

25 Nov 2018

I See You Are Writing Some Rust

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.

18 Nov 2018

Opinionated Formatting

This week in class we talked about code linting and code formatting using ESLint and Prettier. These kinds of tools automate a lot of the otherwise labour-intensive and easy-to-miss nitpicks reviewers often leave on pull requests, freeing up time to review much more important elements such as design and code structure. Many tech companies - Google, AirBnB, Microsoft to name a few - have their own code style guides, much in the same way that writing organizations (news organizations and scientific publishing, for example) have documents that outline how to maintain consistency in publications across many hundreds of outlets and thousands of writers, and the idea of a style guide is not new. However, the idea of automating this style checking to help developers maintain a consistent style has gained a lot more traction in recent years thanks in part to style and formatting tools coming standard and enabled by default in many modern programming languages.

Formatting as a First-Class Tool

I was first exposed to this idea of a universal formatter as part of a language’s toolchain when I started experimenting with Go. Go’s gofmt enforces a consistent style not just within one project, but across the entire ecosystem of Go code, making even foreign codebases more accessible because of their consistent style. This consistency helps to reduce visual noise, making it easier to focus on what the code says rather than how it’s formatted. This idea of a universal formatter appears in other languages like Rust’s own rustfmt, though there are many other tools for enforcing style that predate these tools, such as rubocop for Ruby and astyle for C and C++.

In Go and Rust, these formatters increase productivity dramatically because as long as you write syntactically correct code, it can be the ugliest code you have ever written and by passing it through the formatter (and many editors and IDEs will even format the source for you when you save!) you can leave it up to the formatter to make the code pretty and readable. This means less time fiddling with alignment, worrying about indentation, and dithering over where to break your function call chain to best communicate your intent. It also means that wars over format like tabs versus spaces are dead; the formatter is the absolute arbitrator of the correct style, and because the formatter is consistent across the entire ecosystem there is a lot of pressure for users to conform instead of trying to tweak the formatter to their own personal preferences.

We still have an open issue on supernova for implementing rustfmt as part of our continuous integration process that we hope to close out with a pull request relatively soon so we can be sure all the code contributed to supernova follows the same format as other projects.

Build Infrastructure Weirdness

Speaking of our continuous integration process, we ran into a very curious issue with 0xazure/supernova#24 this week where our builds started failing on the beta release channel. The build failure is caused by clippy’s new_ret_no_self lint which checks to ensure that, as a convention, new methods are used to make a new instance of a type and return that instance as the return value. In the issue, the build is only failing on the beta release channel which was surprising because I was expecting this lint to have failed the build on the stable release channel as well if it failed on beta. To further confuse the issue the build on nightly, which per our configuration for supernova is allowed to fail, was successful.

Digging into the problem some more, it looks like we are running into rust-lang-nursery/rust-clippy#3313 where the new_ret_no_self lint is incorrectly triggering on a new function that does return Self, it’s just wrapped by a container type or tuple.

Indeed, we can see this from our implementation of Config::new

pub fn new(mut args: env::Args) -> Result<Config, &'static str> {
    args.next();

    let username = match args.next() {
        None => return Err("No username provided"),
        Some(arg) => arg,
    };

    let token = args.next();

    Ok(Config { username, token })
}

which triggers the resulting lint failure

error: methods called `new` usually return `Self`
  --> src/lib.rs:18:5
   |
18 | /     pub fn new(mut args: env::Args) -> Result<Config, &'static str> {
19 | |         args.next();
20 | |
21 | |         let username = match args.next() {
...  |
28 | |         Ok(Config { username, token })
29 | |     }
   | |_____^
   |
   = note: `-D clippy::new-ret-no-self` implied by `-D warnings`
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_ret_no_self

even though our return type is Result<Config, &'static str> which unwraps to Config on success and a static str when there is an error in creating a new instance.

Investigating the Root Cause

An important part of build infrastructure is reproducibility: the ability to run a build with the same inputs and get the same outputs. Without reproducibility we have flaky tests that no one wants to run and worse, no one trusts. In the case of supernova we have a build matrix to test on all three release channels: stable, beta, and nightly, and we need to make sure testing on these channels happens in a predictable way.

It turns out the issue results from how clippy is installed in each environment. The recommended way to install clippy is as a rustup component using rustup component add clippy-preview. However, because clippy is published as a component for rustup rather than as some kind of version-pinned project dependency, this command does not install the same version of clippy across all release channels. This can be verified as follows:

$ cargo +stable clippy --version
clippy 0.0.212 (125907ad 2018-09-17)

$ cargo +beta clippy --version
clippy 0.0.212 (b1d03437 2018-10-19)

$ cargo +nightly clippy --version
clippy 0.0.212 (d8b42690 2018-11-04)

Note that while all of the build numbers are the same (v0.0.212), the commit hashes and dates are all different.

It is important to verify that the tool(s) you’re using to test or lint your project are the same version in all of your environments, otherwise you’ll end up with confusing build failures like the one we did here. In our case we are testing against beta and nightly to have an idea of future changes to the Rust compiler and any new lints that may get added in the future, so failures on anything but stable are nice-to-have information rather than complete show-stoppers. In other cases, or in different matrices, it’s even more important that the test environment is as consistent as possible and that the number of variables that are being changed are as small as possible to make tracing failures relatively simple.

Lint tools are great for catching low-hanging fruit in code review, but you can’t blindly trust them. When there is a failure, it takes a person’s knowledge of the project to determine if the failure is legitimate or if there’s a problem in the tool or lint rule and to determine if it’s a problem with the submitted code, a problem with the tool configuration, or a false positive in the tool as in this case with clippy’s new_ret_no_self lint.

Fixing the Problem

After reaching out to some friends in the #rust IRC channel, we decided to not run clippy on the beta toolchain to avoid more false positives like this in the future. We are keeping clippy enabled for the nightly release channel because we are allowing nightly to fail on Travis so while we will investigate those failures it will not block landing any pull requests if for some reason nightly or clippy on nightly finds fault with our code.

I also recently filed 0xazure/supernova#32 to provide better visibility into the versions of tools we install to match how Travis prints out tooling versions for tools that come automatically installed with the Rust build environment. This should help us track down version discrepancies and make trouble-shooting failures much quicker.

After landing the above fix (and an extra tweak so we only run Travis against the master branch), our builds went fully green for the first time since we enabled Travis on the project! Setting up automated builds can take a lot of up-front effort, but it pays big dividends as the project grows to ensure the quality of the software being written. Now we just need some tests so we can verify our code is actually correct…