Finish implementing Value, use it in helpers

Rewrite helpers like `get_bool` to use `get::<Option<Value<bool>>>`
instead of duplicating the logic that's already with the typed access of
configuration. This is more along the effort to centralize all
deserialization of configuration into typed values instead of using
ad-hoc accessors in a number of locations.

Author: alexcrichton

src/cargo/util/config/de.rs

@@ -113,10 +113,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
         V: de::Visitor<'de>,
     {
         if name == value::NAME && fields == value::FIELDS {
-            return visitor.visit_map(ValueDeserializer {
-                hits: 0,
-                deserializer: self,
-            });
+            return visitor.visit_map(ValueDeserializer { hits: 0, de: self });
         }
         visitor.visit_map(ConfigMapAccess::new_struct(self, fields)?)
     }
@@ -154,37 +151,11 @@ 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 == "ConfigRelativePath" {
-            match self.config.get_string_priv(&self.key)? {
-                Some(v) => {
-                    let path = v
-                        .definition
-                        .root(self.config)
-                        .join(v.val)
-                        .display()
-                        .to_string();
-                    visitor.visit_newtype_struct(path.into_deserializer())
-                }
-                None => Err(ConfigError::missing(&self.key)),
-            }
-        } 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
+        enum identifier ignored_any newtype_struct
     }
 }
 
@@ -371,7 +342,7 @@ impl<'de> de::SeqAccess<'de> for ConfigSeqAccess {
 
 struct ValueDeserializer<'config> {
     hits: u32,
-    deserializer: Deserializer<'config>,
+    de: Deserializer<'config>,
 }
 
 impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> {
@@ -397,18 +368,80 @@ impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> {
     where
         V: de::DeserializeSeed<'de>,
     {
+        macro_rules! bail {
+            ($($t:tt)*) => (return Err(failure::format_err!($($t)*).into()))
+        }
+
+        // If this is the first time around we deserialize the `value` field
+        // which is the actual deserializer
         if self.hits == 1 {
-            seed.deserialize(Deserializer {
-                config: self.deserializer.config,
-                key: self.deserializer.key.clone(),
-            })
-        } else {
-            // let env = self.deserializer.key.to_env();
-            // if self.deserializer.config.env.contains_key(&env) {
-            // } else {
-            // }
-            // if let someself.deserializer.config.get_env(&self.deserializer.key)
-            panic!()
+            return seed.deserialize(self.de.clone());
+        }
+
+        // ... otherwise we're deserializing the `definition` field, so we need
+        // to figure out where the field we just deserialized was defined at.
+        let env = self.de.key.as_env_key();
+        if self.de.config.env.contains_key(env) {
+            return seed.deserialize(Tuple2Deserializer(1i32, env));
+        }
+        let val = match self.de.config.get_cv(self.de.key.as_config_key())? {
+            Some(val) => val,
+            None => bail!("failed to find definition of `{}`", self.de.key),
+        };
+        let path = match val.definition_path().to_str() {
+            Some(s) => s,
+            None => bail!("failed to convert {:?} to utf-8", val.definition_path()),
+        };
+        seed.deserialize(Tuple2Deserializer(0i32, path))
+    }
+}
+
+struct Tuple2Deserializer<T, U>(T, U);
+
+impl<'de, T, U> de::Deserializer<'de> for Tuple2Deserializer<T, U>
+where
+    T: IntoDeserializer<'de, ConfigError>,
+    U: IntoDeserializer<'de, ConfigError>,
+{
+    type Error = ConfigError;
+
+    fn deserialize_any<V>(self, visitor: V) -> Result<V::Value, ConfigError>
+    where
+        V: de::Visitor<'de>,
+    {
+        struct SeqVisitor<T, U> {
+            first: Option<T>,
+            second: Option<U>,
         }
+        impl<'de, T, U> de::SeqAccess<'de> for SeqVisitor<T, U>
+        where
+            T: IntoDeserializer<'de, ConfigError>,
+            U: IntoDeserializer<'de, ConfigError>,
+        {
+            type Error = ConfigError;
+            fn next_element_seed<K>(&mut self, seed: K) -> Result<Option<K::Value>, Self::Error>
+            where
+                K: de::DeserializeSeed<'de>,
+            {
+                if let Some(first) = self.first.take() {
+                    return seed.deserialize(first.into_deserializer()).map(Some);
+                }
+                if let Some(second) = self.second.take() {
+                    return seed.deserialize(second.into_deserializer()).map(Some);
+                }
+                Ok(None)
+            }
+        }
+
+        visitor.visit_seq(SeqVisitor {
+            first: Some(self.0),
+            second: Some(self.1),
+        })
+    }
+
+    serde::forward_to_deserialize_any! {
+        bool u8 u16 u32 u64 i8 i16 i32 i64 f32 f64 char str string seq
+        bytes byte_buf map struct option unit newtype_struct
+        ignored_any unit_struct tuple_struct tuple enum identifier
     }
 }

src/cargo/util/config/mod.rs

@@ -39,6 +39,9 @@ pub use value::{Definition, OptValue, Value};
 mod key;
 use key::ConfigKey;
 
+mod path;
+pub use path::ConfigRelativePath;
+
 /// Configuration information for cargo. This is not specific to a build, it is information
 /// relating to cargo itself.
 ///
@@ -417,8 +420,7 @@ impl Config {
     }
 
     pub fn get_string(&self, key: &str) -> CargoResult<OptValue<String>> {
-        self.get_string_priv(&ConfigKey::from_str(key))
-            .map_err(|e| e.into())
+        self.get::<Option<Value<String>>>(key)
     }
 
     fn get_string_priv(&self, key: &ConfigKey) -> Result<OptValue<String>, ConfigError> {
@@ -439,8 +441,7 @@ impl Config {
     }
 
     pub fn get_bool(&self, key: &str) -> CargoResult<OptValue<bool>> {
-        self.get_bool_priv(&ConfigKey::from_str(key))
-            .map_err(|e| e.into())
+        self.get::<Option<Value<bool>>>(key)
     }
 
     fn get_bool_priv(&self, key: &ConfigKey) -> Result<OptValue<bool>, ConfigError> {
@@ -464,6 +465,15 @@ impl Config {
         }
     }
 
+    pub fn get_path(&self, key: &str) -> CargoResult<OptValue<PathBuf>> {
+        self.get::<Option<Value<ConfigRelativePath>>>(key).map(|v| {
+            v.map(|v| Value {
+                val: v.val.resolve(self),
+                definition: v.definition,
+            })
+        })
+    }
+
     fn string_to_path(&self, value: String, definition: &Definition) -> PathBuf {
         let is_path = value.contains('/') || (cfg!(windows) && value.contains('\\'));
         if is_path {
@@ -474,17 +484,6 @@ impl Config {
         }
     }
 
-    pub fn get_path(&self, key: &str) -> CargoResult<OptValue<PathBuf>> {
-        if let Some(val) = self.get_string(key)? {
-            Ok(Some(Value {
-                val: self.string_to_path(val.val, &val.definition),
-                definition: val.definition,
-            }))
-        } else {
-            Ok(None)
-        }
-    }
-
     pub fn get_path_and_args(&self, key: &str) -> CargoResult<OptValue<(PathBuf, Vec<String>)>> {
         if let Some(mut val) = self.get_list_or_split_string(key)? {
             if !val.val.is_empty() {
@@ -514,24 +513,26 @@ impl Config {
     }
 
     pub fn get_list_or_split_string(&self, key: &str) -> CargoResult<OptValue<Vec<String>>> {
-        if let Some(value) = self.get_env::<String>(&ConfigKey::from_str(key))? {
-            return Ok(Some(Value {
-                val: value.val.split(' ').map(str::to_string).collect(),
-                definition: value.definition,
-            }));
+        #[derive(Deserialize)]
+        #[serde(untagged)]
+        enum Target {
+            String(String),
+            List(Vec<String>),
         }
 
-        match self.get_cv(key)? {
-            Some(CV::List(i, path)) => Ok(Some(Value {
-                val: i.into_iter().map(|(s, _)| s).collect(),
-                definition: Definition::Path(path),
-            })),
-            Some(CV::String(i, path)) => Ok(Some(Value {
-                val: i.split(' ').map(str::to_string).collect(),
-                definition: Definition::Path(path),
-            })),
-            Some(val) => self.expected("list or string", key, &val),
+        match self.get::<Option<Value<Target>>>(key)? {
             None => Ok(None),
+            Some(Value {
+                val: Target::String(s),
+                definition,
+            }) => Ok(Some(Value {
+                val: s.split(' ').map(str::to_string).collect(),
+                definition,
+            })),
+            Some(Value {
+                val: Target::List(val),
+                definition,
+            }) => Ok(Some(Value { val, definition })),
         }
     }
 
@@ -549,8 +550,7 @@ impl Config {
     // Recommended to use `get` if you want a specific type, such as an unsigned value.
     // Example: `config.get::<Option<u32>>("some.key")?`.
     pub fn get_i64(&self, key: &str) -> CargoResult<OptValue<i64>> {
-        self.get_integer(&ConfigKey::from_str(key))
-            .map_err(|e| e.into())
+        self.get::<Option<Value<i64>>>(key)
     }
 
     fn get_integer(&self, key: &ConfigKey) -> Result<OptValue<i64>, ConfigError> {
@@ -1102,18 +1102,6 @@ impl From<failure::Error> for ConfigError {
     }
 }
 
-/// Use with the `get` API to fetch a string that will be converted to a
-/// `PathBuf`. Relative paths are converted to absolute paths based on the
-/// location of the config file.
-#[derive(Debug, Eq, PartialEq, Clone, Deserialize)]
-pub struct ConfigRelativePath(PathBuf);
-
-impl ConfigRelativePath {
-    pub fn path(self) -> PathBuf {
-        self.0
-    }
-}
-
 #[derive(Eq, PartialEq, Clone)]
 pub enum ConfigValue {
     Integer(i64, PathBuf),

src/cargo/util/config/path.rs

@@ -0,0 +1,16 @@
+use crate::util::config::{Config, Value};
+use serde::Deserialize;
+use std::path::PathBuf;
+
+/// Use with the `get` API to fetch a string that will be converted to a
+/// `PathBuf`. Relative paths are converted to absolute paths based on the
+/// location of the config file.
+#[derive(Deserialize)]
+#[serde(transparent)]
+pub struct ConfigRelativePath(Value<String>);
+
+impl ConfigRelativePath {
+    pub fn resolve(self, config: &Config) -> PathBuf {
+        config.string_to_path(self.0.val, &self.0.definition)
+    }
+}

tests/testsuite/bad_config.rs

@@ -17,8 +17,8 @@ fn bad1() {
         .with_status(101)
         .with_stderr(
             "\
-[ERROR] expected table for configuration key `target.nonexistent-target`, \
-but found string in [..]config
+[ERROR] invalid configuration for key `target.nonexistent-target`
+expected a table, but found a string for `[..]` in [..]config
 ",
         )
         .run();

tests/testsuite/config.rs

@@ -706,6 +706,7 @@ ns2 = 456
     let config = new_config(&[("CARGO_NSE", "987"), ("CARGO_NS2", "654")]);
 
     #[derive(Debug, Deserialize, Eq, PartialEq)]
+    #[serde(transparent)]
     struct NewS(i32);
     assert_eq!(config.get::<NewS>("ns").unwrap(), NewS(123));
     assert_eq!(config.get::<NewS>("ns2").unwrap(), NewS(654));
@@ -734,35 +735,35 @@ abs = '{}'
         config
             .get::<config::ConfigRelativePath>("p1")
             .unwrap()
-            .path(),
+            .resolve(&config),
         paths::root().join("foo/bar")
     );
     assert_eq!(
         config
             .get::<config::ConfigRelativePath>("p2")
             .unwrap()
-            .path(),
+            .resolve(&config),
         paths::root().join("../abc")
     );
     assert_eq!(
         config
             .get::<config::ConfigRelativePath>("p3")
             .unwrap()
-            .path(),
+            .resolve(&config),
         paths::root().join("d/e")
     );
     assert_eq!(
         config
             .get::<config::ConfigRelativePath>("abs")
             .unwrap()
-            .path(),
+            .resolve(&config),
         paths::home()
     );
     assert_eq!(
         config
             .get::<config::ConfigRelativePath>("epath")
             .unwrap()
-            .path(),
+            .resolve(&config),
         paths::root().join("a/b")
     );
 }