Auto merge of #11087 - weihanglo:issue-11013, r=epage

Report cmd aliasing failure with more contexts

### What does this PR try to resolve?

Commands aliasing resolution should report TOML parsing error to users.

Fixes #11013.

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

ef7a4ef is the most important commit in this PR. `has_key` now throws errors after this PR, so any use `Config::get<Option<…>>()` is affected if there is a malformed config. Had a skim over all usages of `get::<Option<…>>`, `get_string` and `get_path`, I don't feel any of them should ignore errors.

Author: bors

src/bin/cargo/cli.rs

@@ -32,9 +32,6 @@ pub fn main(config: &mut LazyConfig) -> CliResult {
     // the [alias] table).
     let config = config.get_mut();
 
-    // Global args need to be extracted before expanding aliases because the
-    // clap code for extracting a subcommand discards global options
-    // (appearing before the subcommand).
     let (expanded_args, global_args) = expand_aliases(config, args, vec![])?;
 
     if expanded_args
@@ -224,26 +221,34 @@ fn add_ssl(version_string: &mut String) {
     }
 }
 
+/// Expands aliases recursively to collect all the command line arguments.
+///
+/// [`GlobalArgs`] need to be extracted before expanding aliases because the
+/// clap code for extracting a subcommand discards global options
+/// (appearing before the subcommand).
 fn expand_aliases(
     config: &mut Config,
     args: ArgMatches,
     mut already_expanded: Vec<String>,
 ) -> Result<(ArgMatches, GlobalArgs), CliError> {
     if let Some((cmd, args)) = args.subcommand() {
-        match (
-            commands::builtin_exec(cmd),
-            super::aliased_command(config, cmd)?,
-        ) {
-            (Some(_), Some(_)) => {
+        let exec = commands::builtin_exec(cmd);
+        let aliased_cmd = super::aliased_command(config, cmd);
+
+        match (exec, aliased_cmd) {
+            (Some(_), Ok(Some(_))) => {
                 // User alias conflicts with a built-in subcommand
                 config.shell().warn(format!(
                     "user-defined alias `{}` is ignored, because it is shadowed by a built-in command",
                     cmd,
                 ))?;
             }
-            (Some(_), None) => {
-                // Command is built-in and is not conflicting with alias, but contains ignored values.
+            (Some(_), Ok(None) | Err(_)) => {
+                // Here we ignore errors from aliasing as we already favor built-in command,
+                // and alias doesn't involve in this context.
+
                 if let Some(values) = args.get_many::<OsString>("") {
+                    // Command is built-in and is not conflicting with alias, but contains ignored values.
                     return Err(anyhow::format_err!(
                         "\
 trailing arguments after built-in command `{}` are unsupported: `{}`
@@ -255,8 +260,8 @@ To pass the arguments to the subcommand, remove `--`",
                     .into());
                 }
             }
-            (None, None) => {}
-            (_, Some(alias)) => {
+            (None, Ok(None)) => {}
+            (None, Ok(Some(alias))) => {
                 // Check if this alias is shadowing an external subcommand
                 // (binary of the form `cargo-<subcommand>`)
                 // Currently this is only a warning, but after a transition period this will become
@@ -300,6 +305,7 @@ For more information, see issue #10049 <https://github.com/rust-lang/cargo/issue
                 let (expanded_args, _) = expand_aliases(config, new_args, already_expanded)?;
                 return Ok((expanded_args, global_args));
             }
+            (None, Err(e)) => return Err(e.into()),
         }
     };
 

src/bin/cargo/main.rs

@@ -53,6 +53,14 @@ fn builtin_aliases_execs(cmd: &str) -> Option<&(&str, &str, &str)> {
     BUILTIN_ALIASES.iter().find(|alias| alias.0 == cmd)
 }
 
+/// Resolve the aliased command from the [`Config`] with a given command string.
+///
+/// The search fallback chain is:
+///
+/// 1. Get the aliased command as a string.
+/// 2. If an `Err` occurs (missing key, type mismatch, or any possible error),
+///    try to get it as an array again.
+/// 3. If still cannot find any, finds one insides [`BUILTIN_ALIASES`].
 fn aliased_command(config: &Config, command: &str) -> CargoResult<Option<Vec<String>>> {
     let alias_name = format!("alias.{}", command);
     let user_alias = match config.get_string(&alias_name) {

src/cargo/util/config/de.rs

@@ -80,7 +80,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
     where
         V: de::Visitor<'de>,
     {
-        if self.config.has_key(&self.key, self.env_prefix_ok) {
+        if self.config.has_key(&self.key, self.env_prefix_ok)? {
             visitor.visit_some(self)
         } else {
             // Treat missing values as `None`.

src/cargo/util/config/mod.rs

@@ -682,25 +682,25 @@ impl Config {
         }
     }
 
-    fn has_key(&self, key: &ConfigKey, env_prefix_ok: bool) -> bool {
+    /// 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()) {
-            return true;
+            return Ok(true);
         }
-        // See ConfigMapAccess for a description of this.
         if env_prefix_ok {
             let env_prefix = format!("{}_", key.as_env_key());
             if self.env.keys().any(|k| k.starts_with(&env_prefix)) {
-                return true;
+                return Ok(true);
             }
         }
-        if let Ok(o_cv) = self.get_cv(key) {
-            if o_cv.is_some() {
-                return true;
-            }
+        if self.get_cv(key)?.is_some() {
+            return Ok(true);
         }
         self.check_environment_key_case_mismatch(key);
 
-        false
+        Ok(false)
     }
 
     fn check_environment_key_case_mismatch(&self, key: &ConfigKey) {

tests/testsuite/bad_config.rs

@@ -19,8 +19,8 @@ fn bad1() {
         .with_status(101)
         .with_stderr(
             "\
-[ERROR] invalid configuration for key `target.nonexistent-target`
-expected a table, but found a string for `[..]` in [..]config
+[ERROR] expected table for configuration key `target.nonexistent-target`, \
+but found string in [..]config
 ",
         )
         .run();

tests/testsuite/cargo_alias_config.rs

@@ -21,14 +21,88 @@ fn alias_incorrect_config_type() {
 
     p.cargo("b-cargo-test -v")
         .with_status(101)
-        .with_stderr_contains(
+        .with_stderr(
             "\
 [ERROR] invalid configuration for key `alias.b-cargo-test`
 expected a list, but found a integer for [..]",
         )
         .run();
 }
 
+#[cargo_test]
+fn alias_malformed_config_string() {
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            ".cargo/config",
+            r#"
+                [alias]
+                b-cargo-test = `
+            "#,
+        )
+        .build();
+
+    p.cargo("b-cargo-test -v")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] could not load Cargo configuration
+
+Caused by:
+  could not parse TOML configuration in `[..]/config`
+
+Caused by:
+  [..]
+
+Caused by:
+  TOML parse error at line [..]
+    |
+  3 |                 b-cargo-test = `
+    |                                ^
+  Unexpected ```
+  Expected quoted string
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn alias_malformed_config_list() {
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            ".cargo/config",
+            r#"
+                [alias]
+                b-cargo-test = [1, 2]
+            "#,
+        )
+        .build();
+
+    p.cargo("b-cargo-test -v")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] could not load Cargo configuration
+
+Caused by:
+  failed to load TOML configuration from `[..]/config`
+
+Caused by:
+  [..] `alias`
+
+Caused by:
+  [..] `b-cargo-test`
+
+Caused by:
+  expected string but found integer in list
+",
+        )
+        .run();
+}
+
 #[cargo_test]
 fn alias_config() {
     let p = project()

tests/testsuite/config.rs

@@ -1074,10 +1074,6 @@ Dotted key `ssl-version` attempted to extend non-table type (string)
 
 ",
     );
-    assert!(config
-        .get::<Option<SslVersionConfig>>("http.ssl-version")
-        .unwrap()
-        .is_none());
 }
 
 #[cargo_test]