fix: improve message for inactive weak optional feature with edition2024

Author: linyihai

crates/cargo-util-schemas/src/manifest/mod.rs

@@ -58,6 +58,12 @@ pub struct TomlManifest {
     /// Note: this is populated by the caller, rather than automatically
     #[serde(skip)]
     pub _unused_keys: BTreeSet<String>,
+    #[serde(skip)]
+    pub _unused_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
+    #[serde(skip)]
+    pub _unused_dev_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
+    #[serde(skip)]
+    pub _unused_build_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
 }
 
 impl TomlManifest {
@@ -1377,6 +1383,13 @@ pub struct TomlPlatform {
     pub dev_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
     #[serde(rename = "dev_dependencies")]
     pub dev_dependencies2: Option<BTreeMap<PackageName, InheritableDependency>>,
+
+    #[serde(skip)]
+    pub _unused_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
+    #[serde(skip)]
+    pub _unused_dev_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
+    #[serde(skip)]
+    pub _unused_build_dependencies: Option<BTreeMap<PackageName, InheritableDependency>>,
 }
 
 impl TomlPlatform {

crates/resolver-tests/src/lib.rs

@@ -164,6 +164,7 @@ pub fn resolve_with_global_context_raw(
         &BTreeMap::new(),
         None::<&String>,
         None::<RustVersion>,
+        None,
     )
     .unwrap();
     let opts = ResolveOpts::everything();
@@ -483,6 +484,7 @@ pub fn pkg_dep<T: ToPkgId>(name: T, dep: Vec<Dependency>) -> Summary {
         &BTreeMap::new(),
         link,
         None::<RustVersion>,
+        None,
     )
     .unwrap()
 }
@@ -511,6 +513,7 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary {
         &BTreeMap::new(),
         link,
         None::<RustVersion>,
+        None,
     )
     .unwrap()
 }
@@ -525,6 +528,7 @@ pub fn remove_dep(sum: &Summary, ind: usize) -> Summary {
         &BTreeMap::new(),
         sum.links().map(|a| a.as_str()),
         None::<RustVersion>,
+        None,
     )
     .unwrap()
 }

src/cargo/core/resolver/version_prefs.rs

@@ -141,6 +141,7 @@ mod test {
             &features,
             None::<&String>,
             msrv.map(|m| m.parse().unwrap()),
+            None,
         )
         .unwrap()
     }

src/cargo/core/summary.rs

@@ -38,6 +38,7 @@ impl Summary {
         features: &BTreeMap<InternedString, Vec<InternedString>>,
         links: Option<impl Into<InternedString>>,
         rust_version: Option<RustVersion>,
+        unused_dependencies: Option<Vec<Dependency>>,
     ) -> CargoResult<Summary> {
         // ****CAUTION**** If you change anything here that may raise a new
         // error, be sure to coordinate that change with either the index
@@ -51,7 +52,11 @@ impl Summary {
                 )
             }
         }
-        let feature_map = build_feature_map(features, &dependencies)?;
+        let feature_map = build_feature_map(
+            features,
+            &dependencies,
+            &unused_dependencies.unwrap_or_default(),
+        )?;
         Ok(Summary {
             inner: Rc::new(Inner {
                 package_id: pkg_id,
@@ -154,6 +159,7 @@ impl Hash for Summary {
 fn build_feature_map(
     features: &BTreeMap<InternedString, Vec<InternedString>>,
     dependencies: &[Dependency],
+    unused_dependencies: &[Dependency],
 ) -> CargoResult<FeatureMap> {
     use self::FeatureValue::*;
     let mut dep_map = HashMap::new();
@@ -163,6 +169,13 @@ fn build_feature_map(
             .or_insert_with(Vec::new)
             .push(dep);
     }
+    let mut unused_dep_map = HashMap::new();
+    for dep in unused_dependencies.iter() {
+        unused_dep_map
+            .entry(dep.name_in_toml())
+            .or_insert_with(Vec::new)
+            .push(dep);
+    }
 
     let mut map: FeatureMap = features
         .iter()
@@ -312,6 +325,12 @@ fn build_feature_map(
                         )
                     }
 
+                    // editon2024 stops expose implicit features, which will strip weak optional dependencies from `dependencies`
+                    if *weak && unused_dep_map.get(dep_name).is_some() {
+                        bail!(
+                            "feature `{feature}` includes `{fv}`, but missing `dep:{dep_name}` to activate it, see the `dep:` syntax in https://doc.rust-lang.org/stable/cargo/reference/features.html#optional-dependencies"                               
+                        );
+                    }
                     // Validation of the feature name will be performed in the resolver.
                     if !is_any_dep {
                         bail!(

src/cargo/sources/registry/index/mod.rs

@@ -736,7 +736,7 @@ impl IndexSummary {
                 features.entry(name).or_default().extend(values);
             }
         }
-        let mut summary = Summary::new(pkgid, deps, &features, links, rust_version)?;
+        let mut summary = Summary::new(pkgid, deps, &features, links, rust_version, None)?;
         summary.set_checksum(cksum);
 
         let v_max = if bindeps {

src/cargo/util/toml/mod.rs

@@ -296,6 +296,9 @@ fn resolve_toml(
         badges: None,
         lints: None,
         _unused_keys: Default::default(),
+        _unused_dependencies: Default::default(),
+        _unused_dev_dependencies: Default::default(),
+        _unused_build_dependencies: Default::default(),
     };
 
     let package_root = manifest_file.parent().unwrap();
@@ -375,7 +378,10 @@ fn resolve_toml(
             })
             .unwrap_or_default();
 
-        resolved_toml.dependencies = resolve_dependencies(
+        (
+            resolved_toml.dependencies,
+            resolved_toml._unused_dependencies,
+        ) = resolve_dependencies(
             gctx,
             edition,
             &features,
@@ -395,7 +401,10 @@ fn resolve_toml(
             edition,
             warnings,
         )?;
-        resolved_toml.dev_dependencies = resolve_dependencies(
+        (
+            resolved_toml.dev_dependencies,
+            resolved_toml._unused_dev_dependencies,
+        ) = resolve_dependencies(
             gctx,
             edition,
             &features,
@@ -415,7 +424,10 @@ fn resolve_toml(
             edition,
             warnings,
         )?;
-        resolved_toml.build_dependencies = resolve_dependencies(
+        (
+            resolved_toml.build_dependencies,
+            resolved_toml._unused_build_dependencies,
+        ) = resolve_dependencies(
             gctx,
             edition,
             &features,
@@ -428,7 +440,7 @@ fn resolve_toml(
         )?;
         let mut resolved_target = BTreeMap::new();
         for (name, platform) in original_toml.target.iter().flatten() {
-            let resolved_dependencies = resolve_dependencies(
+            let (resolved_dependencies, _unused_dependencies) = resolve_dependencies(
                 gctx,
                 edition,
                 &features,
@@ -448,7 +460,7 @@ fn resolve_toml(
                 edition,
                 warnings,
             )?;
-            let resolved_dev_dependencies = resolve_dependencies(
+            let (resolved_dev_dependencies, _unused_dev_dependencies) = resolve_dependencies(
                 gctx,
                 edition,
                 &features,
@@ -468,7 +480,7 @@ fn resolve_toml(
                 edition,
                 warnings,
             )?;
-            let resolved_build_dependencies = resolve_dependencies(
+            let (resolved_build_dependencies, _unused_build_dependencies) = resolve_dependencies(
                 gctx,
                 edition,
                 &features,
@@ -487,6 +499,9 @@ fn resolve_toml(
                     build_dependencies2: None,
                     dev_dependencies: resolved_dev_dependencies,
                     dev_dependencies2: None,
+                    _unused_dependencies,
+                    _unused_dev_dependencies,
+                    _unused_build_dependencies,
                 },
             );
         }
@@ -694,12 +709,16 @@ fn resolve_dependencies<'a>(
     inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>,
     package_root: &Path,
     warnings: &mut Vec<String>,
-) -> CargoResult<Option<BTreeMap<manifest::PackageName, manifest::InheritableDependency>>> {
+) -> CargoResult<(
+    Option<BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
+    Option<BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
+)> {
     let Some(dependencies) = orig_deps else {
-        return Ok(None);
+        return Ok((None, None));
     };
 
     let mut deps = BTreeMap::new();
+    let mut ununsed_deps = BTreeMap::new();
     for (name_in_toml, v) in dependencies.iter() {
         let mut resolved = dependency_inherit_with(
             v.clone(),
@@ -763,9 +782,14 @@ fn resolve_dependencies<'a>(
                 name_in_toml.clone(),
                 manifest::InheritableDependency::Value(resolved.clone()),
             );
+        } else {
+            ununsed_deps.insert(
+                name_in_toml.clone(),
+                manifest::InheritableDependency::Value(resolved.clone()),
+            );
         }
     }
-    Ok(Some(deps))
+    Ok((Some(deps), Some(ununsed_deps)))
 }
 
 fn load_inheritable_fields(
@@ -1292,36 +1316,52 @@ fn to_real_manifest(
 
     // Collect the dependencies.
     let mut deps = Vec::new();
+    let mut unused_deps = Vec::new();
     let mut manifest_ctx = ManifestContext {
         deps: &mut deps,
         source_id,
         gctx,
         warnings,
         platform: None,
         root: package_root,
+        unused_deps: &mut unused_deps,
     };
-    gather_dependencies(&mut manifest_ctx, resolved_toml.dependencies.as_ref(), None)?;
+    gather_dependencies(
+        &mut manifest_ctx,
+        resolved_toml.dependencies.as_ref(),
+        resolved_toml._unused_dependencies.as_ref(),
+        None,
+    )?;
     gather_dependencies(
         &mut manifest_ctx,
         resolved_toml.dev_dependencies(),
+        None,
         Some(DepKind::Development),
     )?;
     gather_dependencies(
         &mut manifest_ctx,
         resolved_toml.build_dependencies(),
+        resolved_toml._unused_build_dependencies.as_ref(),
         Some(DepKind::Build),
     )?;
     for (name, platform) in resolved_toml.target.iter().flatten() {
         manifest_ctx.platform = Some(name.parse()?);
-        gather_dependencies(&mut manifest_ctx, platform.dependencies.as_ref(), None)?;
+        gather_dependencies(
+            &mut manifest_ctx,
+            platform.dependencies.as_ref(),
+            platform._unused_dependencies.as_ref(),
+            None,
+        )?;
         gather_dependencies(
             &mut manifest_ctx,
             platform.build_dependencies(),
+            None,
             Some(DepKind::Build),
         )?;
         gather_dependencies(
             &mut manifest_ctx,
             platform.dev_dependencies(),
+            None,
             Some(DepKind::Development),
         )?;
     }
@@ -1452,6 +1492,7 @@ fn to_real_manifest(
             .collect(),
         resolved_package.links.as_deref(),
         rust_version.clone(),
+        Some(unused_deps),
     )?;
     if summary.features().contains_key("default-features") {
         warnings.push(
@@ -1582,6 +1623,7 @@ fn to_virtual_manifest(
             warnings,
             platform: None,
             root,
+            unused_deps: &mut Vec::new(),
         };
         (
             replace(&original_toml, &mut manifest_ctx)?,
@@ -1650,23 +1692,31 @@ struct ManifestContext<'a, 'b> {
     warnings: &'a mut Vec<String>,
     platform: Option<Platform>,
     root: &'a Path,
+    unused_deps: &'a mut Vec<Dependency>,
 }
 
 #[tracing::instrument(skip_all)]
 fn gather_dependencies(
     manifest_ctx: &mut ManifestContext<'_, '_>,
     resolved_deps: Option<&BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
+    unuse_resolved_deps: Option<&BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
     kind: Option<DepKind>,
 ) -> CargoResult<()> {
-    let Some(dependencies) = resolved_deps else {
-        return Ok(());
+    if let Some(dependencies) = resolved_deps {
+        for (n, v) in dependencies.iter() {
+            let resolved = v.resolved().expect("previously resolved");
+            let dep = dep_to_dependency(&resolved, n, manifest_ctx, kind)?;
+            manifest_ctx.deps.push(dep);
+        }
+    };
+    if let Some(dependencies) = unuse_resolved_deps {
+        for (n, v) in dependencies.iter() {
+            let resolved = v.resolved().expect("previously resolved");
+            let dep = dep_to_dependency(&resolved, n, manifest_ctx, kind)?;
+            manifest_ctx.unused_deps.push(dep);
+        }
     };
 
-    for (n, v) in dependencies.iter() {
-        let resolved = v.resolved().expect("previously resolved");
-        let dep = dep_to_dependency(&resolved, n, manifest_ctx, kind)?;
-        manifest_ctx.deps.push(dep);
-    }
     Ok(())
 }
 
@@ -1775,6 +1825,7 @@ pub(crate) fn to_dependency<P: ResolveToPath + Clone>(
             warnings,
             platform,
             root,
+            unused_deps: &mut Vec::new(),
         },
         kind,
     )
@@ -2642,6 +2693,9 @@ fn prepare_toml_for_publish(
                             dev_dependencies2: None,
                             build_dependencies: map_deps(gctx, v.build_dependencies(), all)?,
                             build_dependencies2: None,
+                            _unused_dependencies: Default::default(),
+                            _unused_dev_dependencies: Default::default(),
+                            _unused_build_dependencies: Default::default(),
                         },
                     ))
                 })
@@ -2658,6 +2712,9 @@ fn prepare_toml_for_publish(
         cargo_features: me.cargo_features.clone(),
         lints: me.lints.clone(),
         _unused_keys: Default::default(),
+        _unused_dependencies: Default::default(),
+        _unused_dev_dependencies: Default::default(),
+        _unused_build_dependencies: Default::default(),
     };
     strip_features(&mut manifest);
     return Ok(manifest);

tests/testsuite/lints/unused_optional_dependencies.rs

@@ -296,7 +296,7 @@ Caused by:
 error: failed to parse manifest at `[ROOT]/foo/Cargo.toml`
 
 Caused by:
-  feature `foo_feature` includes `dep_name?/dep_feature`, but `dep_name` is not a dependency
+  feature `foo_feature` includes `dep_name?/dep_feature`, but missing `dep:dep_name` to activate it, see the `dep:` syntax in [..]
 ",
         )
         .run();