Auto merge of #9290 - ehuss:non-member-features, r=alexcrichton

Refactor feature handling, and improve error messages.

This changes the way feature strings are handled with an eye towards fixing some improper handling and to improve error messages. The key change is to stop treating all features as free-form strings and instead try to handle them as typed values. This helps avoid needing to deal with parsing different feature syntax (like `dep:` or `foo/bar`) or forgetting to handle it properly.

Overview of refactoring changes:

* `RequestedFeatures` changed to an enum to differentiate between features coming from the command-line, and those that are from a dependency.
* Moved parsing of CLI features to an earlier stage (now stored in `CompileOptions`), and ensures that they are properly handled as `FeatureValue` instead of strings.
* Pushed some feature validation earlier. For example, `DetailedTomlDependency` now validates things so you can see the location for the errant `Cargo.toml` (previously some validation was deep in the resolver, which provided poor errors).

This is a pretty large PR, but at the core it is just changing `RequestedFeatures` and then dealing with the fallout from that. Hopefully this is an improvement overall.

List of user-visible changes:

* Fix handling in resolver V2 of `--features bar?/feat` and `--features dep:bar`
* Better error handling for `bar/feat` and `dep:bar` in dependency declarations.
* Feature keys in the `[features]` table can no longer contain slashes.
* Fixed a minor issue with `cargo tree -e features --all-features -Z namespaced-features`
* Fixed a panic with `cargo tree` involving `-Z weak-dep-features`

I did a small amount of benchmarking, and I wasn't able to record much of a difference.

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)?;
         }
     }