refactor(gctx): ConfigValue getter cleanup (#16067)

### What does this PR try to resolve?

When trying to add support of "array of tables" in config,
there are some minor refactors that could help the migration a bit
easier.

This PR deals with unnecessary helper functions and tries to inline
them.

### How to test and review this PR?

No change in user-facing behavior.

Author: ehuss

src/cargo/ops/resolve.rs

@@ -527,7 +527,7 @@ pub fn add_overrides<'a>(
     ws: &Workspace<'a>,
 ) -> CargoResult<()> {
     let gctx = ws.gctx();
-    let Some(paths) = gctx.get_list("paths")? else {
+    let Some(paths) = gctx.paths_overrides()? else {
         return Ok(());
     };
 

src/cargo/util/context/de.rs

@@ -155,8 +155,24 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> {
         V: de::Visitor<'de>,
     {
         if name == "StringList" {
-            let vals = self.gctx.get_list_or_string(&self.key)?;
-            let vals: Vec<String> = vals.into_iter().map(|vd| vd.0).collect();
+            let mut res = Vec::new();
+
+            match self.gctx.get_cv(&self.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) => {
+                    self.gctx
+                        .expected("string or array of strings", &self.key, &val)?;
+                }
+                None => {}
+            }
+
+            self.gctx.get_env_list(&self.key, &mut res)?;
+
+            let vals: Vec<String> = res.into_iter().map(|vd| vd.0).collect();
             visitor.visit_newtype_struct(vals.into_deserializer())
         } else {
             visitor.visit_newtype_struct(self)
@@ -386,8 +402,15 @@ struct ConfigSeqAccess {
 impl ConfigSeqAccess {
     fn new(de: Deserializer<'_>) -> Result<ConfigSeqAccess, ConfigError> {
         let mut res = Vec::new();
-        if let Some(v) = de.gctx._get_list(&de.key)? {
-            res.extend(v.val);
+
+        match de.gctx.get_cv(&de.key)? {
+            Some(CV::List(val, _definition)) => {
+                res.extend(val);
+            }
+            Some(val) => {
+                de.gctx.expected("list", &de.key, &val)?;
+            }
+            None => {}
         }
 
         de.gctx.get_env_list(&de.key, &mut res)?;

src/cargo/util/context/mod.rs

@@ -986,47 +986,6 @@ impl GlobalContext {
         }
     }
 
-    /// Get a list of strings.
-    ///
-    /// DO NOT USE outside of the config module. `pub` will be removed in the
-    /// future.
-    ///
-    /// NOTE: this does **not** support environment variables. Use `get` instead
-    /// if you want that.
-    pub fn get_list(&self, key: &str) -> CargoResult<OptValue<Vec<(String, Definition)>>> {
-        let key = ConfigKey::from_str(key);
-        self._get_list(&key)
-    }
-
-    fn _get_list(&self, key: &ConfigKey) -> CargoResult<OptValue<Vec<(String, Definition)>>> {
-        match self.get_cv(key)? {
-            Some(CV::List(val, definition)) => Ok(Some(Value { val, definition })),
-            Some(val) => self.expected("list", key, &val),
-            None => Ok(None),
-        }
-    }
-
-    /// 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.
     /// If the key is a non-mergeable list and a value is found in the environment, existing values are cleared.
     fn get_env_list(
@@ -1807,6 +1766,17 @@ impl GlobalContext {
             .unwrap_or_else(|| PathBuf::from(tool_str))
     }
 
+    /// Get the `paths` overrides config value.
+    pub fn paths_overrides(&self) -> CargoResult<OptValue<Vec<(String, Definition)>>> {
+        let key = ConfigKey::from_str("paths");
+        // paths overrides cannot be set via env config, so use get_cv here.
+        match self.get_cv(&key)? {
+            Some(CV::List(val, definition)) => Ok(Some(Value { val, definition })),
+            Some(val) => self.expected("list", &key, &val),
+            None => Ok(None),
+        }
+    }
+
     pub fn jobserver_from_env(&self) -> Option<&jobserver::Client> {
         self.jobserver.as_ref()
     }

tests/testsuite/config_cli.rs

@@ -3,7 +3,6 @@
 use std::{collections::HashMap, fs};
 
 use crate::prelude::*;
-use cargo::util::context::Definition;
 use cargo_test_support::compare::assert_e2e;
 use cargo_test_support::paths;
 use cargo_test_support::str;
@@ -264,33 +263,6 @@ fn merges_table() {
     assert_eq!(gctx.get::<i32>("foo.key5").unwrap(), 9);
 }
 
-#[cargo_test]
-fn merge_array_mixed_def_paths() {
-    // Merging of arrays with different def sites.
-    write_config_toml(
-        "
-        paths = ['file']
-        ",
-    );
-    // Create a directory for CWD to differentiate the paths.
-    let somedir = paths::root().join("somedir");
-    fs::create_dir(&somedir).unwrap();
-    let gctx = GlobalContextBuilder::new()
-        .cwd(&somedir)
-        .config_arg("paths=['cli']")
-        // env is currently ignored for get_list()
-        .env("CARGO_PATHS", "env")
-        .build();
-    let paths = gctx.get_list("paths").unwrap().unwrap();
-    // The definition for the root value is somewhat arbitrary, but currently starts with the file because that is what is loaded first.
-    assert_eq!(paths.definition, Definition::Path(paths::root()));
-    assert_eq!(paths.val.len(), 2);
-    assert_eq!(paths.val[0].0, "file");
-    assert_eq!(paths.val[0].1.root(&gctx), paths::root());
-    assert_eq!(paths.val[1].0, "cli");
-    assert_eq!(paths.val[1].1.root(&gctx), somedir);
-}
-
 #[cargo_test]
 fn enforces_format() {
     // These dotted key expressions should all be fine.

tests/testsuite/paths.rs

@@ -250,3 +250,56 @@ https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html
 "#]])
         .run();
 }
+
+#[cargo_test]
+fn env_paths_overrides_not_supported() {
+    Package::new("file", "0.1.0").publish();
+    Package::new("cli", "0.1.0").publish();
+    Package::new("env", "0.1.0").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                edition = "2015"
+
+                [dependencies]
+                file = "0.1.0"
+                cli = "0.1.0"
+                env = "0.1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("file/Cargo.toml", &basic_manifest("file", "0.2.0"))
+        .file("file/src/lib.rs", "")
+        .file("cli/Cargo.toml", &basic_manifest("cli", "0.2.0"))
+        .file("cli/src/lib.rs", "")
+        .file("env/Cargo.toml", &basic_manifest("env", "0.2.0"))
+        .file("env/src/lib.rs", "")
+        .file(".cargo/config.toml", r#"paths = ["file"]"#)
+        .build();
+
+    p.cargo("check")
+        .arg("--config")
+        .arg("paths=['cli']")
+        // paths overrides ignore env
+        .env("CARGO_PATHS", "env")
+        .with_stderr_data(
+            str![[r#"
+[UPDATING] `dummy-registry` index
+[LOCKING] 3 packages to latest compatible versions
+[DOWNLOADING] crates ...
+[DOWNLOADED] env v0.1.0 (registry `dummy-registry`)
+[CHECKING] file v0.2.0 ([ROOT]/foo/file)
+[CHECKING] cli v0.2.0 ([ROOT]/foo/cli)
+[CHECKING] env v0.1.0
+[CHECKING] foo v0.0.0 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+
+"#]]
+            .unordered(),
+        )
+        .run();
+}