store OsString instead of String in config.env

kylematsuda · kylematsuda · commit b86426246c62 · 2023-02-17T12:19:54.000-07:00
diff --git a/src/cargo/ops/cargo_config.rs b/src/cargo/ops/cargo_config.rs
@@ -93,7 +93,7 @@ fn maybe_env<'config>(
     config: &'config Config,
     key: &ConfigKey,
     cv: &CV,
-) -> Option<Vec<(&'config String, &'config String)>> {
+) -> Option<Vec<(&'config str, &'config str)>> {
     // Only fetching a table is unable to load env values. Leaf entries should
     // work properly.
     match cv {
@@ -102,7 +102,6 @@ fn maybe_env<'config>(
     }
     let mut env: Vec<_> = config
         .env()
-        .iter()
         .filter(|(env_key, _val)| env_key.starts_with(&format!("{}_", key.as_env_key())))
         .collect();
     env.sort_by_key(|x| x.0);
@@ -162,7 +161,7 @@ fn print_toml(config: &Config, opts: &GetOptions<'_>, key: &ConfigKey, cv: &CV)
     }
 }
 
-fn print_toml_env(config: &Config, env: &[(&String, &String)]) {
+fn print_toml_env(config: &Config, env: &[(&str, &str)]) {
     drop_println!(
         config,
         "# The following environment variables may affect the loaded values."
@@ -173,7 +172,7 @@ fn print_toml_env(config: &Config, env: &[(&String, &String)]) {
     }
 }
 
-fn print_json_env(config: &Config, env: &[(&String, &String)]) {
+fn print_json_env(config: &Config, env: &[(&str, &str)]) {
     drop_eprintln!(
         config,
         "note: The following environment variables may affect the loaded values."
@@ -287,7 +286,6 @@ fn print_toml_unmerged(config: &Config, opts: &GetOptions<'_>, key: &ConfigKey)
     // special, and will just naturally get loaded as part of the config.
     let mut env: Vec<_> = config
         .env()
-        .iter()
         .filter(|(env_key, _val)| env_key.starts_with(key.as_env_key()))
         .collect();
     if !env.is_empty() {
diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs
@@ -182,7 +182,6 @@ pub fn registry_credential_config(
         let index = sid.canonical_url();
         let mut names: Vec<_> = config
             .env()
-            .iter()
             .filter_map(|(k, v)| {
                 Some((
                     k.strip_prefix("CARGO_REGISTRIES_")?
diff --git a/src/cargo/util/config/de.rs b/src/cargo/util/config/de.rs
@@ -215,7 +215,7 @@ impl<'config> ConfigMapAccess<'config> {
         if de.config.cli_unstable().advanced_env {
             // `CARGO_PROFILE_DEV_PACKAGE_`
             let env_prefix = format!("{}_", de.key.as_env_key());
-            for env_key in de.config.env.keys() {
+            for env_key in de.config.env_keys() {
                 if env_key.starts_with(&env_prefix) {
                     // `CARGO_PROFILE_DEV_PACKAGE_bar_OPT_LEVEL = 3`
                     let rest = &env_key[env_prefix.len()..];
@@ -265,7 +265,7 @@ impl<'config> ConfigMapAccess<'config> {
         for field in given_fields {
             let mut field_key = de.key.clone();
             field_key.push(field);
-            for env_key in de.config.env.keys() {
+            for env_key in de.config.env_keys() {
                 if env_key.starts_with(field_key.as_env_key()) {
                     fields.insert(KeyKind::Normal(field.to_string()));
                 }
@@ -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.contains_key(env), de.config.get_cv(&de.key)?) {
+            match (de.config.env_has_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/mod.rs b/src/cargo/util/config/mod.rs
@@ -54,7 +54,7 @@ use std::cell::{RefCell, RefMut};
 use std::collections::hash_map::Entry::{Occupied, Vacant};
 use std::collections::{HashMap, HashSet};
 use std::env;
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 use std::fmt;
 use std::fs::{self, File};
 use std::io::prelude::*;
@@ -200,7 +200,7 @@ pub struct Config {
     /// Target Directory via resolved Cli parameter
     target_dir: Option<Filesystem>,
     /// Environment variables, separated to assist testing.
-    env: HashMap<String, String>,
+    env: HashMap<OsString, OsString>,
     /// Environment variables, converted to uppercase to check for case mismatch
     upper_case_env: HashMap<String, String>,
     /// Tracks which sources have been updated to avoid multiple updates.
@@ -260,23 +260,16 @@ impl Config {
             }
         });
 
-        let env: HashMap<_, _> = env::vars_os()
-            .filter_map(|(k, v)| {
-                // Ignore any key/values that are not valid Unicode.
-                match (k.into_string(), v.into_string()) {
-                    (Ok(k), Ok(v)) => Some((k, v)),
-                    _ => None,
-                }
-            })
-            .collect();
+        let env: HashMap<_, _> = env::vars_os().collect();
 
         let upper_case_env = env
-            .clone()
-            .into_iter()
-            .map(|(k, _)| (k.to_uppercase().replace("-", "_"), k))
+            .iter()
+            .filter_map(|(k, _)| k.to_str()) // Only keep valid UTF-8
+            .map(|k| (k.to_uppercase().replace("-", "_"), k.to_owned()))
             .collect();
 
-        let cache_rustc_info = match env.get("CARGO_CACHE_RUSTC_INFO") {
+        let cache_key: &OsStr = "CARGO_CACHE_RUSTC_INFO".as_ref();
+        let cache_rustc_info = match env.get(cache_key) {
             Some(cache) => cache != "0",
             _ => true,
         };
@@ -566,7 +559,7 @@ impl Config {
     pub fn target_dir(&self) -> CargoResult<Option<Filesystem>> {
         if let Some(dir) = &self.target_dir {
             Ok(Some(dir.clone()))
-        } else if let Some(dir) = self.env.get("CARGO_TARGET_DIR") {
+        } else if let Some(dir) = self.get_env_os("CARGO_TARGET_DIR") {
             // Check if the CARGO_TARGET_DIR environment variable is set to an empty string.
             if dir.is_empty() {
                 bail!(
@@ -664,7 +657,7 @@ impl Config {
             // Root table can't have env value.
             return Ok(cv);
         }
-        let env = self.env.get(key.as_env_key());
+        let env = self.get_env_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.
@@ -735,20 +728,28 @@ impl Config {
 
     /// Helper primarily for testing.
     pub fn set_env(&mut self, env: HashMap<String, String>) {
-        self.env = env;
+        self.env = env.into_iter().map(|(k, v)| (k.into(), v.into())).collect();
     }
 
-    /// Returns all environment variables.
-    pub(crate) fn env(&self) -> &HashMap<String, String> {
-        &self.env
+    /// Returns all environment variables as an iterator, filtering out entries
+    /// that are not 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()?)))
+    }
+
+    /// Returns all environment variable keys, filtering out entries that are not valid UTF-8.
+    fn env_keys(&self) -> impl Iterator<Item = &str> {
+        self.env.iter().filter_map(|(k, _)| k.to_str())
     }
 
     fn get_config_env<T>(&self, key: &ConfigKey) -> Result<OptValue<T>, ConfigError>
     where
         T: FromStr,
         <T as FromStr>::Err: fmt::Display,
     {
-        match self.env.get(key.as_env_key()) {
+        match self.get_env_str(key.as_env_key()) {
             Some(value) => {
                 let definition = Definition::Environment(key.as_env_key().to_string());
                 Ok(Some(Value {
@@ -768,33 +769,45 @@ impl Config {
     /// Get the value of environment variable `key` through the `Config` snapshot.
     ///
     /// This can be used similarly to `std::env::var`.
-    pub fn get_env(&self, key: impl AsRef<str>) -> CargoResult<String> {
-        match self.env.get(key.as_ref()) {
-            Some(s) => Ok(s.clone()),
-            None => bail!(
-                "{} could not be found in the environment snapshot",
-                key.as_ref()
-            ),
+    pub fn get_env(&self, key: impl AsRef<OsStr>) -> CargoResult<String> {
+        let key = key.as_ref();
+        let s = match self.env.get(key) {
+            Some(s) => s,
+            None => bail!("{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:?}"),
         }
     }
 
     /// 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<str>) -> Option<String> {
+    pub fn get_env_os(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
         self.env.get(key.as_ref()).cloned()
     }
 
+    /// 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())
+    }
+
     /// 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.contains_key(key.as_env_key()) {
+        if self.env_has_key(key.as_env_key()) {
             return Ok(true);
         }
         if env_prefix_ok {
             let env_prefix = format!("{}_", key.as_env_key());
-            if self.env.keys().any(|k| k.starts_with(&env_prefix)) {
+            if self.env_keys().any(|k| k.starts_with(&env_prefix)) {
                 return Ok(true);
             }
         }
@@ -906,7 +919,7 @@ impl Config {
         key: &ConfigKey,
         output: &mut Vec<(String, Definition)>,
     ) -> CargoResult<()> {
-        let env_val = match self.env.get(key.as_env_key()) {
+        let env_val = match self.get_env_str(key.as_env_key()) {
             Some(v) => v,
             None => {
                 self.check_environment_key_case_mismatch(key);
@@ -1637,7 +1650,7 @@ impl Config {
     ) -> Option<PathBuf> {
         let var = tool.to_uppercase();
 
-        match self.get_env_os(&var) {
+        match self.get_env_os(&var).as_ref().and_then(|s| s.to_str()) {
             Some(tool_path) => {
                 let maybe_relative = tool_path.contains('/') || tool_path.contains('\\');
                 let path = if maybe_relative {
