Improve workspace dependencies lints (#685)

This moves `workspace-duplicates` and `unused-workspace-dependencies` to
be in the `[bans.workspace-dependencies]` section, and adds an
additional `include-path-dependencies` field (defaults to true) that
determines if path dependencies in workspace members are checked for
using `workspace = true`. Since the whole section is now opt-in, both
`duplicates` and `unused` are `deny` by default.

Resolves: #682

docs/src/checks/bans/cfg.md

@@ -38,21 +38,36 @@ If specified, alters how the `wildcard` field behaves:
 
 Being limited to private crates is due to crates.io not allowing packages to be published with `path` or `git` dependencies except for `dev-dependencies`.
 
-### The `workspace-duplicates` field (optional)
+### The `workspace-dependencies` field (optional)
 
-Determines what happens when a more than 1 direct workspace dependency is resolved to the same crate and 1 or more declarations do not use `workspace = true`
+Used to configure how [`[workspace.dependencies]`](https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table) are treated.
 
-* `deny` - Will emit an error for each dependency declaration that does not use `workspace = true`
-* `warn` (default) - Will emit a warning for each dependency declaration that does not use `workspace = true`, but does not fail the check.
-* `allow` - Ignores checking for `workspace = true`
+```ini
+[bans.workspace-dependencies]
+duplicates = 'deny'
+include-path-dependencies = true
+unused = 'deny'
+```
+
+#### The `duplicates` field (optional)
+
+Determines what happens when more than 1 direct workspace dependency is resolved to the same crate and 1 or more declarations do not use `workspace = true`
+
+* `deny` (default) - Will emit an error for each dependency declaration that does not use `workspace = true`
+* `warn` - Will emit a warning for each dependency declaration that does not use `workspace = true`, but does not fail the check.
+* `allow` - Ignores checking for `workspace = true` for dependencies in workspace crates
+
+#### The `include-path-dependencies` field (optional)
+
+If true, path dependencies will be included in the duplication check, otherwise they are completely ignored.
 
-### The `unused-workspace-dependencies` field (optional)
+#### The `unused` field (optional)
 
 Determines what happens when a dependency in [`[workspace.dependencies]`](https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table) is not used in the workspace.
 
-* `deny` - Will emit an error for each dependency that is not actually used in the workspace.
+* `deny` (default) - Will emit an error for each dependency that is not actually used in the workspace.
 * `warn` - Will emit a warning for each dependency that is not actually used in the workspace, but does not fail the check.
-* `allow` - (default) Ignores checking for unused workspace dependencies.
+* `allow` - Ignores checking for unused workspace dependencies.
 
 ### The `highlight` field (optional)
 

src/bans.rs

@@ -205,8 +205,7 @@ pub fn check(
         skipped,
         multiple_versions,
         multiple_versions_include_dev,
-        workspace_duplicates,
-        unused_workspace_dependencies,
+        workspace_dependencies,
         highlight,
         tree_skipped,
         wildcards,
@@ -994,15 +993,17 @@ pub fn check(
 
         // Check the workspace to detect dependencies that are used more than once
         // but don't use a shared [workspace.[dev-/build-]dependencies] declaration
-        if workspace_duplicates != LintLevel::Allow {
-            scope.spawn(|_| {
-                check_workspace_duplicates(
-                    ctx.krates,
-                    ctx.krate_spans,
-                    workspace_duplicates,
-                    &mut ws_duplicate_packs,
-                );
-            });
+        if let Some(ws_deps) = &workspace_dependencies {
+            if ws_deps.duplicates != LintLevel::Allow {
+                scope.spawn(|_| {
+                    check_workspace_duplicates(
+                        ctx.krates,
+                        ctx.krate_spans,
+                        ws_deps,
+                        &mut ws_duplicate_packs,
+                    );
+                });
+            }
         }
     });
 
@@ -1032,16 +1033,18 @@ pub fn check(
         sink.push(pack);
     }
 
-    if unused_workspace_dependencies != LintLevel::Allow {
-        if let Some(id) = krate_spans
-            .workspace_id
-            .filter(|_id| !krate_spans.unused_workspace_deps.is_empty())
-        {
-            sink.push(diags::UnusedWorkspaceDependencies {
-                id,
-                unused: &krate_spans.unused_workspace_deps,
-                level: unused_workspace_dependencies,
-            });
+    if let Some(ws_deps) = workspace_dependencies {
+        if ws_deps.unused != LintLevel::Allow {
+            if let Some(id) = krate_spans
+                .workspace_id
+                .filter(|_id| !krate_spans.unused_workspace_deps.is_empty())
+            {
+                sink.push(diags::UnusedWorkspaceDependencies {
+                    id,
+                    unused: &krate_spans.unused_workspace_deps,
+                    level: ws_deps.unused,
+                });
+            }
         }
     }
 
@@ -1561,7 +1564,7 @@ fn validate_file_checksum(path: &crate::Path, expected: &cfg::Checksum) -> anyho
 fn check_workspace_duplicates(
     krates: &Krates,
     krate_spans: &crate::diag::KrateSpans<'_>,
-    severity: LintLevel,
+    cfg: &cfg::WorkspaceDepsConfig,
     diags: &mut Vec<Pack>,
 ) {
     use crate::diag::Label;
@@ -1592,6 +1595,10 @@ fn check_workspace_duplicates(
         };
 
         for mdep in man.deps(true) {
+            if mdep.dep.path.is_some() && !cfg.include_path_dependencies {
+                continue;
+            }
+
             deps.entry(&mdep.krate.id)
                 .or_default()
                 .push((mdep, krate, man.id));
@@ -1612,7 +1619,7 @@ fn check_workspace_duplicates(
             .workspace_span(kid)
             .zip(krate_spans.workspace_id)
         {
-            labels.push(Label::primary(ws_id, ws_span.key).with_message(format!(
+            labels.push(Label::secondary(ws_id, ws_span.key).with_message(format!(
                 "{}workspace dependency",
                 if ws_span.patched.is_some() {
                     "patched "
@@ -1649,7 +1656,7 @@ fn check_workspace_duplicates(
                 continue;
             }
 
-            labels.push(Label::secondary(id, mdep.key_span));
+            labels.push(Label::primary(id, mdep.key_span));
 
             if let Some(rename) = &mdep.rename {
                 labels.push(
@@ -1680,7 +1687,7 @@ fn check_workspace_duplicates(
         pack.push(diags::WorkspaceDuplicate {
             duplicate,
             labels,
-            severity,
+            severity: cfg.duplicates,
             has_workspace_declaration,
             total_uses: total,
         });

src/bans/cfg.rs

@@ -329,15 +329,40 @@ pub type CrateAllow = PackageSpecOrExtended<Reason>;
 pub type CrateSkip = PackageSpecOrExtended<Reason>;
 pub type TreeSkip = PackageSpecOrExtended<TreeSkipExtended>;
 
+#[cfg_attr(test, derive(serde::Serialize))]
+pub struct WorkspaceDepsConfig {
+    /// How to handle workspace dependencies on the same crate that aren't declared
+    /// in `[workspace.dependencies]`
+    pub duplicates: LintLevel,
+    /// Whether path dependencies are treated as duplicates
+    pub include_path_dependencies: bool,
+    /// How to handle [`workspace.dependencies`] that are not used
+    pub unused: LintLevel,
+}
+
+impl<'de> Deserialize<'de> for WorkspaceDepsConfig {
+    fn deserialize(value: &mut Value<'de>) -> Result<Self, DeserError> {
+        let mut th = TableHelper::new(value)?;
+
+        let duplicates = th.optional("duplicates").unwrap_or(LintLevel::Deny);
+        let include_path_dependencies = th.optional("include-path-dependencies").unwrap_or(true);
+        let unused = th.optional("unused").unwrap_or(LintLevel::Deny);
+
+        th.finalize(None)?;
+
+        Ok(Self {
+            duplicates,
+            include_path_dependencies,
+            unused,
+        })
+    }
+}
+
 pub struct Config {
     /// How to handle multiple versions of the same crate
     pub multiple_versions: LintLevel,
     pub multiple_versions_include_dev: bool,
-    /// How to handle workspace dependencies on the same crate that aren't declared
-    /// in `[workspace.dependencies]`
-    pub workspace_duplicates: LintLevel,
-    /// How to handle [`workspace.dependencies`] that are not used
-    pub unused_workspace_dependencies: LintLevel,
+    pub workspace_dependencies: Option<WorkspaceDepsConfig>,
     /// How the duplicate graphs are highlighted
     pub highlight: GraphHighlight,
     /// The crates that will cause us to emit failures
@@ -378,8 +403,7 @@ impl Default for Config {
         Self {
             multiple_versions: LintLevel::Warn,
             multiple_versions_include_dev: false,
-            workspace_duplicates: LintLevel::Warn,
-            unused_workspace_dependencies: LintLevel::Allow,
+            workspace_dependencies: None,
             highlight: GraphHighlight::All,
             deny: Vec::new(),
             allow: Vec::new(),
@@ -404,12 +428,6 @@ impl<'de> Deserialize<'de> for Config {
         let multiple_versions_include_dev = th
             .optional("multiple-versions-include-dev")
             .unwrap_or_default();
-        let workspace_duplicates = th
-            .optional("workspace-duplicates")
-            .unwrap_or(LintLevel::Warn);
-        let unused_workspace_dependencies = th
-            .optional("unused-workspace-dependencies")
-            .unwrap_or(LintLevel::Allow);
         let highlight = th.optional("highlight").unwrap_or_default();
         let deny = th.optional("deny").unwrap_or_default();
         let allow = th.optional("allow").unwrap_or_default();
@@ -423,13 +441,14 @@ impl<'de> Deserialize<'de> for Config {
         let allow_build_scripts = th.optional("allow-build-scripts");
         let build = th.optional("build");
 
+        let workspace_dependencies = th.optional("workspace-dependencies");
+
         th.finalize(None)?;
 
         Ok(Self {
             multiple_versions,
             multiple_versions_include_dev,
-            workspace_duplicates,
-            unused_workspace_dependencies,
+            workspace_dependencies,
             highlight,
             deny,
             allow,
@@ -737,8 +756,7 @@ impl crate::cfg::UnvalidatedConfig for Config {
             file_id: ctx.cfg_id,
             multiple_versions: self.multiple_versions,
             multiple_versions_include_dev: self.multiple_versions_include_dev,
-            workspace_duplicates: self.workspace_duplicates,
-            unused_workspace_dependencies: self.unused_workspace_dependencies,
+            workspace_dependencies: self.workspace_dependencies,
             highlight: self.highlight,
             denied,
             denied_multiple_versions,
@@ -911,8 +929,7 @@ pub struct ValidConfig {
     pub file_id: FileId,
     pub multiple_versions: LintLevel,
     pub multiple_versions_include_dev: bool,
-    pub workspace_duplicates: LintLevel,
-    pub unused_workspace_dependencies: LintLevel,
+    pub workspace_dependencies: Option<WorkspaceDepsConfig>,
     pub highlight: GraphHighlight,
     pub(crate) denied: Vec<ValidKrateBan>,
     pub(crate) denied_multiple_versions: Vec<PackageSpec>,

src/bans/diags.rs

@@ -860,7 +860,7 @@ impl<'k> From<WorkspaceDuplicate<'k>> for Diag {
                 if wd.has_workspace_declaration {
                     "but not all declarations use the shared workspace dependency"
                 } else {
-                    "and there was no shared workspace dependency for it"
+                    "and there is no shared workspace dependency for it"
                 }
             ))
             .with_code(Code::WorkspaceDuplicate)

src/bans/snapshots/cargo_deny__bans__cfg__test__deserializes_ban_cfg.snap

@@ -6,8 +6,11 @@ expression: validated
   "file_id": 0,
   "multiple_versions": "deny",
   "multiple_versions_include_dev": false,
-  "workspace_duplicates": "warn",
-  "unused_workspace_dependencies": "allow",
+  "workspace_dependencies": {
+    "duplicates": "allow",
+    "include_path_dependencies": false,
+    "unused": "allow"
+  },
   "highlight": "SimplestPath",
   "denied": [
     {

tests/bans.rs

@@ -284,8 +284,10 @@ fn deny_duplicate_workspace_items() {
         },
         r#"
 multiple-versions = 'allow'
-workspace-duplicates = 'deny'
-unused-workspace-dependencies = 'warn'
+
+[workspace-dependencies]
+include-path-dependencies = true
+unused = 'warn'
 "#,
     );