Repository: mozilla/cargo-vet
Pull request #224 (2022-06-23 20:04:08)

remove useless_unaudited warning infra, it's buggy, ineffective, and divergent


Patch:
diff --git a/src/resolver.rs b/src/resolver.rs
index 750afae..4296f62 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -115,8 +115,6 @@ pub struct Success {
     pub vetted_partially: Vec<PackageIdx>,
     /// Third-party packages that were successfully vetted using only 'audits'
     pub vetted_fully: Vec<PackageIdx>,
-    /// Third-party packages that have an unaudited entry that can be removed.
-    pub useless_unaudited: Vec<PackageIdx>,
 }
 
 #[derive(Debug, Clone)]
@@ -1325,7 +1323,6 @@ pub fn resolve<'a>(
     let mut vetted_with_unaudited = vec![];
     let mut vetted_partially = vec![];
     let mut vetted_fully = vec![];
-    let mut useless_unaudited = vec![];
     for &pkgidx in &graph.topo_index {
         let package = &graph.nodes[pkgidx];
         if !package.is_third_party {
@@ -1341,10 +1338,6 @@ pub fn resolve<'a>(
         } else {
             vetted_partially.push(pkgidx);
         }
-
-        if result.directly_unaudited && !result.needed_unaudited {
-            useless_unaudited.push(pkgidx);
-        }
     }
 
     ResolveReport {
@@ -1355,7 +1348,6 @@ pub fn resolve<'a>(
             vetted_with_unaudited,
             vetted_partially,
             vetted_fully,
-            useless_unaudited,
         }),
     }
 }
@@ -2362,13 +2354,7 @@ impl<'a> ResolveReport<'a> {
     }
 
     pub fn _has_warnings(&self) -> bool {
-        if let Conclusion::Success(success) = &self.conclusion {
-            // If there are useless_unaudited, we'll warn about those
-            !success.useless_unaudited.is_empty()
-        } else {
-            // Errors aren't warnings
-            false
-        }
+        false
     }
 
     pub fn compute_suggest(
@@ -2663,7 +2649,6 @@ impl<'a> ResolveReport<'a> {
                     "vetted_fully": success.vetted_fully.iter().map(json_package).collect::<Vec<_>>(),
                     "vetted_partially": success.vetted_partially.iter().map(json_package).collect::<Vec<_>>(),
                     "vetted_with_unaudited": success.vetted_with_unaudited.iter().map(json_package).collect::<Vec<_>>(),
-                    "useless_unaudited": success.useless_unaudited.iter().map(json_package).collect::<Vec<_>>(),
                 })
             }
             Conclusion::FailForViolationConflict(fail) => json!({
@@ -2714,7 +2699,7 @@ impl Success {
     pub fn print_human(
         &self,
         out: &mut dyn Out,
-        report: &ResolveReport,
+        _report: &ResolveReport,
         _cfg: &Config,
     ) -> Result<(), std::io::Error> {
         let fully_audited_count = self.vetted_fully.len();
@@ -2759,19 +2744,6 @@ impl Success {
 
             writeln!(out, ")")?;
         }
-
-        // Warn about useless unaudited entries
-        if !self.useless_unaudited.is_empty() {
-            writeln!(
-                out,
-                "  warning: some dependencies are listed in unaudited, but didn't need it:"
-            )?;
-            for &pkgidx in &self.useless_unaudited {
-                let package = &report.graph.nodes[pkgidx];
-                writeln!(out, "    {}:{}", package.name, package.version)?;
-            }
-        }
-
         Ok(())
     }
 }


Test patch:
diff --git a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap
index 778ec53..ccf7683 100644
--- a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap
+++ b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap
@@ -3,6 +3,4 @@ source: src/tests/vet.rs
 expression: output
 ---
 Vetting Succeeded (1 fully audited, 3 unaudited)
-  warning: some dependencies are listed in unaudited, but didn't need it:
-    root-package:10.0.0
 
diff --git a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap
index 778ec53..ccf7683 100644
--- a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap
+++ b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap
@@ -3,6 +3,4 @@ source: src/tests/vet.rs
 expression: output
 ---
 Vetting Succeeded (1 fully audited, 3 unaudited)
-  warning: some dependencies are listed in unaudited, but didn't need it:
-    root-package:10.0.0
 
diff --git a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-unaudited-in-direct-full.snap b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-unaudited-in-direct-full.snap
index e88c964..f229132 100644
--- a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-unaudited-in-direct-full.snap
+++ b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-unaudited-in-direct-full.snap
@@ -3,6 +3,4 @@ source: src/tests/vet.rs
 expression: output
 ---
 Vetting Succeeded (3 fully audited)
-  warning: some dependencies are listed in unaudited, but didn't need it:
-    third-party1:10.0.0
 
diff --git a/src/tests/vet.rs b/src/tests/vet.rs
index d776514..fbf3860 100644
--- a/src/tests/vet.rs
+++ b/src/tests/vet.rs
@@ -1584,8 +1584,8 @@ fn builtin_dev_detection_empty_deeper() {
 
 #[test]
 fn builtin_simple_unaudited_extra() {
-    // (Warn) there's an extra unused unaudited entry, but the other is needed
-    // BUSTED: this test is broken (doesn't emit warning)
+    // (Pass) there's an extra unused unaudited entry, but the other is needed
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
@@ -1612,8 +1612,8 @@ fn builtin_simple_unaudited_extra() {
 
 #[test]
 fn builtin_simple_unaudited_not_a_real_dep() {
-    // (Warn) there's an unaudited entry for a package that isn't in our tree at all.
-    // BUSTED: this test is broken (doesn't emit warning)
+    // (Pass) there's an unaudited entry for a package that isn't in our tree at all.
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
@@ -1635,8 +1635,8 @@ fn builtin_simple_unaudited_not_a_real_dep() {
 
 #[test]
 fn builtin_simple_deps_unaudited_overbroad() {
-    // (Warn) the unaudited entry is needed but it's overbroad
-    // BUSTED: this test is broken (doesn't emit warning)
+    // (Pass) the unaudited entry is needed but it's overbroad
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple_deps();
@@ -1714,8 +1714,8 @@ fn builtin_complex_unaudited_partial_twins() {
 
 #[test]
 fn builtin_simple_unaudited_in_delta() {
-    // (Warn) An audited entry overlaps a delta and isn't needed
-    // BUSTED: this test is broken (doesn't emit warning)
+    // (Pass) An audited entry overlaps a delta and isn't needed
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
@@ -1746,8 +1746,8 @@ fn builtin_simple_unaudited_in_delta() {
 
 #[test]
 fn builtin_simple_unaudited_in_full() {
-    // (Warn) An audited entry overlaps a full audit and isn't needed
-    // BUSTED: this test is broken (doesn't emit warning)
+    // (Pass) An audited entry overlaps a full audit and isn't needed
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
@@ -1778,7 +1778,8 @@ fn builtin_simple_unaudited_in_full() {
 
 #[test]
 fn builtin_simple_unaudited_in_direct_full() {
-    // (Warn) An audited entry overlaps a full audit which is the cur version and isn't needed
+    // (Pass) An audited entry overlaps a full audit which is the cur version and isn't needed
+    // (This test used to warn when we tried to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
@@ -1975,8 +1976,8 @@ fn builtin_simple_unaudited_nested_stronger_req() {
 
 #[test]
 fn builtin_simple_deps_unaudited_adds_uneeded_criteria() {
-    // (Warn) An audited entry overlaps a full audit which is the cur version and isn't needed
-    // BUSTED: this test is broken (doesn't emit warning)
+    // (Pass) An audited entry overlaps a full audit which is the cur version and isn't needed
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple_deps();
@@ -2008,9 +2009,8 @@ fn builtin_simple_deps_unaudited_adds_uneeded_criteria() {
 
 #[test]
 fn builtin_dev_detection_unaudited_adds_uneeded_criteria_indirect() {
-    // (Warn) An audited entry overlaps a full audit which is the cur version and isn't needed
-    // BUSTED: this test is broken (doesn't emit warning)
-    // TODO: or is this test wrong? should the delta apply?
+    // (Pass) An audited entry overlaps a full audit which is the cur version and isn't needed
+    // (This test could warn if we try to detect "useless unaudited" eagerly)
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::dev_detection();
@@ -2128,8 +2128,8 @@ fn builtin_simple_delta_cycle() {
 
 #[test]
 fn builtin_simple_noop_delta() {
-    // (Warn) completely pointless noop delta
-    // BUSTED: fails to warn about a 5->5 delta
+    // (Pass) completely pointless noop delta
+    // (This test could warn if we try to detect "useless deltas")
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
diff --git a/tests/snapshots/test_cli__test-project-json.snap b/tests/snapshots/test_cli__test-project-json.snap
index 94d1b71..c113983 100644
--- a/tests/snapshots/test_cli__test-project-json.snap
+++ b/tests/snapshots/test_cli__test-project-json.snap
@@ -5,7 +5,6 @@ expression: format_outputs(&output)
 stdout:
 {
   "conclusion": "success",
-  "useless_unaudited": [],
   "vetted_fully": [
     {
       "name": "atty",