Simplify checking feature syntax

Author: Eh2406

src/cargo/core/summary.rs

@@ -156,13 +156,15 @@ fn build_feature_map(
     dependencies: &[Dependency],
 ) -> CargoResult<FeatureMap> {
     use self::FeatureValue::*;
-    let mut dep_map = HashMap::new();
+    // A map of dependency names to whether there are any that are optional.
+    let mut dep_map: HashMap<InternedString, bool> = HashMap::new();
     for dep in dependencies.iter() {
-        dep_map
-            .entry(dep.name_in_toml())
-            .or_insert_with(Vec::new)
-            .push(dep);
+        let has_optional = dep_map.entry(dep.name_in_toml()).or_insert(false);
+        if dep.is_optional() {
+            *has_optional = true;
+        }
     }
+    let dep_map = dep_map; // We are done mutating this variable
 
     let mut map: FeatureMap = features
         .iter()
@@ -180,117 +182,78 @@ fn build_feature_map(
     let explicitly_listed: HashSet<_> = map
         .values()
         .flatten()
-        .filter_map(|fv| match fv {
-            Dep { dep_name } => Some(*dep_name),
-            _ => None,
-        })
+        .filter_map(|fv| fv.explicitly_name())
         .collect();
     for dep in dependencies {
         if !dep.is_optional() {
             continue;
         }
-        let dep_name_in_toml = dep.name_in_toml();
-        if features.contains_key(&dep_name_in_toml) || explicitly_listed.contains(&dep_name_in_toml)
-        {
+        let dep_name = dep.name_in_toml();
+        if features.contains_key(&dep_name) || explicitly_listed.contains(&dep_name) {
             continue;
         }
-        let fv = Dep {
-            dep_name: dep_name_in_toml,
-        };
-        map.insert(dep_name_in_toml, vec![fv]);
+        map.insert(dep_name, vec![Dep { dep_name }]);
     }
+    let map = map; // We are done mutating this variable
 
     // Validate features are listed properly.
     for (feature, fvs) in &map {
         FeatureName::new(feature)?;
         for fv in fvs {
             // Find data for the referenced dependency...
-            let dep_data = {
-                match fv {
-                    Feature(dep_name) | Dep { dep_name, .. } | DepFeature { dep_name, .. } => {
-                        dep_map.get(dep_name)
-                    }
-                }
-            };
-            let is_optional_dep = dep_data
-                .iter()
-                .flat_map(|d| d.iter())
-                .any(|d| d.is_optional());
+            let dep_data = dep_map.get(&fv.dep_name());
             let is_any_dep = dep_data.is_some();
+            let is_optional_dep = dep_data.is_some_and(|&o| o);
             match fv {
                 Feature(f) => {
                     if !features.contains_key(f) {
                         if !is_any_dep {
                             bail!(
-                                "feature `{}` includes `{}` which is neither a dependency \
-                                 nor another feature",
-                                feature,
-                                fv
-                            );
+                                "feature `{feature}` includes `{fv}` which is neither a dependency \
+                                 nor another feature"                              
+                              );
                         }
                         if is_optional_dep {
                             if !map.contains_key(f) {
                                 bail!(
-                                    "feature `{}` includes `{}`, but `{}` is an \
+                                    "feature `{feature}` includes `{fv}`, but `{f}` is an \
                                      optional dependency without an implicit feature\n\
-                                     Use `dep:{}` to enable the dependency.",
-                                    feature,
-                                    fv,
-                                    f,
-                                    f
+                                     Use `dep:{f}` to enable the dependency."
                                 );
                             }
                         } else {
-                            bail!("feature `{}` includes `{}`, but `{}` is not an optional dependency\n\
+                            bail!("feature `{feature}` includes `{fv}`, but `{f}` is not an optional dependency\n\
                                 A non-optional dependency of the same name is defined; \
-                                consider adding `optional = true` to its definition.",
-                                feature, fv, f);
+                                consider adding `optional = true` to its definition.");
                         }
                     }
                 }
                 Dep { dep_name } => {
                     if !is_any_dep {
-                        bail!(
-                            "feature `{}` includes `{}`, but `{}` is not listed as a dependency",
-                            feature,
-                            fv,
-                            dep_name
-                        );
+                        bail!("feature `{feature}` includes `{fv}`, but `{dep_name}` is not listed as a dependency");
                     }
                     if !is_optional_dep {
                         bail!(
-                            "feature `{}` includes `{}`, but `{}` is not an optional dependency\n\
+                            "feature `{feature}` includes `{fv}`, but `{dep_name}` is not an optional dependency\n\
                              A non-optional dependency of the same name is defined; \
-                             consider adding `optional = true` to its definition.",
-                            feature,
-                            fv,
-                            dep_name
+                             consider adding `optional = true` to its definition."
                         );
                     }
                 }
                 DepFeature {
                     dep_name,
                     dep_feature,
                     weak,
-                    ..
                 } => {
                     // Early check for some unlikely syntax.
                     if dep_feature.contains('/') {
-                        bail!(
-                            "multiple slashes in feature `{}` (included by feature `{}`) are not allowed",
-                            fv,
-                            feature
-                        );
+                        bail!("multiple slashes in feature `{fv}` (included by feature `{feature}`) are not allowed");
                     }
 
                     // dep: cannot be combined with /
                     if let Some(stripped_dep) = dep_name.strip_prefix("dep:") {
                         let has_other_dep = explicitly_listed.contains(stripped_dep);
-                        let is_optional = dep_map
-                            .get(stripped_dep)
-                            .iter()
-                            .flat_map(|d| d.iter())
-                            .any(|d| d.is_optional());
+                        let is_optional = dep_map.get(stripped_dep).is_some_and(|&o| o);
                         let extra_help = if *weak || has_other_dep || !is_optional {
                             // In this case, the user should just remove dep:.
                             // Note that "hiding" an optional dependency
@@ -314,18 +277,14 @@ fn build_feature_map(
 
                     // Validation of the feature name will be performed in the resolver.
                     if !is_any_dep {
-                        bail!(
-                            "feature `{}` includes `{}`, but `{}` is not a dependency",
-                            feature,
-                            fv,
-                            dep_name
-                        );
+                        bail!("feature `{feature}` includes `{fv}`, but `{dep_name}` is not a dependency");
                     }
                     if *weak && !is_optional_dep {
-                        bail!("feature `{}` includes `{}` with a `?`, but `{}` is not an optional dependency\n\
+                        bail!(
+                            "feature `{feature}` includes `{fv}` with a `?`, but `{dep_name}` 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);
+                            consider removing the `?` or changing the dependency to be optional"
+                        );
                     }
                 }
             }
@@ -341,15 +300,13 @@ fn build_feature_map(
             _ => None,
         })
         .collect();
-    if let Some(dep) = dependencies
+    if let Some((dep, _)) = dep_map
         .iter()
-        .find(|dep| dep.is_optional() && !used.contains(&dep.name_in_toml()))
+        .find(|&(dep, &is_optional)| is_optional && !used.contains(dep))
     {
         bail!(
-            "optional dependency `{}` is not included in any feature\n\
-            Make sure that `dep:{}` is included in one of features in the [features] table.",
-            dep.name_in_toml(),
-            dep.name_in_toml(),
+            "optional dependency `{dep}` is not included in any feature\n\
+            Make sure that `dep:{dep}` is included in one of features in the [features] table."
         );
     }
 
@@ -376,19 +333,13 @@ pub enum FeatureValue {
 
 impl FeatureValue {
     pub fn new(feature: InternedString) -> FeatureValue {
-        match feature.find('/') {
-            Some(pos) => {
-                let (dep, dep_feat) = feature.split_at(pos);
-                let dep_feat = &dep_feat[1..];
-                let (dep, weak) = if let Some(dep) = dep.strip_suffix('?') {
-                    (dep, true)
-                } else {
-                    (dep, false)
-                };
+        match feature.split_once('/') {
+            Some((dep, dep_feat)) => {
+                let dep_name = dep.strip_suffix('?');
                 FeatureValue::DepFeature {
-                    dep_name: InternedString::new(dep),
+                    dep_name: InternedString::new(dep_name.unwrap_or(dep)),
                     dep_feature: InternedString::new(dep_feat),
-                    weak,
+                    weak: dep_name.is_some(),
                 }
             }
             None => {
@@ -403,25 +354,35 @@ impl FeatureValue {
         }
     }
 
-    /// Returns `true` if this feature explicitly used `dep:` syntax.
-    pub fn has_dep_prefix(&self) -> bool {
-        matches!(self, FeatureValue::Dep { .. })
+    fn explicitly_name(&self) -> Option<InternedString> {
+        match self {
+            FeatureValue::Dep { dep_name, .. } => Some(*dep_name),
+            _ => None,
+        }
+    }
+
+    fn dep_name(&self) -> InternedString {
+        match self {
+            FeatureValue::Feature(dep_name)
+            | FeatureValue::Dep { dep_name, .. }
+            | FeatureValue::DepFeature { dep_name, .. } => *dep_name,
+        }
     }
 }
 
 impl fmt::Display for FeatureValue {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use self::FeatureValue::*;
         match self {
-            Feature(feat) => write!(f, "{}", feat),
-            Dep { dep_name } => write!(f, "dep:{}", dep_name),
+            Feature(feat) => write!(f, "{feat}"),
+            Dep { dep_name } => write!(f, "dep:{dep_name}"),
             DepFeature {
                 dep_name,
                 dep_feature,
                 weak,
             } => {
                 let weak = if *weak { "?" } else { "" };
-                write!(f, "{}{}/{}", dep_name, weak, dep_feature)
+                write!(f, "{dep_name}{weak}/{dep_feature}")
             }
         }
     }