Auto merge of #8393 - ludumipsum:unstable_flags_in_config, r=alexcric…

…hton

Allow configuring unstable flags via config file

# Summary

This fixes #8127 by mapping the `unstable` key in `.cargo/config` to Z flags.

It should have no impact on stable/beta cargo, and on nightlies it gives folks the ability to configure Z flags for an entire project or workspace. This is meant to make it easier to try things like the [new features resolver](#8088) behavior, mtime-on-use, build-std, or timing reports across a whole project.

I've also included a small (but entirely independent) ergonomics change -- treating dashes and underscores identically in Z flags. That's along for the ride in this PR as the last commit, and is included here because it makes for more idiomatic toml file keys (`print_im_a_teapot = yes` vs `print-im-a-teapot = yes`). Please let me know if y'all would prefer that be in a separate PR, or not happen at all.

# Test Plan

Apologies if I've missed anything -- this is my first cargo contrib and I've tried to hew to the contributing guide. If I've slipped up, please let me know how and I'll fix it.

NB. My linux machine doesn't have multilib set up, so I disabled cross tests.

 * `cargo test` passes for each commit in the stack
 * I formatted each commit in the stack with `rustfmt`
 * New tests are included alongside the relevant change for each change
 * I've validated each test by locally undoing the code change they support and confirming failure.
 * The CLI wins, for both enable and disabling Z flags, as you'd expect.

Keys in `unstable` which do not correspond with a Z flag will trigger an error indicating the invalid flag came from a config file read:

```
Invalid [unstable] entry in Cargo config

Caused by:
  unknown `-Z` flag specified: an-invalid-flag
```

If you'd like to see a test case which isn't represented here, I'm happy to add it. Just let me know.

# Documentation

I've included commits in this stack updating the only docs page that seemed relevant to me, skimming the book -- `unstable.md`.

src/cargo/core/compiler/rustdoc.rs

@@ -52,7 +52,8 @@ impl<'de> serde::de::Deserialize<'de> for RustdocExternMode {
     }
 }
 
-#[derive(serde::Deserialize, Debug)]
+#[derive(serde::Deserialize, Debug, Default)]
+#[serde(default)]
 pub struct RustdocExternMap {
     registries: HashMap<String, String>,
     std: Option<RustdocExternMode>,

src/cargo/core/features.rs

@@ -333,7 +333,8 @@ impl Features {
 ///    and then test for your flag or your value and act accordingly.
 ///
 /// If you have any trouble with this, please let us know!
-#[derive(Default, Debug)]
+#[derive(Default, Debug, Deserialize)]
+#[serde(default, rename_all = "kebab-case")]
 pub struct CliUnstable {
     pub print_im_a_teapot: bool,
     pub unstable_options: bool,

src/cargo/util/config/de.rs

@@ -4,6 +4,7 @@ use crate::util::config::value;
 use crate::util::config::{Config, ConfigError, ConfigKey};
 use crate::util::config::{ConfigValue as CV, Definition, Value};
 use serde::{de, de::IntoDeserializer};
+use std::collections::HashSet;
 use std::vec;
 
 /// Serde deserializer used to convert config values to a target type using
@@ -269,37 +270,54 @@ impl<'config> ConfigMapAccess<'config> {
 
     fn new_struct(
         de: Deserializer<'config>,
-        fields: &'static [&'static str],
+        given_fields: &'static [&'static str],
     ) -> Result<ConfigMapAccess<'config>, ConfigError> {
-        let fields: Vec<KeyKind> = fields
-            .iter()
-            .map(|field| KeyKind::Normal(field.to_string()))
-            .collect();
+        let table = de.config.get_table(&de.key)?;
 
         // Assume that if we're deserializing a struct it exhaustively lists all
         // possible fields on this key that we're *supposed* to use, so take
         // this opportunity to warn about any keys that aren't recognized as
         // fields and warn about them.
-        if let Some(mut v) = de.config.get_table(&de.key)? {
-            for (t_key, value) in v.val.drain() {
-                if fields.iter().any(|k| match k {
-                    KeyKind::Normal(s) => s == &t_key,
-                    KeyKind::CaseSensitive(s) => s == &t_key,
-                }) {
-                    continue;
-                }
+        if let Some(v) = table.as_ref() {
+            let unused_keys = v
+                .val
+                .iter()
+                .filter(|(k, _v)| !given_fields.iter().any(|gk| gk == k));
+            for (unused_key, unused_value) in unused_keys {
                 de.config.shell().warn(format!(
                     "unused config key `{}.{}` in `{}`",
                     de.key,
-                    t_key,
-                    value.definition()
+                    unused_key,
+                    unused_value.definition()
                 ))?;
             }
         }
 
+        let mut fields = HashSet::new();
+
+        // If the caller is interested in a field which we can provide from
+        // the environment, get it from there.
+        for field in given_fields {
+            let mut field_key = de.key.clone();
+            field_key.push(field);
+            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()));
+                }
+            }
+        }
+
+        // Add everything from the config table we're interested in that we
+        // haven't already provided via an environment variable
+        if let Some(v) = table {
+            for key in v.val.keys() {
+                fields.insert(KeyKind::Normal(key.clone()));
+            }
+        }
+
         Ok(ConfigMapAccess {
             de,
-            fields,
+            fields: fields.into_iter().collect(),
             field_index: 0,
         })
     }

src/cargo/util/config/mod.rs

@@ -742,10 +742,17 @@ impl Config {
                 .unwrap_or(false);
         self.target_dir = cli_target_dir;
 
+        // If nightly features are enabled, allow setting Z-flags from config
+        // using the `unstable` table. Ignore that block otherwise.
         if nightly_features_allowed() {
-            if let Some(val) = self.get::<Option<bool>>("unstable.mtime_on_use")? {
-                self.unstable_flags.mtime_on_use |= val;
+            if let Some(unstable_flags) = self.get::<Option<CliUnstable>>("unstable")? {
+                self.unstable_flags = unstable_flags;
             }
+            // NB. It's not ideal to parse these twice, but doing it again here
+            //     allows the CLI to override config files for both enabling
+            //     and disabling, and doing it up top allows CLI Zflags to
+            //     control config parsing behavior.
+            self.unstable_flags.parse(unstable_flags)?;
         }
 
         Ok(())

src/cargo/util/toml/mod.rs

@@ -394,7 +394,7 @@ impl<'de> de::Deserialize<'de> for U32OrBool {
 }
 
 #[derive(Deserialize, Serialize, Clone, Debug, Default, Eq, PartialEq)]
-#[serde(rename_all = "kebab-case")]
+#[serde(default, rename_all = "kebab-case")]
 pub struct TomlProfile {
     pub opt_level: Option<TomlOptLevel>,
     pub lto: Option<StringOrBool>,

src/doc/src/reference/unstable.md

@@ -7,6 +7,16 @@ see a list of flags available.
 `-Z unstable-options` is a generic flag for enabling other unstable
 command-line flags. Options requiring this will be called out below.
 
+Anything which can be configured with a Z flag can also be set in the cargo
+config file (`.cargo/config.toml`) in the `unstable` table. For example:
+
+```toml
+[unstable]
+mtime-on-use = 'yes'
+multitarget = 'yes'
+timings = 'yes'
+```
+
 Some unstable features will require you to specify the `cargo-features` key in
 `Cargo.toml`.
 

tests/testsuite/config.rs

@@ -687,10 +687,7 @@ Caused by:
         f3: i64,
         big: i64,
     }
-    assert_error(
-        config.get::<S>("S").unwrap_err(),
-        "missing config key `S.f3`",
-    );
+    assert_error(config.get::<S>("S").unwrap_err(), "missing field `f3`");
 }
 
 #[cargo_test]
@@ -1094,6 +1091,78 @@ Caused by:
         .is_none());
 }
 
+#[cargo_test]
+/// Assert that unstable options can be configured with the `unstable` table in
+/// cargo config files
+fn unstable_table_notation() {
+    cargo::core::enable_nightly_features();
+    write_config(
+        "\
+[unstable]
+print-im-a-teapot = true
+",
+    );
+    let config = ConfigBuilder::new().build();
+    assert_eq!(config.cli_unstable().print_im_a_teapot, true);
+}
+
+#[cargo_test]
+/// Assert that dotted notation works for configuring unstable options
+fn unstable_dotted_notation() {
+    cargo::core::enable_nightly_features();
+    write_config(
+        "\
+unstable.print-im-a-teapot = true
+",
+    );
+    let config = ConfigBuilder::new().build();
+    assert_eq!(config.cli_unstable().print_im_a_teapot, true);
+}
+
+#[cargo_test]
+/// Assert that Zflags on the CLI take precedence over those from config
+fn unstable_cli_precedence() {
+    cargo::core::enable_nightly_features();
+    write_config(
+        "\
+unstable.print-im-a-teapot = true
+",
+    );
+    let config = ConfigBuilder::new().build();
+    assert_eq!(config.cli_unstable().print_im_a_teapot, true);
+
+    let config = ConfigBuilder::new()
+        .unstable_flag("print-im-a-teapot=no")
+        .build();
+    assert_eq!(config.cli_unstable().print_im_a_teapot, false);
+}
+
+#[cargo_test]
+/// Assert that atempting to set an unstable flag that doesn't exist via config
+/// is ignored on stable
+fn unstable_invalid_flag_ignored_on_stable() {
+    write_config(
+        "\
+unstable.an-invalid-flag = 'yes'
+",
+    );
+    assert!(ConfigBuilder::new().build_err().is_ok());
+}
+
+#[cargo_test]
+/// Assert that unstable options can be configured with the `unstable` table in
+/// cargo config files
+fn unstable_flags_ignored_on_stable() {
+    write_config(
+        "\
+[unstable]
+print-im-a-teapot = true
+",
+    );
+    let config = ConfigBuilder::new().build();
+    assert_eq!(config.cli_unstable().print_im_a_teapot, false);
+}
+
 #[cargo_test]
 fn table_merge_failure() {
     // Config::merge fails to merge entries in two tables.
@@ -1177,6 +1246,27 @@ fn struct_with_opt_inner_struct() {
     assert_eq!(f.inner.unwrap().value.unwrap(), 12);
 }
 
+#[cargo_test]
+fn struct_with_default_inner_struct() {
+    // Struct with serde defaults.
+    // Check that can be defined with environment variable.
+    #[derive(Deserialize, Default)]
+    #[serde(default)]
+    struct Inner {
+        value: i32,
+    }
+    #[derive(Deserialize, Default)]
+    #[serde(default)]
+    struct Foo {
+        inner: Inner,
+    }
+    let config = ConfigBuilder::new()
+        .env("CARGO_FOO_INNER_VALUE", "12")
+        .build();
+    let f: Foo = config.get("foo").unwrap();
+    assert_eq!(f.inner.value, 12);
+}
+
 #[cargo_test]
 fn overlapping_env_config() {
     // Issue where one key is a prefix of another.
@@ -1208,6 +1298,100 @@ fn overlapping_env_config() {
     assert_eq!(s.debug, Some(1));
 }
 
+#[cargo_test]
+fn overlapping_env_with_defaults_errors_out() {
+    // Issue where one key is a prefix of another.
+    // This is a limitation of mapping environment variables on to a hierarchy.
+    // Check that we error out when we hit ambiguity in this way, rather than
+    // the more-surprising defaulting through.
+    // If, in the future, we can handle this more correctly, feel free to delete
+    // this test.
+    #[derive(Deserialize, Default)]
+    #[serde(default, rename_all = "kebab-case")]
+    struct Ambig {
+        debug: u32,
+        debug_assertions: bool,
+    }
+    let config = ConfigBuilder::new()
+        .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
+        .build();
+    let err = config.get::<Ambig>("ambig").err().unwrap();
+    assert!(format!("{}", err).contains("missing config key `ambig.debug`"));
+
+    let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "5").build();
+    let s: Ambig = config.get("ambig").unwrap();
+    assert_eq!(s.debug_assertions, bool::default());
+    assert_eq!(s.debug, 5);
+
+    let config = ConfigBuilder::new()
+        .env("CARGO_AMBIG_DEBUG", "1")
+        .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
+        .build();
+    let s: Ambig = config.get("ambig").unwrap();
+    assert_eq!(s.debug_assertions, true);
+    assert_eq!(s.debug, 1);
+}
+
+#[cargo_test]
+fn struct_with_overlapping_inner_struct_and_defaults() {
+    // Struct with serde defaults.
+    // Check that can be defined with environment variable.
+    #[derive(Deserialize, Default)]
+    #[serde(default)]
+    struct Inner {
+        value: i32,
+    }
+
+    // Containing struct with a prefix of inner
+    //
+    // This is a limitation of mapping environment variables on to a hierarchy.
+    // Check that we error out when we hit ambiguity in this way, rather than
+    // the more-surprising defaulting through.
+    // If, in the future, we can handle this more correctly, feel free to delete
+    // this case.
+    #[derive(Deserialize, Default)]
+    #[serde(default)]
+    struct PrefixContainer {
+        inn: bool,
+        inner: Inner,
+    }
+    let config = ConfigBuilder::new()
+        .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
+        .build();
+    let err = config
+        .get::<PrefixContainer>("prefixcontainer")
+        .err()
+        .unwrap();
+    assert!(format!("{}", err).contains("missing config key `prefixcontainer.inn`"));
+    let config = ConfigBuilder::new()
+        .env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
+        .env("CARGO_PREFIXCONTAINER_INN", "true")
+        .build();
+    let f: PrefixContainer = config.get("prefixcontainer").unwrap();
+    assert_eq!(f.inner.value, 12);
+    assert_eq!(f.inn, true);
+
+    // Containing struct where the inner value's field is a prefix of another
+    //
+    // This is a limitation of mapping environment variables on to a hierarchy.
+    // Check that we error out when we hit ambiguity in this way, rather than
+    // the more-surprising defaulting through.
+    // If, in the future, we can handle this more correctly, feel free to delete
+    // this case.
+    #[derive(Deserialize, Default)]
+    #[serde(default)]
+    struct InversePrefixContainer {
+        inner_field: bool,
+        inner: Inner,
+    }
+    let config = ConfigBuilder::new()
+        .env("CARGO_INVERSEPREFIXCONTAINER_INNER_VALUE", "12")
+        .build();
+    let f: InversePrefixContainer = config.get("inverseprefixcontainer").unwrap();
+    assert_eq!(f.inner_field, bool::default());
+    assert_eq!(f.inner.value, 12);
+}
+
 #[cargo_test]
 fn string_list_tricky_env() {
     // Make sure StringList handles typed env values.