Auto merge of #7891 - ehuss:stringlist-fixes, r=Eh2406

Improvements to StringList config handling.

`StringList` was using an untagged enum to deserialize a string or list.  Unfortunately, serde does not handle untagged enums very well.  The error messages are not very good, and it doesn't interact with untyped deserializers (like our environment variables).  This switches it to a newtype struct, and then has hard-coded support for it in the deserializer.  This fixes some deserialization errors (like treating `true` as a boolean) and provides better error messages.

`StringList` is currently used for `build.rustflags`, `target.*.rustflags`, and `target.*.runner`.

Fixes #7780
Fixes #7781

Author: bors

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();