factor out three hashmaps in Config into a new struct for better encapsulation

kylematsuda · kylematsuda · commit f810234b9b2a · 2023-03-10T11:03:18.000-07:00
diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs
@@ -424,7 +424,7 @@ impl<'config> ValueDeserializer<'config> {
         let definition = {
             let env = de.key.as_env_key();
             let env_def = Definition::Environment(env.to_string());
-            match (de.config.env_has_key(env), de.config.get_cv(&de.key)?) {
+            match (de.config.env.contains_key(env), de.config.get_cv(&de.key)?) {
                 (true, Some(cv)) => {
                     // Both, pick highest priority.
                     if env_def.is_higher_priority(cv.definition()) {
diff --git a/src/cargo/util/config/environment.rs b/src/cargo/util/config/environment.rs
@@ -0,0 +1,147 @@
+//! Encapsulates snapshotting of environment variables.
+
+use std::collections::HashMap;
+use std::ffi::{OsStr, OsString};
+
+use crate::util::errors::CargoResult;
+use anyhow::{anyhow, bail};
+
+/// Generate `case_insensitive_env` and `normalized_env` from the `env`.
+fn make_case_insensitive_and_normalized_env(
+    env: &HashMap<OsString, OsString>,
+) -> (HashMap<String, String>, HashMap<String, String>) {
+    let case_insensitive_env: HashMap<_, _> = env
+        .keys()
+        .filter_map(|k| k.to_str())
+        .map(|k| (k.to_uppercase(), k.to_owned()))
+        .collect();
+    let normalized_env = env
+        .iter()
+        // Only keep entries where both the key and value are valid UTF-8
+        .filter_map(|(k, v)| Some((k.to_str()?, v.to_str()?)))
+        .map(|(k, _)| (k.to_uppercase().replace("-", "_"), k.to_owned()))
+        .collect();
+    (case_insensitive_env, normalized_env)
+}
+
+#[derive(Debug)]
+pub struct Env {
+    /// A snapshot of the process's environment variables.
+    env: HashMap<OsString, OsString>,
+    /// A map from normalized (upper case and with "-" replaced by "_") env keys to the actual key
+    /// in the environment.
+    /// The "normalized" key is the format expected by Cargo.
+    /// This is used to warn users when env keys are not provided in this format.
+    normalized_env: HashMap<String, String>,
+    /// A map from uppercased env keys to the actual key in the environment.
+    /// This is relevant on Windows, where env keys are case-insensitive.
+    /// For example, this might hold a pair `("PATH", "Path")`.
+    case_insensitive_env: HashMap<String, String>,
+}
+
+impl Env {
+    /// Create a new `Env` from process's environment variables.
+    pub fn new() -> Self {
+        let env: HashMap<_, _> = std::env::vars_os().collect();
+        let (case_insensitive_env, normalized_env) = make_case_insensitive_and_normalized_env(&env);
+        Self {
+            env,
+            case_insensitive_env,
+            normalized_env,
+        }
+    }
+
+    /// Set the env directly from a `HashMap`.
+    /// This should be used for debugging purposes only.
+    pub(super) fn from_map(env: HashMap<String, String>) -> Self {
+        let env = env.into_iter().map(|(k, v)| (k.into(), v.into())).collect();
+        let (case_insensitive_env, normalized_env) = make_case_insensitive_and_normalized_env(&env);
+        Self {
+            env,
+            case_insensitive_env,
+            normalized_env,
+        }
+    }
+
+    /// Returns all environment variables as an iterator,
+    /// keeping only entries where both the key and value are valid UTF-8.
+    pub fn iter_str(&self) -> impl Iterator<Item = (&str, &str)> {
+        self.env
+            .iter()
+            .filter_map(|(k, v)| Some((k.to_str()?, v.to_str()?)))
+    }
+
+    /// Returns all environment variable keys, filtering out keys that are not valid UTF-8.
+    pub fn keys_str(&self) -> impl Iterator<Item = &str> {
+        self.env.keys().filter_map(|k| k.to_str())
+    }
+
+    /// Get the value of environment variable `key` through the `Config` snapshot.
+    ///
+    /// This can be used similarly to `std::env::var_os`.
+    /// On Windows, we check for case mismatch since environment keys are case-insensitive.
+    pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
+        match self.env.get(key.as_ref()) {
+            Some(s) => Some(s.clone()),
+            None => {
+                if cfg!(windows) {
+                    self.get_env_case_insensitive(key).cloned()
+                } else {
+                    None
+                }
+            }
+        }
+    }
+
+    /// Get the value of environment variable `key` through the `self.env` snapshot.
+    ///
+    /// This can be used similarly to `std::env::var`.
+    /// On Windows, we check for case mismatch since environment keys are case-insensitive.
+    pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<String> {
+        let key = key.as_ref();
+        let s = self
+            .get_env_os(key)
+            .ok_or_else(|| anyhow!("{key:?} could not be found in the environment snapshot"))?;
+
+        match s.to_str() {
+            Some(s) => Ok(s.to_owned()),
+            None => bail!("environment variable value is not valid unicode: {s:?}"),
+        }
+    }
+
+    /// Performs a case-insensitive lookup of `key` in the environment.
+    ///
+    /// This is relevant on Windows, where environment variables are case-insensitive.
+    /// Note that this only works on keys that are valid UTF-8 and it uses Unicode uppercase,
+    /// which may differ from the OS's notion of uppercase.
+    fn get_env_case_insensitive(&self, key: impl AsRef<OsStr>) -> Option<&OsString> {
+        let upper_case_key = key.as_ref().to_str()?.to_uppercase();
+        let env_key: &OsStr = self.case_insensitive_env.get(&upper_case_key)?.as_ref();
+        self.env.get(env_key)
+    }
+
+    /// Get the value of environment variable `key` as a `&str`.
+    /// Returns `None` if `key` is not in `self.env` or if the value is not valid UTF-8.
+    ///
+    /// This is intended for use in private methods of `Config`,
+    /// and does not check for env key case mismatch.
+    pub(super) fn get_str(&self, key: impl AsRef<OsStr>) -> Option<&str> {
+        self.env.get(key.as_ref()).and_then(|s| s.to_str())
+    }
+
+    /// Check if the environment contains `key`.
+    ///
+    /// This is intended for use in private methods of `Config`,
+    /// and does not check for env key case mismatch.
+    pub(super) fn contains_key(&self, key: impl AsRef<OsStr>) -> bool {
+        self.env.contains_key(key.as_ref())
+    }
+
+    /// Looks up a normalized `key` in the `normalized_env`.
+    /// Returns the corresponding (non-normalized) env key if it exists, else `None`.
+    ///
+    /// This is used by `Config::check_environment_key_mismatch`.
+    pub(super) fn get_normalized(&self, key: &str) -> Option<&str> {
+        self.normalized_env.get(key).map(|s| s.as_ref())
+    }
+}
diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs
@@ -100,6 +100,9 @@ pub use path::{ConfigRelativePath, PathAndArgs};
 mod target;
 pub use target::{TargetCfgConfig, TargetConfig};
 
+mod environment;
+use environment::Env;
+
 // Helper macro for creating typed access methods.
 macro_rules! get_value_typed {
     ($name:ident, $ty:ty, $variant:ident, $expected:expr) => {
@@ -124,30 +127,6 @@ macro_rules! get_value_typed {
     };
 }
 
-/// Generate `case_insensitive_env` and `normalized_env` from the `env`.
-fn make_case_insensitive_and_normalized_env(
-    env: &HashMap<OsString, OsString>,
-) -> (HashMap<String, String>, HashMap<String, String>) {
-    // See `Config.case_insensitive_env`.
-    // Maps from uppercased key to actual environment key.
-    // For example, `"PATH" => "Path"`.
-    let case_insensitive_env: HashMap<_, _> = env
-        .keys()
-        .filter_map(|k| k.to_str())
-        .map(|k| (k.to_uppercase(), k.to_owned()))
-        .collect();
-    // See `Config.normalized_env`.
-    // Maps from normalized (uppercased with "-" replaced by "_") key
-    // to actual environment key. For example, `"MY_KEY" => "my-key"`.
-    let normalized_env = env
-        .iter()
-        // Only keep entries where both the key and value are valid UTF-8
-        .filter_map(|(k, v)| Some((k.to_str()?, v.to_str()?)))
-        .map(|(k, _)| (k.to_uppercase().replace("-", "_"), k.to_owned()))
-        .collect();
-    (case_insensitive_env, normalized_env)
-}
-
 /// Indicates why a config value is being loaded.
 #[derive(Clone, Copy, Debug)]
 enum WhyLoad {
@@ -223,15 +202,8 @@ pub struct Config {
     creation_time: Instant,
     /// Target Directory via resolved Cli parameter
     target_dir: Option<Filesystem>,
-    /// Environment variables, separated to assist testing.
-    env: HashMap<OsString, OsString>,
-    /// Environment variables converted to uppercase to check for case mismatch
-    /// (relevant on Windows, where environment variables are case-insensitive).
-    case_insensitive_env: HashMap<String, String>,
-    /// Environment variables converted to uppercase and with "-" replaced by "_"
-    /// (the format expected by Cargo). This only contains entries where the key and variable are
-    /// both valid UTF-8.
-    normalized_env: HashMap<String, String>,
+    /// Environment variable snapshot.
+    env: Env,
     /// Tracks which sources have been updated to avoid multiple updates.
     updated_sources: LazyCell<RefCell<HashSet<SourceId>>>,
     /// Cache of credentials from configuration or credential providers.
@@ -289,11 +261,10 @@ impl Config {
             }
         });
 
-        let env: HashMap<_, _> = env::vars_os().collect();
-        let (case_insensitive_env, normalized_env) = make_case_insensitive_and_normalized_env(&env);
+        let env = Env::new();
 
-        let cache_key: &OsStr = "CARGO_CACHE_RUSTC_INFO".as_ref();
-        let cache_rustc_info = match env.get(cache_key) {
+        let cache_key = "CARGO_CACHE_RUSTC_INFO";
+        let cache_rustc_info = match env.get_env_os(cache_key) {
             Some(cache) => cache != "0",
             _ => true,
         };
@@ -327,8 +298,6 @@ impl Config {
             creation_time: Instant::now(),
             target_dir: None,
             env,
-            case_insensitive_env,
-            normalized_env,
             updated_sources: LazyCell::new(),
             credential_cache: LazyCell::new(),
             package_cache_lock: RefCell::new(None),
@@ -683,7 +652,7 @@ impl Config {
             // Root table can't have env value.
             return Ok(cv);
         }
-        let env = self.get_env_str(key.as_env_key());
+        let env = self.env.get_str(key.as_env_key());
         let env_def = Definition::Environment(key.as_env_key().to_string());
         let use_env = match (&cv, env) {
             // Lists are always merged.
@@ -754,32 +723,26 @@ impl Config {
 
     /// Helper primarily for testing.
     pub fn set_env(&mut self, env: HashMap<String, String>) {
-        self.env = env.into_iter().map(|(k, v)| (k.into(), v.into())).collect();
-        let (case_insensitive_env, normalized_env) =
-            make_case_insensitive_and_normalized_env(&self.env);
-        self.case_insensitive_env = case_insensitive_env;
-        self.normalized_env = normalized_env;
+        self.env = Env::from_map(env);
     }
 
-    /// Returns all environment variables as an iterator, filtering out entries
-    /// that are not valid UTF-8.
+    /// Returns all environment variables as an iterator,
+    /// keeping only entries where both the key and value are valid UTF-8.
     pub(crate) fn env(&self) -> impl Iterator<Item = (&str, &str)> {
-        self.env
-            .iter()
-            .filter_map(|(k, v)| Some((k.to_str()?, v.to_str()?)))
+        self.env.iter_str()
     }
 
-    /// Returns all environment variable keys, filtering out entries that are not valid UTF-8.
+    /// Returns all environment variable keys, filtering out keys that are not valid UTF-8.
     fn env_keys(&self) -> impl Iterator<Item = &str> {
-        self.env.iter().filter_map(|(k, _)| k.to_str())
+        self.env.keys_str()
     }
 
     fn get_config_env<T>(&self, key: &ConfigKey) -> Result<OptValue<T>, ConfigError>
     where
         T: FromStr,
         <T as FromStr>::Err: fmt::Display,
     {
-        match self.get_env_str(key.as_env_key()) {
+        match self.env.get_str(key.as_env_key()) {
             Some(value) => {
                 let definition = Definition::Environment(key.as_env_key().to_string());
                 Ok(Some(Value {
@@ -800,59 +763,21 @@ impl Config {
     ///
     /// This can be used similarly to `std::env::var`.
     pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<String> {
-        let key = key.as_ref();
-        let s = self
-            .get_env_os(key)
-            .ok_or_else(|| anyhow!("{key:?} could not be found in the environment snapshot"))?;
-
-        match s.to_str() {
-            Some(s) => Ok(s.to_owned()),
-            None => bail!("environment variable value is not valid unicode: {s:?}"),
-        }
+        self.env.get_env(key)
     }
 
     /// Get the value of environment variable `key` through the `Config` snapshot.
     ///
     /// This can be used similarly to `std::env::var_os`.
     pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
-        match self.env.get(key.as_ref()) {
-            Some(s) => Some(s.clone()),
-            None => {
-                if cfg!(windows) {
-                    self.get_env_case_insensitive(key).cloned()
-                } else {
-                    None
-                }
-            }
-        }
-    }
-
-    /// Wrapper for `self.env.get` when `key` should be case-insensitive.
-    /// This is relevant on Windows, where environment variables are case-insensitive.
-    /// Note that this only works on keys that are valid UTF-8.
-    fn get_env_case_insensitive(&self, key: impl AsRef<OsStr>) -> Option<&OsString> {
-        let upper_case_key = key.as_ref().to_str()?.to_uppercase();
-        // `self.case_insensitive_env` holds pairs like `("PATH", "Path")`
-        // or `("MY-VAR", "my-var")`.
-        let env_key: &OsStr = self.case_insensitive_env.get(&upper_case_key)?.as_ref();
-        self.env.get(env_key)
-    }
-
-    /// Get the value of environment variable `key`.
-    /// Returns `None` if `key` is not in `self.env` or if the value is not valid UTF-8.
-    fn get_env_str(&self, key: impl AsRef<OsStr>) -> Option<&str> {
-        self.env.get(key.as_ref()).and_then(|s| s.to_str())
-    }
-
-    fn env_has_key(&self, key: impl AsRef<OsStr>) -> bool {
-        self.env.contains_key(key.as_ref())
+        self.env.get_env_os(key)
     }
 
     /// Check if the [`Config`] contains a given [`ConfigKey`].
     ///
     /// See `ConfigMapAccess` for a description of `env_prefix_ok`.
     fn has_key(&self, key: &ConfigKey, env_prefix_ok: bool) -> CargoResult<bool> {
-        if self.env_has_key(key.as_env_key()) {
+        if self.env.contains_key(key.as_env_key()) {
             return Ok(true);
         }
         if env_prefix_ok {
@@ -870,7 +795,7 @@ impl Config {
     }
 
     fn check_environment_key_case_mismatch(&self, key: &ConfigKey) {
-        if let Some(env_key) = self.normalized_env.get(key.as_env_key()) {
+        if let Some(env_key) = self.env.get_normalized(key.as_env_key()) {
             let _ = self.shell().warn(format!(
                 "Environment variables are expected to use uppercase letters and underscores, \
                 the variable `{}` will be ignored and have no effect",
@@ -969,7 +894,7 @@ impl Config {
         key: &ConfigKey,
         output: &mut Vec<(String, Definition)>,
     ) -> CargoResult<()> {
-        let env_val = match self.get_env_str(key.as_env_key()) {
+        let env_val = match self.env.get_str(key.as_env_key()) {
             Some(v) => v,
             None => {
                 self.check_environment_key_case_mismatch(key);
