diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 8a0596cb5fd..9d986f47f72 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -24,7 +24,7 @@ use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::edit_distance; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; -use crate::util::lints::check_implicit_features; +use crate::util::lints::{check_implicit_features, unused_dependencies}; use crate::util::toml::{read_manifest, InheritableFields}; use crate::util::{context::ConfigRelativePath, Filesystem, GlobalContext, IntoUrl}; use cargo_util::paths; @@ -1158,6 +1158,7 @@ impl<'gctx> Workspace<'gctx> { .collect(); check_implicit_features(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?; + unused_dependencies(pkg, &path, &normalized_lints, &mut error_count, self.gctx)?; if error_count > 0 { Err(crate::util::errors::AlreadyPrintedError::new(anyhow!( "encountered {error_count} errors(s) while running lints" diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index aaf731b4e85..88c90e31af4 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -53,12 +53,13 @@ use tracing::{debug, trace, warn}; use crate::core::compiler::RustcTargetData; use crate::core::resolver::features::{DiffMap, FeatureOpts, FeatureResolver, FeaturesFor}; use crate::core::resolver::{HasDevUnits, Resolve, ResolveBehavior}; -use crate::core::PackageIdSpecQuery as _; use crate::core::{Edition, MaybePackage, Package, PackageId, Workspace}; +use crate::core::{FeatureValue, PackageIdSpecQuery as _}; use crate::ops::resolve::WorkspaceResolve; use crate::ops::{self, CompileOptions}; use crate::util::diagnostic_server::{Message, RustfixDiagnosticServer}; use crate::util::errors::CargoResult; +use crate::util::interning::InternedString; use crate::util::GlobalContext; use crate::util::{existing_vcs_repo, LockServer, LockServerClient}; use crate::{drop_eprint, drop_eprintln}; @@ -253,6 +254,7 @@ fn migrate_manifests(ws: &Workspace<'_>, pkgs: &[&Package]) -> CargoResult<()> { let mut fixes = 0; let root = document.as_table_mut(); + fixes += add_feature_for_unused_deps(pkg, root); if rename_table(root, "project", "package") { fixes += 1; } @@ -287,6 +289,45 @@ fn rename_table(parent: &mut dyn toml_edit::TableLike, old: &str, new: &str) -> true } +fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableLike) -> usize { + let manifest = pkg.manifest(); + + let activated_opt_deps = manifest + .resolved_toml() + .features() + .map(|map| { + map.values() + .flatten() + .filter_map(|f| match FeatureValue::new(InternedString::new(f)) { + FeatureValue::Dep { dep_name } => Some(dep_name.as_str()), + _ => None, + }) + .collect::>() + }) + .unwrap_or_default(); + + let mut fixes = 0; + for dep in manifest.dependencies() { + let dep_name_in_toml = dep.name_in_toml(); + if dep.is_optional() && !activated_opt_deps.contains(dep_name_in_toml.as_str()) { + fixes += 1; + if let Some(features) = parent + .entry("features") + .or_insert(toml_edit::table()) + .as_table_like_mut() + { + features.insert( + dep_name_in_toml.as_str(), + toml_edit::Item::Value(toml_edit::Value::Array(toml_edit::Array::from_iter( + &[format!("dep:{}", dep_name_in_toml)], + ))), + ); + } + } + } + fixes +} + fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<()> { let root = ws.root_maybe(); match root { diff --git a/src/cargo/util/lints.rs b/src/cargo/util/lints.rs index 89a156a478a..aec3e8e52cc 100644 --- a/src/cargo/util/lints.rs +++ b/src/cargo/util/lints.rs @@ -1,3 +1,4 @@ +use crate::core::dependency::DepKind; use crate::core::FeatureValue::Dep; use crate::core::{Edition, FeatureValue, Package}; use crate::util::interning::InternedString; @@ -6,6 +7,7 @@ use annotate_snippets::{Level, Renderer, Snippet}; use cargo_util_schemas::manifest::{TomlLintLevel, TomlToolLints}; use pathdiff::diff_paths; use std::collections::HashSet; +use std::fmt::Display; use std::ops::Range; use std::path::Path; use toml_edit::ImDocument; @@ -107,6 +109,17 @@ pub enum LintLevel { Forbid, } +impl Display for LintLevel { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + LintLevel::Allow => write!(f, "allow"), + LintLevel::Warn => write!(f, "warn"), + LintLevel::Deny => write!(f, "deny"), + LintLevel::Forbid => write!(f, "forbid"), + } + } +} + impl LintLevel { pub fn to_diagnostic_level(self) -> Level { match self { @@ -144,10 +157,10 @@ impl From for LintLevel { /// [RFC #3491]: https://rust-lang.github.io/rfcs/3491-remove-implicit-features.html const IMPLICIT_FEATURES: Lint = Lint { name: "implicit_features", - desc: "warn about the use of unstable features", + desc: "implicit features for optional dependencies is deprecated and will be unavailable in the 2024 edition", groups: &[], default_level: LintLevel::Allow, - edition_lint_opts: Some((Edition::Edition2024, LintLevel::Deny)), + edition_lint_opts: None, }; pub fn check_implicit_features( @@ -157,56 +170,63 @@ pub fn check_implicit_features( error_count: &mut usize, gctx: &GlobalContext, ) -> CargoResult<()> { - let lint_level = IMPLICIT_FEATURES.level(lints, pkg.manifest().edition()); + let edition = pkg.manifest().edition(); + // In Edition 2024+, instead of creating optional features, the dependencies are unused. + // See `UNUSED_OPTIONAL_DEPENDENCY` + if edition >= Edition::Edition2024 { + return Ok(()); + } + + let lint_level = IMPLICIT_FEATURES.level(lints, edition); if lint_level == LintLevel::Allow { return Ok(()); } let manifest = pkg.manifest(); - let user_defined_features = manifest.resolved_toml().features(); - let features = user_defined_features.map_or(HashSet::new(), |f| { - f.keys().map(|k| InternedString::new(&k)).collect() - }); - // Add implicit features for optional dependencies if they weren't - // explicitly listed anywhere. - let explicitly_listed = user_defined_features.map_or(HashSet::new(), |f| { - f.values() - .flatten() - .filter_map(|v| match FeatureValue::new(v.into()) { - Dep { dep_name } => Some(dep_name), - _ => None, - }) - .collect() - }); + let activated_opt_deps = manifest + .resolved_toml() + .features() + .map(|map| { + map.values() + .flatten() + .filter_map(|f| match FeatureValue::new(InternedString::new(f)) { + Dep { dep_name } => Some(dep_name.as_str()), + _ => None, + }) + .collect::>() + }) + .unwrap_or_default(); + let mut emitted_source = None; for dep in manifest.dependencies() { let dep_name_in_toml = dep.name_in_toml(); - if !dep.is_optional() - || features.contains(&dep_name_in_toml) - || explicitly_listed.contains(&dep_name_in_toml) - { + if !dep.is_optional() || activated_opt_deps.contains(dep_name_in_toml.as_str()) { continue; } if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny { *error_count += 1; } + let mut toml_path = vec![dep.kind().kind_table(), dep_name_in_toml.as_str()]; + let platform = dep.platform().map(|p| p.to_string()); + if let Some(platform) = platform.as_ref() { + toml_path.insert(0, platform); + toml_path.insert(0, "target"); + } let level = lint_level.to_diagnostic_level(); let manifest_path = rel_cwd_manifest_path(path, gctx); - let message = level.title("unused optional dependency").snippet( + let mut message = level.title(IMPLICIT_FEATURES.desc).snippet( Snippet::source(manifest.contents()) .origin(&manifest_path) - .annotation( - level.span( - get_span( - manifest.document(), - &["dependencies", &dep_name_in_toml], - false, - ) - .unwrap(), - ), - ) + .annotation(level.span(get_span(manifest.document(), &toml_path, false).unwrap())) .fold(true), ); + if emitted_source.is_none() { + emitted_source = Some(format!( + "`cargo::{}` is set to `{lint_level}`", + IMPLICIT_FEATURES.name + )); + message = message.footer(Level::Note.title(emitted_source.as_ref().unwrap())); + } let renderer = Renderer::styled().term_width( gctx.shell() .err_width() @@ -217,3 +237,114 @@ pub fn check_implicit_features( } Ok(()) } + +const UNUSED_OPTIONAL_DEPENDENCY: Lint = Lint { + name: "unused_optional_dependency", + desc: "unused optional dependency", + groups: &[], + default_level: LintLevel::Warn, + edition_lint_opts: None, +}; + +pub fn unused_dependencies( + pkg: &Package, + path: &Path, + lints: &TomlToolLints, + error_count: &mut usize, + gctx: &GlobalContext, +) -> CargoResult<()> { + let edition = pkg.manifest().edition(); + // Unused optional dependencies can only exist on edition 2024+ + if edition < Edition::Edition2024 { + return Ok(()); + } + + let lint_level = UNUSED_OPTIONAL_DEPENDENCY.level(lints, edition); + if lint_level == LintLevel::Allow { + return Ok(()); + } + let mut emitted_source = None; + let manifest = pkg.manifest(); + let original_toml = manifest.original_toml(); + // Unused dependencies were stripped from the manifest, leaving only the used ones + let used_dependencies = manifest + .dependencies() + .into_iter() + .map(|d| d.name_in_toml().to_string()) + .collect::>(); + let mut orig_deps = vec![ + ( + original_toml.dependencies.as_ref(), + vec![DepKind::Normal.kind_table()], + ), + ( + original_toml.dev_dependencies.as_ref(), + vec![DepKind::Development.kind_table()], + ), + ( + original_toml.build_dependencies.as_ref(), + vec![DepKind::Build.kind_table()], + ), + ]; + for (name, platform) in original_toml.target.iter().flatten() { + orig_deps.push(( + platform.dependencies.as_ref(), + vec!["target", name, DepKind::Normal.kind_table()], + )); + orig_deps.push(( + platform.dev_dependencies.as_ref(), + vec!["target", name, DepKind::Development.kind_table()], + )); + orig_deps.push(( + platform.build_dependencies.as_ref(), + vec!["target", name, DepKind::Normal.kind_table()], + )); + } + for (deps, toml_path) in orig_deps { + if let Some(deps) = deps { + for name in deps.keys() { + if !used_dependencies.contains(name.as_str()) { + if lint_level == LintLevel::Forbid || lint_level == LintLevel::Deny { + *error_count += 1; + } + let toml_path = toml_path + .iter() + .map(|s| *s) + .chain(std::iter::once(name.as_str())) + .collect::>(); + let level = lint_level.to_diagnostic_level(); + let manifest_path = rel_cwd_manifest_path(path, gctx); + + let mut message = level.title(UNUSED_OPTIONAL_DEPENDENCY.desc).snippet( + Snippet::source(manifest.contents()) + .origin(&manifest_path) + .annotation(level.span( + get_span(manifest.document(), toml_path.as_slice(), false).unwrap(), + )) + .fold(true), + ); + if emitted_source.is_none() { + emitted_source = Some(format!( + "`cargo::{}` is set to `{lint_level}`", + UNUSED_OPTIONAL_DEPENDENCY.name + )); + message = + message.footer(Level::Note.title(emitted_source.as_ref().unwrap())); + } + let help = format!( + "remove the dependency or activate it in a feature with `dep:{name}`" + ); + message = message.footer(Level::Help.title(&help)); + let renderer = Renderer::styled().term_width( + gctx.shell() + .err_width() + .diagnostic_terminal_width() + .unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH), + ); + writeln!(gctx.shell().err(), "{}", renderer.render(message))?; + } + } + } + } + Ok(()) +} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 020b08d2ac5..81825a94853 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1,5 +1,5 @@ use annotate_snippets::{Level, Renderer, Snippet}; -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::rc::Rc; @@ -20,6 +20,7 @@ use crate::core::compiler::{CompileKind, CompileTarget}; use crate::core::dependency::{Artifact, ArtifactTarget, DepKind}; use crate::core::manifest::{ManifestMetadata, TargetSourcePath}; use crate::core::resolver::ResolveBehavior; +use crate::core::FeatureValue::Dep; use crate::core::{find_workspace_root, resolve_relative_path, CliUnstable, FeatureValue}; use crate::core::{Dependency, Manifest, Package, PackageId, Summary, Target}; use crate::core::{Edition, EitherManifest, Feature, Features, VirtualManifest, Workspace}; @@ -300,12 +301,34 @@ fn resolve_toml( if let Some(original_package) = original_toml.package() { let resolved_package = resolve_package_toml(original_package, package_root, &inherit)?; + let edition = resolved_package + .resolved_edition() + .expect("previously resolved") + .map_or(Edition::default(), |e| { + Edition::from_str(&e).unwrap_or_default() + }); resolved_toml.package = Some(resolved_package); + let activated_opt_deps = resolved_toml + .features + .as_ref() + .map(|map| { + map.values() + .flatten() + .filter_map(|f| match FeatureValue::new(InternedString::new(f)) { + Dep { dep_name } => Some(dep_name.as_str()), + _ => None, + }) + .collect::>() + }) + .unwrap_or_default(); + resolved_toml.dependencies = resolve_dependencies( gctx, + edition, &features, original_toml.dependencies.as_ref(), + &activated_opt_deps, None, &inherit, package_root, @@ -313,8 +336,10 @@ fn resolve_toml( )?; resolved_toml.dev_dependencies = resolve_dependencies( gctx, + edition, &features, original_toml.dev_dependencies(), + &activated_opt_deps, Some(DepKind::Development), &inherit, package_root, @@ -322,8 +347,10 @@ fn resolve_toml( )?; resolved_toml.build_dependencies = resolve_dependencies( gctx, + edition, &features, original_toml.build_dependencies(), + &activated_opt_deps, Some(DepKind::Build), &inherit, package_root, @@ -333,8 +360,10 @@ fn resolve_toml( for (name, platform) in original_toml.target.iter().flatten() { let resolved_dependencies = resolve_dependencies( gctx, + edition, &features, platform.dependencies.as_ref(), + &activated_opt_deps, None, &inherit, package_root, @@ -342,8 +371,10 @@ fn resolve_toml( )?; let resolved_dev_dependencies = resolve_dependencies( gctx, + edition, &features, platform.dev_dependencies(), + &activated_opt_deps, Some(DepKind::Development), &inherit, package_root, @@ -351,8 +382,10 @@ fn resolve_toml( )?; let resolved_build_dependencies = resolve_dependencies( gctx, + edition, &features, platform.build_dependencies(), + &activated_opt_deps, Some(DepKind::Build), &inherit, package_root, @@ -561,8 +594,10 @@ fn default_readme_from_package_root(package_root: &Path) -> Option { #[tracing::instrument(skip_all)] fn resolve_dependencies<'a>( gctx: &GlobalContext, + edition: Edition, features: &Features, orig_deps: Option<&BTreeMap>, + activated_opt_deps: &HashSet<&str>, kind: Option, inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>, package_root: &Path, @@ -610,10 +645,18 @@ fn resolve_dependencies<'a>( } } - deps.insert( - name_in_toml.clone(), - manifest::InheritableDependency::Value(resolved.clone()), - ); + // if the dependency is not optional, it is always used + // if the dependency is optional and activated, it is used + // if the dependency is optional and not activated, it is not used + let is_dep_activated = + !resolved.is_optional() || activated_opt_deps.contains(name_in_toml.as_str()); + // If the edition is less than 2024, we don't need to check for unused optional dependencies + if edition < Edition::Edition2024 || is_dep_activated { + deps.insert( + name_in_toml.clone(), + manifest::InheritableDependency::Value(resolved.clone()), + ); + } } Ok(Some(deps)) } diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 485511ab764..7997a68ae62 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2049,3 +2049,138 @@ edition = "2021" "# ); } + +#[cargo_test] +fn add_feature_for_unused_dep() { + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.1.0").publish(); + Package::new("target-dep", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "foo" +version = "0.1.0" +edition = "2021" + +[dependencies] +bar = { version = "0.1.0", optional = true } + +[build-dependencies] +baz = { version = "0.1.0", optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +target-dep = { version = "0.1.0", optional = true } +"#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("fix --edition --allow-no-vcs") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_stderr( + "\ +[MIGRATING] Cargo.toml from 2021 edition to 2024 +[FIXED] Cargo.toml (3 fixes) +[UPDATING] `dummy-registry` index +[LOCKING] 4 packages to latest compatible versions +[CHECKING] foo v0.1.0 ([CWD]) +[MIGRATING] src/lib.rs from 2021 edition to 2024 +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s +", + ) + .run(); + assert_eq!( + p.read_file("Cargo.toml"), + r#" +[package] +name = "foo" +version = "0.1.0" +edition = "2021" + +[dependencies] +bar = { version = "0.1.0", optional = true } + +[build-dependencies] +baz = { version = "0.1.0", optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +target-dep = { version = "0.1.0", optional = true } + +[features] +bar = ["dep:bar"] +baz = ["dep:baz"] +target-dep = ["dep:target-dep"] +"# + ); +} + +#[cargo_test] +fn add_feature_for_unused_dep_existing_table() { + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.1.0").publish(); + Package::new("target-dep", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" +[package] +name = "foo" +version = "0.1.0" +edition = "2021" + +[dependencies] +bar = { version = "0.1.0", optional = true } + +[build-dependencies] +baz = { version = "0.1.0", optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +target-dep = { version = "0.1.0", optional = true } + +[features] +target-dep = ["dep:target-dep"] +"#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("fix --edition --allow-no-vcs") + .masquerade_as_nightly_cargo(&["edition2024"]) + .with_stderr( + "\ +[MIGRATING] Cargo.toml from 2021 edition to 2024 +[FIXED] Cargo.toml (2 fixes) +[UPDATING] `dummy-registry` index +[LOCKING] 4 packages to latest compatible versions +[CHECKING] foo v0.1.0 ([CWD]) +[MIGRATING] src/lib.rs from 2021 edition to 2024 +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s +", + ) + .run(); + assert_eq!( + p.read_file("Cargo.toml"), + r#" +[package] +name = "foo" +version = "0.1.0" +edition = "2021" + +[dependencies] +bar = { version = "0.1.0", optional = true } + +[build-dependencies] +baz = { version = "0.1.0", optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +target-dep = { version = "0.1.0", optional = true } + +[features] +target-dep = ["dep:target-dep"] +bar = ["dep:bar"] +baz = ["dep:baz"] +"# + ); +} diff --git a/tests/testsuite/lints/implicit_features/edition_2021/mod.rs b/tests/testsuite/lints/implicit_features/edition_2021/mod.rs index ec43952e2b5..7ab75c6c7b8 100644 --- a/tests/testsuite/lints/implicit_features/edition_2021/mod.rs +++ b/tests/testsuite/lints/implicit_features/edition_2021/mod.rs @@ -1,7 +1,7 @@ use cargo_test_support::prelude::*; use cargo_test_support::project; use cargo_test_support::registry::Package; -use cargo_test_support::str; +use cargo_test_support::{file, str}; #[cargo_test] fn case() { @@ -23,12 +23,10 @@ bar = { version = "0.1.0", optional = true } .build(); snapbox::cmd::Command::cargo_ui() - .masquerade_as_nightly_cargo(&["always_nightly"]) .current_dir(p.root()) .arg("check") - .arg("--quiet") .assert() .success() .stdout_matches(str![""]) - .stderr_matches(str![""]); + .stderr_matches(file!["stderr.term.svg"]); } diff --git a/tests/testsuite/lints/implicit_features/edition_2021/stderr.term.svg b/tests/testsuite/lints/implicit_features/edition_2021/stderr.term.svg new file mode 100644 index 00000000000..f0726266833 --- /dev/null +++ b/tests/testsuite/lints/implicit_features/edition_2021/stderr.term.svg @@ -0,0 +1,33 @@ + + + + + + + Updating `dummy-registry` index + + Locking 2 packages to latest compatible versions + + Checking foo v0.1.0 ([ROOT]/foo) + + Finished[..] + + + + + + diff --git a/tests/testsuite/lints/implicit_features/edition_2021_warn/mod.rs b/tests/testsuite/lints/implicit_features/edition_2021_warn/mod.rs index a18006072a2..a7169ea4d95 100644 --- a/tests/testsuite/lints/implicit_features/edition_2021_warn/mod.rs +++ b/tests/testsuite/lints/implicit_features/edition_2021_warn/mod.rs @@ -6,6 +6,8 @@ use cargo_test_support::{file, project}; #[cargo_test] fn case() { Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.1.0").publish(); + Package::new("target-dep", "0.1.0").publish(); let p = project() .file( "Cargo.toml", @@ -18,6 +20,12 @@ edition = "2021" [dependencies] bar = { version = "0.1.0", optional = true } +[build-dependencies] +baz = { version = "0.1.0", optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +target-dep = { version = "0.1.0", optional = true } + [lints.cargo] implicit-features = "warn" "#, @@ -26,10 +34,10 @@ implicit-features = "warn" .build(); snapbox::cmd::Command::cargo_ui() - .masquerade_as_nightly_cargo(&["always_nightly"]) + .masquerade_as_nightly_cargo(&["cargo-lints"]) .current_dir(p.root()) .arg("check") - .arg("--quiet") + .arg("-Zcargo-lints") .assert() .success() .stdout_matches(str![""]) diff --git a/tests/testsuite/lints/implicit_features/edition_2021_warn/stderr.term.svg b/tests/testsuite/lints/implicit_features/edition_2021_warn/stderr.term.svg index e41fb2b3535..75040932d82 100644 --- a/tests/testsuite/lints/implicit_features/edition_2021_warn/stderr.term.svg +++ b/tests/testsuite/lints/implicit_features/edition_2021_warn/stderr.term.svg @@ -1,8 +1,10 @@ - +