Run clippy with nightly rust on CI (#6347)

This enables all default lints, and a selection of pedantic ones.

It also fixes all code to be clean with respect to those lints.

Each commit is independently reviewable.

It's a shame that the lint selecting can't be shared across creates in some way.

.travis.yml

@@ -153,8 +153,13 @@ matrix:
     - <<: *default_test_config
       env:
         - SHARD="Self checks, lint, and JVM tests"
+      before_install:
+        - sudo apt-get install -y pkg-config fuse libfuse-dev
+        - sudo modprobe fuse
+        - sudo chmod 666 /dev/fuse
+        - sudo chown root:$USER /etc/fuse.conf
       script:
-        - ./build-support/bin/ci.sh -fkmrjt "${SHARD}"
+        - ./build-support/bin/ci.sh -fkmrjst "${SHARD}"
 
     - <<: *default_test_config
       env:

build-support/bin/ci.sh

@@ -13,7 +13,7 @@ function usage() {
   cat <<EOF
 Runs commons tests for local or hosted CI.
 
-Usage: $0 (-h|-3fxbkmrjlpuneycitz)
+Usage: $0 (-h|-3fxbkmrjlpuneycitzs)
  -h           print out this help message
  -3           After pants is bootstrapped, set --python-setup-interpreter-constraints such that any
               python tests run with Python 3.
@@ -34,6 +34,7 @@ Usage: $0 (-h|-3fxbkmrjlpuneycitz)
               to run only even tests: '-u 0/2', odd: '-u 1/2'
  -n           run contrib python tests
  -e           run rust tests
+ -s           run clippy on rust code
  -y SHARD_NUMBER/TOTAL_SHARDS
               if running contrib python tests, divide them into
               TOTAL_SHARDS shards and just run those in SHARD_NUMBER
@@ -64,7 +65,7 @@ python_contrib_shard="0/1"
 python_intg_shard="0/1"
 python_three="false"
 
-while getopts "h3fxbkmrjlpeu:ny:ci:tz" opt; do
+while getopts "h3fxbkmrjlpesu:ny:ci:tz" opt; do
   case ${opt} in
     h) usage ;;
     3) python_three="true" ;;
@@ -79,6 +80,7 @@ while getopts "h3fxbkmrjlpeu:ny:ci:tz" opt; do
     p) run_python="true" ;;
     u) python_unit_shard=${OPTARG} ;;
     e) run_rust_tests="true" ;;
+    s) run_rust_clippy="true" ;;
     n) run_contrib="true" ;;
     y) python_contrib_shard=${OPTARG} ;;
     c) run_integration="true" ;;
@@ -231,6 +233,13 @@ if [[ "${run_rust_tests:-false}" == "true" ]]; then
   end_travis_section
 fi
 
+if [[ "${run_rust_clippy:-false}" == "true" ]]; then
+  start_travis_section "RustClippy" "Running Clippy on rust code"
+  (
+    "${REPO_ROOT}/build-support/bin/native/cargo" +nightly clippy --manifest-path="${REPO_ROOT}/src/rust/engine/Cargo.toml" --all
+  ) || die "Pants clippy failure"
+fi
+
 # NB: this only tests python tests right now -- the command needs to be edited if test targets in
 # other languages are tagged with 'platform_specific_behavior' in the future.
 if [[ "${test_platform_specific_behavior:-false}" == 'true' ]]; then

build-support/bin/native/bootstrap_rust.sh

@@ -7,12 +7,6 @@ REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../../.. && pwd -P)"
 # + fingerprint_data: Fingerprints the data on stdin.
 source "${REPO_ROOT}/build-support/common.sh"
 
-RUST_TOOLCHAIN="$(cat ${REPO_ROOT}/rust-toolchain)"
-readonly RUST_COMPONENTS=(
-  "rustfmt-preview"
-  "rust-src"
-)
-
 readonly rust_toolchain_root="${CACHE_ROOT}/rust"
 export CARGO_HOME="${rust_toolchain_root}/cargo"
 export RUSTUP_HOME="${rust_toolchain_root}/rustup"
@@ -24,6 +18,17 @@ function cargo_bin() {
 }
 
 function bootstrap_rust() {
+  RUST_TOOLCHAIN="$(cat ${REPO_ROOT}/rust-toolchain)"
+  RUST_COMPONENTS=(
+    "rustfmt-preview"
+    "rust-src"
+  )
+
+  if [[ "$#" -eq 1 && "$1" == "+nightly" ]]; then
+    RUST_TOOLCHAIN="nightly"
+    RUST_COMPONENTS=("${RUST_COMPONENTS[@]}" "clippy-preview")
+  fi
+
   # Control a pants-specific rust toolchain.
   if [[ ! -x "${RUSTUP}" ]]
   then
@@ -39,15 +44,15 @@ function bootstrap_rust() {
   local -r cargo="${CARGO_HOME}/bin/cargo"
   local -r cargo_components_fp=$(echo "${RUST_COMPONENTS[@]}" | fingerprint_data)
   local -r cargo_versioned="cargo-${RUST_TOOLCHAIN}-${cargo_components_fp}"
-  if [[ ! -x "${rust_toolchain_root}/${cargo_versioned}" ]]
+  if [[ ! -x "${rust_toolchain_root}/${cargo_versioned}" || "${RUST_TOOLCHAIN}" == "nightly" ]]
   then
     # If rustup was already bootstrapped against a different toolchain in the past, freshen it and
     # ensure the toolchain and components we need are installed.
     "${RUSTUP}" self update
     "${RUSTUP}" toolchain install ${RUST_TOOLCHAIN}
     "${RUSTUP}" component add --toolchain ${RUST_TOOLCHAIN} ${RUST_COMPONENTS[@]} >&2
 
-    ln -fs "$(cargo_bin)" "${rust_toolchain_root}/${cargo_versioned}"
+    ln -fs "$(RUSTUP_TOOLCHAIN="${RUST_TOOLCHAIN}" cargo_bin)" "${rust_toolchain_root}/${cargo_versioned}"
   fi
 
   local -r symlink_farm_root="${REPO_ROOT}/build-support/bin/native"

build-support/bin/native/cargo.sh

@@ -9,7 +9,13 @@ REPO_ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && cd ../../.. && pwd -P)
 # Exposes:
 # + bootstrap_rust: Bootstraps a Pants-controlled rust toolchain and associated extras.
 source "${REPO_ROOT}/build-support/bin/native/bootstrap_rust.sh"
-bootstrap_rust >&2
+
+nightly=""
+if [[ "$@" =~ '+nightly' ]]; then
+  nightly="+nightly"
+fi
+
+bootstrap_rust "${nightly}" >&2
 
 download_binary="${REPO_ROOT}/build-support/bin/download_binary.sh"
 

src/rust/engine/Cargo.toml

@@ -69,6 +69,7 @@ default-members = [
 
 [dependencies]
 boxfuture = { path = "boxfuture" }
+dirs = "1"
 enum_primitive = "0.1.1"
 fnv = "1.0.5"
 fs = { path = "fs" }

src/rust/engine/async_semaphore/src/lib.rs

@@ -1,3 +1,20 @@
+// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source.
+#![cfg_attr(
+  feature = "cargo-clippy",
+  deny(
+    clippy, default_trait_access, expl_impl_clone_on_copy, if_not_else, needless_continue,
+    single_match_else, unseparated_literal_suffix, used_underscore_binding
+  )
+)]
+// It is often more clear to show that nothing is being moved.
+#![cfg_attr(feature = "cargo-clippy", allow(match_ref_pats))]
+// Subjective style.
+#![cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, redundant_field_names))]
+// Default isn't as big a deal as people seem to think it is.
+#![cfg_attr(feature = "cargo-clippy", allow(new_without_default, new_without_default_derive))]
+// Arc<Mutex> can be more clear than needing to grok Orderings:
+#![cfg_attr(feature = "cargo-clippy", allow(mutex_atomic))]
+
 extern crate futures;
 
 use std::collections::VecDeque;

src/rust/engine/boxfuture/src/lib.rs

@@ -2,6 +2,23 @@
 // https://github.com/alexcrichton/futures-rs/issues/228 has background for its removal.
 // This avoids needing to call Box::new() around every future that we produce.
 
+// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source.
+#![cfg_attr(
+  feature = "cargo-clippy",
+  deny(
+    clippy, default_trait_access, expl_impl_clone_on_copy, if_not_else, needless_continue,
+    single_match_else, unseparated_literal_suffix, used_underscore_binding
+  )
+)]
+// It is often more clear to show that nothing is being moved.
+#![cfg_attr(feature = "cargo-clippy", allow(match_ref_pats))]
+// Subjective style.
+#![cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, redundant_field_names))]
+// Default isn't as big a deal as people seem to think it is.
+#![cfg_attr(feature = "cargo-clippy", allow(new_without_default, new_without_default_derive))]
+// Arc<Mutex> can be more clear than needing to grok Orderings:
+#![cfg_attr(feature = "cargo-clippy", allow(mutex_atomic))]
+
 extern crate futures;
 
 use futures::future::Future;

src/rust/engine/build_utils/src/lib.rs

@@ -1,3 +1,20 @@
+// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source.
+#![cfg_attr(
+  feature = "cargo-clippy",
+  deny(
+    clippy, default_trait_access, expl_impl_clone_on_copy, if_not_else, needless_continue,
+    single_match_else, unseparated_literal_suffix, used_underscore_binding
+  )
+)]
+// It is often more clear to show that nothing is being moved.
+#![cfg_attr(feature = "cargo-clippy", allow(match_ref_pats))]
+// Subjective style.
+#![cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, redundant_field_names))]
+// Default isn't as big a deal as people seem to think it is.
+#![cfg_attr(feature = "cargo-clippy", allow(new_without_default, new_without_default_derive))]
+// Arc<Mutex> can be more clear than needing to grok Orderings:
+#![cfg_attr(feature = "cargo-clippy", allow(mutex_atomic))]
+
 use std::env;
 use std::io;
 use std::ops::Deref;

src/rust/engine/fs/brfs/Cargo.toml

@@ -7,6 +7,7 @@ publish = false
 [dependencies]
 bazel_protos = { path = "../../process_execution/bazel_protos" }
 clap = "2"
+dirs = "1"
 env_logger = "0.5.4"
 errno = "0.2.3"
 fs = { path = ".." }

src/rust/engine/fs/brfs/src/main.rs

@@ -1,5 +1,23 @@
+// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source.
+#![cfg_attr(
+  feature = "cargo-clippy",
+  deny(
+    clippy, default_trait_access, expl_impl_clone_on_copy, if_not_else, needless_continue,
+    single_match_else, unseparated_literal_suffix, used_underscore_binding
+  )
+)]
+// It is often more clear to show that nothing is being moved.
+#![cfg_attr(feature = "cargo-clippy", allow(match_ref_pats))]
+// Subjective style.
+#![cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, redundant_field_names))]
+// Default isn't as big a deal as people seem to think it is.
+#![cfg_attr(feature = "cargo-clippy", allow(new_without_default, new_without_default_derive))]
+// Arc<Mutex> can be more clear than needing to grok Orderings:
+#![cfg_attr(feature = "cargo-clippy", allow(mutex_atomic))]
+
 extern crate bazel_protos;
 extern crate clap;
+extern crate dirs;
 extern crate env_logger;
 extern crate errno;
 extern crate fs;
@@ -53,7 +71,7 @@ fn attr_for(inode: Inode, size: u64, kind: fuse::FileType, perm: u16) -> fuse::F
 }
 
 pub fn digest_from_filepath(str: &str) -> Result<Digest, String> {
-  let mut parts = str.split("-");
+  let mut parts = str.split('-');
   let fingerprint_str = parts
     .next()
     .ok_or_else(|| format!("Invalid digest: {} wasn't of form fingerprint-size", str))?;
@@ -342,16 +360,14 @@ impl BuildResultFS {
 
               Ok(entries)
             }
-            Ok(None) => {
-              return Err(libc::ENOENT);
-            }
+            Ok(None) => Err(libc::ENOENT),
             Err(err) => {
               error!("Error loading directory {:?}: {}", digest, err);
-              return Err(libc::EINVAL);
+              Err(libc::EINVAL)
             }
           }
         }
-        _ => return Err(libc::ENOENT),
+        _ => Err(libc::ENOENT),
       },
     }
   }
@@ -396,11 +412,11 @@ impl fuse::Filesystem for BuildResultFS {
         let maybe_cache_entry = self
           .inode_digest_cache
           .get(&parent)
-          .map(|entry| entry.clone())
+          .cloned()
           .ok_or(libc::ENOENT);
         maybe_cache_entry
           .and_then(|cache_entry| {
-            let parent_digest = cache_entry.digest.clone();
+            let parent_digest = cache_entry.digest;
             self
               .store
               .load_directory(parent_digest)
@@ -499,9 +515,8 @@ impl fuse::Filesystem for BuildResultFS {
             let mut reply = reply.lock().unwrap();
             reply.take().unwrap().data(&bytes.slice(begin, end));
           })
-          .map(|v| match v {
-            Some(_) => {}
-            None => {
+          .map(|v| {
+            if v.is_none() {
               let maybe_reply = reply2.lock().unwrap().take();
               if let Some(reply) = maybe_reply {
                 reply.error(libc::ENOENT);
@@ -585,7 +600,7 @@ pub fn mount<'a, P: AsRef<Path>>(
 }
 
 fn main() {
-  let default_store_path = std::env::home_dir()
+  let default_store_path = dirs::home_dir()
     .expect("Couldn't find homedir")
     .join(".cache")
     .join("pants")
@@ -641,17 +656,25 @@ fn main() {
   }.expect("Error making store");
 
   let _fs = mount(mount_path, store).expect("Error mounting");
-  loop {}
+  loop {
+    std::thread::sleep(std::time::Duration::from_secs(1));
+  }
 }
 
 #[cfg(target_os = "macos")]
 fn unmount(mount_path: &str) -> i32 {
-  unsafe { libc::unmount(CString::new(mount_path).unwrap().as_ptr(), 0) }
+  unsafe {
+    let path = CString::new(mount_path).unwrap();
+    libc::unmount(path.as_ptr(), 0)
+  }
 }
 
 #[cfg(target_os = "linux")]
 fn unmount(mount_path: &str) -> i32 {
-  unsafe { libc::umount(CString::new(mount_path).unwrap().as_ptr()) }
+  unsafe {
+    let path = CString::new(mount_path).unwrap();
+    libc::umount(path.as_ptr())
+  }
 }
 
 #[cfg(test)]

src/rust/engine/fs/fs_util/src/main.rs

@@ -1,3 +1,20 @@
+// Enable all clippy lints except for many of the pedantic ones. It's a shame this needs to be copied and pasted across crates, but there doesn't appear to be a way to include inner attributes from a common source.
+#![cfg_attr(
+  feature = "cargo-clippy",
+  deny(
+    clippy, default_trait_access, expl_impl_clone_on_copy, if_not_else, needless_continue,
+    single_match_else, unseparated_literal_suffix, used_underscore_binding
+  )
+)]
+// It is often more clear to show that nothing is being moved.
+#![cfg_attr(feature = "cargo-clippy", allow(match_ref_pats))]
+// Subjective style.
+#![cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, redundant_field_names))]
+// Default isn't as big a deal as people seem to think it is.
+#![cfg_attr(feature = "cargo-clippy", allow(new_without_default, new_without_default_derive))]
+// Arc<Mutex> can be more clear than needing to grok Orderings:
+#![cfg_attr(feature = "cargo-clippy", allow(mutex_atomic))]
+
 #[macro_use]
 extern crate boxfuture;
 extern crate bytes;
@@ -294,7 +311,7 @@ fn execute(top_match: &clap::ArgMatches) -> Result<(), ExitError> {
           .and_then(move |paths| {
             Snapshot::from_path_stats(
               store_copy.clone(),
-              fs::OneOffStoreFileByDigest::new(store_copy, posix_fs),
+              &fs::OneOffStoreFileByDigest::new(store_copy, posix_fs),
               paths,
             )
           })