Improvements to StringList config handling.

Author: ehuss

src/cargo/util/config/de.rs

@@ -176,11 +176,28 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
         visitor.visit_seq(ConfigSeqAccess::new(self)?)
     }
 
+    fn deserialize_newtype_struct<V>(
+        self,
+        name: &'static str,
+        visitor: V,
+    ) -> Result<V::Value, Self::Error>
+    where
+        V: de::Visitor<'de>,
+    {
+        if name == "StringList" {
+            let vals = self.config.get_list_or_string(&self.key)?;
+            let vals: Vec<String> = vals.into_iter().map(|vd| vd.0).collect();
+            visitor.visit_newtype_struct(vals.into_deserializer())
+        } else {
+            visitor.visit_newtype_struct(self)
+        }
+    }
+
     // These aren't really supported, yet.
     serde::forward_to_deserialize_any! {
         f32 f64 char str bytes
         byte_buf unit unit_struct
-        enum identifier ignored_any newtype_struct
+        enum identifier ignored_any
     }
 }
 
@@ -345,36 +362,7 @@ impl ConfigSeqAccess {
             res.extend(v.val);
         }
 
-        // Check environment.
-        if let Some(v) = de.config.env.get(de.key.as_env_key()) {
-            let def = Definition::Environment(de.key.as_env_key().to_string());
-            if de.config.cli_unstable().advanced_env && v.starts_with('[') && v.ends_with(']') {
-                // Parse an environment string as a TOML array.
-                let toml_s = format!("value={}", v);
-                let toml_v: toml::Value = toml::de::from_str(&toml_s).map_err(|e| {
-                    ConfigError::new(format!("could not parse TOML list: {}", e), def.clone())
-                })?;
-                let values = toml_v
-                    .as_table()
-                    .unwrap()
-                    .get("value")
-                    .unwrap()
-                    .as_array()
-                    .expect("env var was not array");
-                for value in values {
-                    // TODO: support other types.
-                    let s = value.as_str().ok_or_else(|| {
-                        ConfigError::new(
-                            format!("expected string, found {}", value.type_str()),
-                            def.clone(),
-                        )
-                    })?;
-                    res.push((s.to_string(), def.clone()));
-                }
-            } else {
-                res.extend(v.split_whitespace().map(|s| (s.to_string(), def.clone())));
-            }
-        }
+        de.config.get_env_list(&de.key, &mut res)?;
 
         Ok(ConfigSeqAccess {
             list_iter: res.into_iter(),

src/cargo/util/config/mod.rs

@@ -573,6 +573,70 @@ impl Config {
         }
     }
 
+    /// Helper for StringList type to get something that is a string or list.
+    fn get_list_or_string(&self, key: &ConfigKey) -> CargoResult<Vec<(String, Definition)>> {
+        let mut res = Vec::new();
+        match self.get_cv(&key)? {
+            Some(CV::List(val, _def)) => res.extend(val),
+            Some(CV::String(val, def)) => {
+                let split_vs = val.split_whitespace().map(|s| (s.to_string(), def.clone()));
+                res.extend(split_vs);
+            }
+            Some(val) => {
+                return self.expected("string or array of strings", key, &val);
+            }
+            None => {}
+        }
+
+        self.get_env_list(key, &mut res)?;
+        Ok(res)
+    }
+
+    /// Internal method for getting an environment variable as a list.
+    fn get_env_list(
+        &self,
+        key: &ConfigKey,
+        output: &mut Vec<(String, Definition)>,
+    ) -> CargoResult<()> {
+        let env_val = match self.env.get(key.as_env_key()) {
+            Some(v) => v,
+            None => return Ok(()),
+        };
+
+        let def = Definition::Environment(key.as_env_key().to_string());
+        if self.cli_unstable().advanced_env && env_val.starts_with('[') && env_val.ends_with(']') {
+            // Parse an environment string as a TOML array.
+            let toml_s = format!("value={}", env_val);
+            let toml_v: toml::Value = toml::de::from_str(&toml_s).map_err(|e| {
+                ConfigError::new(format!("could not parse TOML list: {}", e), def.clone())
+            })?;
+            let values = toml_v
+                .as_table()
+                .unwrap()
+                .get("value")
+                .unwrap()
+                .as_array()
+                .expect("env var was not array");
+            for value in values {
+                // TODO: support other types.
+                let s = value.as_str().ok_or_else(|| {
+                    ConfigError::new(
+                        format!("expected string, found {}", value.type_str()),
+                        def.clone(),
+                    )
+                })?;
+                output.push((s.to_string(), def.clone()));
+            }
+        } else {
+            output.extend(
+                env_val
+                    .split_whitespace()
+                    .map(|s| (s.to_string(), def.clone())),
+            );
+        }
+        Ok(())
+    }
+
     /// Low-level method for getting a config value as a `OptValue<HashMap<String, CV>>`.
     ///
     /// NOTE: This does not read from env. The caller is responsible for that.
@@ -1650,43 +1714,11 @@ pub struct CargoBuildConfig {
 /// a = 'a b c'
 /// b = ['a', 'b', 'c']
 /// ```
-#[derive(Debug)]
-pub struct StringList {
-    list: Vec<String>,
-}
+#[derive(Debug, Deserialize)]
+pub struct StringList(Vec<String>);
 
 impl StringList {
     pub fn as_slice(&self) -> &[String] {
-        &self.list
-    }
-}
-
-impl<'de> serde::de::Deserialize<'de> for StringList {
-    fn deserialize<D: serde::de::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
-        #[derive(Deserialize)]
-        #[serde(untagged)]
-        enum Target {
-            String(String),
-            List(Vec<String>),
-        }
-
-        // Add some context to the error. Serde gives a vague message for
-        // untagged enums. See https://github.com/serde-rs/serde/issues/773
-        let result = match Target::deserialize(d) {
-            Ok(r) => r,
-            Err(e) => {
-                return Err(serde::de::Error::custom(format!(
-                    "failed to deserialize, expected a string or array of strings: {}",
-                    e
-                )))
-            }
-        };
-
-        Ok(match result {
-            Target::String(s) => StringList {
-                list: s.split_whitespace().map(str::to_string).collect(),
-            },
-            Target::List(list) => StringList { list },
-        })
+        &self.0
     }
 }

src/cargo/util/config/path.rs

@@ -56,7 +56,7 @@ impl<'de> serde::Deserialize<'de> for PathAndArgs {
         D: serde::Deserializer<'de>,
     {
         let vsl = Value::<StringList>::deserialize(deserializer)?;
-        let mut strings = vsl.val.list;
+        let mut strings = vsl.val.0;
         if strings.is_empty() {
             return Err(D::Error::invalid_length(0, &"at least one element"));
         }

tests/testsuite/bad_config.rs

@@ -1355,8 +1355,9 @@ Caused by:
   could not load config key `target.cfg(not(target_os = \"none\")).runner`
 
 Caused by:
-  failed to deserialize, expected a string or array of strings: \
-  data did not match any variant of untagged enum Target
+  invalid configuration for key `target.cfg(not(target_os = \"none\")).runner`
+expected a string or array of strings, but found a boolean for \
+`target.cfg(not(target_os = \"none\")).runner` in [..]/foo/.cargo/config
 ",
         )
         .run();

tests/testsuite/config.rs

@@ -1205,3 +1205,56 @@ fn overlapping_env_config() {
     assert_eq!(s.debug_assertions, Some(true));
     assert_eq!(s.debug, Some(1));
 }
+
+#[cargo_test]
+fn string_list_tricky_env() {
+    // Make sure StringList handles typed env values.
+    let config = ConfigBuilder::new()
+        .env("CARGO_KEY1", "123")
+        .env("CARGO_KEY2", "true")
+        .env("CARGO_KEY3", "1 2")
+        .build();
+    let x = config.get::<StringList>("key1").unwrap();
+    assert_eq!(x.as_slice(), &["123".to_string()]);
+    let x = config.get::<StringList>("key2").unwrap();
+    assert_eq!(x.as_slice(), &["true".to_string()]);
+    let x = config.get::<StringList>("key3").unwrap();
+    assert_eq!(x.as_slice(), &["1".to_string(), "2".to_string()]);
+}
+
+#[cargo_test]
+fn string_list_wrong_type() {
+    // What happens if StringList is given then wrong type.
+    write_config("some_list = 123");
+    let config = ConfigBuilder::new().build();
+    assert_error(
+        config.get::<StringList>("some_list").unwrap_err(),
+        "\
+invalid configuration for key `some_list`
+expected a string or array of strings, but found a integer for `some_list` in [..]/.cargo/config",
+    );
+
+    write_config("some_list = \"1 2\"");
+    let config = ConfigBuilder::new().build();
+    let x = config.get::<StringList>("some_list").unwrap();
+    assert_eq!(x.as_slice(), &["1".to_string(), "2".to_string()]);
+}
+
+#[cargo_test]
+fn string_list_advanced_env() {
+    // StringList with advanced env.
+    let config = ConfigBuilder::new()
+        .unstable_flag("advanced-env")
+        .env("CARGO_KEY1", "[]")
+        .env("CARGO_KEY2", "['1 2', '3']")
+        .env("CARGO_KEY3", "[123]")
+        .build();
+    let x = config.get::<StringList>("key1").unwrap();
+    assert_eq!(x.as_slice(), &[] as &[String]);
+    let x = config.get::<StringList>("key2").unwrap();
+    assert_eq!(x.as_slice(), &["1 2".to_string(), "3".to_string()]);
+    assert_error(
+        config.get::<StringList>("key3").unwrap_err(),
+        "error in environment variable `CARGO_KEY3`: expected string, found integer",
+    );
+}

tests/testsuite/tool_paths.rs

@@ -278,6 +278,25 @@ fn custom_runner_env() {
         .run();
 }
 
+#[cargo_test]
+#[cfg(unix)] // Assumes `true` is in PATH.
+fn custom_runner_env_true() {
+    // Check for a bug where "true" was interpreted as a boolean instead of
+    // the executable.
+    let target = rustc_host();
+    let p = project().file("src/main.rs", "fn main() {}").build();
+
+    let key = format!(
+        "CARGO_TARGET_{}_RUNNER",
+        target.to_uppercase().replace('-', "_")
+    );
+
+    p.cargo("run")
+        .env(&key, "true")
+        .with_stderr_contains("[RUNNING] `true target/debug/foo[EXE]`")
+        .run();
+}
+
 #[cargo_test]
 fn custom_linker_env() {
     let target = rustc_host();