Simplify feature representation in CargoConfig

5 files changed, 109 insertions, 90 deletions

diff --git a/crates/project-model/src/build_scripts.rs b/crates/project-model/src/build_scripts.rs
index 837ea016193..32db42f1db7 100644
--- a/crates/project-model/src/build_scripts.rs
+++ b/crates/project-model/src/build_scripts.rs
@@ -15,7 +15,7 @@ use rustc_hash::FxHashMap;
 use semver::Version;
 use serde::Deserialize;
 
-use crate::{cfg_flag::CfgFlag, CargoConfig, CargoWorkspace, Package};
+use crate::{cfg_flag::CfgFlag, CargoConfig, CargoFeatures, CargoWorkspace, Package};
 
 #[derive(Debug, Default, Clone, PartialEq, Eq)]
 pub struct WorkspaceBuildScripts {
@@ -49,7 +49,6 @@ impl WorkspaceBuildScripts {
 
         let mut cmd = Command::new(toolchain::cargo());
         cmd.envs(&config.extra_env);
-
         cmd.args(&["check", "--quiet", "--workspace", "--message-format=json"]);
 
         // --all-targets includes tests, benches and examples in addition to the
@@ -61,15 +60,18 @@ impl WorkspaceBuildScripts {
             cmd.args(&["--target", target]);
         }
 
-        if config.all_features {
-            cmd.arg("--all-features");
-        } else {
-            if config.no_default_features {
-                cmd.arg("--no-default-features");
+        match &config.features {
+            CargoFeatures::All => {
+                cmd.arg("--all-features");
             }
-            if !config.features.is_empty() {
-                cmd.arg("--features");
-                cmd.arg(config.features.join(" "));
+            CargoFeatures::Selected { features, no_default_features } => {
+                if *no_default_features {
+                    cmd.arg("--no-default-features");
+                }
+                if !features.is_empty() {
+                    cmd.arg("--features");
+                    cmd.arg(features.join(" "));
+                }
             }
         }
 
diff --git a/crates/project-model/src/cargo_workspace.rs b/crates/project-model/src/cargo_workspace.rs
index 736d80041bd..8e50fe878ae 100644
--- a/crates/project-model/src/cargo_workspace.rs
+++ b/crates/project-model/src/cargo_workspace.rs
@@ -71,35 +71,41 @@ impl Default for UnsetTestCrates {
     }
 }
 
-#[derive(Default, Clone, Debug, PartialEq, Eq)]
-pub struct CargoConfig {
-    /// Do not activate the `default` feature.
-    pub no_default_features: bool,
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum CargoFeatures {
+    All,
+    Selected {
+        /// List of features to activate.
+        features: Vec<String>,
+        /// Do not activate the `default` feature.
+        no_default_features: bool,
+    },
+}
 
-    /// Activate all available features
-    pub all_features: bool,
+impl Default for CargoFeatures {
+    fn default() -> Self {
+        CargoFeatures::Selected { features: vec![], no_default_features: false }
+    }
+}
 
+#[derive(Default, Clone, Debug, PartialEq, Eq)]
+pub struct CargoConfig {
     /// List of features to activate.
-    /// This will be ignored if `cargo_all_features` is true.
-    pub features: Vec<String>,
-
+    pub features: CargoFeatures,
     /// rustc target
     pub target: Option<String>,
-
     /// Don't load sysroot crates (`std`, `core` & friends). Might be useful
     /// when debugging isolated issues.
     pub no_sysroot: bool,
-
     /// rustc private crate source
     pub rustc_source: Option<RustcSource>,
-
     /// crates to disable `#[cfg(test)]` on
     pub unset_test_crates: UnsetTestCrates,
-
+    /// Invoke `cargo check` through the RUSTC_WRAPPER.
     pub wrap_rustc_in_build_scripts: bool,
-
+    /// The command to run instead of `cargo check` for building build scripts.
     pub run_build_script_command: Option<Vec<String>>,
-
+    /// Extra env vars to set when invoking the cargo command
     pub extra_env: FxHashMap<String, String>,
 }
 
@@ -143,7 +149,7 @@ pub struct PackageData {
     pub targets: Vec<Target>,
     /// Does this package come from the local filesystem (and is editable)?
     pub is_local: bool,
-    // Whether this package is a member of the workspace
+    /// Whether this package is a member of the workspace
     pub is_member: bool,
     /// List of packages this package depends on
     pub dependencies: Vec<PackageDependency>,
@@ -249,8 +255,8 @@ impl TargetKind {
     }
 }
 
+// Deserialize helper for the cargo metadata
 #[derive(Deserialize, Default)]
-// Deserialise helper for the cargo metadata
 struct PackageMetadata {
     #[serde(rename = "rust-analyzer")]
     rust_analyzer: Option<RustAnalyzerPackageMetaData>,
@@ -272,16 +278,19 @@ impl CargoWorkspace {
         let mut meta = MetadataCommand::new();
         meta.cargo_path(toolchain::cargo());
         meta.manifest_path(cargo_toml.to_path_buf());
-        if config.all_features {
-            meta.features(CargoOpt::AllFeatures);
-        } else {
-            if config.no_default_features {
-                // FIXME: `NoDefaultFeatures` is mutual exclusive with `SomeFeatures`
-                // https://github.com/oli-obk/cargo_metadata/issues/79
-                meta.features(CargoOpt::NoDefaultFeatures);
+        match &config.features {
+            CargoFeatures::All => {
+                meta.features(CargoOpt::AllFeatures);
             }
-            if !config.features.is_empty() {
-                meta.features(CargoOpt::SomeFeatures(config.features.clone()));
+            CargoFeatures::Selected { features, no_default_features } => {
+                if *no_default_features {
+                    // FIXME: `NoDefaultFeatures` is mutual exclusive with `SomeFeatures`
+                    // https://github.com/oli-obk/cargo_metadata/issues/79
+                    meta.features(CargoOpt::NoDefaultFeatures);
+                }
+                if !features.is_empty() {
+                    meta.features(CargoOpt::SomeFeatures(features.clone()));
+                }
             }
         }
         meta.current_dir(current_dir.as_os_str());
diff --git a/crates/project-model/src/lib.rs b/crates/project-model/src/lib.rs
index b81b7432f65..ce78ce85697 100644
--- a/crates/project-model/src/lib.rs
+++ b/crates/project-model/src/lib.rs
@@ -42,8 +42,8 @@ use rustc_hash::FxHashSet;
 pub use crate::{
     build_scripts::WorkspaceBuildScripts,
     cargo_workspace::{
-        CargoConfig, CargoWorkspace, Package, PackageData, PackageDependency, RustcSource, Target,
-        TargetData, TargetKind, UnsetTestCrates,
+        CargoConfig, CargoFeatures, CargoWorkspace, Package, PackageData, PackageDependency,
+        RustcSource, Target, TargetData, TargetKind, UnsetTestCrates,
     },
     manifest_path::ManifestPath,
     project_json::{ProjectJson, ProjectJsonData},
diff --git a/crates/rust-analyzer/src/cargo_target_spec.rs b/crates/rust-analyzer/src/cargo_target_spec.rs
index 1c39e9391af..e1675a030c0 100644
--- a/crates/rust-analyzer/src/cargo_target_spec.rs
+++ b/crates/rust-analyzer/src/cargo_target_spec.rs
@@ -4,7 +4,7 @@ use std::mem;
 
 use cfg::{CfgAtom, CfgExpr};
 use ide::{FileId, RunnableKind, TestId};
-use project_model::{self, ManifestPath, TargetKind};
+use project_model::{self, CargoFeatures, ManifestPath, TargetKind};
 use vfs::AbsPathBuf;
 
 use crate::{global_state::GlobalStateSnapshot, Result};
@@ -35,41 +35,41 @@ impl CargoTargetSpec {
 
         match kind {
             RunnableKind::Test { test_id, attr } => {
-                args.push("test".to_string());
+                args.push("test".to_owned());
                 extra_args.push(test_id.to_string());
                 if let TestId::Path(_) = test_id {
-                    extra_args.push("--exact".to_string());
+                    extra_args.push("--exact".to_owned());
                 }
-                extra_args.push("--nocapture".to_string());
+                extra_args.push("--nocapture".to_owned());
                 if attr.ignore {
-                    extra_args.push("--ignored".to_string());
+                    extra_args.push("--ignored".to_owned());
                 }
             }
             RunnableKind::TestMod { path } => {
-                args.push("test".to_string());
-                extra_args.push(path.to_string());
-                extra_args.push("--nocapture".to_string());
+                args.push("test".to_owned());
+                extra_args.push(path.clone());
+                extra_args.push("--nocapture".to_owned());
             }
             RunnableKind::Bench { test_id } => {
-                args.push("bench".to_string());
+                args.push("bench".to_owned());
                 extra_args.push(test_id.to_string());
                 if let TestId::Path(_) = test_id {
-                    extra_args.push("--exact".to_string());
+                    extra_args.push("--exact".to_owned());
                 }
-                extra_args.push("--nocapture".to_string());
+                extra_args.push("--nocapture".to_owned());
             }
             RunnableKind::DocTest { test_id } => {
-                args.push("test".to_string());
-                args.push("--doc".to_string());
+                args.push("test".to_owned());
+                args.push("--doc".to_owned());
                 extra_args.push(test_id.to_string());
-                extra_args.push("--nocapture".to_string());
+                extra_args.push("--nocapture".to_owned());
             }
             RunnableKind::Bin => {
                 let subcommand = match spec {
                     Some(CargoTargetSpec { target_kind: TargetKind::Test, .. }) => "test",
                     _ => "run",
                 };
-                args.push(subcommand.to_string());
+                args.push(subcommand.to_owned());
             }
         }
 
@@ -82,29 +82,35 @@ impl CargoTargetSpec {
         };
 
         let cargo_config = snap.config.cargo();
-        if cargo_config.all_features {
-            args.push("--all-features".to_string());
 
-            for feature in target_required_features {
-                args.push("--features".to_string());
-                args.push(feature);
-            }
-        } else {
-            let mut features = Vec::new();
-            if let Some(cfg) = cfg.as_ref() {
-                required_features(cfg, &mut features);
+        match &cargo_config.features {
+            CargoFeatures::All => {
+                args.push("--all-features".to_owned());
+                for feature in target_required_features {
+                    args.push("--features".to_owned());
+                    args.push(feature);
+                }
             }
+            CargoFeatures::Selected { features, no_default_features } => {
+                let mut feats = Vec::new();
+                if let Some(cfg) = cfg.as_ref() {
+                    required_features(cfg, &mut feats);
+                }
 
-            features.extend(cargo_config.features);
-            features.extend(target_required_features);
+                feats.extend(features.iter().cloned());
+                feats.extend(target_required_features);
 
-            features.dedup();
-            for feature in features {
-                args.push("--features".to_string());
-                args.push(feature);
+                feats.dedup();
+                for feature in feats {
+                    args.push("--features".to_owned());
+                    args.push(feature);
+                }
+
+                if *no_default_features {
+                    args.push("--no-default-features".to_owned());
+                }
             }
         }
-
         Ok((args, extra_args))
     }
 
@@ -136,7 +142,7 @@ impl CargoTargetSpec {
     }
 
     pub(crate) fn push_to(self, buf: &mut Vec<String>, kind: &RunnableKind) {
-        buf.push("--package".to_string());
+        buf.push("--package".to_owned());
         buf.push(self.package);
 
         // Can't mix --doc with other target flags
@@ -145,23 +151,23 @@ impl CargoTargetSpec {
         }
         match self.target_kind {
             TargetKind::Bin => {
-                buf.push("--bin".to_string());
+                buf.push("--bin".to_owned());
                 buf.push(self.target);
             }
             TargetKind::Test => {
-                buf.push("--test".to_string());
+                buf.push("--test".to_owned());
                 buf.push(self.target);
             }
             TargetKind::Bench => {
-                buf.push("--bench".to_string());
+                buf.push("--bench".to_owned());
                 buf.push(self.target);
             }
             TargetKind::Example => {
-                buf.push("--example".to_string());
+                buf.push("--example".to_owned());
                 buf.push(self.target);
             }
             TargetKind::Lib => {
-                buf.push("--lib".to_string());
+                buf.push("--lib".to_owned());
             }
             TargetKind::Other | TargetKind::BuildScript => (),
         }
diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs
index 9ef79e6f381..0d0e246029b 100644
--- a/crates/rust-analyzer/src/config.rs
+++ b/crates/rust-analyzer/src/config.rs
@@ -22,7 +22,8 @@ use ide_db::{
 use itertools::Itertools;
 use lsp_types::{ClientCapabilities, MarkupKind};
 use project_model::{
-    CargoConfig, ProjectJson, ProjectJsonData, ProjectManifest, RustcSource, UnsetTestCrates,
+    CargoConfig, CargoFeatures, ProjectJson, ProjectJsonData, ProjectManifest, RustcSource,
+    UnsetTestCrates,
 };
 use rustc_hash::{FxHashMap, FxHashSet};
 use serde::{de::DeserializeOwned, Deserialize};
@@ -90,7 +91,7 @@ config_data! {
         /// List of features to activate.
         ///
         /// Set this to `"all"` to pass `--all-features` to cargo.
-        cargo_features: CargoFeatures      = "[]",
+        cargo_features: CargoFeaturesDef      = "[]",
         /// Whether to pass `--no-default-features` to cargo.
         cargo_noDefaultFeatures: bool    = "false",
         /// Internal config for debugging, disables loading of sysroot crates.
@@ -114,7 +115,7 @@ config_data! {
         /// `#rust-analyzer.cargo.features#`.
         ///
         /// Set to `"all"` to pass `--all-features` to Cargo.
-        checkOnSave_features: Option<CargoFeatures>      = "null",
+        checkOnSave_features: Option<CargoFeaturesDef>      = "null",
         /// Whether to pass `--no-default-features` to Cargo. Defaults to
         /// `#rust-analyzer.cargo.noDefaultFeatures#`.
         checkOnSave_noDefaultFeatures: Option<bool>      = "null",
@@ -1028,11 +1029,12 @@ impl Config {
         });
 
         CargoConfig {
-            no_default_features: self.data.cargo_noDefaultFeatures,
-            all_features: matches!(self.data.cargo_features, CargoFeatures::All),
             features: match &self.data.cargo_features {
-                CargoFeatures::All => vec![],
-                CargoFeatures::Listed(it) => it.clone(),
+                CargoFeaturesDef::All => CargoFeatures::All,
+                CargoFeaturesDef::Selected(features) => CargoFeatures::Selected {
+                    features: features.clone(),
+                    no_default_features: self.data.cargo_noDefaultFeatures,
+                },
             },
             target: self.data.cargo_target.clone(),
             no_sysroot: self.data.cargo_noSysroot,
@@ -1086,7 +1088,7 @@ impl Config {
                     .unwrap_or(self.data.cargo_noDefaultFeatures),
                 all_features: matches!(
                     self.data.checkOnSave_features.as_ref().unwrap_or(&self.data.cargo_features),
-                    CargoFeatures::All
+                    CargoFeaturesDef::All
                 ),
                 features: match self
                     .data
@@ -1094,8 +1096,8 @@ impl Config {
                     .clone()
                     .unwrap_or_else(|| self.data.cargo_features.clone())
                 {
-                    CargoFeatures::All => vec![],
-                    CargoFeatures::Listed(it) => it,
+                    CargoFeaturesDef::All => vec![],
+                    CargoFeaturesDef::Selected(it) => it,
                 },
                 extra_args: self.data.checkOnSave_extraArgs.clone(),
                 extra_env: self.check_on_save_extra_env(),
@@ -1564,10 +1566,10 @@ enum CallableCompletionDef {
 
 #[derive(Deserialize, Debug, Clone)]
 #[serde(untagged)]
-enum CargoFeatures {
+enum CargoFeaturesDef {
     #[serde(deserialize_with = "de_unit_v::all")]
     All,
-    Listed(Vec<String>),
+    Selected(Vec<String>),
 }
 
 #[derive(Deserialize, Debug, Clone)]
@@ -1912,7 +1914,7 @@ fn field_props(field: &str, ty: &str, doc: &[&str], default: &str) -> serde_json
                 "Only show mutable reborrow hints."
             ]
         },
-        "CargoFeatures" => set! {
+        "CargoFeaturesDef" => set! {
             "anyOf": [
                 {
                     "type": "string",
@@ -1929,7 +1931,7 @@ fn field_props(field: &str, ty: &str, doc: &[&str], default: &str) -> serde_json
                 }
             ],
         },
-        "Option<CargoFeatures>" => set! {
+        "Option<CargoFeaturesDef>" => set! {
             "anyOf": [
                 {
                     "type": "string",