Auto merge of #5835 - dwijnand:clippy, r=alexcrichton

Resolve some Clippy warnings

I'm not sure how these popped up since my PR 8 days ago.

My current hypotheses:
* changes in latest nightly rust/clippy
* new or changed cargo code
* I missed these as I was only touching `src/bin/cargo/main.rs`

For future reference I now iterate with:

    touch src/bin/cargo/main.rs src/cargo/lib.rs && cargo +nightly clippy

src/bin/cargo/commands/fix.rs

@@ -110,18 +110,15 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     // Unlike other commands default `cargo fix` to all targets to fix as much
     // code as we can.
     let mut opts = args.compile_options(config, mode)?;
-    match opts.filter {
-        CompileFilter::Default { .. } => {
-            opts.filter = CompileFilter::Only {
-                all_targets: true,
-                lib: true,
-                bins: FilterRule::All,
-                examples: FilterRule::All,
-                benches: FilterRule::All,
-                tests: FilterRule::All,
-            };
+    if let CompileFilter::Default { .. } = opts.filter {
+        opts.filter = CompileFilter::Only {
+            all_targets: true,
+            lib: true,
+            bins: FilterRule::All,
+            examples: FilterRule::All,
+            benches: FilterRule::All,
+            tests: FilterRule::All,
         }
-        _ => {}
     }
     ops::fix(&ws, &mut ops::FixOptions {
         edition: args.value_of("edition"),

src/bin/cargo/main.rs

@@ -1,5 +1,5 @@
-// we have lots of arguments, cleaning this up would be a large project
-#![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments))]
+#![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments))] // large project
+#![cfg_attr(feature = "cargo-clippy", allow(redundant_closure))]  // there's a false positive
 
 extern crate cargo;
 extern crate clap;
@@ -135,7 +135,6 @@ fn find_closest(config: &Config, cmd: &str) -> Option<String> {
 
 fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> CliResult {
     let command_exe = format!("cargo-{}{}", cmd, env::consts::EXE_SUFFIX);
-    #[cfg_attr(feature = "cargo-clippy", allow(redundant_closure))] // false positive
     let path = search_directories(config)
         .iter()
         .map(|dir| dir.join(&command_exe))

src/cargo/core/compiler/job.rs

@@ -19,8 +19,6 @@ trait FnBox<A, R> {
 }
 
 impl<A, R, F: FnOnce(A) -> R> FnBox<A, R> for F {
-    // False positive: https://github.com/rust-lang-nursery/rust-clippy/issues/1123
-    #[cfg_attr(feature = "cargo-clippy", allow(boxed_local))]
     fn call_box(self: Box<F>, a: A) -> R {
         (*self)(a)
     }

src/cargo/core/manifest.rs

@@ -17,8 +17,6 @@ use util::errors::*;
 use util::toml::TomlManifest;
 use util::Config;
 
-// While unfortunate, resolving the size difference with Box would be a large project
-#[cfg_attr(feature = "cargo-clippy", allow(large_enum_variant))]
 pub enum EitherManifest {
     Real(Manifest),
     Virtual(VirtualManifest),
@@ -208,7 +206,6 @@ struct NonHashedPathBuf {
     path: PathBuf,
 }
 
-#[cfg_attr(feature = "cargo-clippy", allow(derive_hash_xor_eq))] // current intentional incoherence
 impl Hash for NonHashedPathBuf {
     fn hash<H: Hasher>(&self, _: &mut H) {
         // ...

src/cargo/core/profiles.rs

@@ -495,8 +495,6 @@ impl Profile {
     /// Compare all fields except `name`, which doesn't affect compilation.
     /// This is necessary for `Unit` deduplication for things like "test" and
     /// "dev" which are essentially the same.
-    // The complexity of the result type is exempted because it's limited in scope.
-    #[cfg_attr(feature = "cargo-clippy", allow(type_complexity))]
     fn comparable(
         &self,
     ) -> (

src/cargo/core/resolver/mod.rs

@@ -106,8 +106,6 @@ mod types;
 ///
 /// * `print_warnings` - whether or not to print backwards-compatibility
 ///   warnings and such
-// While unfortunate, generalising this over different hashers would be a large project
-#[cfg_attr(feature = "cargo-clippy", allow(implicit_hasher))]
 pub fn resolve(
     summaries: &[(Summary, Method)],
     replacements: &[(PackageIdSpec, Dependency)],

src/cargo/core/summary.rs

@@ -35,7 +35,7 @@ impl Summary {
     pub fn new<K>(
         pkg_id: PackageId,
         dependencies: Vec<Dependency>,
-        features: BTreeMap<K, Vec<impl AsRef<str>>>,
+        features: &BTreeMap<K, Vec<impl AsRef<str>>>,
         links: Option<impl AsRef<str>>,
         namespaced_features: bool,
     ) -> CargoResult<Summary>

src/cargo/core/workspace.rs

@@ -87,8 +87,6 @@ struct Packages<'cfg> {
 }
 
 #[derive(Debug)]
-// While unfortunate, resolving the size difference with Box would be a large project
-#[cfg_attr(feature = "cargo-clippy", allow(large_enum_variant))]
 enum MaybePackage {
     Package(Package),
     Virtual(VirtualManifest),
@@ -777,12 +775,6 @@ impl<'cfg> Packages<'cfg> {
     }
 }
 
-impl<'a, 'cfg> Members<'a, 'cfg> {
-    pub fn is_empty(self) -> bool {
-        self.count() == 0
-    }
-}
-
 impl<'a, 'cfg> Iterator for Members<'a, 'cfg> {
     type Item = &'a Package;
 

src/cargo/lib.rs

@@ -1,14 +1,18 @@
 #![cfg_attr(test, deny(warnings))]
-// Currently, Cargo does not use clippy for its source code.
-// But if someone runs it they should know that
-// @alexcrichton disagree with clippy on some style things
-#![cfg_attr(feature = "cargo-clippy", allow(explicit_iter_loop))]
-#![cfg_attr(feature = "cargo-clippy", allow(explicit_into_iter_loop))]
-// also we use closures as an alternative to try catch blocks
-#![cfg_attr(feature = "cargo-clippy", allow(redundant_closure_call))]
 
-// we have some complicated functions, cleaning this up would be a large project
-#![cfg_attr(feature = "cargo-clippy", allow(cyclomatic_complexity))]
+// Clippy isn't enforced by CI, and know that @alexcrichton isn't a fan :)
+#![cfg_attr(feature = "cargo-clippy", allow(boxed_local))]             // bug rust-lang-nursery/rust-clippy#1123
+#![cfg_attr(feature = "cargo-clippy", allow(cyclomatic_complexity))]   // large project
+#![cfg_attr(feature = "cargo-clippy", allow(derive_hash_xor_eq))]      // there's an intentional incoherence
+#![cfg_attr(feature = "cargo-clippy", allow(explicit_into_iter_loop))] // (unclear why)
+#![cfg_attr(feature = "cargo-clippy", allow(explicit_iter_loop))]      // (unclear why)
+#![cfg_attr(feature = "cargo-clippy", allow(identity_op))]             // used for vertical alignment
+#![cfg_attr(feature = "cargo-clippy", allow(implicit_hasher))]         // large project
+#![cfg_attr(feature = "cargo-clippy", allow(large_enum_variant))]      // large project
+#![cfg_attr(feature = "cargo-clippy", allow(redundant_closure_call))]  // closures over try catch blocks
+#![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments))]      // large project
+#![cfg_attr(feature = "cargo-clippy", allow(type_complexity))]         // there's an exceptionally complex type
+#![cfg_attr(feature = "cargo-clippy", allow(wrong_self_convention))]   // perhaps Rc should be special cased in Clippy?
 
 extern crate atty;
 extern crate clap;

src/cargo/ops/cargo_compile.rs

@@ -122,7 +122,7 @@ impl Packages {
         })
     }
 
-    pub fn into_package_id_specs(&self, ws: &Workspace) -> CargoResult<Vec<PackageIdSpec>> {
+    pub fn to_package_id_specs(&self, ws: &Workspace) -> CargoResult<Vec<PackageIdSpec>> {
         let specs = match *self {
             Packages::All => ws.members()
                 .map(Package::package_id)
@@ -185,7 +185,8 @@ pub fn compile<'a>(
     ws: &Workspace<'a>,
     options: &CompileOptions<'a>,
 ) -> CargoResult<Compilation<'a>> {
-    compile_with_exec(ws, options, Arc::new(DefaultExecutor))
+    let exec: Arc<Executor> = Arc::new(DefaultExecutor);
+    compile_with_exec(ws, options, &exec)
 }
 
 /// Like `compile` but allows specifying a custom `Executor` that will be able to intercept build
@@ -193,7 +194,7 @@ pub fn compile<'a>(
 pub fn compile_with_exec<'a>(
     ws: &Workspace<'a>,
     options: &CompileOptions<'a>,
-    exec: Arc<Executor>,
+    exec: &Arc<Executor>,
 ) -> CargoResult<Compilation<'a>> {
     ws.emit_warnings()?;
     compile_ws(ws, None, options, exec)
@@ -203,7 +204,7 @@ pub fn compile_ws<'a>(
     ws: &Workspace<'a>,
     source: Option<Box<Source + 'a>>,
     options: &CompileOptions<'a>,
-    exec: Arc<Executor>,
+    exec: &Arc<Executor>,
 ) -> CargoResult<Compilation<'a>> {
     let CompileOptions {
         config,
@@ -224,7 +225,7 @@ pub fn compile_ws<'a>(
         Kind::Host
     };
 
-    let specs = spec.into_package_id_specs(ws)?;
+    let specs = spec.to_package_id_specs(ws)?;
     let features = Method::split_features(features);
     let method = Method::Required {
         dev_deps: ws.require_optional_deps() || filter.need_dev_deps(build_config.mode),

src/cargo/ops/cargo_doc.rs

@@ -18,7 +18,7 @@ pub struct DocOptions<'a> {
 
 /// Main method for `cargo doc`.
 pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> {
-    let specs = options.compile_opts.spec.into_package_id_specs(ws)?;
+    let specs = options.compile_opts.spec.to_package_id_specs(ws)?;
     let resolve = ops::resolve_ws_precisely(
         ws,
         None,

src/cargo/ops/cargo_generate_lockfile.rs

@@ -38,7 +38,7 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions) -> CargoResult<()>
         bail!("cannot specify both aggressive and precise simultaneously")
     }
 
-    if ws.members().is_empty() {
+    if ws.members().count() == 0 {
         bail!("you can't generate a lockfile for an empty workspace.")
     }
 

src/cargo/ops/cargo_install.rs

@@ -12,7 +12,7 @@ use toml;
 
 use core::{Dependency, Edition, Package, PackageIdSpec, Source, SourceId};
 use core::{PackageId, Workspace};
-use core::compiler::DefaultExecutor;
+use core::compiler::{DefaultExecutor, Executor};
 use ops::{self, CompileFilter};
 use sources::{GitSource, PathSource, SourceConfigMap};
 use util::{internal, Config};
@@ -262,8 +262,9 @@ fn install_one(
         check_overwrites(&dst, pkg, &opts.filter, &list, force)?;
     }
 
+    let exec: Arc<Executor> = Arc::new(DefaultExecutor);
     let compile =
-        ops::compile_ws(&ws, Some(source), opts, Arc::new(DefaultExecutor)).chain_err(|| {
+        ops::compile_ws(&ws, Some(source), opts, &exec).chain_err(|| {
             if let Some(td) = td_opt.take() {
                 // preserve the temporary directory, so the user can inspect it
                 td.into_path();

src/cargo/ops/cargo_output_metadata.rs

@@ -45,7 +45,7 @@ fn metadata_no_deps(ws: &Workspace, _opt: &OutputMetadataOptions) -> CargoResult
 }
 
 fn metadata_full(ws: &Workspace, opt: &OutputMetadataOptions) -> CargoResult<ExportInfo> {
-    let specs = Packages::All.into_package_id_specs(ws)?;
+    let specs = Packages::All.to_package_id_specs(ws)?;
     let deps = ops::resolve_ws_precisely(
         ws,
         None,

src/cargo/ops/cargo_package.rs

@@ -10,7 +10,7 @@ use git2;
 use tar::{Archive, Builder, EntryType, Header};
 
 use core::{Package, Source, SourceId, Workspace};
-use core::compiler::{BuildConfig, CompileMode, DefaultExecutor};
+use core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor};
 use sources::PathSource;
 use util::{self, internal, Config, FileLock};
 use util::paths;
@@ -333,6 +333,7 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult
     let pkg_fingerprint = src.last_modified_file(&new_pkg)?;
     let ws = Workspace::ephemeral(new_pkg, config, None, true)?;
 
+    let exec: Arc<Executor> = Arc::new(DefaultExecutor);
     ops::compile_ws(
         &ws,
         None,
@@ -350,7 +351,7 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult
             target_rustc_args: None,
             export_dir: None,
         },
-        Arc::new(DefaultExecutor),
+        &exec,
     )?;
 
     // Check that build.rs didn't modify any files in the src directory.

src/cargo/ops/resolve.rs

@@ -133,8 +133,6 @@ fn resolve_with_registry<'cfg>(
 ///
 /// The previous resolve normally comes from a lockfile. This function does not
 /// read or write lockfiles from the filesystem.
-// While unfortunate, generalising this over different hashers would be a large project
-#[cfg_attr(feature = "cargo-clippy", allow(implicit_hasher))]
 pub fn resolve_with_previous<'a, 'cfg>(
     registry: &mut PackageRegistry<'cfg>,
     ws: &Workspace<'cfg>,

src/cargo/sources/registry/index.rs

@@ -253,7 +253,7 @@ impl<'cfg> RegistryIndex<'cfg> {
             .into_iter()
             .map(|dep| dep.into_dep(&self.source_id))
             .collect::<CargoResult<Vec<_>>>()?;
-        let summary = Summary::new(pkgid, deps, features, links, false)?;
+        let summary = Summary::new(pkgid, deps, &features, links, false)?;
         let summary = summary.set_checksum(cksum.clone());
         self.hashes
             .entry(name.as_str())

src/cargo/util/errors.rs

@@ -209,7 +209,7 @@ impl From<clap::Error> for CliError {
 
 pub fn process_error(
     msg: &str,
-    status: Option<&ExitStatus>,
+    status: Option<ExitStatus>,
     output: Option<&Output>,
 ) -> ProcessError {
     let exit = match status {
@@ -237,12 +237,12 @@ pub fn process_error(
 
     return ProcessError {
         desc,
-        exit: status.cloned(),
+        exit: status,
         output: output.cloned(),
     };
 
     #[cfg(unix)]
-    fn status_to_string(status: &ExitStatus) -> String {
+    fn status_to_string(status: ExitStatus) -> String {
         use std::os::unix::process::*;
         use libc;
 
@@ -272,7 +272,7 @@ pub fn process_error(
     }
 
     #[cfg(windows)]
-    fn status_to_string(status: &ExitStatus) -> String {
+    fn status_to_string(status: ExitStatus) -> String {
         status.to_string()
     }
 }

src/cargo/util/process_builder.rs

@@ -144,7 +144,7 @@ impl ProcessBuilder {
         } else {
             Err(process_error(
                 &format!("process didn't exit successfully: {}", self),
-                Some(&exit),
+                Some(exit),
                 None,
             ).into())
         }
@@ -193,7 +193,7 @@ impl ProcessBuilder {
         } else {
             Err(process_error(
                 &format!("process didn't exit successfully: {}", self),
-                Some(&output.status),
+                Some(output.status),
                 Some(&output),
             ).into())
         }
@@ -271,13 +271,13 @@ impl ProcessBuilder {
             if !output.status.success() {
                 return Err(process_error(
                     &format!("process didn't exit successfully: {}", self),
-                    Some(&output.status),
+                    Some(output.status),
                     to_print,
                 ).into());
             } else if let Some(e) = callback_error {
                 let cx = process_error(
                     &format!("failed to parse process output: {}", self),
-                    Some(&output.status),
+                    Some(output.status),
                     to_print,
                 );
                 return Err(e.context(cx).into());

src/cargo/util/toml/mod.rs

@@ -151,8 +151,6 @@ type TomlBenchTarget = TomlTarget;
 
 #[derive(Debug, Serialize)]
 #[serde(untagged)]
-// While unfortunate, resolving the size difference with Box would be a large project
-#[cfg_attr(feature = "cargo-clippy", allow(large_enum_variant))]
 pub enum TomlDependency {
     Simple(String),
     Detailed(DetailedTomlDependency),
@@ -894,7 +892,7 @@ impl TomlManifest {
         let summary = Summary::new(
             pkgid,
             deps,
-            me.features
+            &me.features
                 .as_ref()
                 .map(|x| {
                     x.iter()

src/crates-io/lib.rs

@@ -1,4 +1,5 @@
 #![allow(unknown_lints)]
+#![cfg_attr(feature = "cargo-clippy", allow(identity_op))] // used for vertical alignment
 
 extern crate curl;
 #[macro_use]

tests/testsuite/resolve.rs

@@ -38,7 +38,7 @@ fn resolve_with_config(
         }
     }
     let mut registry = MyRegistry(registry);
-    let summary = Summary::new(pkg.clone(), deps, BTreeMap::<String, Vec<String>>::new(), None::<String>, false).unwrap();
+    let summary = Summary::new(pkg.clone(), deps, &BTreeMap::<String, Vec<String>>::new(), None::<String>, false).unwrap();
     let method = Method::Everything;
     let resolve = resolver::resolve(
         &[(summary, method)],
@@ -100,13 +100,13 @@ macro_rules! pkg {
         let pkgid = $pkgid.to_pkgid();
         let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().as_str())} else {None};
 
-        Summary::new(pkgid, d, BTreeMap::<String, Vec<String>>::new(), link, false).unwrap()
+        Summary::new(pkgid, d, &BTreeMap::<String, Vec<String>>::new(), link, false).unwrap()
     });
 
     ($pkgid:expr) => ({
         let pkgid = $pkgid.to_pkgid();
         let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().as_str())} else {None};
-        Summary::new(pkgid, Vec::new(), BTreeMap::<String, Vec<String>>::new(), link, false).unwrap()
+        Summary::new(pkgid, Vec::new(), &BTreeMap::<String, Vec<String>>::new(), link, false).unwrap()
     })
 }
 
@@ -121,7 +121,7 @@ fn pkg(name: &str) -> Summary {
     } else {
         None
     };
-    Summary::new(pkg_id(name), Vec::new(), BTreeMap::<String, Vec<String>>::new(), link, false).unwrap()
+    Summary::new(pkg_id(name), Vec::new(), &BTreeMap::<String, Vec<String>>::new(), link, false).unwrap()
 }
 
 fn pkg_id(name: &str) -> PackageId {
@@ -145,7 +145,7 @@ fn pkg_loc(name: &str, loc: &str) -> Summary {
     Summary::new(
         pkg_id_loc(name, loc),
         Vec::new(),
-        BTreeMap::<String, Vec<String>>::new(),
+        &BTreeMap::<String, Vec<String>>::new(),
         link,
         false,
     ).unwrap()