Auto merge of #9791 - nipunn1313:cargo_crash_alias_loop, r=alexcrichton

Teach cargo to failfast on recursive/corecursive aliases

Eg.
[alias]
test-1 = test-2
test-2 = test-3
test-3 = test-1

Previously it would stack overflow
Pulled out non controversial bits from from #9768

Author: bors

crates/cargo-test-support/src/tools.rs

@@ -1,6 +1,6 @@
 //! Common executables that can be reused by various tests.
 
-use crate::{basic_manifest, paths, project};
+use crate::{basic_manifest, paths, project, Project};
 use lazy_static::lazy_static;
 use std::path::PathBuf;
 use std::sync::Mutex;
@@ -38,3 +38,22 @@ pub fn echo_wrapper() -> PathBuf {
     *lock = Some(path.clone());
     path
 }
+
+/// Returns a project which builds a cargo-echo simple subcommand
+pub fn echo_subcommand() -> Project {
+    let p = project()
+        .at("cargo-echo")
+        .file("Cargo.toml", &basic_manifest("cargo-echo", "0.0.1"))
+        .file(
+            "src/main.rs",
+            r#"
+                fn main() {
+                    let args: Vec<_> = ::std::env::args().skip(1).collect();
+                    println!("{}", args.join(" "));
+                }
+            "#,
+        )
+        .build();
+    p.cargo("build").run();
+    p
+}

src/bin/cargo/cli.rs

@@ -1,3 +1,4 @@
+use anyhow::anyhow;
 use cargo::core::{features, CliUnstable};
 use cargo::{self, drop_print, drop_println, CliResult, Config};
 use clap::{AppSettings, Arg, ArgMatches};
@@ -123,7 +124,7 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'",
     // 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)?;
+    let (expanded_args, global_args) = expand_aliases(config, args, vec![])?;
     let (cmd, subcommand_args) = match expanded_args.subcommand() {
         (cmd, Some(args)) => (cmd, args),
         _ => {
@@ -159,6 +160,7 @@ pub fn get_version_string(is_verbose: bool) -> String {
 fn expand_aliases(
     config: &mut Config,
     args: ArgMatches<'static>,
+    mut already_expanded: Vec<String>,
 ) -> Result<(ArgMatches<'static>, GlobalArgs), CliError> {
     if let (cmd, Some(args)) = args.subcommand() {
         match (
@@ -197,7 +199,21 @@ fn expand_aliases(
                 let new_args = cli()
                     .setting(AppSettings::NoBinaryName)
                     .get_matches_from_safe(alias)?;
-                let (expanded_args, _) = expand_aliases(config, new_args)?;
+
+                let (new_cmd, _) = new_args.subcommand();
+                already_expanded.push(cmd.to_string());
+                if already_expanded.contains(&new_cmd.to_string()) {
+                    // Crash if the aliases are corecursive / unresolvable
+                    return Err(anyhow!(
+                        "alias {} has unresolvable recursive definition: {} -> {}",
+                        already_expanded[0],
+                        already_expanded.join(" -> "),
+                        new_cmd,
+                    )
+                    .into());
+                }
+
+                let (expanded_args, _) = expand_aliases(config, new_args, already_expanded)?;
                 return Ok((expanded_args, global_args));
             }
         }

tests/testsuite/cargo_alias_config.rs

@@ -1,5 +1,8 @@
 //! Tests for `[alias]` config command aliases.
 
+use std::env;
+
+use cargo_test_support::tools::echo_subcommand;
 use cargo_test_support::{basic_bin_manifest, project};
 
 #[cargo_test]
@@ -50,7 +53,7 @@ fn alias_config() {
 }
 
 #[cargo_test]
-fn recursive_alias() {
+fn dependent_alias() {
     let p = project()
         .file("Cargo.toml", &basic_bin_manifest("foo"))
         .file("src/main.rs", "fn main() {}")
@@ -73,6 +76,84 @@ fn recursive_alias() {
         .run();
 }
 
+#[cargo_test]
+fn default_args_alias() {
+    let echo = echo_subcommand();
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            ".cargo/config",
+            r#"
+                [alias]
+                echo = "echo --flag1 --flag2"
+                test-1 = "echo"
+                build = "build --verbose"
+            "#,
+        )
+        .build();
+
+    let mut paths: Vec<_> = env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect();
+    paths.push(echo.target_debug_dir());
+    let path = env::join_paths(paths).unwrap();
+
+    p.cargo("echo")
+        .env("PATH", &path)
+        .with_status(101)
+        .with_stderr("error: alias echo has unresolvable recursive definition: echo -> echo")
+        .run();
+
+    p.cargo("test-1")
+        .env("PATH", &path)
+        .with_status(101)
+        .with_stderr(
+            "error: alias test-1 has unresolvable recursive definition: test-1 -> echo -> echo",
+        )
+        .run();
+
+    // Builtins are not expanded by rule
+    p.cargo("build")
+        .with_stderr(
+            "\
+[WARNING] user-defined alias `build` is ignored, because it is shadowed by a built-in command
+[COMPILING] foo v0.5.0 ([..])
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn corecursive_alias() {
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            ".cargo/config",
+            r#"
+                [alias]
+                test-1 = "test-2 --flag1"
+                test-2 = "test-3 --flag2"
+                test-3 = "test-1 --flag3"
+            "#,
+        )
+        .build();
+
+    p.cargo("test-1")
+        .with_status(101)
+        .with_stderr(
+            "error: alias test-1 has unresolvable recursive definition: test-1 -> test-2 -> test-3 -> test-1",
+        )
+        .run();
+
+    p.cargo("test-2")
+        .with_status(101)
+        .with_stderr(
+            "error: alias test-2 has unresolvable recursive definition: test-2 -> test-3 -> test-1 -> test-2",
+        )
+        .run();
+}
+
 #[cargo_test]
 fn alias_list_test() {
     let p = project()

tests/testsuite/cargo_command.rs

@@ -7,10 +7,11 @@ use std::path::{Path, PathBuf};
 use std::process::Stdio;
 use std::str;
 
-use cargo_test_support::cargo_process;
-use cargo_test_support::paths;
 use cargo_test_support::registry::Package;
-use cargo_test_support::{basic_bin_manifest, basic_manifest, cargo_exe, project, project_in_home};
+use cargo_test_support::tools::echo_subcommand;
+use cargo_test_support::{
+    basic_bin_manifest, cargo_exe, cargo_process, paths, project, project_in_home,
+};
 
 fn path() -> Vec<PathBuf> {
     env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect()
@@ -283,31 +284,17 @@ fn cargo_subcommand_env() {
 
 #[cargo_test]
 fn cargo_subcommand_args() {
-    let p = project()
-        .at("cargo-foo")
-        .file("Cargo.toml", &basic_manifest("cargo-foo", "0.0.1"))
-        .file(
-            "src/main.rs",
-            r#"
-                fn main() {
-                    let args: Vec<_> = ::std::env::args().collect();
-                    println!("{}", args.join(" "));
-                }
-            "#,
-        )
-        .build();
-
-    p.cargo("build").run();
-    let cargo_foo_bin = p.bin("cargo-foo");
+    let p = echo_subcommand();
+    let cargo_foo_bin = p.bin("cargo-echo");
     assert!(cargo_foo_bin.is_file());
 
     let mut path = path();
     path.push(p.target_debug_dir());
     let path = env::join_paths(path.iter()).unwrap();
 
-    cargo_process("foo bar -v --help")
+    cargo_process("echo bar -v --help")
         .env("PATH", &path)
-        .with_stdout("[CWD]/cargo-foo/target/debug/cargo-foo[EXE] foo bar -v --help")
+        .with_stdout("echo bar -v --help")
         .run();
 }