Auto merge of #6970 - ehuss:clippy-fixes, r=Eh2406

Clippy fixes

Some of these are arguably not strictly better, but didn't really seem worse to me either.

I tried changing all `&Unit` to `Unit`, but I felt like the code was worse with more frequent use of `*` and `&`.

src/cargo/core/compiler/context/mod.rs

@@ -219,7 +219,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                 self.compilation
                     .rustdocflags
                     .entry(unit.pkg.package_id())
-                    .or_insert(rustdocflags.to_vec());
+                    .or_insert_with(|| rustdocflags.to_vec());
             }
 
             super::output_depinfo(&mut self, unit)?;

src/cargo/core/compiler/custom_build.rs

@@ -110,7 +110,8 @@ fn emit_build_output(state: &JobState<'_>, output: &BuildOutput, package_id: Pac
         linked_paths: &library_paths,
         cfgs: &output.cfgs,
         env: &output.env,
-    }.to_json_string();
+    }
+    .to_json_string();
     state.stdout(&msg);
 }
 

src/cargo/core/compiler/job_queue.rs

@@ -216,7 +216,7 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
 
         self.queue.queue(*unit, job, queue_deps);
         *self.counts.entry(unit.pkg.package_id()).or_insert(0) += 1;
-        return Ok(());
+        Ok(())
     }
 
     /// Executes all jobs necessary to build the dependency graph.

src/cargo/core/compiler/unit.rs

@@ -166,6 +166,6 @@ impl<'a> UnitInterner<'a> {
         }
         me.cache.insert(Box::new(item.clone()));
         let item = me.cache.get(item).unwrap();
-        return unsafe { &*(&**item as *const UnitInner<'a>) };
+        unsafe { &*(&**item as *const UnitInner<'a>) }
     }
 }

src/cargo/core/resolver/mod.rs

@@ -657,7 +657,7 @@ fn activate(
 
     let candidate = match registry.replacement_summary(candidate_pid) {
         Some(replace) => {
-            if cx.flag_activated(&replace, &method)? && activated {
+            if cx.flag_activated(replace, &method)? && activated {
                 return Ok(None);
             }
             trace!(

src/cargo/core/resolver/resolve.rs

@@ -55,7 +55,7 @@ impl Resolve {
                             .find(|d| d.kind() == Kind::Normal)
                             .and_then(|d| {
                                 if d.is_public() {
-                                    Some(dep_package.clone())
+                                    Some(*dep_package)
                                 } else {
                                     None
                                 }
@@ -64,7 +64,7 @@ impl Resolve {
                     })
                     .collect::<HashSet<PackageId>>();
 
-                (p.clone(), public_deps)
+                (*p, public_deps)
             })
             .collect();
 

src/cargo/lib.rs

@@ -1,22 +1,29 @@
 #![cfg_attr(test, deny(warnings))]
-#![warn(rust_2018_idioms)]
 // While we're getting used to 2018:
+#![warn(rust_2018_idioms)]
 // Clippy isn't enforced by CI (@alexcrichton isn't a fan).
-#![allow(clippy::boxed_local)] // bug rust-lang-nursery/rust-clippy#1123
+#![allow(clippy::blacklisted_name)] // frequently used in tests
 #![allow(clippy::cyclomatic_complexity)] // large project
 #![allow(clippy::derive_hash_xor_eq)] // there's an intentional incoherence
 #![allow(clippy::explicit_into_iter_loop)] // explicit loops are clearer
 #![allow(clippy::explicit_iter_loop)] // explicit loops are clearer
 #![allow(clippy::identity_op)] // used for vertical alignment
 #![allow(clippy::implicit_hasher)] // large project
 #![allow(clippy::large_enum_variant)] // large project
+#![allow(clippy::new_without_default)] // explicit is maybe clearer
 #![allow(clippy::redundant_closure)] // closures can be less verbose
 #![allow(clippy::redundant_closure_call)] // closures over try catch blocks
 #![allow(clippy::too_many_arguments)] // large project
 #![allow(clippy::type_complexity)] // there's an exceptionally complex type
 #![allow(clippy::wrong_self_convention)] // perhaps `Rc` should be special-cased in Clippy?
 #![warn(clippy::needless_borrow)]
 #![warn(clippy::redundant_clone)]
+// Unit is now interned, and would probably be better as pass-by-copy, but
+// doing so causes a lot of & and * shenanigans that makes the code arguably
+// less clear and harder to read.
+#![allow(clippy::trivially_copy_pass_by_ref)]
+// exhaustively destructuring ensures future fields are handled
+#![allow(clippy::unneeded_field_pattern)]
 
 use std::fmt;
 

src/cargo/ops/cargo_generate_lockfile.rs

@@ -54,8 +54,15 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
                 // by precise package update later.
                 Some(_) => {
                     let mut registry = PackageRegistry::new(opts.config)?;
-                    ops::resolve_with_previous(&mut registry, ws, Method::Everything,
-                                               None, None, &[], true)?
+                    ops::resolve_with_previous(
+                        &mut registry,
+                        ws,
+                        Method::Everything,
+                        None,
+                        None,
+                        &[],
+                        true,
+                    )?
                 }
             }
         }

src/cargo/ops/cargo_package.rs

@@ -440,7 +440,7 @@ fn tar(
     }
 
     if pkg.include_lockfile() {
-        let new_lock = build_lock(&ws)?;
+        let new_lock = build_lock(ws)?;
 
         config
             .shell()
@@ -609,7 +609,7 @@ fn run_verify(ws: &Workspace<'_>, tar: &FileLock, opts: &PackageOpts<'_>) -> Car
     {
         // FIXME: Turn this on at some point in the future
         //Some(vec!["-D exported_private_dependencies".to_string()])
-        None
+        Some(vec![])
     } else {
         None
     };

src/cargo/ops/common_for_install_and_uninstall.rs

@@ -211,7 +211,7 @@ impl InstallTracker {
                 // `cargo install --path ...` is always rebuilt.
                 return Ok((Freshness::Dirty, duplicates));
             }
-            if matching_duplicates.iter().all(|dupe_pkg_id| {
+            let is_up_to_date = |dupe_pkg_id| {
                 let info = self
                     .v2
                     .installs
@@ -229,7 +229,8 @@ impl InstallTracker {
                     && dupe_pkg_id.source_id() == source_id
                     && precise_equal
                     && info.is_up_to_date(opts, target, &exes)
-            }) {
+            };
+            if matching_duplicates.iter().all(is_up_to_date) {
                 Ok((Freshness::Fresh, duplicates))
             } else {
                 Ok((Freshness::Dirty, duplicates))

src/cargo/sources/path.rs

@@ -145,7 +145,7 @@ impl<'cfg> PathSource<'cfg> {
             .exclude()
             .iter()
             .chain(pkg.manifest().include().iter())
-            .any(|p| p.starts_with("!"));
+            .any(|p| p.starts_with('!'));
         // Don't warn about glob mismatch if it doesn't parse.
         let glob_is_valid = glob_exclude.is_ok() && glob_include.is_ok() && !has_negate;
         let glob_exclude = glob_exclude.unwrap_or_else(|_| Vec::new());
@@ -479,12 +479,9 @@ impl<'cfg> PathSource<'cfg> {
             if name.map(|s| s.starts_with('.')) == Some(true) {
                 continue;
             }
-            if is_root {
+            if is_root && name == Some("target") {
                 // Skip Cargo artifacts.
-                match name {
-                    Some("target") => continue,
-                    _ => {}
-                }
+                continue;
             }
             PathSource::walk(&path, ret, false, filter)?;
         }

src/cargo/sources/registry/index.rs

@@ -268,7 +268,7 @@ impl<'cfg> RegistryIndex<'cfg> {
     where
         'a: 'b,
     {
-        let source_id = self.source_id.clone();
+        let source_id = self.source_id;
 
         // First up actually parse what summaries we have available. If Cargo
         // has run previously this will parse a Cargo-specific cache file rather
@@ -337,7 +337,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         for path in UncanonicalizedIter::new(&raw_path).take(1024) {
             let summaries = Summaries::parse(
                 index_version.as_ref().map(|s| &**s),
-                &root,
+                root,
                 &cache_root,
                 path.as_ref(),
                 self.source_id,
@@ -671,7 +671,7 @@ impl<'a> SummariesCache<'a> {
             contents.extend_from_slice(data);
             contents.push(0);
         }
-        return contents;
+        contents
     }
 }
 

src/cargo/util/config.rs

@@ -704,7 +704,7 @@ impl Config {
     fn resolve_registry_index(&self, index: Value<String>) -> CargoResult<Url> {
         let base = index
             .definition
-            .root(&self)
+            .root(self)
             .join("truncated-by-url_with_base");
         // Parse val to check it is a URL, not a relative path without a protocol.
         let _parsed = index.val.to_url()?;
@@ -857,7 +857,7 @@ impl Config {
              `acquire_package_cache_lock` before we got to this stack frame",
         );
         assert!(ret.starts_with(self.home_path.as_path_unlocked()));
-        return ret;
+        ret
     }
 
     /// Acquires an exclusive lock on the global "package cache"

tests/testsuite/build.rs

@@ -4573,11 +4573,13 @@ fn pipelining_works() {
     foo.cargo("build")
         .env("CARGO_BUILD_PIPELINING", "true")
         .with_stdout("")
-        .with_stderr("\
+        .with_stderr(
+            "\
 [COMPILING] [..]
 [COMPILING] [..]
 [FINISHED] [..]
-")
+",
+        )
         .run();
 }
 
@@ -4628,13 +4630,15 @@ fn forward_rustc_output() {
 
     foo.cargo("build")
         .with_stdout("a\nb\n{}")
-        .with_stderr("\
+        .with_stderr(
+            "\
 [COMPILING] [..]
 [COMPILING] [..]
 c
 d
 {a
 [FINISHED] [..]
-")
+",
+        )
         .run();
 }

tests/testsuite/freshness.rs

@@ -1,5 +1,6 @@
 use filetime::FileTime;
 use std::fs::{self, File, OpenOptions};
+use std::io;
 use std::io::prelude::*;
 use std::net::TcpListener;
 use std::path::{Path, PathBuf};
@@ -1269,10 +1270,11 @@ fn fingerprint_cleaner(mut dir: PathBuf, timestamp: filetime::FileTime) {
     for fing in fs::read_dir(&dir).unwrap() {
         let fing = fing.unwrap();
 
-        if fs::read_dir(fing.path()).unwrap().all(|f| {
+        let outdated = |f: io::Result<fs::DirEntry>| {
             filetime::FileTime::from_last_modification_time(&f.unwrap().metadata().unwrap())
                 <= timestamp
-        }) {
+        };
+        if fs::read_dir(fing.path()).unwrap().all(outdated) {
             fs::remove_dir_all(fing.path()).unwrap();
             println!("remove: {:?}", fing.path());
             // a real cleaner would remove the big files in deps and build as well

tests/testsuite/registry.rs

@@ -2008,7 +2008,6 @@ fn readonly_registry_still_works() {
     // make sure we un-readonly the files afterwards so "cargo clean" can remove them (#6934)
     chmod_readonly(&paths::home(), false);
 
-
     fn chmod_readonly(path: &Path, readonly: bool) {
         for entry in t!(path.read_dir()) {
             let entry = t!(entry);

tests/testsuite/rustc.rs

@@ -113,7 +113,8 @@ fn build_with_args_to_one_of_multiple_binaries() {
         -C debuginfo=2 -C debug-assertions [..]`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
 ",
-        ).run();
+        )
+        .run();
 }
 
 #[test]

tests/testsuite/support/mod.rs

@@ -884,8 +884,14 @@ impl Execs {
                 panic!("`.stream()` is for local debugging")
             }
             process.exec_with_streaming(
-                &mut |out| Ok(println!("{}", out)),
-                &mut |err| Ok(eprintln!("{}", err)),
+                &mut |out| {
+                    println!("{}", out);
+                    Ok(())
+                },
+                &mut |err| {
+                    eprintln!("{}", err);
+                    Ok(())
+                },
                 true,
             )
         } else {
@@ -1166,7 +1172,7 @@ impl Execs {
                 let e = out.lines();
 
                 let mut diffs = self.diff_lines(a.clone(), e.clone(), true);
-                while let Some(..) = a.next() {
+                while a.next().is_some() {
                     let a = self.diff_lines(a.clone(), e.clone(), true);
                     if a.len() < diffs.len() {
                         diffs = a;
@@ -1214,7 +1220,7 @@ impl Execs {
                 let e = out.lines();
 
                 let mut diffs = self.diff_lines(a.clone(), e.clone(), true);
-                while let Some(..) = a.next() {
+                while a.next().is_some() {
                     let a = self.diff_lines(a.clone(), e.clone(), true);
                     if a.len() < diffs.len() {
                         diffs = a;

tests/testsuite/update.rs

@@ -556,7 +556,6 @@ fn update_precise_first_run() {
 ",
         )
         .run();
-
 }
 
 #[test]