Auto merge of #9574 - ehuss:weak-namespaced, r=alexcrichton

bors · bors · commit a2589dda38d0 · 2021-06-22T21:02:31.000Z
Unify weak and namespaced features.

This unifies weak and namespaced features in order to simplify the syntax and semantics.  Previously there were four different ways to specify the feature of a dependency:

* `package-name/feature-name` — Enables feature `package-name` on self and enables `feature-name` on the dependency. (Today's behavior.)
* `package-name?/feature-name` — Only enables `feature-name` on the given package if it that package is enabled and will also activates a feature named `package-name` (which must be defined implicitly or explicitly).
* `dep:package-name/feature-name` — Enables dependency `package-name`, and enables `feature-name` on that dependency. This does NOT enable a feature named "package-name".
* `dep:package-name?/feature-name` — Only enables `feature-name` on the given package if it that package is enabled.  This does NOT enable a feature named "package-name".

This changes it so there are only two:

* `package-name/feature-name` — Today's behavior.
* `package-name?/feature-name` — Only enables `feature-name` on the given package if it that package is enabled.  This does NOT enable a feature named "package-name" (the same behavior as `dep:package-name?/feature-name` above).

This is a fairly subtle change, and in most cases probably won't be noticed.  However, it simplifies things which helps with writing documentation and explaining how it works.
diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs
@@ -405,12 +405,12 @@ impl Requirements<'_> {
         &mut self,
         package: InternedString,
         feat: InternedString,
-        dep_prefix: bool,
+        weak: bool,
     ) -> Result<(), RequirementError> {
         // If `package` is indeed an optional dependency then we activate the
         // feature named `package`, but otherwise if `package` is a required
         // dependency then there's no feature associated with it.
-        if !dep_prefix
+        if !weak
             && self
                 .summary
                 .dependencies()
@@ -456,12 +456,11 @@ impl Requirements<'_> {
             FeatureValue::DepFeature {
                 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)?,
+                weak,
+            } => self.require_dep_feature(*dep_name, *dep_feature, *weak)?,
         };
         Ok(())
     }
diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs
@@ -244,10 +244,7 @@ impl CliFeatures {
             match feature {
                 // Maybe call validate_feature_name here once it is an error?
                 FeatureValue::Feature(_) => {}
-                FeatureValue::Dep { .. }
-                | FeatureValue::DepFeature {
-                    dep_prefix: true, ..
-                } => {
+                FeatureValue::Dep { .. } => {
                     bail!(
                         "feature `{}` is not allowed to use explicit `dep:` syntax",
                         feature
@@ -441,10 +438,8 @@ pub struct FeatureResolver<'a, 'cfg> {
     ///
     /// 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)>>,
+    /// set of features to activate.
+    deferred_weak_dependencies: HashMap<(PackageId, bool, InternedString), HashSet<InternedString>>,
 }
 
 impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
@@ -591,17 +586,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
             FeatureValue::DepFeature {
                 dep_name,
                 dep_feature,
-                dep_prefix,
                 weak,
             } => {
-                self.activate_dep_feature(
-                    pkg_id,
-                    for_host,
-                    *dep_name,
-                    *dep_feature,
-                    *dep_prefix,
-                    *weak,
-                )?;
+                self.activate_dep_feature(pkg_id, for_host, *dep_name, *dep_feature, *weak)?;
             }
         }
         Ok(())
@@ -675,17 +662,14 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
                     continue;
                 }
                 if let Some(to_enable) = &to_enable {
-                    for (dep_feature, dep_prefix) in to_enable {
+                    for dep_feature 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)?;
                     }
@@ -704,7 +688,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
         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) {
@@ -733,16 +716,16 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
                         self.deferred_weak_dependencies
                             .entry((pkg_id, for_host, dep_name))
                             .or_default()
-                            .insert((dep_feature, dep_prefix));
+                            .insert(dep_feature);
                         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
+                    if !weak {
+                        // The old behavior before weak dependencies were
+                        // added is to also enables a feature of the same
                         // name.
                         self.activate_rec(pkg_id, for_host, dep_name)?;
                     }
diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs
@@ -218,12 +218,7 @@ fn build_feature_map(
         .values()
         .flatten()
         .filter_map(|fv| match fv {
-            Dep { dep_name }
-            | DepFeature {
-                dep_name,
-                dep_prefix: true,
-                ..
-            } => Some(*dep_name),
+            Dep { dep_name } => Some(*dep_name),
             _ => None,
         })
         .collect();
@@ -391,9 +386,6 @@ pub enum FeatureValue {
     DepFeature {
         dep_name: InternedString,
         dep_feature: InternedString,
-        /// 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.
@@ -407,11 +399,6 @@ impl FeatureValue {
             Some(pos) => {
                 let (dep, dep_feat) = feature.split_at(pos);
                 let dep_feat = &dep_feat[1..];
-                let (dep, dep_prefix) = if let Some(dep) = dep.strip_prefix("dep:") {
-                    (dep, true)
-                } else {
-                    (dep, false)
-                };
                 let (dep, weak) = if let Some(dep) = dep.strip_suffix('?') {
                     (dep, true)
                 } else {
@@ -420,7 +407,6 @@ impl FeatureValue {
                 FeatureValue::DepFeature {
                     dep_name: InternedString::new(dep),
                     dep_feature: InternedString::new(dep_feat),
-                    dep_prefix,
                     weak,
                 }
             }
@@ -438,14 +424,7 @@ impl FeatureValue {
 
     /// Returns `true` if this feature explicitly used `dep:` syntax.
     pub fn has_dep_prefix(&self) -> bool {
-        matches!(
-            self,
-            FeatureValue::Dep { .. }
-                | FeatureValue::DepFeature {
-                    dep_prefix: true,
-                    ..
-                }
-        )
+        matches!(self, FeatureValue::Dep { .. })
     }
 }
 
@@ -458,12 +437,10 @@ impl fmt::Display for FeatureValue {
             DepFeature {
                 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)
+                write!(f, "{}{}/{}", dep_name, weak, dep_feature)
             }
         }
     }
diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs
@@ -1163,14 +1163,10 @@ impl<'cfg> Workspace<'cfg> {
                     }
                 }
                 // This should be enforced by CliFeatures.
-                FeatureValue::Dep { .. }
-                | FeatureValue::DepFeature {
-                    dep_prefix: true, ..
-                } => panic!("unexpected dep: syntax {}", feature),
+                FeatureValue::Dep { .. } => panic!("unexpected dep: syntax {}", feature),
                 FeatureValue::DepFeature {
                     dep_name,
                     dep_feature,
-                    dep_prefix: _,
                     weak: _,
                 } => {
                     if dependencies.contains_key(dep_name) {
@@ -1283,14 +1279,10 @@ impl<'cfg> Workspace<'cfg> {
                         .map(|s| s.to_string())
                         .collect::<Vec<_>>()
                 }
-                FeatureValue::Dep { .. }
-                | FeatureValue::DepFeature {
-                    dep_prefix: true, ..
-                } => panic!("unexpected dep: syntax {}", feature),
+                FeatureValue::Dep { .. } => panic!("unexpected dep: syntax {}", feature),
                 FeatureValue::DepFeature {
                     dep_name,
                     dep_feature,
-                    dep_prefix: _,
                     weak: _,
                 } => {
                     // Finds set of `pkg/feat` that are very similar to current `pkg/feat`.
@@ -1446,14 +1438,10 @@ impl<'cfg> Workspace<'cfg> {
                     cwd_features.insert(feature.clone());
                 }
                 // This should be enforced by CliFeatures.
-                FeatureValue::Dep { .. }
-                | FeatureValue::DepFeature {
-                    dep_prefix: true, ..
-                } => panic!("unexpected dep: syntax {}", feature),
+                FeatureValue::Dep { .. } => panic!("unexpected dep: syntax {}", feature),
                 FeatureValue::DepFeature {
                     dep_name,
                     dep_feature,
-                    dep_prefix: _,
                     weak: _,
                 } => {
                     // I think weak can be ignored here.
diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs
@@ -1225,10 +1225,7 @@ fn validate_required_features(
                     ))?;
                 }
             }
-            FeatureValue::Dep { .. }
-            | FeatureValue::DepFeature {
-                dep_prefix: true, ..
-            } => {
+            FeatureValue::Dep { .. } => {
                 anyhow::bail!(
                     "invalid feature `{}` in required-features of target `{}`: \
                     `dep:` prefixed feature values are not allowed in required-features",
@@ -1248,7 +1245,6 @@ fn validate_required_features(
             FeatureValue::DepFeature {
                 dep_name,
                 dep_feature,
-                dep_prefix: false,
                 weak: false,
             } => {
                 match resolve
diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs
@@ -488,7 +488,6 @@ fn add_cli_features(
             FeatureValue::DepFeature {
                 dep_name,
                 dep_feature,
-                dep_prefix: _,
                 weak,
             } => {
                 let dep_connections = match graph.dep_name_map[&package_index].get(&dep_name) {
@@ -596,12 +595,12 @@ fn add_feature_rec(
             FeatureValue::DepFeature {
                 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: _,
+                // Note: `weak` is mostly handled when the graph is built in
+                // `is_dep_activated` which is responsible for skipping
+                // unactivated weak dependencies. Here it is only used to
+                // determine if the feature of the dependency name is
+                // activated on self.
+                weak,
             } => {
                 let dep_indexes = match graph.dep_name_map[&package_index].get(dep_name) {
                     Some(indexes) => indexes.clone(),
@@ -619,7 +618,7 @@ fn add_feature_rec(
                 };
                 for (dep_index, is_optional) in dep_indexes {
                     let dep_pkg_id = graph.package_id_for_index(dep_index);
-                    if is_optional && !dep_prefix {
+                    if is_optional && !weak {
                         // Activate the optional dep on self.
                         add_feature(
                             graph,
diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs
diff --git a/tests/testsuite/weak_dep_features.rs b/tests/testsuite/weak_dep_features.rs
