Merge #5507

5507: Require quotes around key-value cfg flags in rust-project.json r=matklad a=matklad

This matches rustc command-line flags, as well as the build.rs format.



bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/ra_project_model/src/cargo_workspace.rs

@@ -14,6 +14,8 @@ use ra_arena::{Arena, Idx};
 use ra_db::Edition;
 use rustc_hash::FxHashMap;
 
+use crate::cfg_flag::CfgFlag;
+
 /// `CargoWorkspace` represents the logical structure of, well, a Cargo
 /// workspace. It pretty closely mirrors `cargo metadata` output.
 ///
@@ -78,7 +80,7 @@ pub struct PackageData {
     pub dependencies: Vec<PackageDependency>,
     pub edition: Edition,
     pub features: Vec<String>,
-    pub cfgs: Vec<String>,
+    pub cfgs: Vec<CfgFlag>,
     pub out_dir: Option<AbsPathBuf>,
     pub proc_macro_dylib_path: Option<AbsPathBuf>,
 }
@@ -276,7 +278,7 @@ impl CargoWorkspace {
 pub struct ExternResources {
     out_dirs: FxHashMap<PackageId, AbsPathBuf>,
     proc_dylib_paths: FxHashMap<PackageId, AbsPathBuf>,
-    cfgs: FxHashMap<PackageId, Vec<String>>,
+    cfgs: FxHashMap<PackageId, Vec<CfgFlag>>,
 }
 
 pub fn load_extern_resources(
@@ -303,6 +305,18 @@ pub fn load_extern_resources(
         if let Ok(message) = message {
             match message {
                 Message::BuildScriptExecuted(BuildScript { package_id, out_dir, cfgs, .. }) => {
+                    let cfgs = {
+                        let mut acc = Vec::new();
+                        for cfg in cfgs {
+                            match cfg.parse::<CfgFlag>() {
+                                Ok(it) => acc.push(it),
+                                Err(err) => {
+                                    anyhow::bail!("invalid cfg from cargo-metadata: {}", err)
+                                }
+                            };
+                        }
+                        acc
+                    };
                     // cargo_metadata crate returns default (empty) path for
                     // older cargos, which is not absolute, so work around that.
                     if out_dir != PathBuf::default() {

crates/ra_project_model/src/cfg_flag.rs

@@ -0,0 +1,51 @@
+//! Parsing of CfgFlags as command line arguments, as in
+//!
+//! rustc main.rs --cfg foo --cfg 'feature="bar"'
+use std::str::FromStr;
+
+use ra_cfg::CfgOptions;
+use stdx::split_delim;
+
+#[derive(Clone, Eq, PartialEq, Debug)]
+pub enum CfgFlag {
+    Atom(String),
+    KeyValue { key: String, value: String },
+}
+
+impl FromStr for CfgFlag {
+    type Err = String;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let res = match split_delim(s, '=') {
+            Some((key, value)) => {
+                if !(value.starts_with('"') && value.ends_with('"')) {
+                    return Err(format!("Invalid cfg ({:?}), value should be in quotes", s));
+                }
+                let key = key.to_string();
+                let value = value[1..value.len() - 1].to_string();
+                CfgFlag::KeyValue { key, value }
+            }
+            None => CfgFlag::Atom(s.into()),
+        };
+        Ok(res)
+    }
+}
+
+impl<'de> serde::Deserialize<'de> for CfgFlag {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        String::deserialize(deserializer)?.parse().map_err(serde::de::Error::custom)
+    }
+}
+
+impl Extend<CfgFlag> for CfgOptions {
+    fn extend<T: IntoIterator<Item = CfgFlag>>(&mut self, iter: T) {
+        for cfg_flag in iter {
+            match cfg_flag {
+                CfgFlag::Atom(it) => self.insert_atom(it.into()),
+                CfgFlag::KeyValue { key, value } => self.insert_key_value(key.into(), value.into()),
+            }
+        }
+    }
+}

crates/ra_project_model/src/lib.rs

@@ -3,11 +3,12 @@
 mod cargo_workspace;
 mod project_json;
 mod sysroot;
+mod cfg_flag;
 
 use std::{
     fs::{self, read_dir, ReadDir},
     io,
-    process::{Command, Output},
+    process::Command,
 };
 
 use anyhow::{bail, Context, Result};
@@ -15,13 +16,15 @@ use paths::{AbsPath, AbsPathBuf};
 use ra_cfg::CfgOptions;
 use ra_db::{CrateGraph, CrateId, CrateName, Edition, Env, FileId};
 use rustc_hash::{FxHashMap, FxHashSet};
-use stdx::split_delim;
+
+use crate::cfg_flag::CfgFlag;
 
 pub use crate::{
     cargo_workspace::{CargoConfig, CargoWorkspace, Package, Target, TargetKind},
     project_json::{ProjectJson, ProjectJsonData},
     sysroot::Sysroot,
 };
+
 pub use ra_proc_macro::ProcMacroClient;
 
 #[derive(Debug, Clone, Eq, PartialEq)]
@@ -250,7 +253,7 @@ impl ProjectWorkspace {
         let mut crate_graph = CrateGraph::default();
         match self {
             ProjectWorkspace::Json { project } => {
-                let mut target_cfg_map = FxHashMap::<Option<&str>, CfgOptions>::default();
+                let mut cfg_cache: FxHashMap<Option<&str>, Vec<CfgFlag>> = FxHashMap::default();
                 let crates: FxHashMap<_, _> = project
                     .crates
                     .iter()
@@ -266,11 +269,12 @@ impl ProjectWorkspace {
                             .map(|it| proc_macro_client.by_dylib_path(&it));
 
                         let target = krate.target.as_deref().or(target);
-                        let target_cfgs = target_cfg_map
-                            .entry(target.clone())
-                            .or_insert_with(|| get_rustc_cfg_options(target.as_deref()));
-                        let mut cfg_options = krate.cfg.clone();
-                        cfg_options.append(target_cfgs);
+                        let target_cfgs = cfg_cache
+                            .entry(target)
+                            .or_insert_with(|| get_rustc_cfg_options(target));
+
+                        let mut cfg_options = CfgOptions::default();
+                        cfg_options.extend(target_cfgs.iter().chain(krate.cfg.iter()).cloned());
 
                         // FIXME: No crate name in json definition such that we cannot add OUT_DIR to env
                         Some((
@@ -307,7 +311,8 @@ impl ProjectWorkspace {
                 }
             }
             ProjectWorkspace::Cargo { cargo, sysroot } => {
-                let mut cfg_options = get_rustc_cfg_options(target);
+                let mut cfg_options = CfgOptions::default();
+                cfg_options.extend(get_rustc_cfg_options(target));
 
                 let sysroot_crates: FxHashMap<_, _> = sysroot
                     .crates()
@@ -354,6 +359,7 @@ impl ProjectWorkspace {
 
                 // Add test cfg for non-sysroot crates
                 cfg_options.insert_atom("test".into());
+                cfg_options.insert_atom("debug_assertions".into());
 
                 // Next, create crates for each package, target pair
                 for pkg in cargo.packages() {
@@ -367,15 +373,7 @@ impl ProjectWorkspace {
                                 for feature in cargo[pkg].features.iter() {
                                     opts.insert_key_value("feature".into(), feature.into());
                                 }
-                                for cfg in cargo[pkg].cfgs.iter() {
-                                    match cfg.find('=') {
-                                        Some(split) => opts.insert_key_value(
-                                            cfg[..split].into(),
-                                            cfg[split + 1..].trim_matches('"').into(),
-                                        ),
-                                        None => opts.insert_atom(cfg.into()),
-                                    };
-                                }
+                                opts.extend(cargo[pkg].cfgs.iter().cloned());
                                 opts
                             };
                             let mut env = Env::default();
@@ -503,51 +501,35 @@ impl ProjectWorkspace {
     }
 }
 
-fn get_rustc_cfg_options(target: Option<&str>) -> CfgOptions {
-    let mut cfg_options = CfgOptions::default();
+fn get_rustc_cfg_options(target: Option<&str>) -> Vec<CfgFlag> {
+    let mut res = Vec::new();
 
     // Some nightly-only cfgs, which are required for stdlib
-    {
-        cfg_options.insert_atom("target_thread_local".into());
-        for &target_has_atomic in ["8", "16", "32", "64", "cas", "ptr"].iter() {
-            cfg_options.insert_key_value("target_has_atomic".into(), target_has_atomic.into());
-            cfg_options
-                .insert_key_value("target_has_atomic_load_store".into(), target_has_atomic.into());
+    res.push(CfgFlag::Atom("target_thread_local".into()));
+    for &ty in ["8", "16", "32", "64", "cas", "ptr"].iter() {
+        for &key in ["target_has_atomic", "target_has_atomic_load_store"].iter() {
+            res.push(CfgFlag::KeyValue { key: key.to_string(), value: ty.into() });
         }
     }
 
-    let rustc_cfgs = || -> Result<String> {
-        // `cfg(test)` and `cfg(debug_assertion)` are handled outside, so we suppress them here.
+    let rustc_cfgs = {
         let mut cmd = Command::new(ra_toolchain::rustc());
         cmd.args(&["--print", "cfg", "-O"]);
         if let Some(target) = target {
             cmd.args(&["--target", target]);
         }
-        let output = output(cmd)?;
-        Ok(String::from_utf8(output.stdout)?)
-    }();
+        utf8_stdout(cmd)
+    };
 
     match rustc_cfgs {
-        Ok(rustc_cfgs) => {
-            for line in rustc_cfgs.lines() {
-                match split_delim(line, '=') {
-                    None => cfg_options.insert_atom(line.into()),
-                    Some((key, value)) => {
-                        let value = value.trim_matches('"');
-                        cfg_options.insert_key_value(key.into(), value.into());
-                    }
-                }
-            }
-        }
+        Ok(rustc_cfgs) => res.extend(rustc_cfgs.lines().map(|it| it.parse().unwrap())),
         Err(e) => log::error!("failed to get rustc cfgs: {:#}", e),
     }
 
-    cfg_options.insert_atom("debug_assertions".into());
-
-    cfg_options
+    res
 }
 
-fn output(mut cmd: Command) -> Result<Output> {
+fn utf8_stdout(mut cmd: Command) -> Result<String> {
     let output = cmd.output().with_context(|| format!("{:?} failed", cmd))?;
     if !output.status.success() {
         match String::from_utf8(output.stderr) {
@@ -557,5 +539,6 @@ fn output(mut cmd: Command) -> Result<Output> {
             _ => bail!("{:?} failed, {}", cmd, output.status),
         }
     }
-    Ok(output)
+    let stdout = String::from_utf8(output.stdout)?;
+    Ok(stdout)
 }

crates/ra_project_model/src/project_json.rs

@@ -3,11 +3,11 @@
 use std::path::PathBuf;
 
 use paths::{AbsPath, AbsPathBuf};
-use ra_cfg::CfgOptions;
 use ra_db::{CrateId, CrateName, Dependency, Edition};
-use rustc_hash::{FxHashMap, FxHashSet};
+use rustc_hash::FxHashMap;
 use serde::{de, Deserialize};
-use stdx::split_delim;
+
+use crate::cfg_flag::CfgFlag;
 
 /// Roots and crates that compose this Rust project.
 #[derive(Clone, Debug, Eq, PartialEq)]
@@ -22,7 +22,7 @@ pub struct Crate {
     pub(crate) root_module: AbsPathBuf,
     pub(crate) edition: Edition,
     pub(crate) deps: Vec<Dependency>,
-    pub(crate) cfg: CfgOptions,
+    pub(crate) cfg: Vec<CfgFlag>,
     pub(crate) target: Option<String>,
     pub(crate) env: FxHashMap<String, String>,
     pub(crate) proc_macro_dylib_path: Option<AbsPathBuf>,
@@ -65,18 +65,7 @@ impl ProjectJson {
                                 name: dep_data.name,
                             })
                             .collect::<Vec<_>>(),
-                        cfg: {
-                            let mut cfg = CfgOptions::default();
-                            for entry in &crate_data.cfg {
-                                match split_delim(entry, '=') {
-                                    Some((key, value)) => {
-                                        cfg.insert_key_value(key.into(), value.into());
-                                    }
-                                    None => cfg.insert_atom(entry.into()),
-                                }
-                            }
-                            cfg
-                        },
+                        cfg: crate_data.cfg,
                         target: crate_data.target,
                         env: crate_data.env,
                         proc_macro_dylib_path: crate_data
@@ -103,7 +92,7 @@ struct CrateData {
     edition: EditionData,
     deps: Vec<DepData>,
     #[serde(default)]
-    cfg: FxHashSet<String>,
+    cfg: Vec<CfgFlag>,
     target: Option<String>,
     #[serde(default)]
     env: FxHashMap<String, String>,

crates/ra_project_model/src/sysroot.rs

@@ -6,7 +6,7 @@ use anyhow::{bail, format_err, Result};
 use paths::{AbsPath, AbsPathBuf};
 use ra_arena::{Arena, Idx};
 
-use crate::output;
+use crate::utf8_stdout;
 
 #[derive(Default, Debug, Clone, Eq, PartialEq)]
 pub struct Sysroot {
@@ -92,15 +92,14 @@ fn get_or_install_rust_src(cargo_toml: &AbsPath) -> Result<AbsPathBuf> {
     let current_dir = cargo_toml.parent().unwrap();
     let mut rustc = Command::new(ra_toolchain::rustc());
     rustc.current_dir(current_dir).args(&["--print", "sysroot"]);
-    let rustc_output = output(rustc)?;
-    let stdout = String::from_utf8(rustc_output.stdout)?;
+    let stdout = utf8_stdout(rustc)?;
     let sysroot_path = AbsPath::assert(Path::new(stdout.trim()));
     let src_path = sysroot_path.join("lib/rustlib/src/rust/src");
 
     if !src_path.exists() {
         let mut rustup = Command::new(ra_toolchain::rustup());
         rustup.current_dir(current_dir).args(&["component", "add", "rust-src"]);
-        let _output = output(rustup)?;
+        utf8_stdout(rustup)?;
     }
     if !src_path.exists() {
         bail!(

crates/rust-analyzer/tests/heavy_tests/main.rs

@@ -318,7 +318,7 @@ fn test_missing_module_code_action_in_json_project() {
             "root_module": path.join("src/lib.rs"),
             "deps": [],
             "edition": "2015",
-            "cfg": [ "cfg_atom_1", "feature=cfg_1"],
+            "cfg": [ "cfg_atom_1", "feature=\"cfg_1\""],
         } ]
     });