Auto merge of #13778 - Muscraft:unused-dependencies, r=epage

Unused dependencies cleanup

The implementation of #12826 was used as a demonstration for #12235, in #13621. The demonstration implementation was far from ideal and was lacking a few features. This PR makes the implementation "feature complete", and fixes known bugs with the initial implementation.

Author: bors

Diff for: 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::CargoResolverConfig, context::CargoResolverPrecedence, context::ConfigRelativePath,
@@ -1194,6 +1194,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 for: 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::<HashSet<_>>()
+        })
+        .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 for: 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<TomlLintLevel> 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::<HashSet<_>>()
+        })
+        .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::<HashSet<String>>();
+    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::<Vec<_>>();
+                    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(())
+}