From 4a1207caaa6ff574f63da0f0c4c797fc3ea2cf22 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sat, 9 Nov 2019 13:46:10 +0200 Subject: [PATCH 01/16] add clippy to the pipelines --- azure-pipelines.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 61d5c15699f..c8cb8eaacb7 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -66,6 +66,28 @@ jobs: variables: TOOLCHAIN: stable +- job: clippy + pool: + vmImage: ubuntu-16.04 + steps: + - template: ci/azure-install-rust.yml + - bash: rustup component add clippy + displayName: "Install clippy" + - bash: cargo clippy --all-targets -- -D warnings + displayName: "Check clippy (cargo)" + - bash: cd crates/cargo-test-macro && cargo clippy --all-targets -- -D warnings + displayName: "Check clippy (cargo-test-macro)" + - bash: cd crates/cargo-test-support && cargo clippy --all-targets -- -D warnings + displayName: "Check clippy (cargo-test-support)" + - bash: cd crates/crates-io && cargo clippy --all-targets -- -D warnings + displayName: "Check clippy (crates-io)" + - bash: cd crates/resolver-tests && cargo clippy --all-targets -- -D warnings + displayName: "Check clippy (resolver-tests)" + - bash: cd crates/cargo-platform && cargo clippy --all-targets -- -D warnings + displayName: "Check clippy (cargo-platform)" + variables: + TOOLCHAIN: beta + - job: resolver pool: vmImage: ubuntu-16.04 From b98673fc4292d33205e7569138812ff6f84c0784 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sat, 9 Nov 2019 11:44:07 +0200 Subject: [PATCH 02/16] remove suppression of clippy lint `too_many_arguments` --- src/bin/cargo/main.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index 1b1f1888d25..b9e7abb6857 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -1,5 +1,4 @@ #![warn(rust_2018_idioms)] // while we're getting used to 2018 -#![allow(clippy::too_many_arguments)] // large project #![allow(clippy::redundant_closure)] // there's a false positive #![warn(clippy::needless_borrow)] #![warn(clippy::redundant_clone)] From 018e8dea6e1af745e5dcb5d02aface55c7c14044 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sat, 9 Nov 2019 14:02:39 +0200 Subject: [PATCH 03/16] implement clippy suggestion to use `or_insert_with` --- src/cargo/core/resolver/encode.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 04d9ebd38b4..36937c26b08 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -570,7 +570,7 @@ impl<'a> EncodeState<'a> { for id in resolve.iter() { let slot = map .entry(id.name()) - .or_insert(HashMap::new()) + .or_insert_with(HashMap::new) .entry(id.version()) .or_insert(0); *slot += 1; From 230d35336767ef09bc2b1b120d34b2c58bb95548 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sat, 9 Nov 2019 14:03:06 +0200 Subject: [PATCH 04/16] implement clippy suggestion to use `if` as an expression --- src/cargo/core/resolver/encode.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 36937c26b08..aeea43c84f8 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -564,8 +564,7 @@ pub struct EncodeState<'a> { impl<'a> EncodeState<'a> { pub fn new(resolve: &'a Resolve) -> EncodeState<'a> { - let mut counts = None; - if *resolve.version() == ResolveVersion::V2 { + let counts = if *resolve.version() == ResolveVersion::V2 { let mut map = HashMap::new(); for id in resolve.iter() { let slot = map @@ -575,8 +574,10 @@ impl<'a> EncodeState<'a> { .or_insert(0); *slot += 1; } - counts = Some(map); - } + Some(map) + } else { + None + }; EncodeState { counts } } } From b31bf0444a7ac148a5870cf42ac349c8be6f7267 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sat, 9 Nov 2019 14:11:14 +0200 Subject: [PATCH 05/16] implement clippy suggestion to remove needless `return` --- src/cargo/util/config/de.rs | 4 ++-- src/cargo/util/config/key.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs index 4471727db0c..0824e4629d1 100644 --- a/src/cargo/util/config/de.rs +++ b/src/cargo/util/config/de.rs @@ -254,7 +254,7 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { }; let result = seed.deserialize(name.into_deserializer()).map(Some); self.next = Some(key); - return result; + result } None => Ok(None), } @@ -273,7 +273,7 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> { key: self.de.key.clone(), }); self.de.key.pop(); - return result; + result } } diff --git a/src/cargo/util/config/key.rs b/src/cargo/util/config/key.rs index 9fabe9a00fb..42c82bd2769 100644 --- a/src/cargo/util/config/key.rs +++ b/src/cargo/util/config/key.rs @@ -46,7 +46,7 @@ impl ConfigKey { for part in key.split('.') { cfg.push(part); } - return cfg; + cfg } /// Pushes a new sub-key on this `ConfigKey`. This sub-key should be From b7e17785a83a9107dba70f0fee25db180098d8a6 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sat, 9 Nov 2019 15:51:06 +0200 Subject: [PATCH 06/16] implement clippy suggestion to use `or_else` instead of `or` to avoid unintended side effects --- src/cargo/core/profiles.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index f184ee3e44a..13b2260f024 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -515,7 +515,7 @@ impl ProfileMaker { Some(ref toml) => toml, None => return Ok(()), }; - let overrides = match toml.package.as_ref().or(toml.overrides.as_ref()) { + let overrides = match toml.package.as_ref().or_else(|| toml.overrides.as_ref()) { Some(overrides) => overrides, None => return Ok(()), }; @@ -612,7 +612,7 @@ fn merge_toml_overrides( merge_profile(profile, build_override); } } - if let Some(overrides) = toml.package.as_ref().or(toml.overrides.as_ref()) { + if let Some(overrides) = toml.package.as_ref().or_else(|| toml.overrides.as_ref()) { if !is_member { if let Some(all) = overrides.get(&ProfilePackageSpec::All) { merge_profile(profile, all); From e1501f17ab0f27a03a3c12ac0b963c356373a068 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 10:23:10 +0200 Subject: [PATCH 07/16] refactor method to avoid calling `unwrap()` explicitly --- src/cargo/ops/registry.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 1d81c04c6c7..4bea42704c6 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -371,13 +371,18 @@ fn registry( let mut src = RegistrySource::remote(sid, &HashSet::new(), config); // Only update the index if the config is not available or `force` is set. let cfg = src.config(); - let cfg = if force_update || cfg.is_err() { + let mut updated_cfg = || { src.update() .chain_err(|| format!("failed to update {}", sid))?; - cfg.or_else(|_| src.config())? + src.config() + }; + + let cfg = if force_update { + updated_cfg()? } else { - cfg.unwrap() + cfg.or_else(|_| updated_cfg())? }; + cfg.and_then(|cfg| cfg.api) .ok_or_else(|| format_err!("{} does not support API commands", sid))? }; From 37ec84d09a8f090a475685787f6cc351cf2391b8 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 11:13:27 +0200 Subject: [PATCH 08/16] implement clippy suggestion to remove needless `return` --- crates/cargo-test-macro/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index b16915762a4..dae129ade66 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -52,7 +52,7 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream { )))); } - return ret; + ret } fn contains_ident(t: &TokenStream, ident: &str) -> bool { From b3fe6b7d27ab98a1d102d3276c1770b5b51b0b24 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 11:23:50 +0200 Subject: [PATCH 09/16] implement clippy suggestion to remove redundant `to_string` --- crates/cargo-test-support/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 6b26d0ed00d..ea67b611080 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -1141,9 +1141,9 @@ impl Execs { // Do the template replacements on the expected string. let matcher = match &self.process_builder { - None => matcher.to_string(), + None => matcher, Some(p) => match p.get_cwd() { - None => matcher.to_string(), + None => matcher, Some(cwd) => replace_path(&matcher, cwd, "[CWD]"), }, }; From b590183d87ae14cb88e8dfaeea253929e362fe21 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 11:29:54 +0200 Subject: [PATCH 10/16] suppress clippy lint `needless_doctest_main` --- crates/cargo-test-support/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index ea67b611080..c0780890819 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -105,6 +105,8 @@ dependency. */ +#![allow(clippy::needless_doctest_main)] // according to @ehuss this lint is fussy + use std::env; use std::ffi::OsStr; use std::fmt; From 6d21f9e08c5cfba9f21f73dd6b14d6077175a2ea Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 11:56:49 +0200 Subject: [PATCH 11/16] silence several clippy lints in tests --- crates/resolver-tests/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index c77a050758e..f7dbe52c407 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -1,3 +1,6 @@ +#![allow(clippy::many_single_char_names)] +#![allow(clippy::needless_range_loop)] // false positives + use std::cell::RefCell; use std::cmp::PartialEq; use std::cmp::{max, min}; From 60f88a4b7a72c2d30a5b669eed5ea66fcbe6d97c Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 15:14:02 +0200 Subject: [PATCH 12/16] implement clippy suggestion not to use `assert_eq` on `()` --- src/cargo/util/network.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/network.rs b/src/cargo/util/network.rs index 7637c1c2a46..74cb770a093 100644 --- a/src/cargo/util/network.rs +++ b/src/cargo/util/network.rs @@ -112,7 +112,7 @@ fn with_retry_repeats_the_call_then_works() { let config = Config::default().unwrap(); *config.shell() = Shell::from_write(Box::new(Vec::new())); let result = with_retry(&config, || results.pop().unwrap()); - assert_eq!(result.unwrap(), ()) + assert!(result.is_ok()) } #[test] @@ -135,7 +135,7 @@ fn with_retry_finds_nested_spurious_errors() { let config = Config::default().unwrap(); *config.shell() = Shell::from_write(Box::new(Vec::new())); let result = with_retry(&config, || results.pop().unwrap()); - assert_eq!(result.unwrap(), ()) + assert!(result.is_ok()) } #[test] From e8a8f764323665983b83247a907ecc8421ab5edc Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 15:21:24 +0200 Subject: [PATCH 13/16] suppress clippy lint `block_in_if_condition_stmt` in the testsuite because fixing it upsets rustfmt --- tests/testsuite/main.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 75048c5378a..f5fff0bbdef 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -3,6 +3,7 @@ #![allow(clippy::blacklisted_name)] #![allow(clippy::explicit_iter_loop)] #![allow(clippy::redundant_closure)] +#![allow(clippy::block_in_if_condition_stmt)] // clippy doesn't agree with rustfmt 😂 #![warn(clippy::needless_borrow)] #![warn(clippy::redundant_clone)] From 1c5861c8b4cf1d3a77bd9d870e6de47c489613cc Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Sun, 10 Nov 2019 15:35:29 +0200 Subject: [PATCH 14/16] implement clippy suggestion to remove redundant `clone` --- crates/resolver-tests/tests/resolve.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 2670efd609f..5fcc3a8a4f3 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -234,7 +234,7 @@ fn pub_fail() { pkg!(("e", "0.0.6") => [dep_req_kind("a", "<= 0.0.4", Kind::Normal, true),]), pkg!(("kB", "0.0.3") => [dep_req("a", ">= 0.0.5"),dep("e"),]), ]; - let reg = registry(input.clone()); + let reg = registry(input); assert!(resolve_and_validated(vec![dep("kB")], ®, None).is_err()); } From 2a49ac94d07710d38d8bb002bd0e16ef31f5c480 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Tue, 12 Nov 2019 10:44:56 +0200 Subject: [PATCH 15/16] suppress clippy lint `inefficient_to_string` - according to reviewers' preference --- crates/cargo-test-support/src/lib.rs | 1 + src/cargo/lib.rs | 1 + tests/testsuite/main.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index c0780890819..52703fba28d 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -106,6 +106,7 @@ dependency. */ #![allow(clippy::needless_doctest_main)] // according to @ehuss this lint is fussy +#![allow(clippy::inefficient_to_string)] // this causes suggestions that result in `(*s).to_string()` use std::env; use std::ffi::OsStr; diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 3adb0c1e968..0c214c3d38b 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -17,6 +17,7 @@ #![allow(clippy::type_complexity)] // there's an exceptionally complex type #![allow(clippy::wrong_self_convention)] // perhaps `Rc` should be special-cased in Clippy? #![allow(clippy::write_with_newline)] // too pedantic +#![allow(clippy::inefficient_to_string)] // this causes suggestions that result in `(*s).to_string()` #![warn(clippy::needless_borrow)] #![warn(clippy::redundant_clone)] // Unit is now interned, and would probably be better as pass-by-copy, but diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index f5fff0bbdef..280fe025653 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -4,6 +4,7 @@ #![allow(clippy::explicit_iter_loop)] #![allow(clippy::redundant_closure)] #![allow(clippy::block_in_if_condition_stmt)] // clippy doesn't agree with rustfmt 😂 +#![allow(clippy::inefficient_to_string)] // this causes suggestions that result in `(*s).to_string()` #![warn(clippy::needless_borrow)] #![warn(clippy::redundant_clone)] From 14326519003594eb75091d65f778be85331434e2 Mon Sep 17 00:00:00 2001 From: Igor Makarov Date: Fri, 15 Nov 2019 10:51:52 +0200 Subject: [PATCH 16/16] Remove clippy from pipelines - @Eh2406 asked that the commit will be kept This reverts commit 4a1207caaa6ff574f63da0f0c4c797fc3ea2cf22. --- azure-pipelines.yml | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index c8cb8eaacb7..61d5c15699f 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -66,28 +66,6 @@ jobs: variables: TOOLCHAIN: stable -- job: clippy - pool: - vmImage: ubuntu-16.04 - steps: - - template: ci/azure-install-rust.yml - - bash: rustup component add clippy - displayName: "Install clippy" - - bash: cargo clippy --all-targets -- -D warnings - displayName: "Check clippy (cargo)" - - bash: cd crates/cargo-test-macro && cargo clippy --all-targets -- -D warnings - displayName: "Check clippy (cargo-test-macro)" - - bash: cd crates/cargo-test-support && cargo clippy --all-targets -- -D warnings - displayName: "Check clippy (cargo-test-support)" - - bash: cd crates/crates-io && cargo clippy --all-targets -- -D warnings - displayName: "Check clippy (crates-io)" - - bash: cd crates/resolver-tests && cargo clippy --all-targets -- -D warnings - displayName: "Check clippy (resolver-tests)" - - bash: cd crates/cargo-platform && cargo clippy --all-targets -- -D warnings - displayName: "Check clippy (cargo-platform)" - variables: - TOOLCHAIN: beta - - job: resolver pool: vmImage: ubuntu-16.04