Auto merge of rust-lang#11077 - weihanglo:issue-10992, r=ehuss

Config file loaded via CLI takes priority over env vars

### What does this PR try to resolve?

Fixes rust-lang#10992

Behaviour changes: After this commit, config files loaded via CLI take
priority over env vars. Config files loaded via [`config-include`]
feature also get a higher priority over env vars, if the initial config
file is being loaded via CLI.

Cargo knows why it loads a specific config file with `WhyLoad` enum,
and store in the field of `Definition::Cli(…)`. With this additional data,
config files loaded via `--config <path>` get a `Definition::Cli(Some(…))`.
In contrast, `--config <value>` with ordinary values become `Definition::Cli(None)`.

The error message of the `Definition::Cli(Some(…))` is identical to
`Definition::Path(…)`, so we won't lose any path information to debug.

[`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include

### How should we test and review this PR?

Reviewing commit by commit is probably fine. The first two commits adding tests to `config-cli` and `config-include`, which exercises the expected behaviour we are going to fix in the next commits.

To check the precedence, set up a project with an extra config file, such as

```
# Expect to have a target-dir named `foo`
CARGO_BUILD_TARGET_DIR='env' cargo c --config 'build.target-dir = "cli"' --config .cargo/foo.toml

# Inside .cargo/foo.toml
[build]
target-dir = "foo"
```

### Additional information

This is a bit hacky IMO. I don't like leaking the path info to `Definition::Cli`. However, it is really tricky to provide information for deserialization before values get merged.

src/cargo/util/config/de.rs

@@ -473,7 +473,13 @@ impl<'de, 'config> de::MapAccess<'de> for ValueDeserializer<'config> {
             Definition::Environment(env) => {
                 seed.deserialize(Tuple2Deserializer(1i32, env.as_str()))
             }
-            Definition::Cli => seed.deserialize(Tuple2Deserializer(2i32, "")),
+            Definition::Cli(path) => {
+                let str = path
+                    .as_ref()
+                    .map(|p| p.to_string_lossy())
+                    .unwrap_or_default();
+                seed.deserialize(Tuple2Deserializer(2i32, str))
+            }
         }
     }
 }

src/cargo/util/config/mod.rs

@@ -121,6 +121,20 @@ macro_rules! get_value_typed {
     };
 }
 
+/// Indicates why a config value is being loaded.
+#[derive(Clone, Copy, Debug)]
+enum WhyLoad {
+    /// Loaded due to a request from the global cli arg `--config`
+    ///
+    /// Indirect configs loaded via [`config-include`] are also seen as from cli args,
+    /// if the initial config is being loaded from cli.
+    ///
+    /// [`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include
+    Cli,
+    /// Loaded due to config file discovery.
+    FileDiscovery,
+}
+
 /// Configuration information for cargo. This is not specific to a build, it is information
 /// relating to cargo itself.
 #[derive(Debug)]
@@ -1005,12 +1019,15 @@ impl Config {
         self.load_values_from(&self.cwd)
     }
 
+    /// Like [`load_values`](Config::load_values) but without merging config values.
+    ///
+    /// This is primarily crafted for `cargo config` command.
     pub(crate) fn load_values_unmerged(&self) -> CargoResult<Vec<ConfigValue>> {
         let mut result = Vec::new();
         let mut seen = HashSet::new();
         let home = self.home_path.clone().into_path_unlocked();
         self.walk_tree(&self.cwd, &home, |path| {
-            let mut cv = self._load_file(path, &mut seen, false)?;
+            let mut cv = self._load_file(path, &mut seen, false, WhyLoad::FileDiscovery)?;
             if self.cli_unstable().config_include {
                 self.load_unmerged_include(&mut cv, &mut seen, &mut result)?;
             }
@@ -1021,6 +1038,9 @@ impl Config {
         Ok(result)
     }
 
+    /// Like [`load_includes`](Config::load_includes) but without merging config values.
+    ///
+    /// This is primarily crafted for `cargo config` command.
     fn load_unmerged_include(
         &self,
         cv: &mut CV,
@@ -1029,23 +1049,26 @@ impl Config {
     ) -> CargoResult<()> {
         let includes = self.include_paths(cv, false)?;
         for (path, abs_path, def) in includes {
-            let mut cv = self._load_file(&abs_path, seen, false).with_context(|| {
-                format!("failed to load config include `{}` from `{}`", path, def)
-            })?;
+            let mut cv = self
+                ._load_file(&abs_path, seen, false, WhyLoad::FileDiscovery)
+                .with_context(|| {
+                    format!("failed to load config include `{}` from `{}`", path, def)
+                })?;
             self.load_unmerged_include(&mut cv, seen, output)?;
             output.push(cv);
         }
         Ok(())
     }
 
+    /// Start a config file discovery from a path and merges all config values found.
     fn load_values_from(&self, path: &Path) -> CargoResult<HashMap<String, ConfigValue>> {
         // This definition path is ignored, this is just a temporary container
         // representing the entire file.
         let mut cfg = CV::Table(HashMap::new(), Definition::Path(PathBuf::from(".")));
         let home = self.home_path.clone().into_path_unlocked();
 
         self.walk_tree(path, &home, |path| {
-            let value = self.load_file(path, true)?;
+            let value = self.load_file(path)?;
             cfg.merge(value, false).with_context(|| {
                 format!("failed to merge configuration at `{}`", path.display())
             })?;
@@ -1059,15 +1082,28 @@ impl Config {
         }
     }
 
-    fn load_file(&self, path: &Path, includes: bool) -> CargoResult<ConfigValue> {
-        self._load_file(path, &mut HashSet::new(), includes)
+    /// Loads a config value from a path.
+    ///
+    /// This is used during config file discovery.
+    fn load_file(&self, path: &Path) -> CargoResult<ConfigValue> {
+        self._load_file(path, &mut HashSet::new(), true, WhyLoad::FileDiscovery)
     }
 
+    /// Loads a config value from a path with options.
+    ///
+    /// This is actual implementation of loading a config value from a path.
+    ///
+    /// * `includes` determines whether to load configs from [`config-include`].
+    /// * `seen` is used to check for cyclic includes.
+    /// * `why_load` tells why a config is being loaded.
+    ///
+    /// [`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include
     fn _load_file(
         &self,
         path: &Path,
         seen: &mut HashSet<PathBuf>,
         includes: bool,
+        why_load: WhyLoad,
     ) -> CargoResult<ConfigValue> {
         if !seen.insert(path.to_path_buf()) {
             bail!(
@@ -1080,15 +1116,18 @@ impl Config {
         let toml = cargo_toml::parse(&contents, path, self).with_context(|| {
             format!("could not parse TOML configuration in `{}`", path.display())
         })?;
-        let value =
-            CV::from_toml(Definition::Path(path.to_path_buf()), toml).with_context(|| {
-                format!(
-                    "failed to load TOML configuration from `{}`",
-                    path.display()
-                )
-            })?;
+        let def = match why_load {
+            WhyLoad::Cli => Definition::Cli(Some(path.into())),
+            WhyLoad::FileDiscovery => Definition::Path(path.into()),
+        };
+        let value = CV::from_toml(def, toml).with_context(|| {
+            format!(
+                "failed to load TOML configuration from `{}`",
+                path.display()
+            )
+        })?;
         if includes {
-            self.load_includes(value, seen)
+            self.load_includes(value, seen, why_load)
         } else {
             Ok(value)
         }
@@ -1098,8 +1137,14 @@ impl Config {
     ///
     /// Returns `value` with the given include files merged into it.
     ///
-    /// `seen` is used to check for cyclic includes.
-    fn load_includes(&self, mut value: CV, seen: &mut HashSet<PathBuf>) -> CargoResult<CV> {
+    /// * `seen` is used to check for cyclic includes.
+    /// * `why_load` tells why a config is being loaded.
+    fn load_includes(
+        &self,
+        mut value: CV,
+        seen: &mut HashSet<PathBuf>,
+        why_load: WhyLoad,
+    ) -> CargoResult<CV> {
         // Get the list of files to load.
         let includes = self.include_paths(&mut value, true)?;
         // Check unstable.
@@ -1109,7 +1154,7 @@ impl Config {
         // Accumulate all values here.
         let mut root = CV::Table(HashMap::new(), value.definition().clone());
         for (path, abs_path, def) in includes {
-            self._load_file(&abs_path, seen, true)
+            self._load_file(&abs_path, seen, true, why_load)
                 .and_then(|include| root.merge(include, true))
                 .with_context(|| {
                     format!("failed to load config include `{}` from `{}`", path, def)
@@ -1127,8 +1172,8 @@ impl Config {
     ) -> CargoResult<Vec<(String, PathBuf, Definition)>> {
         let abs = |path: &str, def: &Definition| -> (String, PathBuf, Definition) {
             let abs_path = match def {
-                Definition::Path(p) => p.parent().unwrap().join(&path),
-                Definition::Environment(_) | Definition::Cli => self.cwd().join(&path),
+                Definition::Path(p) | Definition::Cli(Some(p)) => p.parent().unwrap().join(&path),
+                Definition::Environment(_) | Definition::Cli(None) => self.cwd().join(&path),
             };
             (path.to_string(), abs_path, def.clone())
         };
@@ -1162,7 +1207,7 @@ impl Config {
 
     /// Parses the CLI config args and returns them as a table.
     pub(crate) fn cli_args_as_table(&self) -> CargoResult<ConfigValue> {
-        let mut loaded_args = CV::Table(HashMap::new(), Definition::Cli);
+        let mut loaded_args = CV::Table(HashMap::new(), Definition::Cli(None));
         let cli_args = match &self.cli_config {
             Some(cli_args) => cli_args,
             None => return Ok(loaded_args),
@@ -1178,7 +1223,7 @@ impl Config {
                         anyhow::format_err!("config path {:?} is not utf-8", arg_as_path)
                     })?
                     .to_string();
-                self._load_file(&self.cwd().join(&str_path), &mut seen, true)
+                self._load_file(&self.cwd().join(&str_path), &mut seen, true, WhyLoad::Cli)
                     .with_context(|| format!("failed to load config from `{}`", str_path))?
             } else {
                 // We only want to allow "dotted key" (see https://toml.io/en/v1.0.0#keys)
@@ -1273,11 +1318,11 @@ impl Config {
                     );
                 }
 
-                CV::from_toml(Definition::Cli, toml_v)
+                CV::from_toml(Definition::Cli(None), toml_v)
                     .with_context(|| format!("failed to convert --config argument `{arg}`"))?
             };
             let tmp_table = self
-                .load_includes(tmp_table, &mut HashSet::new())
+                .load_includes(tmp_table, &mut HashSet::new(), WhyLoad::Cli)
                 .with_context(|| "failed to load --config include".to_string())?;
             loaded_args
                 .merge(tmp_table, true)
@@ -1431,7 +1476,7 @@ impl Config {
             None => return Ok(()),
         };
 
-        let mut value = self.load_file(&credentials, true)?;
+        let mut value = self.load_file(&credentials)?;
         // Backwards compatibility for old `.cargo/credentials` layout.
         {
             let (value_map, def) = match value {

src/cargo/util/config/value.rs

@@ -59,7 +59,8 @@ pub enum Definition {
     /// Defined in an environment variable, includes the environment key.
     Environment(String),
     /// Passed in on the command line.
-    Cli,
+    /// A path is attached when the config value is a path to a config file.
+    Cli(Option<PathBuf>),
 }
 
 impl Definition {
@@ -69,8 +70,8 @@ impl Definition {
     /// CLI and env are the current working directory.
     pub fn root<'a>(&'a self, config: &'a Config) -> &'a Path {
         match self {
-            Definition::Path(p) => p.parent().unwrap().parent().unwrap(),
-            Definition::Environment(_) | Definition::Cli => config.cwd(),
+            Definition::Path(p) | Definition::Cli(Some(p)) => p.parent().unwrap().parent().unwrap(),
+            Definition::Environment(_) | Definition::Cli(None) => config.cwd(),
         }
     }
 
@@ -80,8 +81,8 @@ impl Definition {
     pub fn is_higher_priority(&self, other: &Definition) -> bool {
         matches!(
             (self, other),
-            (Definition::Cli, Definition::Environment(_))
-                | (Definition::Cli, Definition::Path(_))
+            (Definition::Cli(_), Definition::Environment(_))
+                | (Definition::Cli(_), Definition::Path(_))
                 | (Definition::Environment(_), Definition::Path(_))
         )
     }
@@ -100,9 +101,9 @@ impl PartialEq for Definition {
 impl fmt::Display for Definition {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Definition::Path(p) => p.display().fmt(f),
+            Definition::Path(p) | Definition::Cli(Some(p)) => p.display().fmt(f),
             Definition::Environment(key) => write!(f, "environment variable `{}`", key),
-            Definition::Cli => write!(f, "--config cli option"),
+            Definition::Cli(None) => write!(f, "--config cli option"),
         }
     }
 }
@@ -218,8 +219,11 @@ impl<'de> de::Deserialize<'de> for Definition {
         match discr {
             0 => Ok(Definition::Path(value.into())),
             1 => Ok(Definition::Environment(value)),
-            2 => Ok(Definition::Cli),
-            _ => panic!("unexpected discriminant {} value {}", discr, value),
+            2 => {
+                let path = (value.len() > 0).then_some(value.into());
+                Ok(Definition::Cli(path))
+            }
+            _ => panic!("unexpected discriminant {discr} value {value}"),
         }
     }
 }

tests/testsuite/config_cli.rs

@@ -1,6 +1,8 @@
 //! Tests for the --config CLI option.
 
-use super::config::{assert_error, assert_match, read_output, write_config, ConfigBuilder};
+use super::config::{
+    assert_error, assert_match, read_output, write_config, write_config_at, ConfigBuilder,
+};
 use cargo::util::config::Definition;
 use cargo_test_support::paths;
 use std::{collections::HashMap, fs};
@@ -53,6 +55,72 @@ fn cli_priority() {
     assert_eq!(config.get::<bool>("term.quiet").unwrap(), true);
 }
 
+#[cargo_test]
+fn merge_primitives_for_multiple_cli_occurences() {
+    let config_path0 = ".cargo/file0.toml";
+    write_config_at(config_path0, "k = 'file0'");
+    let config_path1 = ".cargo/file1.toml";
+    write_config_at(config_path1, "k = 'file1'");
+
+    // k=env0
+    let config = ConfigBuilder::new().env("CARGO_K", "env0").build();
+    assert_eq!(config.get::<String>("k").unwrap(), "env0");
+
+    // k=env0
+    // --config k='cli0'
+    // --config k='cli1'
+    let config = ConfigBuilder::new()
+        .env("CARGO_K", "env0")
+        .config_arg("k='cli0'")
+        .config_arg("k='cli1'")
+        .build();
+    assert_eq!(config.get::<String>("k").unwrap(), "cli1");
+
+    // Env has a lower priority when comparing with file from CLI arg.
+    //
+    // k=env0
+    // --config k='cli0'
+    // --config k='cli1'
+    // --config .cargo/file0.toml
+    let config = ConfigBuilder::new()
+        .env("CARGO_K", "env0")
+        .config_arg("k='cli0'")
+        .config_arg("k='cli1'")
+        .config_arg(config_path0)
+        .build();
+    assert_eq!(config.get::<String>("k").unwrap(), "file0");
+
+    // k=env0
+    // --config k='cli0'
+    // --config k='cli1'
+    // --config .cargo/file0.toml
+    // --config k='cli2'
+    let config = ConfigBuilder::new()
+        .env("CARGO_K", "env0")
+        .config_arg("k='cli0'")
+        .config_arg("k='cli1'")
+        .config_arg(config_path0)
+        .config_arg("k='cli2'")
+        .build();
+    assert_eq!(config.get::<String>("k").unwrap(), "cli2");
+
+    // k=env0
+    // --config k='cli0'
+    // --config k='cli1'
+    // --config .cargo/file0.toml
+    // --config k='cli2'
+    // --config .cargo/file1.toml
+    let config = ConfigBuilder::new()
+        .env("CARGO_K", "env0")
+        .config_arg("k='cli0'")
+        .config_arg("k='cli1'")
+        .config_arg(config_path0)
+        .config_arg("k='cli2'")
+        .config_arg(config_path1)
+        .build();
+    assert_eq!(config.get::<String>("k").unwrap(), "file1");
+}
+
 #[cargo_test]
 fn merges_array() {
     // Array entries are appended.