Auto merge of #12515 - arlosi:high-pri-last, r=weihanglo

config: merge lists in precedence order

When merging configuration lists, the current order does not match the expected precedence. This makes merged lists follow precedence order, with higher precedence items merged later in lists.

When a list in configuration exists in multiple places, Cargo merges the lists together. The ordering of this merging is unexpected and does not follow the precedence rules that non-list configuration uses.

The current merging order appears to be:
* project-specific `config.toml`
* global `config.toml`
* command-line (`--config`)
* environment variable (`CARGO_*`)

This PR changes the order to follow the precedence rules with higher precedence configuration merging later in the lists.
* global `config.toml`
* project-specific `config.toml`
* environment variable (`CARGO_*`)
* command-line (`--config`)

This aligns with config such as `build.rustflags` where later flags take precedence over earlier ones.

Since `--config` is relatively new, it's unlikely to cause too much breakage by making it come after environment variables.

Switching global and project-specific ordering is more likely to cause breakage, since it's been around longer (reported as an issue in #8128). Projects relying on global configuration flags (in `$CARGO_HOME\config.toml` or in `.cargo/config.toml` further from the project) being merged first in lists will be broken.

For most uses of merged lists (such as `build.rustflags`), if the flags do not conflict with each other, there will be no impact.

Fixes #12506
Fixes #8128

src/cargo/util/config/mod.rs

@@ -938,6 +938,7 @@ impl Config {
                     .map(|s| (s.to_string(), def.clone())),
             );
         }
+        output.sort_by(|a, b| a.1.cmp(&b.1));
         Ok(())
     }
 
@@ -2106,16 +2107,22 @@ impl ConfigValue {
 
     /// Merge the given value into self.
     ///
-    /// If `force` is true, primitive (non-container) types will override existing values.
-    /// If false, the original will be kept and the new value ignored.
+    /// If `force` is true, primitive (non-container) types will override existing values
+    /// of equal priority. For arrays, incoming values of equal priority will be placed later.
     ///
     /// Container types (tables and arrays) are merged with existing values.
     ///
     /// Container and non-container types cannot be mixed.
     fn merge(&mut self, from: ConfigValue, force: bool) -> CargoResult<()> {
         match (self, from) {
             (&mut CV::List(ref mut old, _), CV::List(ref mut new, _)) => {
-                old.extend(mem::take(new).into_iter());
+                if force {
+                    old.append(new);
+                } else {
+                    new.append(old);
+                    mem::swap(new, old);
+                }
+                old.sort_by(|a, b| a.1.cmp(&b.1));
             }
             (&mut CV::Table(ref mut old, _), CV::Table(ref mut new, _)) => {
                 for (key, value) in mem::take(new) {

src/cargo/util/config/value.rs

@@ -10,6 +10,7 @@
 
 use crate::util::config::Config;
 use serde::de;
+use std::cmp::Ordering;
 use std::fmt;
 use std::marker;
 use std::mem;
@@ -63,6 +64,24 @@ pub enum Definition {
     Cli(Option<PathBuf>),
 }
 
+impl PartialOrd for Definition {
+    fn partial_cmp(&self, other: &Definition) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Ord for Definition {
+    fn cmp(&self, other: &Definition) -> Ordering {
+        if mem::discriminant(self) == mem::discriminant(other) {
+            Ordering::Equal
+        } else if self.is_higher_priority(other) {
+            Ordering::Greater
+        } else {
+            Ordering::Less
+        }
+    }
+}
+
 impl Definition {
     /// Root directory where this is defined.
     ///

src/doc/src/reference/config.md

@@ -28,7 +28,8 @@ with a configuration file in your home directory.
 If a key is specified in multiple config files, the values will get merged
 together. Numbers, strings, and booleans will use the value in the deeper
 config directory taking precedence over ancestor directories, where the
-home directory is the lowest priority. Arrays will be joined together.
+home directory is the lowest priority. Arrays will be joined together
+with higher precedence items being placed later in the merged array.
 
 At present, when being invoked from a workspace, Cargo does not read config
 files from crates within the workspace. i.e. if a workspace has two crates in

tests/testsuite/cargo_config/mod.rs

@@ -84,7 +84,7 @@ fn get_toml() {
 alias.foo = \"abc --xyz\"
 alias.sub-example = [\"sub\", \"example\"]
 build.jobs = 99
-build.rustflags = [\"--flag-directory\", \"--flag-global\"]
+build.rustflags = [\"--flag-global\", \"--flag-directory\"]
 extra-table.somekey = \"somevalue\"
 profile.dev.opt-level = 3
 profile.dev.package.foo.opt-level = 1
@@ -111,7 +111,7 @@ target.\"cfg(target_os = \\\"linux\\\")\".runner = \"runme\"
     cargo_process("config get build.rustflags -Zunstable-options")
         .cwd(&sub_folder.parent().unwrap())
         .masquerade_as_nightly_cargo(&["cargo-config"])
-        .with_stdout("build.rustflags = [\"--flag-directory\", \"--flag-global\"]")
+        .with_stdout("build.rustflags = [\"--flag-global\", \"--flag-directory\"]")
         .with_stderr("")
         .run();
 
@@ -171,8 +171,8 @@ fn get_json() {
               "build": {
                 "jobs": 99,
                 "rustflags": [
-                  "--flag-directory",
-                  "--flag-global"
+                  "--flag-global",
+                  "--flag-directory"
                 ]
               },
               "extra-table": {
@@ -259,8 +259,8 @@ alias.sub-example = [
 ]
 build.jobs = 99 # [ROOT]/home/.cargo/config.toml
 build.rustflags = [
-    \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
     \"--flag-global\", # [ROOT]/home/.cargo/config.toml
+    \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
 ]
 extra-table.somekey = \"somevalue\" # [ROOT]/home/.cargo/config.toml
 profile.dev.opt-level = 3 # [ROOT]/home/.cargo/config.toml
@@ -280,8 +280,8 @@ target.\"cfg(target_os = \\\"linux\\\")\".runner = \"runme\" # [ROOT]/home/.carg
         .with_stdout(
             "\
 build.rustflags = [
-    \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
     \"--flag-global\", # [ROOT]/home/.cargo/config.toml
+    \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
     \"env1\", # environment variable `CARGO_BUILD_RUSTFLAGS`
     \"env2\", # environment variable `CARGO_BUILD_RUSTFLAGS`
 ]
@@ -310,12 +310,12 @@ fn show_origin_toml_cli() {
         .with_stdout(
             "\
 build.rustflags = [
-    \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
     \"--flag-global\", # [ROOT]/home/.cargo/config.toml
-    \"cli1\", # --config cli option
-    \"cli2\", # --config cli option
+    \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
     \"env1\", # environment variable `CARGO_BUILD_RUSTFLAGS`
     \"env2\", # environment variable `CARGO_BUILD_RUSTFLAGS`
+    \"cli1\", # --config cli option
+    \"cli2\", # --config cli option
 ]
 ",
         )
@@ -471,7 +471,7 @@ fn includes() {
     cargo_process("config get build.rustflags -Zunstable-options -Zconfig-include")
         .cwd(&sub_folder.parent().unwrap())
         .masquerade_as_nightly_cargo(&["cargo-config", "config-include"])
-        .with_stdout(r#"build.rustflags = ["--flag-other", "--flag-directory", "--flag-global"]"#)
+        .with_stdout(r#"build.rustflags = ["--flag-global", "--flag-other", "--flag-directory"]"#)
         .with_stderr("")
         .run();
 
@@ -481,9 +481,9 @@ fn includes() {
         .with_stdout(
             "\
 build.rustflags = [
+    \"--flag-global\", # [ROOT]/home/.cargo/config.toml
     \"--flag-other\", # [ROOT]/foo/.cargo/other.toml
     \"--flag-directory\", # [ROOT]/foo/.cargo/config.toml
-    \"--flag-global\", # [ROOT]/home/.cargo/config.toml
 ]
 ",
         )

tests/testsuite/config.rs

@@ -541,18 +541,18 @@ expected boolean, but found array",
         config.get::<VSOB>("b").unwrap(),
         VSOB::VecString(vec![
             "b".to_string(),
-            "clib".to_string(),
             "env1".to_string(),
-            "env2".to_string()
+            "env2".to_string(),
+            "clib".to_string(),
         ])
     );
     assert_eq!(
         config.get::<VSOB>("c").unwrap(),
         VSOB::VecString(vec![
             "c".to_string(),
-            "clic".to_string(),
             "e1".to_string(),
-            "e2".to_string()
+            "e2".to_string(),
+            "clic".to_string(),
         ])
     );
 }
@@ -1582,12 +1582,12 @@ known-hosts = [
         .as_ref()
         .unwrap();
     assert_eq!(kh.len(), 4);
-    assert_eq!(kh[0].val, "example.org ...");
-    assert_eq!(kh[0].definition, Definition::Path(foo_path.clone()));
-    assert_eq!(kh[1].val, "example.com ...");
+    assert_eq!(kh[0].val, "example.com ...");
+    assert_eq!(kh[0].definition, Definition::Path(root_path.clone()));
+    assert_eq!(kh[1].val, "example.net ...");
     assert_eq!(kh[1].definition, Definition::Path(root_path.clone()));
-    assert_eq!(kh[2].val, "example.net ...");
-    assert_eq!(kh[2].definition, Definition::Path(root_path.clone()));
+    assert_eq!(kh[2].val, "example.org ...");
+    assert_eq!(kh[2].definition, Definition::Path(foo_path.clone()));
     assert_eq!(kh[3].val, "env-example");
     assert_eq!(
         kh[3].definition,

tests/testsuite/config_cli.rs

@@ -143,11 +143,9 @@ fn merges_array() {
         .env("CARGO_BUILD_RUSTFLAGS", "--env1 --env2")
         .config_arg("build.rustflags = ['--cli']")
         .build();
-    // The order of cli/env is a little questionable here, but would require
-    // much more complex merging logic.
     assert_eq!(
         config.get::<Vec<String>>("build.rustflags").unwrap(),
-        ["--file", "--cli", "--env1", "--env2"]
+        ["--file", "--env1", "--env2", "--cli"]
     );
 
     // With advanced-env.
@@ -158,7 +156,7 @@ fn merges_array() {
         .build();
     assert_eq!(
         config.get::<Vec<String>>("build.rustflags").unwrap(),
-        ["--file", "--cli", "--env"]
+        ["--file", "--env", "--cli"]
     );
 
     // Merges multiple instances.
@@ -202,7 +200,7 @@ fn string_list_array() {
             .get::<cargo::util::config::StringList>("build.rustflags")
             .unwrap()
             .as_slice(),
-        ["--file", "--cli", "--env1", "--env2"]
+        ["--file", "--env1", "--env2", "--cli"]
     );
 
     // With advanced-env.
@@ -216,7 +214,7 @@ fn string_list_array() {
             .get::<cargo::util::config::StringList>("build.rustflags")
             .unwrap()
             .as_slice(),
-        ["--file", "--cli", "--env"]
+        ["--file", "--env", "--cli"]
     );
 }