Auto merge of #8818 - ehuss:weak-dep-features, r=alexcrichton

Implement weak dependency features.

This adds the feature syntax `dep_name?/feat_name` with a `?` to only enable `feat_name` if the optional dependency `dep_name` is enabled through some other means. See `unstable.md` for documentation.

This only works with the new feature resolver. I don't think I understand the dependency resolver well enough to implement it there. It would require teaching it to defer activating a feature, but due to the backtracking nature, I don't really know how to accomplish that. I don't think it matters, the main drawback is that the dependency resolver will be slightly more constrained, but in practice I doubt it will ever matter.

Closes #3494

**Question**
* An alternate syntax I considered was `dep_name?feat_name` (without the slash), what do people think?  For some reason the `?/` seems kinda awkward to me.

Author: bors

src/bin/cargo/cli.rs

@@ -43,6 +43,7 @@ Available unstable (nightly-only) flags:
     -Z doctest-xcompile    -- Compile and run doctests for non-host target using runner config
     -Z terminal-width      -- Provide a terminal width to rustc for error truncation
     -Z namespaced-features -- Allow features with `dep:` prefix
+    -Z weak-dep-features    -- Allow `dep_name?/feature` feature syntax
 
 Run with 'cargo -Z [FLAG] [SUBCOMMAND]'"
         );

src/cargo/core/features.rs

@@ -358,6 +358,7 @@ pub struct CliUnstable {
     pub rustdoc_map: bool,
     pub terminal_width: Option<Option<usize>>,
     pub namespaced_features: bool,
+    pub weak_dep_features: bool,
 }
 
 fn deserialize_build_std<'de, D>(deserializer: D) -> Result<Option<Vec<String>>, D::Error>
@@ -464,6 +465,7 @@ impl CliUnstable {
             "rustdoc-map" => self.rustdoc_map = parse_empty(k, v)?,
             "terminal-width" => self.terminal_width = Some(parse_usize_opt(v)?),
             "namespaced-features" => self.namespaced_features = parse_empty(k, v)?,
+            "weak-dep-features" => self.weak_dep_features = parse_empty(k, v)?,
             _ => bail!("unknown `-Z` flag specified: {}", k),
         }
 

src/cargo/core/resolver/dep_cache.rs

@@ -494,6 +494,10 @@ impl Requirements<'_> {
                 dep_name,
                 dep_feature,
                 dep_prefix,
+                // Weak features are always activated in the dependency
+                // resolver. They will be narrowed inside the new feature
+                // resolver.
+                weak: _,
             } => self.require_dep_feature(*dep_name, *dep_feature, *dep_prefix)?,
         };
         Ok(())

src/cargo/core/resolver/features.rs

@@ -177,6 +177,10 @@ impl FeatureOpts {
         if let ForceAllTargets::Yes = force_all_targets {
             opts.ignore_inactive_targets = false;
         }
+        if unstable_flags.weak_dep_features {
+            // Force this ON because it only works with the new resolver.
+            opts.new_resolver = true;
+        }
         Ok(opts)
     }
 }
@@ -311,6 +315,15 @@ pub struct FeatureResolver<'a, 'cfg> {
     /// This has to be separate from `FeatureOpts.decouple_host_deps` because
     /// `for_host` tracking is also needed for `itarget` to work properly.
     track_for_host: bool,
+    /// `dep_name?/feat_name` features that will be activated if `dep_name` is
+    /// ever activated.
+    ///
+    /// The key is the `(package, for_host, dep_name)` of the package whose
+    /// dependency will trigger the addition of new features. The value is the
+    /// set of `(feature, dep_prefix)` features to activate (`dep_prefix` is a
+    /// bool that indicates if `dep:` prefix was used).
+    deferred_weak_dependencies:
+        HashMap<(PackageId, bool, InternedString), HashSet<(InternedString, bool)>>,
 }
 
 impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
@@ -353,6 +366,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
             activated_dependencies: HashMap::new(),
             processed_deps: HashSet::new(),
             track_for_host,
+            deferred_weak_dependencies: HashMap::new(),
         };
         r.do_resolve(specs, requested_features)?;
         log::debug!("features={:#?}", r.activated_features);
@@ -378,7 +392,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
         for (member, requested_features) in &member_features {
             let fvs = self.fvs_from_requested(member.package_id(), requested_features);
             let for_host = self.track_for_host && self.is_proc_macro(member.package_id());
-            self.activate_pkg(member.package_id(), &fvs, for_host)?;
+            self.activate_pkg(member.package_id(), for_host, &fvs)?;
             if for_host {
                 // Also activate without for_host. This is needed if the
                 // proc-macro includes other targets (like binaries or tests),
@@ -387,7 +401,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
                 // `--workspace`), this forces feature unification with normal
                 // dependencies. This is part of the bigger problem where
                 // features depend on which packages are built.
-                self.activate_pkg(member.package_id(), &fvs, false)?;
+                self.activate_pkg(member.package_id(), false, &fvs)?;
             }
         }
         Ok(())
@@ -396,17 +410,18 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
     fn activate_pkg(
         &mut self,
         pkg_id: PackageId,
-        fvs: &[FeatureValue],
         for_host: bool,
+        fvs: &[FeatureValue],
     ) -> CargoResult<()> {
+        log::trace!("activate_pkg {} {}", pkg_id.name(), for_host);
         // Add an empty entry to ensure everything is covered. This is intended for
         // finding bugs where the resolver missed something it should have visited.
         // Remove this in the future if `activated_features` uses an empty default.
         self.activated_features
             .entry((pkg_id, self.opts.decouple_host_deps && for_host))
             .or_insert_with(BTreeSet::new);
         for fv in fvs {
-            self.activate_fv(pkg_id, fv, for_host)?;
+            self.activate_fv(pkg_id, for_host, fv)?;
         }
         if !self.processed_deps.insert((pkg_id, for_host)) {
             // Already processed dependencies. There's no need to process them
@@ -433,7 +448,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
                 }
                 // Recurse into the dependency.
                 let fvs = self.fvs_from_dependency(dep_pkg_id, dep);
-                self.activate_pkg(dep_pkg_id, &fvs, dep_for_host)?;
+                self.activate_pkg(dep_pkg_id, dep_for_host, &fvs)?;
             }
         }
         Ok(())
@@ -443,59 +458,31 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
     fn activate_fv(
         &mut self,
         pkg_id: PackageId,
-        fv: &FeatureValue,
         for_host: bool,
+        fv: &FeatureValue,
     ) -> CargoResult<()> {
+        log::trace!("activate_fv {} {} {}", pkg_id.name(), for_host, fv);
         match fv {
             FeatureValue::Feature(f) => {
-                self.activate_rec(pkg_id, *f, for_host)?;
+                self.activate_rec(pkg_id, for_host, *f)?;
             }
             FeatureValue::Dep { dep_name } => {
-                // Mark this dependency as activated.
-                self.activated_dependencies
-                    .entry((pkg_id, self.opts.decouple_host_deps && for_host))
-                    .or_default()
-                    .insert(*dep_name);
-                // Activate the optional dep.
-                for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
-                    for (dep, dep_for_host) in deps {
-                        if dep.name_in_toml() != *dep_name {
-                            continue;
-                        }
-                        let fvs = self.fvs_from_dependency(dep_pkg_id, dep);
-                        self.activate_pkg(dep_pkg_id, &fvs, dep_for_host)?;
-                    }
-                }
+                self.activate_dependency(pkg_id, for_host, *dep_name)?;
             }
             FeatureValue::DepFeature {
                 dep_name,
                 dep_feature,
                 dep_prefix,
+                weak,
             } => {
-                // Activate a feature within a dependency.
-                for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
-                    for (dep, dep_for_host) in deps {
-                        if dep.name_in_toml() != *dep_name {
-                            continue;
-                        }
-                        if dep.is_optional() {
-                            // Activate the dependency on self.
-                            let fv = FeatureValue::Dep {
-                                dep_name: *dep_name,
-                            };
-                            self.activate_fv(pkg_id, &fv, for_host)?;
-                            if !dep_prefix {
-                                // To retain compatibility with old behavior,
-                                // this also enables a feature of the same
-                                // name.
-                                self.activate_rec(pkg_id, *dep_name, for_host)?;
-                            }
-                        }
-                        // Activate the feature on the dependency.
-                        let fv = FeatureValue::new(*dep_feature);
-                        self.activate_fv(dep_pkg_id, &fv, dep_for_host)?;
-                    }
-                }
+                self.activate_dep_feature(
+                    pkg_id,
+                    for_host,
+                    *dep_name,
+                    *dep_feature,
+                    *dep_prefix,
+                    *weak,
+                )?;
             }
         }
         Ok(())
@@ -506,9 +493,15 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
     fn activate_rec(
         &mut self,
         pkg_id: PackageId,
-        feature_to_enable: InternedString,
         for_host: bool,
+        feature_to_enable: InternedString,
     ) -> CargoResult<()> {
+        log::trace!(
+            "activate_rec {} {} feat={}",
+            pkg_id.name(),
+            for_host,
+            feature_to_enable
+        );
         let enabled = self
             .activated_features
             .entry((pkg_id, self.opts.decouple_host_deps && for_host))
@@ -534,7 +527,111 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
             }
         };
         for fv in fvs {
-            self.activate_fv(pkg_id, fv, for_host)?;
+            self.activate_fv(pkg_id, for_host, fv)?;
+        }
+        Ok(())
+    }
+
+    /// Activate a dependency (`dep:dep_name` syntax).
+    fn activate_dependency(
+        &mut self,
+        pkg_id: PackageId,
+        for_host: bool,
+        dep_name: InternedString,
+    ) -> CargoResult<()> {
+        // Mark this dependency as activated.
+        let save_for_host = self.opts.decouple_host_deps && for_host;
+        self.activated_dependencies
+            .entry((pkg_id, save_for_host))
+            .or_default()
+            .insert(dep_name);
+        // Check for any deferred features.
+        let to_enable = self
+            .deferred_weak_dependencies
+            .remove(&(pkg_id, for_host, dep_name));
+        // Activate the optional dep.
+        for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
+            for (dep, dep_for_host) in deps {
+                if dep.name_in_toml() != dep_name {
+                    continue;
+                }
+                if let Some(to_enable) = &to_enable {
+                    for (dep_feature, dep_prefix) in to_enable {
+                        log::trace!(
+                            "activate deferred {} {} -> {}/{}",
+                            pkg_id.name(),
+                            for_host,
+                            dep_name,
+                            dep_feature
+                        );
+                        if !dep_prefix {
+                            self.activate_rec(pkg_id, for_host, dep_name)?;
+                        }
+                        let fv = FeatureValue::new(*dep_feature);
+                        self.activate_fv(dep_pkg_id, dep_for_host, &fv)?;
+                    }
+                }
+                let fvs = self.fvs_from_dependency(dep_pkg_id, dep);
+                self.activate_pkg(dep_pkg_id, dep_for_host, &fvs)?;
+            }
+        }
+        Ok(())
+    }
+
+    /// Activate a feature within a dependency (`dep_name/feat_name` syntax).
+    fn activate_dep_feature(
+        &mut self,
+        pkg_id: PackageId,
+        for_host: bool,
+        dep_name: InternedString,
+        dep_feature: InternedString,
+        dep_prefix: bool,
+        weak: bool,
+    ) -> CargoResult<()> {
+        for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) {
+            for (dep, dep_for_host) in deps {
+                if dep.name_in_toml() != dep_name {
+                    continue;
+                }
+                if dep.is_optional() {
+                    let save_for_host = self.opts.decouple_host_deps && for_host;
+                    if weak
+                        && !self
+                            .activated_dependencies
+                            .get(&(pkg_id, save_for_host))
+                            .map(|deps| deps.contains(&dep_name))
+                            .unwrap_or(false)
+                    {
+                        // This is weak, but not yet activated. Defer in case
+                        // something comes along later and enables it.
+                        log::trace!(
+                            "deferring feature {} {} -> {}/{}",
+                            pkg_id.name(),
+                            for_host,
+                            dep_name,
+                            dep_feature
+                        );
+                        self.deferred_weak_dependencies
+                            .entry((pkg_id, for_host, dep_name))
+                            .or_default()
+                            .insert((dep_feature, dep_prefix));
+                        continue;
+                    }
+
+                    // Activate the dependency on self.
+                    let fv = FeatureValue::Dep { dep_name };
+                    self.activate_fv(pkg_id, for_host, &fv)?;
+                    if !dep_prefix {
+                        // To retain compatibility with old behavior,
+                        // this also enables a feature of the same
+                        // name.
+                        self.activate_rec(pkg_id, for_host, dep_name)?;
+                    }
+                }
+                // Activate the feature on the dependency.
+                let fv = FeatureValue::new(dep_feature);
+                self.activate_fv(dep_pkg_id, dep_for_host, &fv)?;
+            }
         }
         Ok(())
     }

src/cargo/core/summary.rs

@@ -86,7 +86,11 @@ impl Summary {
 
     /// Returns an error if this Summary is using an unstable feature that is
     /// not enabled.
-    pub fn unstable_gate(&self, namespaced_features: bool) -> CargoResult<()> {
+    pub fn unstable_gate(
+        &self,
+        namespaced_features: bool,
+        weak_dep_features: bool,
+    ) -> CargoResult<()> {
         if !namespaced_features {
             if self.inner.has_namespaced_features {
                 bail!(
@@ -101,6 +105,22 @@ impl Summary {
                 )
             }
         }
+        if !weak_dep_features {
+            for (feat_name, features) in self.features() {
+                for fv in features {
+                    if matches!(fv, FeatureValue::DepFeature{weak: true, ..}) {
+                        bail!(
+                            "optional dependency features with `?` syntax are only \
+                             allowed on the nightly channel and requires the \
+                             `-Z weak-dep-features` flag on the command line\n\
+                             Feature `{}` had feature value `{}`.",
+                            feat_name,
+                            fv
+                        );
+                    }
+                }
+            }
+        }
         Ok(())
     }
 
@@ -293,7 +313,7 @@ fn build_feature_map(
                         );
                     }
                 }
-                DepFeature { dep_name, .. } => {
+                DepFeature { dep_name, weak, .. } => {
                     // Validation of the feature name will be performed in the resolver.
                     if !is_any_dep {
                         bail!(
@@ -303,6 +323,12 @@ fn build_feature_map(
                             dep_name
                         );
                     }
+                    if *weak && !is_optional_dep {
+                        bail!("feature `{}` includes `{}` with a `?`, but `{}` is not an optional dependency\n\
+                            A non-optional dependency of the same name is defined; \
+                            consider removing the `?` or changing the dependency to be optional",
+                            feature, fv, dep_name);
+                    }
                 }
             }
         }
@@ -346,6 +372,10 @@ pub enum FeatureValue {
         /// If this is true, then the feature used the `dep:` prefix, which
         /// prevents enabling the feature named `dep_name`.
         dep_prefix: bool,
+        /// If `true`, indicates the `?` syntax is used, which means this will
+        /// not automatically enable the dependency unless the dependency is
+        /// activated through some other means.
+        weak: bool,
     },
 }
 
@@ -360,10 +390,16 @@ impl FeatureValue {
                 } else {
                     (dep, false)
                 };
+                let (dep, weak) = if let Some(dep) = dep.strip_suffix('?') {
+                    (dep, true)
+                } else {
+                    (dep, false)
+                };
                 FeatureValue::DepFeature {
                     dep_name: InternedString::new(dep),
                     dep_feature: InternedString::new(dep_feat),
                     dep_prefix,
+                    weak,
                 }
             }
             None => {
@@ -393,13 +429,13 @@ impl fmt::Display for FeatureValue {
             DepFeature {
                 dep_name,
                 dep_feature,
-                dep_prefix: true,
-            } => write!(f, "dep:{}/{}", dep_name, dep_feature),
-            DepFeature {
-                dep_name,
-                dep_feature,
-                dep_prefix: false,
-            } => write!(f, "{}/{}", dep_name, dep_feature),
+                dep_prefix,
+                weak,
+            } => {
+                let dep_prefix = if *dep_prefix { "dep:" } else { "" };
+                let weak = if *weak { "?" } else { "" };
+                write!(f, "{}{}{}/{}", dep_prefix, dep_name, weak, dep_feature)
+            }
         }
     }
 }

src/cargo/ops/cargo_compile.rs

@@ -1082,11 +1082,20 @@ fn validate_required_features(
                     target_name
                 );
             }
+            FeatureValue::DepFeature { weak: true, .. } => {
+                anyhow::bail!(
+                    "invalid feature `{}` in required-features of target `{}`: \
+                    optional dependency with `?` is not allowed in required-features",
+                    fv,
+                    target_name
+                );
+            }
             // Handling of dependent_crate/dependent_crate_feature syntax
             FeatureValue::DepFeature {
                 dep_name,
                 dep_feature,
                 dep_prefix: false,
+                weak: false,
             } => {
                 match resolve
                     .deps(summary.package_id())

src/cargo/ops/tree/graph.rs

@@ -568,6 +568,11 @@ fn add_feature_rec(
                 dep_name,
                 dep_feature,
                 dep_prefix,
+                // `weak` is ignored, because it will be skipped if the
+                // corresponding dependency is not found in the map, which
+                // means it wasn't activated. Skipping is handled by
+                // `is_dep_activated` when the graph was built.
+                weak: _,
             } => {
                 let dep_indexes = match graph.dep_name_map[&package_index].get(dep_name) {
                     Some(indexes) => indexes.clone(),

src/cargo/sources/registry/index.rs

@@ -272,6 +272,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         let source_id = self.source_id;
         let config = self.config;
         let namespaced_features = self.config.cli_unstable().namespaced_features;
+        let weak_dep_features = self.config.cli_unstable().weak_dep_features;
 
         // First up actually parse what summaries we have available. If Cargo
         // has run previously this will parse a Cargo-specific cache file rather
@@ -299,7 +300,11 @@ impl<'cfg> RegistryIndex<'cfg> {
                     }
                 },
             )
-            .filter(move |is| is.summary.unstable_gate(namespaced_features).is_ok()))
+            .filter(move |is| {
+                is.summary
+                    .unstable_gate(namespaced_features, weak_dep_features)
+                    .is_ok()
+            }))
     }
 
     fn load_summaries(

src/cargo/util/toml/mod.rs

@@ -1197,7 +1197,8 @@ impl TomlManifest {
             me.features.as_ref().unwrap_or(&empty_features),
             project.links.as_deref(),
         )?;
-        summary.unstable_gate(config.cli_unstable().namespaced_features)?;
+        let unstable = config.cli_unstable();
+        summary.unstable_gate(unstable.namespaced_features, unstable.weak_dep_features)?;
 
         let metadata = ManifestMetadata {
             description: project.description.clone(),

src/doc/src/reference/unstable.md

@@ -906,3 +906,26 @@ error[E0308]: mismatched types
 
 error: aborting due to previous error
 ```
+
+### Weak dependency features
+* Tracking Issue: [#8832](https://github.com/rust-lang/cargo/issues/8832)
+
+The `-Z weak-dep-features` command-line options enables the ability to use
+`dep_name?/feat_name` syntax in the `[features]` table. The `?` indicates that
+the optional dependency `dep_name` will not be automatically enabled. The
+feature `feat_name` will only be added if something else enables the
+`dep_name` dependency.
+
+Example:
+
+```toml
+[dependencies]
+serde = { version = "1.0.117", optional = true, default-features = false }
+
+[features]
+std = ["serde?/std"]
+```
+
+In this example, the `std` feature enables the `std` feature on the `serde`
+dependency. However, unlike the normal `serde/std` syntax, it will not enable
+the optional dependency `serde` unless something else has included it.

tests/testsuite/main.rs

@@ -120,6 +120,7 @@ mod vendor;
 mod verify_project;
 mod version;
 mod warn_on_failure;
+mod weak_dep_features;
 mod workspaces;
 mod yank;