Refactor feature handling, and improve error messages.

Author: ehuss

src/bin/cargo/commands/metadata.rs

@@ -44,9 +44,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
     };
 
     let options = OutputMetadataOptions {
-        features: values(args, "features"),
-        all_features: args.is_present("all-features"),
-        no_default_features: args.is_present("no-default-features"),
+        cli_features: args.cli_features()?,
         no_deps: args.is_present("no-deps"),
         filter_platforms: args._values_of("filter-platform"),
         version,

src/bin/cargo/commands/package.rs

@@ -45,9 +45,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
             allow_dirty: args.is_present("allow-dirty"),
             targets: args.targets(),
             jobs: args.jobs()?,
-            features: args._values_of("features"),
-            all_features: args.is_present("all-features"),
-            no_default_features: args.is_present("no-default-features"),
+            cli_features: args.cli_features()?,
         },
     )?;
     Ok(())

src/bin/cargo/commands/publish.rs

@@ -45,9 +45,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
             jobs: args.jobs()?,
             dry_run: args.is_present("dry-run"),
             registry,
-            features: args._values_of("features"),
-            all_features: args.is_present("all-features"),
-            no_default_features: args.is_present("no-default-features"),
+            cli_features: args.cli_features()?,
         },
     )?;
     Ok(())

src/bin/cargo/commands/tree.rs

@@ -190,9 +190,7 @@ subtree of the package given to -p.\n\
     let charset = tree::Charset::from_str(args.value_of("charset").unwrap())
         .map_err(|e| anyhow::anyhow!("{}", e))?;
     let opts = tree::TreeOptions {
-        features: values(args, "features"),
-        all_features: args.is_present("all-features"),
-        no_default_features: args.is_present("no-default-features"),
+        cli_features: args.cli_features()?,
         packages,
         target,
         edge_kinds,

src/cargo/core/compiler/standard_lib.rs

@@ -3,8 +3,8 @@
 use crate::core::compiler::UnitInterner;
 use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit};
 use crate::core::profiles::{Profiles, UnitFor};
-use crate::core::resolver::features::{FeaturesFor, RequestedFeatures, ResolvedFeatures};
-use crate::core::resolver::{HasDevUnits, ResolveOpts};
+use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures};
+use crate::core::resolver::HasDevUnits;
 use crate::core::{Dependency, PackageId, PackageSet, Resolve, SourceId, Workspace};
 use crate::ops::{self, Packages};
 use crate::util::errors::CargoResult;
@@ -107,18 +107,14 @@ pub fn resolve_std<'cfg>(
             "default".to_string(),
         ],
     };
-    // dev_deps setting shouldn't really matter here.
-    let opts = ResolveOpts::new(
-        /*dev_deps*/ false,
-        RequestedFeatures::from_command_line(
-            &features, /*all_features*/ false, /*uses_default_features*/ false,
-        ),
-    );
+    let cli_features = CliFeatures::from_command_line(
+        &features, /*all_features*/ false, /*uses_default_features*/ false,
+    )?;
     let resolve = ops::resolve_ws_with_opts(
         &std_ws,
         target_data,
         requested_targets,
-        &opts,
+        &cli_features,
         &specs,
         HasDevUnits::No,
         crate::core::resolver::features::ForceAllTargets::No,

src/cargo/core/resolver/context.rs

@@ -1,6 +1,7 @@
 use super::dep_cache::RegistryQueryer;
 use super::errors::ActivateResult;
 use super::types::{ConflictMap, ConflictReason, FeaturesSet, ResolveOpts};
+use super::RequestedFeatures;
 use crate::core::{Dependency, PackageId, SourceId, Summary};
 use crate::util::interning::InternedString;
 use crate::util::Graph;
@@ -160,23 +161,32 @@ impl Context {
             }
         }
         debug!("checking if {} is already activated", summary.package_id());
-        if opts.features.all_features {
-            return Ok(false);
-        }
-
-        let has_default_feature = summary.features().contains_key("default");
-        Ok(match self.resolve_features.get(&id) {
-            Some(prev) => {
-                opts.features.features.is_subset(prev)
-                    && (!opts.features.uses_default_features
-                        || prev.contains("default")
-                        || !has_default_feature)
-            }
-            None => {
-                opts.features.features.is_empty()
-                    && (!opts.features.uses_default_features || !has_default_feature)
+        match &opts.features {
+            // This returns `false` for CliFeatures just for simplicity. It
+            // would take a bit of work to compare since they are not in the
+            // same format as DepFeatures (and that may be expensive
+            // performance-wise). Also, it should only occur once for a root
+            // package. The only drawback is that it may re-activate a root
+            // package again, which should only affect performance, but that
+            // should be rare. Cycles should still be detected since those
+            // will have `DepFeatures` edges.
+            RequestedFeatures::CliFeatures(_) => return Ok(false),
+            RequestedFeatures::DepFeatures {
+                features,
+                uses_default_features,
+            } => {
+                let has_default_feature = summary.features().contains_key("default");
+                Ok(match self.resolve_features.get(&id) {
+                    Some(prev) => {
+                        features.is_subset(prev)
+                            && (!uses_default_features
+                                || prev.contains("default")
+                                || !has_default_feature)
+                    }
+                    None => features.is_empty() && (!uses_default_features || !has_default_feature),
+                })
             }
-        })
+        }
     }
 
     /// If the package is active returns the `ContextAge` when it was added

src/cargo/core/resolver/dep_cache.rs

@@ -12,7 +12,9 @@
 use crate::core::resolver::context::Context;
 use crate::core::resolver::errors::describe_path;
 use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
-use crate::core::resolver::{ActivateError, ActivateResult, ResolveOpts};
+use crate::core::resolver::{
+    ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts,
+};
 use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary};
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::interning::InternedString;
@@ -281,15 +283,6 @@ pub fn resolve_features<'b>(
             .unwrap_or(&default_dep)
             .clone();
         base.extend(dep.features().iter());
-        for feature in base.iter() {
-            if feature.contains('/') {
-                return Err(anyhow::format_err!(
-                    "feature names may not contain slashes: `{}`",
-                    feature
-                )
-                .into());
-            }
-        }
         ret.push((dep.clone(), Rc::new(base)));
     }
 
@@ -317,30 +310,46 @@ fn build_requirements<'a, 'b: 'a>(
 ) -> ActivateResult<Requirements<'a>> {
     let mut reqs = Requirements::new(s);
 
-    if opts.features.all_features {
-        for key in s.features().keys() {
-            if let Err(e) = reqs.require_feature(*key) {
+    let handle_default = |uses_default_features, reqs: &mut Requirements<'_>| {
+        if uses_default_features && s.features().contains_key("default") {
+            if let Err(e) = reqs.require_feature(InternedString::new("default")) {
                 return Err(e.into_activate_error(parent, s));
             }
         }
-    } else {
-        for &f in opts.features.features.iter() {
-            let fv = FeatureValue::new(f);
-            if fv.has_dep_prefix() {
-                return Err(ActivateError::Fatal(anyhow::format_err!(
-                    "feature value `{}` is not allowed to use explicit `dep:` syntax",
-                    fv
-                )));
-            }
-            if let Err(e) = reqs.require_value(&fv) {
-                return Err(e.into_activate_error(parent, s));
+        Ok(())
+    };
+
+    match &opts.features {
+        RequestedFeatures::CliFeatures(CliFeatures {
+            features,
+            all_features,
+            uses_default_features,
+        }) => {
+            if *all_features {
+                for key in s.features().keys() {
+                    if let Err(e) = reqs.require_feature(*key) {
+                        return Err(e.into_activate_error(parent, s));
+                    }
+                }
+            } else {
+                for fv in features.iter() {
+                    if let Err(e) = reqs.require_value(&fv) {
+                        return Err(e.into_activate_error(parent, s));
+                    }
+                }
+                handle_default(*uses_default_features, &mut reqs)?;
             }
         }
-    }
-
-    if opts.features.uses_default_features && s.features().contains_key("default") {
-        if let Err(e) = reqs.require_feature(InternedString::new("default")) {
-            return Err(e.into_activate_error(parent, s));
+        RequestedFeatures::DepFeatures {
+            features,
+            uses_default_features,
+        } => {
+            for feature in features.iter() {
+                if let Err(e) = reqs.require_feature(*feature) {
+                    return Err(e.into_activate_error(parent, s));
+                }
+            }
+            handle_default(*uses_default_features, &mut reqs)?;
         }
     }