From cdec1d01dfee60d1a932ecd16e20ad006de9ec43 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 28 Nov 2024 14:14:09 +0100 Subject: [PATCH] Add unnecessary-skip diagnostic --- src/bans.rs | 62 ++++- src/bans/diags.rs | 21 ++ src/bans/graph.rs | 15 +- .../cargo_deny__diag__test__codes_unique.snap | 2 + tests/bans.rs | 27 ++ .../bans__unused_skips_generate_warnings.snap | 241 ++++++++++++++++++ 6 files changed, 350 insertions(+), 18 deletions(-) create mode 100644 tests/snapshots/bans__unused_skips_generate_warnings.snap diff --git a/src/bans.rs b/src/bans.rs index b3e1cfdf..f1d749f4 100644 --- a/src/bans.rs +++ b/src/bans.rs @@ -314,12 +314,16 @@ pub fn check( struct MultiDetector<'a> { name: &'a str, - dupes: smallvec::SmallVec<[usize; 2]>, + dupes: smallvec::SmallVec<[(usize, bool); 4]>, + // Keep track of the crates that actually have > 1 version, regardless of skips + // if a skip is encountered for a krate that only has 1 version, warn about it + krates_with_dupes: Vec<&'a str>, } let mut multi_detector = MultiDetector { name: &ctx.krates.krates().next().unwrap().name, dupes: smallvec::SmallVec::new(), + krates_with_dupes: Vec::new(), }; let filtered_krates = if !multiple_versions_include_dev { @@ -376,12 +380,25 @@ pub fn check( .collect(), ); - let report_duplicates = |multi_detector: &MultiDetector<'_>, sink: &mut diag::ErrorSink| { - if multi_detector.dupes.len() <= 1 { + let report_duplicates = |multi_detector: &mut MultiDetector<'_>, sink: &mut diag::ErrorSink| { + let skipped = multi_detector + .dupes + .iter() + .filter(|(_, skipped)| *skipped) + .count(); + if multi_detector.dupes.len() > 1 { + multi_detector.krates_with_dupes.push(multi_detector.name); + } + + if multi_detector.dupes.len() - skipped <= 1 { return; } - let lint_level = if multi_detector.dupes.iter().any(|kindex| { + let lint_level = if multi_detector.dupes.iter().any(|(kindex, skipped)| { + if *skipped { + return false; + } + let krate = &ctx.krates[*kindex]; dmv.matches(krate).is_some() }) { @@ -408,7 +425,11 @@ pub fn check( let mut kids = smallvec::SmallVec::<[Dupe; 2]>::new(); - for dup in multi_detector.dupes.iter().cloned() { + for dup in multi_detector + .dupes + .iter() + .filter_map(|(ind, skipped)| (!*skipped).then_some(*ind)) + { let krate = &ctx.krates[dup]; let span = &ctx.krate_spans.lock_span(&krate.id).total; @@ -850,6 +871,15 @@ pub fn check( if should_add_dupe(&krate.id) { if let Some(matches) = skipped.matches(krate) { + if multi_detector.name != krate.name { + report_duplicates(&mut multi_detector, &mut sink); + + multi_detector.name = &krate.name; + multi_detector.dupes.clear(); + } + + multi_detector.dupes.push((i, true)); + for rm in matches { pack.push(diags::Skipped { krate, @@ -863,13 +893,13 @@ pub fn check( } } else if !tree_skipper.matches(krate, &mut pack) { if multi_detector.name != krate.name { - report_duplicates(&multi_detector, &mut sink); + report_duplicates(&mut multi_detector, &mut sink); multi_detector.name = &krate.name; multi_detector.dupes.clear(); } - multi_detector.dupes.push(i); + multi_detector.dupes.push((i, false)); 'wildcards: { if wildcards != LintLevel::Allow && !krate.is_git_source() { @@ -958,7 +988,7 @@ pub fn check( } if i == last { - report_duplicates(&multi_detector, &mut sink); + report_duplicates(&mut multi_detector, &mut sink); } tx.push(i, krate, pack); @@ -1050,12 +1080,16 @@ pub fn check( let mut pack = Pack::new(Check::Bans); - for skip in skip_hit - .into_iter() - .zip(skipped.0.into_iter()) - .filter_map(|(hit, skip)| (!hit).then_some(skip)) - { - pack.push(diags::UnmatchedSkip { skip_cfg: &skip }); + for (hit, skip) in skip_hit.into_iter().zip(skipped.0.into_iter()) { + if !hit { + pack.push(diags::UnmatchedSkip { skip_cfg: &skip }); + } else if multi_detector + .krates_with_dupes + .binary_search(&skip.spec.name.value.as_str()) + .is_err() + { + pack.push(diags::UnnecessarySkip { skip_cfg: &skip }); + } } for wrapper in ban_wrappers diff --git a/src/bans/diags.rs b/src/bans/diags.rs index 984abdf0..70a3cfa7 100644 --- a/src/bans/diags.rs +++ b/src/bans/diags.rs @@ -28,6 +28,7 @@ pub enum Code { Skipped, Wildcard, UnmatchedSkip, + UnnecessarySkip, AllowedByWrapper, UnmatchedWrapper, SkippedByRoot, @@ -222,6 +223,26 @@ impl<'a> From> for Diag { } } +pub(crate) struct UnnecessarySkip<'a> { + pub(crate) skip_cfg: &'a SpecAndReason, +} + +impl<'a> From> for Diag { + fn from(us: UnnecessarySkip<'a>) -> Self { + Diagnostic::new(Severity::Warning) + .with_message(format!( + "skip '{}' applied to a crate with only one version", + us.skip_cfg.spec, + )) + .with_code(Code::UnnecessarySkip) + .with_labels( + us.skip_cfg + .to_labels(Some("unnecessary skip configuration")), + ) + .into() + } +} + pub(crate) struct UnusedWrapper { pub(crate) wrapper_cfg: CfgCoord, } diff --git a/src/bans/graph.rs b/src/bans/graph.rs index 9ada5068..4530c667 100644 --- a/src/bans/graph.rs +++ b/src/bans/graph.rs @@ -88,7 +88,7 @@ pub(crate) fn create_graph( dup_name: &str, highlight: GraphHighlight, krates: &crate::Krates, - dup_ids: &[usize], + dup_ids: &[(usize, bool)], ) -> Result { use pg::visit::{EdgeRef, NodeRef}; @@ -97,16 +97,23 @@ pub(crate) fn create_graph( let mut node_stack = Vec::with_capacity(dup_ids.len()); - let duplicates: Vec<_> = dup_ids.iter().map(|di| krates[*di].id.clone()).collect(); + let duplicates: Vec<_> = dup_ids + .iter() + .filter_map(|(di, skipped)| (!*skipped).then_some(krates[*di].id.clone())) + .collect(); - for (index, dupid) in dup_ids.iter().zip(duplicates.iter()) { + for (index, dupid) in dup_ids + .iter() + .filter_map(|(index, skipped)| (!*skipped).then_some(*index)) + .zip(duplicates.iter()) + { let dn = DupNode { kid: dupid, feature: None, }; let nid = graph.add_node(dn); node_map.insert(dn, nid); - node_stack.push((krates::NodeId::new(*index), nid)); + node_stack.push((krates::NodeId::new(index), nid)); } { diff --git a/src/snapshots/cargo_deny__diag__test__codes_unique.snap b/src/snapshots/cargo_deny__diag__test__codes_unique.snap index ec0cc7fb..9f1bf64b 100644 --- a/src/snapshots/cargo_deny__diag__test__codes_unique.snap +++ b/src/snapshots/cargo_deny__diag__test__codes_unique.snap @@ -1,6 +1,7 @@ --- source: src/diag.rs expression: unique +snapshot_kind: text --- { "accepted", @@ -52,6 +53,7 @@ expression: unique "unmatched-skip-root", "unmatched-source", "unmatched-wrapper", + "unnecessary-skip", "unresolved-workspace-dependency", "unsound", "unused-workspace-dependency", diff --git a/tests/bans.rs b/tests/bans.rs index b8d9d762..e8115bd8 100644 --- a/tests/bans.rs +++ b/tests/bans.rs @@ -327,3 +327,30 @@ unused = 'warn' insta::assert_json_snapshot!(diags); } + +/// Ensures skips generate warnings if they aren't needed +#[test] +fn unused_skips_generate_warnings() { + let diags = gather_bans( + func_name!(), + KrateGather { + name: "workspace", + no_default_features: true, + targets: &["x86_64-unknown-linux-gnu", "x86_64-pc-windows-msvc"], + ..Default::default() + }, + r#" +multiple-versions = 'deny' +skip = [ + # This actually has 3 versions, skip the two lower ones + 'spdx:<0.10.0', + # This crate, but not exact version, is in the graph + 'smallvec@1.0.0', + # This crate is in the graph, but there is only one version + 'serde_json', +] +"#, + ); + + insta::assert_json_snapshot!(diags); +} diff --git a/tests/snapshots/bans__unused_skips_generate_warnings.snap b/tests/snapshots/bans__unused_skips_generate_warnings.snap new file mode 100644 index 00000000..b9bc8a56 --- /dev/null +++ b/tests/snapshots/bans__unused_skips_generate_warnings.snap @@ -0,0 +1,241 @@ +--- +source: tests/bans.rs +expression: diags +snapshot_kind: text +--- +[ + { + "fields": { + "code": "skipped", + "graphs": [ + { + "Krate": { + "name": "serde_json", + "version": "1.0.118" + }, + "parents": [ + { + "Krate": { + "name": "cargo_metadata", + "version": "0.18.1" + }, + "parents": [ + { + "Krate": { + "name": "krates", + "version": "0.16.6" + }, + "parents": [ + { + "Krate": { + "name": "wildcards-test-allow-git", + "version": "0.1.0" + }, + "parents": [ + { + "Krate": { + "name": "member-one", + "version": "0.1.0" + }, + "parents": [ + { + "Krate": { + "kind": "build", + "name": "member-two", + "version": "0.1.0" + }, + "parents": [ + { + "Krate": { + "name": "root", + "version": "0.1.0" + } + } + ] + }, + { + "Krate": { + "name": "root", + "version": "0.1.0" + }, + "repeat": true + } + ] + }, + { + "Krate": { + "kind": "build", + "name": "member-two", + "version": "0.1.0" + }, + "repeat": true + } + ] + } + ] + } + ] + } + ] + } + ], + "labels": [ + { + "column": 6, + "line": 9, + "message": "skipped here", + "span": "serde_json" + } + ], + "message": "crate 'serde_json = 1.0.118' skipped when checking for duplicates", + "severity": "note" + }, + "type": "diagnostic" + }, + { + "fields": { + "code": "skipped", + "graphs": [ + { + "Krate": { + "name": "spdx", + "version": "0.6.0" + }, + "parents": [ + { + "Krate": { + "kind": "build", + "name": "member-one", + "version": "0.1.0" + }, + "parents": [ + { + "Krate": { + "kind": "build", + "name": "member-two", + "version": "0.1.0" + }, + "parents": [ + { + "Krate": { + "name": "root", + "version": "0.1.0" + } + } + ] + }, + { + "Krate": { + "name": "root", + "version": "0.1.0" + }, + "repeat": true + } + ] + }, + { + "Krate": { + "name": "member-two", + "version": "0.1.0" + }, + "repeat": true + } + ] + } + ], + "labels": [ + { + "column": 6, + "line": 5, + "message": "skipped here", + "span": "spdx:<0.10.0" + } + ], + "message": "crate 'spdx = 0.6.0' skipped when checking for duplicates", + "severity": "note" + }, + "type": "diagnostic" + }, + { + "fields": { + "code": "skipped", + "graphs": [ + { + "Krate": { + "name": "spdx", + "version": "0.9.0" + }, + "parents": [ + { + "Krate": { + "kind": "build", + "name": "member-two", + "version": "0.1.0" + }, + "parents": [ + { + "Krate": { + "name": "root", + "version": "0.1.0" + } + } + ] + }, + { + "Krate": { + "name": "root", + "version": "0.1.0" + }, + "repeat": true + } + ] + } + ], + "labels": [ + { + "column": 6, + "line": 5, + "message": "skipped here", + "span": "spdx:<0.10.0" + } + ], + "message": "crate 'spdx = 0.9.0' skipped when checking for duplicates", + "severity": "note" + }, + "type": "diagnostic" + }, + { + "fields": { + "code": "unmatched-skip", + "graphs": [], + "labels": [ + { + "column": 6, + "line": 7, + "message": "unmatched skip configuration", + "span": "smallvec@1.0.0" + } + ], + "message": "skipped crate 'smallvec = =1.0.0' was not encountered", + "severity": "warning" + }, + "type": "diagnostic" + }, + { + "fields": { + "code": "unnecessary-skip", + "graphs": [], + "labels": [ + { + "column": 6, + "line": 9, + "message": "unnecessary skip configuration", + "span": "serde_json" + } + ], + "message": "skip 'serde_json' applied to a crate with only one version", + "severity": "warning" + }, + "type": "diagnostic" + } +]