Skip to content

Commit

Permalink
Rearrange workspace lints
Browse files Browse the repository at this point in the history
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 false) that determines if path dependencies in workspace members are checked for using workspace = true
  • Loading branch information
Jake-Shadle committed Aug 2, 2024
1 parent 3e5b9ce commit f0e54cb
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 54 deletions.
25 changes: 20 additions & 5 deletions docs/src/checks/bans/cfg.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,30 @@ 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`
```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` (default) - Ignores checking for `workspace = true` for workspace crates
* `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.

Expand Down
57 changes: 32 additions & 25 deletions src/bans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
);
});
}
}
});

Expand Down Expand Up @@ -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,
});
}
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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 "
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -1680,7 +1687,7 @@ fn check_workspace_duplicates(
pack.push(diags::WorkspaceDuplicate {
duplicate,
labels,
severity,
severity: cfg.duplicates,
has_workspace_declaration,
total_uses: total,
});
Expand Down
56 changes: 37 additions & 19 deletions src/bans/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,15 +329,41 @@ 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_default();
let unused = th.optional("unused").unwrap_or(LintLevel::Allow);

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
Expand Down Expand Up @@ -378,8 +404,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(),
Expand All @@ -404,12 +429,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::Allow);
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();
Expand All @@ -423,13 +442,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,
Expand Down Expand Up @@ -737,8 +757,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,
Expand Down Expand Up @@ -911,8 +930,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>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
{
Expand Down
6 changes: 4 additions & 2 deletions tests/bans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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'
"#,
);

Expand Down
6 changes: 5 additions & 1 deletion tests/cfg/bans.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
[bans]
multiple-versions = "deny"
workspace-duplicates = "warn"
wildcards = "deny"
allow-wildcard-paths = true
highlight = "simplest-path"
Expand All @@ -20,6 +19,11 @@ deny = [
]
skip-tree = [{ name = "blah", depth = 20 }]

[bans.workspace-dependencies]
duplicates = "allow"
include-path-dependencies = false
unused = "allow"

[[bans.skip]]
name = "rand"
version = "=0.6.5"
Expand Down

0 comments on commit f0e54cb

Please sign in to comment.