From cff5cff2f3a6687dfaf12b92762e70545e0abefe Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 22 May 2020 22:30:28 +0200 Subject: [PATCH 1/3] Make the name of the crate available in cargo UI tests --- clippy_dev/src/new_lint.rs | 17 ++++++++++++----- .../cargo_common_metadata/fail/src/main.rs | 1 + .../cargo_common_metadata/pass/src/main.rs | 1 + .../multiple_crate_versions/fail/src/main.rs | 1 + .../multiple_crate_versions/pass/src/main.rs | 1 + .../wildcard_dependencies/fail/src/main.rs | 1 + .../wildcard_dependencies/pass/src/main.rs | 1 + 7 files changed, 18 insertions(+), 5 deletions(-) diff --git a/clippy_dev/src/new_lint.rs b/clippy_dev/src/new_lint.rs index 08a2e0c0918f..c0b2dac2f60f 100644 --- a/clippy_dev/src/new_lint.rs +++ b/clippy_dev/src/new_lint.rs @@ -76,7 +76,8 @@ fn create_test(lint: &LintData) -> io::Result<()> { path.push("src"); fs::create_dir(&path)?; - write_file(path.join("main.rs"), get_test_file_contents(lint_name))?; + let header = format!("// compile-flags: --crate-name={}", lint_name); + write_file(path.join("main.rs"), get_test_file_contents(lint_name, Some(&header)))?; Ok(()) } @@ -90,7 +91,7 @@ fn create_test(lint: &LintData) -> io::Result<()> { create_project_layout(lint.name, &test_dir, "pass", "This file should not trigger the lint") } else { let test_path = format!("tests/ui/{}.rs", lint.name); - let test_contents = get_test_file_contents(lint.name); + let test_contents = get_test_file_contents(lint.name, None); write_file(lint.project_root.join(test_path), test_contents) } } @@ -119,8 +120,8 @@ fn to_camel_case(name: &str) -> String { .collect() } -fn get_test_file_contents(lint_name: &str) -> String { - format!( +fn get_test_file_contents(lint_name: &str, header_commands: Option<&str>) -> String { + let mut contents = format!( "#![warn(clippy::{})] fn main() {{ @@ -128,7 +129,13 @@ fn main() {{ }} ", lint_name - ) + ); + + if let Some(header) = header_commands { + contents = format!("{}\n{}", header, contents); + } + + contents } fn get_manifest_contents(lint_name: &str, hint: &str) -> String { diff --git a/tests/ui-cargo/cargo_common_metadata/fail/src/main.rs b/tests/ui-cargo/cargo_common_metadata/fail/src/main.rs index c67166fc4b00..27841e18aa9e 100644 --- a/tests/ui-cargo/cargo_common_metadata/fail/src/main.rs +++ b/tests/ui-cargo/cargo_common_metadata/fail/src/main.rs @@ -1,3 +1,4 @@ +// compile-flags: --crate-name=cargo_common_metadata #![warn(clippy::cargo_common_metadata)] fn main() {} diff --git a/tests/ui-cargo/cargo_common_metadata/pass/src/main.rs b/tests/ui-cargo/cargo_common_metadata/pass/src/main.rs index c67166fc4b00..27841e18aa9e 100644 --- a/tests/ui-cargo/cargo_common_metadata/pass/src/main.rs +++ b/tests/ui-cargo/cargo_common_metadata/pass/src/main.rs @@ -1,3 +1,4 @@ +// compile-flags: --crate-name=cargo_common_metadata #![warn(clippy::cargo_common_metadata)] fn main() {} diff --git a/tests/ui-cargo/multiple_crate_versions/fail/src/main.rs b/tests/ui-cargo/multiple_crate_versions/fail/src/main.rs index 4bc61dd62992..1b2d3ec9459f 100644 --- a/tests/ui-cargo/multiple_crate_versions/fail/src/main.rs +++ b/tests/ui-cargo/multiple_crate_versions/fail/src/main.rs @@ -1,3 +1,4 @@ +// compile-flags: --crate-name=multiple_crate_versions #![warn(clippy::multiple_crate_versions)] fn main() {} diff --git a/tests/ui-cargo/multiple_crate_versions/pass/src/main.rs b/tests/ui-cargo/multiple_crate_versions/pass/src/main.rs index 4bc61dd62992..1b2d3ec9459f 100644 --- a/tests/ui-cargo/multiple_crate_versions/pass/src/main.rs +++ b/tests/ui-cargo/multiple_crate_versions/pass/src/main.rs @@ -1,3 +1,4 @@ +// compile-flags: --crate-name=multiple_crate_versions #![warn(clippy::multiple_crate_versions)] fn main() {} diff --git a/tests/ui-cargo/wildcard_dependencies/fail/src/main.rs b/tests/ui-cargo/wildcard_dependencies/fail/src/main.rs index 3491ccb0d47d..581babfeacbf 100644 --- a/tests/ui-cargo/wildcard_dependencies/fail/src/main.rs +++ b/tests/ui-cargo/wildcard_dependencies/fail/src/main.rs @@ -1,3 +1,4 @@ +// compile-flags: --crate-name=wildcard_dependencies #![warn(clippy::wildcard_dependencies)] fn main() {} diff --git a/tests/ui-cargo/wildcard_dependencies/pass/src/main.rs b/tests/ui-cargo/wildcard_dependencies/pass/src/main.rs index 3491ccb0d47d..581babfeacbf 100644 --- a/tests/ui-cargo/wildcard_dependencies/pass/src/main.rs +++ b/tests/ui-cargo/wildcard_dependencies/pass/src/main.rs @@ -1,3 +1,4 @@ +// compile-flags: --crate-name=wildcard_dependencies #![warn(clippy::wildcard_dependencies)] fn main() {} From 8642fc97dd1a9b4f0291726c47ec97d15599d74d Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 22 May 2020 22:39:19 +0200 Subject: [PATCH 2/3] multiple_crate_versions: skip dev and build deps --- clippy_lints/src/multiple_crate_versions.rs | 59 ++++++++++++++----- .../5041_allow_dev_build/Cargo.toml | 17 ++++++ .../5041_allow_dev_build/src/main.rs | 4 ++ 3 files changed, 66 insertions(+), 14 deletions(-) create mode 100644 tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/Cargo.toml create mode 100644 tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/src/main.rs diff --git a/clippy_lints/src/multiple_crate_versions.rs b/clippy_lints/src/multiple_crate_versions.rs index c4decfc94011..b24ec897ef5a 100644 --- a/clippy_lints/src/multiple_crate_versions.rs +++ b/clippy_lints/src/multiple_crate_versions.rs @@ -1,11 +1,14 @@ //! lint on multiple versions of a crate being used use crate::utils::{run_lints, span_lint}; +use rustc_hir::def_id::LOCAL_CRATE; use rustc_hir::{Crate, CRATE_HIR_ID}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::DUMMY_SP; +use cargo_metadata::{DependencyKind, MetadataCommand, Node, Package, PackageId}; +use if_chain::if_chain; use itertools::Itertools; declare_clippy_lint! { @@ -34,37 +37,65 @@ declare_clippy_lint! { declare_lint_pass!(MultipleCrateVersions => [MULTIPLE_CRATE_VERSIONS]); impl LateLintPass<'_, '_> for MultipleCrateVersions { + #[allow(clippy::find_map)] fn check_crate(&mut self, cx: &LateContext<'_, '_>, _: &Crate<'_>) { if !run_lints(cx, &[MULTIPLE_CRATE_VERSIONS], CRATE_HIR_ID) { return; } - let metadata = if let Ok(metadata) = cargo_metadata::MetadataCommand::new().exec() { + let metadata = if let Ok(metadata) = MetadataCommand::new().exec() { metadata } else { span_lint(cx, MULTIPLE_CRATE_VERSIONS, DUMMY_SP, "could not read cargo metadata"); - return; }; + let local_name = cx.tcx.crate_name(LOCAL_CRATE).as_str(); let mut packages = metadata.packages; packages.sort_by(|a, b| a.name.cmp(&b.name)); - for (name, group) in &packages.into_iter().group_by(|p| p.name.clone()) { - let group: Vec = group.collect(); + if_chain! { + if let Some(resolve) = &metadata.resolve; + if let Some(local_id) = packages.iter().find(|p| p.name == *local_name).map(|p| &p.id); + then { + for (name, group) in &packages.iter().group_by(|p| p.name.clone()) { + let group: Vec<&Package> = group.collect(); + + if group.len() <= 1 { + continue; + } - if group.len() > 1 { - let mut versions: Vec<_> = group.into_iter().map(|p| p.version).collect(); - versions.sort(); - let versions = versions.iter().join(", "); + if group.iter().all(|p| is_normal_dep(&resolve.nodes, local_id, &p.id)) { + let mut versions: Vec<_> = group.into_iter().map(|p| &p.version).collect(); + versions.sort(); + let versions = versions.iter().join(", "); - span_lint( - cx, - MULTIPLE_CRATE_VERSIONS, - DUMMY_SP, - &format!("multiple versions for dependency `{}`: {}", name, versions), - ); + span_lint( + cx, + MULTIPLE_CRATE_VERSIONS, + DUMMY_SP, + &format!("multiple versions for dependency `{}`: {}", name, versions), + ); + } + } } } } } + +fn is_normal_dep(nodes: &[Node], local_id: &PackageId, dep_id: &PackageId) -> bool { + fn depends_on(node: &Node, dep_id: &PackageId) -> bool { + node.deps.iter().any(|dep| { + dep.pkg == *dep_id + && dep + .dep_kinds + .iter() + .any(|info| matches!(info.kind, DependencyKind::Normal)) + }) + } + + nodes + .iter() + .filter(|node| depends_on(node, dep_id)) + .any(|node| node.id == *local_id || is_normal_dep(nodes, local_id, &node.id)) +} diff --git a/tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/Cargo.toml b/tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/Cargo.toml new file mode 100644 index 000000000000..72731fbc75d0 --- /dev/null +++ b/tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/Cargo.toml @@ -0,0 +1,17 @@ +# Should not lint for dev or build dependencies. See issue 5041. + +[package] +name = "multiple_crate_versions" +version = "0.1.0" +publish = false + +# One of the versions of winapi is only a dev dependency: allowed +[dependencies] +ctrlc = "=3.1.0" +[dev-dependencies] +ansi_term = "=0.11.0" + +# Both versions of winapi are a build dependency: allowed +[build-dependencies] +ctrlc = "=3.1.0" +ansi_term = "=0.11.0" diff --git a/tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/src/main.rs b/tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/src/main.rs new file mode 100644 index 000000000000..1b2d3ec9459f --- /dev/null +++ b/tests/ui-cargo/multiple_crate_versions/5041_allow_dev_build/src/main.rs @@ -0,0 +1,4 @@ +// compile-flags: --crate-name=multiple_crate_versions +#![warn(clippy::multiple_crate_versions)] + +fn main() {} From ec0a00e53980619a6313ff4f01099a1aebcfd9e6 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sun, 24 May 2020 21:17:54 +0200 Subject: [PATCH 3/3] Use find_map instead of find() + map() --- clippy_lints/src/multiple_crate_versions.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/multiple_crate_versions.rs b/clippy_lints/src/multiple_crate_versions.rs index b24ec897ef5a..b6770804e180 100644 --- a/clippy_lints/src/multiple_crate_versions.rs +++ b/clippy_lints/src/multiple_crate_versions.rs @@ -37,7 +37,6 @@ declare_clippy_lint! { declare_lint_pass!(MultipleCrateVersions => [MULTIPLE_CRATE_VERSIONS]); impl LateLintPass<'_, '_> for MultipleCrateVersions { - #[allow(clippy::find_map)] fn check_crate(&mut self, cx: &LateContext<'_, '_>, _: &Crate<'_>) { if !run_lints(cx, &[MULTIPLE_CRATE_VERSIONS], CRATE_HIR_ID) { return; @@ -56,7 +55,9 @@ impl LateLintPass<'_, '_> for MultipleCrateVersions { if_chain! { if let Some(resolve) = &metadata.resolve; - if let Some(local_id) = packages.iter().find(|p| p.name == *local_name).map(|p| &p.id); + if let Some(local_id) = packages + .iter() + .find_map(|p| if p.name == *local_name { Some(&p.id) } else { None }); then { for (name, group) in &packages.iter().group_by(|p| p.name.clone()) { let group: Vec<&Package> = group.collect();