diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 5f3b7be4ec7..c0e96e8bf58 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -131,6 +131,9 @@ pub struct Workspace<'gctx> { /// Local overlay configuration. See [`crate::sources::overlay`]. local_overlays: HashMap, + + /// A set of exclude patterns from the workspace manifest that actually matched a crate. + used_exclude_patterns: HashSet, } // Separate structure for tracking loaded packages (to avoid loading anything @@ -178,7 +181,7 @@ impl WorkspaceConfig { match self { WorkspaceConfig::Root(ances_root_config) => { debug!("find_root - found a root checking exclusion"); - if !ances_root_config.is_excluded(look_from) { + if ances_root_config.is_excluded(look_from).is_none() { debug!("find_root - found!"); Some(self_path.to_owned()) } else { @@ -267,6 +270,7 @@ impl<'gctx> Workspace<'gctx> { resolve_publish_time: None, custom_metadata: None, local_overlays: HashMap::new(), + used_exclude_patterns: HashSet::new(), } } @@ -627,6 +631,11 @@ impl<'gctx> Workspace<'gctx> { self.packages.packages.values() } + /// Returns a set of all workspace exclude patterns that actually matched a path. + pub fn used_exclude_patterns(&self) -> &HashSet { + &self.used_exclude_patterns + } + /// Returns an iterator over all packages in this workspace pub fn members(&self) -> impl Iterator { let packages = &self.packages; @@ -910,10 +919,11 @@ impl<'gctx> Workspace<'gctx> { // manifest path, both because `members_paths` doesn't // include `/Cargo.toml`, and because excluded paths may not // be crates. - let exclude = members_paths.iter().any(|(m, _)| *m == normalized_path) - && workspace_config.is_excluded(&normalized_path); - if exclude { - continue; + if members_paths.iter().any(|(m, _)| *m == normalized_path) { + if let Some(pattern) = workspace_config.is_excluded(&normalized_path) { + self.used_exclude_patterns.insert(pattern.clone()); + continue; + } } bail!( "package `{}` is listed in default-members{} but is not a member\n\ @@ -962,7 +972,8 @@ impl<'gctx> Workspace<'gctx> { if let WorkspaceConfig::Root(ref root_config) = *self.packages.load(root_manifest)?.workspace_config() { - if root_config.is_excluded(&manifest_path) { + if let Some(pattern) = root_config.is_excluded(&manifest_path) { + self.used_exclude_patterns.insert(pattern.clone()); return Ok(()); } } @@ -1982,11 +1993,11 @@ impl WorkspaceRootConfig { /// Checks the path against the `excluded` list. /// /// This method does **not** consider the `members` list. - fn is_excluded(&self, manifest_path: &Path) -> bool { + fn is_excluded(&self, manifest_path: &Path) -> Option<&String> { let excluded = self .exclude .iter() - .any(|ex| manifest_path.starts_with(self.root_dir.join(ex))); + .find(|ex| manifest_path.starts_with(self.root_dir.join(*ex))); let explicit_member = match self.members { Some(ref members) => members @@ -1995,7 +2006,7 @@ impl WorkspaceRootConfig { None => false, }; - !explicit_member && excluded + if !explicit_member { excluded } else { None } } /// Checks if the path is explicitly listed as a workspace member. @@ -2202,7 +2213,7 @@ pub fn find_workspace_root_with_membership_check( // Verify the workspace includes this package in its members if let WorkspaceConfig::Root(ref root_config) = *ws_manifest.workspace_config() { if root_config.is_explicitly_listed_member(manifest_path) - && !root_config.is_excluded(manifest_path) + && root_config.is_excluded(manifest_path).is_none() { return Ok(Some(ws_manifest_path)); } @@ -2217,7 +2228,7 @@ pub fn find_workspace_root_with_membership_check( let manifest = read_manifest(candidate_manifest_path, source_id, gctx)?; if let WorkspaceConfig::Root(ref root_config) = *manifest.workspace_config() { if root_config.is_explicitly_listed_member(manifest_path) - && !root_config.is_excluded(manifest_path) + && root_config.is_excluded(manifest_path).is_none() { return Ok(Some(candidate_manifest_path.to_path_buf())); } @@ -2244,7 +2255,7 @@ fn find_workspace_root_with_loader( // root. Note we skip the first item since that is just the path itself for current in manifest_path.ancestors().skip(1) { if let Some(ws_config) = roots.get(current) { - if !ws_config.is_excluded(manifest_path) { + if ws_config.is_excluded(manifest_path).is_none() { // Add `Cargo.toml` since ws_root is the root and not the file return Ok(Some(current.join("Cargo.toml"))); } diff --git a/src/cargo/diagnostics/rules/mod.rs b/src/cargo/diagnostics/rules/mod.rs index fadb93040ac..7a2624043b2 100644 --- a/src/cargo/diagnostics/rules/mod.rs +++ b/src/cargo/diagnostics/rules/mod.rs @@ -16,6 +16,7 @@ mod text_direction_codepoint_in_literal; mod unknown_lints; pub mod unused_dependencies; mod unused_workspace_dependencies; +mod unused_workspace_exclude; mod unused_workspace_package_fields; use super::LintGroup; @@ -50,6 +51,10 @@ pub const PARSE_PASS_RULES: &[ParsePassRule<'static>] = &[ rule: unused_workspace_dependencies::lint_workspace, lint: unused_workspace_dependencies::LINT, }, + ParsePassRule::LintWorkspace { + rule: unused_workspace_exclude::lint_workspace, + lint: unused_workspace_exclude::LINT, + }, ParsePassRule::LintWorkspace { rule: unused_workspace_package_fields::lint_workspace, lint: unused_workspace_package_fields::LINT, @@ -123,6 +128,7 @@ pub static LINTS: &[&crate::diagnostics::Lint] = &[ unknown_lints::LINT, unused_dependencies::LINT, unused_workspace_dependencies::LINT, + unused_workspace_exclude::LINT, unused_workspace_package_fields::LINT, ]; diff --git a/src/cargo/diagnostics/rules/unused_workspace_exclude.rs b/src/cargo/diagnostics/rules/unused_workspace_exclude.rs new file mode 100644 index 00000000000..dfe7178f43d --- /dev/null +++ b/src/cargo/diagnostics/rules/unused_workspace_exclude.rs @@ -0,0 +1,100 @@ +use std::path::Path; + +use cargo_util_terminal::report::{AnnotationKind, Group, Level, Origin, Snippet}; +use tracing::instrument; + +use super::SUSPICIOUS; +use crate::diagnostics::{ + Lint, LintLevelProduct, ScopedDiagnosticStats, get_key_value_span, workspace_rel_path, +}; +use crate::{ + CargoResult, GlobalContext, + core::{MaybePackage, Workspace}, +}; + +pub static LINT: &Lint = &Lint { + name: "unused_workspace_exclude", + desc: "unused workspace exclude", + primary_group: &SUSPICIOUS, + msrv: Some(super::CARGO_LINTS_MSRV), + feature_gate: None, + docs: Some( + r#" +### What it does +Checks for any entry in `[workspace.exclude]` that does not match any workspace member + +### Why it is bad +They can give the false impression that a package is excluded when it is actually not present + +### Example +```toml +[workspace] +exclude = ["does-not-exist"] +``` +"#, + ), +}; + +#[instrument(skip_all)] +pub(crate) fn lint_workspace( + ws: &Workspace<'_>, + maybe_pkg: &MaybePackage, + manifest_path: &Path, + level: LintLevelProduct, + pkg_stats: &mut ScopedDiagnosticStats<'_>, + gctx: &GlobalContext, +) -> CargoResult<()> { + let LintLevelProduct { + level: lint_level, + source, + } = level; + + let Some(original_toml) = maybe_pkg.original_toml() else { + return Ok(()); + }; + + let Some(workspace) = original_toml.workspace.as_ref() else { + return Ok(()); + }; + + let Some(exclude) = workspace.exclude.as_ref() else { + return Ok(()); + }; + + let used_exclude_patterns = ws.used_exclude_patterns(); + + for (i, unused) in exclude + .iter() + .filter(|pattern| !used_exclude_patterns.contains(*pattern)) + .enumerate() + { + let document = maybe_pkg.document(); + let contents = maybe_pkg.contents(); + let level = lint_level.to_diagnostic_level(); + let manifest_path = workspace_rel_path(ws, manifest_path); + let emitted_source = LINT.emitted_source(lint_level, source); + + let mut primary = + Group::with_title(level.primary_title(format!("unused exclude pattern '{}'", unused))); + if let Some(document) = document + && let Some(contents) = contents + { + let mut snippet = Snippet::source(contents).path(&manifest_path); + if let Some(span) = get_key_value_span(document, &["workspace", "exclude"]) { + snippet = snippet.annotation(AnnotationKind::Primary.span(span.key)); + } + primary = primary.element(snippet); + } else { + primary = primary.element(Origin::path(&manifest_path)); + } + if i == 0 { + primary = primary.element(Level::NOTE.message(emitted_source)); + } + let report = vec![primary]; + + pkg_stats.record_lint(lint_level); + gctx.shell().print_report(&report, lint_level.force())?; + } + + Ok(()) +} diff --git a/src/doc/src/reference/lints.md b/src/doc/src/reference/lints.md index af6ebb29f5e..95a29e58b4d 100644 --- a/src/doc/src/reference/lints.md +++ b/src/doc/src/reference/lints.md @@ -37,6 +37,7 @@ These lints are all set to the 'warn' level by default. - [`unknown_lints`](#unknown_lints) - [`unused_dependencies`](#unused_dependencies) - [`unused_workspace_dependencies`](#unused_workspace_dependencies) +- [`unused_workspace_exclude`](#unused_workspace_exclude) - [`unused_workspace_package_fields`](#unused_workspace_package_fields) ## Deny-by-default @@ -542,6 +543,26 @@ regex = "1" ``` +## `unused_workspace_exclude` +Group: `suspicious` + +Level: `warn` + +MSRV: `1.79.0` + +### What it does +Checks for any entry in `[workspace.exclude]` that does not match any workspace member + +### Why it is bad +They can give the false impression that a package is excluded when it is actually not present + +### Example +```toml +[workspace] +exclude = ["does-not-exist"] +``` + + ## `unused_workspace_package_fields` Group: `suspicious` diff --git a/tests/testsuite/lints/mod.rs b/tests/testsuite/lints/mod.rs index 7fc47efaf00..e8ea683a57d 100644 --- a/tests/testsuite/lints/mod.rs +++ b/tests/testsuite/lints/mod.rs @@ -19,6 +19,7 @@ mod text_direction_codepoint; mod unknown_lints; mod unused_dependencies; mod unused_workspace_dependencies; +mod unused_workspace_exclude; mod unused_workspace_package_fields; mod warning; diff --git a/tests/testsuite/lints/unused_workspace_exclude.rs b/tests/testsuite/lints/unused_workspace_exclude.rs new file mode 100644 index 00000000000..8d77eb53716 --- /dev/null +++ b/tests/testsuite/lints/unused_workspace_exclude.rs @@ -0,0 +1,188 @@ +use crate::prelude::*; +use cargo_test_support::{project, str}; + +#[cargo_test] +fn unused_exclude_missing_directory() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["crates/*"] + exclude = ["crates/does-not-exist"] + + [workspace.lints.cargo] + unused_workspace_exclude = "warn" + "#, + ) + .file( + "crates/foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + + [lints] + workspace = true + "#, + ) + .file("crates/foo/src/lib.rs", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] unused exclude pattern 'crates/does-not-exist' + --> Cargo.toml:4:17 + | +4 | exclude = ["crates/does-not-exist"] + | ^^^^^^^ + | + = [NOTE] `cargo::unused_workspace_exclude` is set to `warn` in `[lints]` +[WARNING] workspace (manifest) generated 1 warning +[CHECKING] foo v0.0.1 ([ROOT]/foo/crates/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test] +fn unused_exclude_directory_without_manifest() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["crates/*"] + exclude = ["crates/bar"] + + [workspace.lints.cargo] + unused_workspace_exclude = "warn" + "#, + ) + .file( + "crates/foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + + [lints] + workspace = true + "#, + ) + .file("crates/foo/src/lib.rs", "") + .file("crates/bar/some_file.txt", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[CHECKING] foo v0.0.1 ([ROOT]/foo/crates/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test] +fn unused_exclude_glob_matches_nothing() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["crates/*"] + exclude = ["crates/not-*"] + + [workspace.lints.cargo] + unused_workspace_exclude = "warn" + "#, + ) + .file( + "crates/foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + + [lints] + workspace = true + "#, + ) + .file("crates/foo/src/lib.rs", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[WARNING] unused exclude pattern 'crates/not-*' + --> Cargo.toml:4:17 + | +4 | exclude = ["crates/not-*"] + | ^^^^^^^ + | + = [NOTE] `cargo::unused_workspace_exclude` is set to `warn` in `[lints]` +[WARNING] workspace (manifest) generated 1 warning +[CHECKING] foo v0.0.1 ([ROOT]/foo/crates/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + +#[cargo_test] +fn unused_exclude_valid_no_warning() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["crates/*"] + exclude = ["crates/bar"] + + [workspace.lints.cargo] + unused_workspace_exclude = "warn" + "#, + ) + .file( + "crates/foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + + [lints] + workspace = true + "#, + ) + .file("crates/foo/src/lib.rs", "") + .file( + "crates/bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + edition = "2015" + + [lints] + workspace = true + "#, + ) + .file("crates/bar/src/lib.rs", "") + .build(); + + p.cargo("check -Zcargo-lints") + .masquerade_as_nightly_cargo(&["cargo-lints"]) + .with_stderr_data(str![[r#" +[CHECKING] foo v0.0.1 ([ROOT]/foo/crates/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +}