Auto merge of #11029 - epage:config, r=weihanglo

refactor(cli): Lazy load config

This is trying to clarify `-C` support when it is implemented in #10952

Cargo currently has two initialization states for Config, `Config::default` (process start) and `config.configure` (after parsing args).  The most help we provide for a developer touching this code is a giant `CAUTION` comment in one of the relevant functions.

Currently, #10952 adds another configuration state in the middle where the `current_dir` has been set.

The goal of this PR is to remove that third configuration state by
- Lazy loading `Config::default` so it can be called after parsing `-C`
- Allowing `-C` support to assert that the config isn't loaded yet to catch bugs with it

The hope is this will make the intent of the code clearer and reduce the chance for bugs.

In doing this, there are two intermediate refactorings
- Make help behave like other subcommands
  - Before, we had hacks to intercept raw arguments and to intercept clap errors and assume what their intention was to be able to implement our help system.
  - This flips it around and makes help like any other subcommand,
simplifying cargo initialization.
  - We had to upgrade clap because this exposed a bug where `Command::print_help` wasn't respecting `disable_colored_help(true)`
- Delay fix's access to config

Personally, I also find both changes make the intent of the code clearer.

To review this, I recommend looking at the individual commits.   As this is just refactors, this has no impact on testing.

Author: bors

Cargo.toml

@@ -61,7 +61,7 @@ toml_edit =  { version = "0.14.3", features = ["serde", "easy", "perf"] }
 unicode-xid = "0.2.0"
 url = "2.2.2"
 walkdir = "2.2"
-clap = "3.2.1"
+clap = "3.2.18"
 unicode-width = "0.1.5"
 openssl = { version = '0.10.11', optional = true }
 im-rc = "15.0.0"

src/bin/cargo/cli.rs

@@ -1,10 +1,8 @@
 use anyhow::anyhow;
+use cargo::core::shell::Shell;
 use cargo::core::{features, CliUnstable};
 use cargo::{self, drop_print, drop_println, CliResult, Config};
-use clap::{
-    error::{ContextKind, ContextValue},
-    AppSettings, Arg, ArgMatches,
-};
+use clap::{AppSettings, Arg, ArgMatches};
 use itertools::Itertools;
 use std::collections::HashMap;
 use std::fmt::Write;
@@ -24,36 +22,13 @@ lazy_static::lazy_static! {
     ]);
 }
 
-pub fn main(config: &mut Config) -> CliResult {
+pub fn main(config: &mut LazyConfig) -> CliResult {
+    let args = cli().try_get_matches()?;
+
     // CAUTION: Be careful with using `config` until it is configured below.
     // In general, try to avoid loading config values unless necessary (like
     // the [alias] table).
-
-    if commands::help::handle_embedded_help(config) {
-        return Ok(());
-    }
-
-    let args = match cli().try_get_matches() {
-        Ok(args) => args,
-        Err(e) => {
-            if e.kind() == clap::ErrorKind::UnrecognizedSubcommand {
-                // An unrecognized subcommand might be an external subcommand.
-                let cmd = e
-                    .context()
-                    .find_map(|c| match c {
-                        (ContextKind::InvalidSubcommand, &ContextValue::String(ref cmd)) => {
-                            Some(cmd)
-                        }
-                        _ => None,
-                    })
-                    .expect("UnrecognizedSubcommand implies the presence of InvalidSubcommand");
-                return super::execute_external_subcommand(config, cmd, &[cmd, "--help"])
-                    .map_err(|_| e.into());
-            } else {
-                return Err(e.into());
-            }
-        }
-    };
+    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
@@ -412,7 +387,7 @@ impl GlobalArgs {
     }
 }
 
-fn cli() -> App {
+pub fn cli() -> App {
     let is_rustup = std::env::var_os("RUSTUP_HOME").is_some();
     let usage = if is_rustup {
         "cargo [+toolchain] [OPTIONS] [SUBCOMMAND]"
@@ -425,6 +400,8 @@ fn cli() -> App {
         // Doesn't mix well with our list of common cargo commands.  See clap-rs/clap#3108 for
         // opening clap up to allow us to style our help template
         .disable_colored_help(true)
+        // Provide a custom help subcommand for calling into man pages
+        .disable_help_subcommand(true)
         .override_usage(usage)
         .help_template(
             "\
@@ -488,6 +465,49 @@ See 'cargo help <command>' for more information on a specific command.\n",
         .subcommands(commands::builtin())
 }
 
+/// Delay loading [`Config`] until access.
+///
+/// In the common path, the [`Config`] is dependent on CLI parsing and shouldn't be loaded until
+/// after that is done but some other paths (like fix or earlier errors) might need access to it,
+/// so this provides a way to share the instance and the implementation across these different
+/// accesses.
+pub struct LazyConfig {
+    config: Option<Config>,
+}
+
+impl LazyConfig {
+    pub fn new() -> Self {
+        Self { config: None }
+    }
+
+    /// Check whether the config is loaded
+    ///
+    /// This is useful for asserts in case the environment needs to be setup before loading
+    pub fn is_init(&self) -> bool {
+        self.config.is_some()
+    }
+
+    /// Get the config, loading it if needed
+    ///
+    /// On error, the process is terminated
+    pub fn get(&mut self) -> &Config {
+        self.get_mut()
+    }
+
+    /// Get the config, loading it if needed
+    ///
+    /// On error, the process is terminated
+    pub fn get_mut(&mut self) -> &mut Config {
+        self.config.get_or_insert_with(|| match Config::default() {
+            Ok(cfg) => cfg,
+            Err(e) => {
+                let mut shell = Shell::new();
+                cargo::exit_with_error(e.into(), &mut shell)
+            }
+        })
+    }
+}
+
 #[test]
 fn verify_cli() {
     cli().debug_assert();

src/bin/cargo/commands/help.rs

@@ -1,4 +1,5 @@
 use crate::aliased_command;
+use crate::command_prelude::*;
 use cargo::util::errors::CargoResult;
 use cargo::{drop_println, Config};
 use cargo_util::paths::resolve_executable;
@@ -10,43 +11,26 @@ use std::path::Path;
 
 const COMPRESSED_MAN: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/man.tgz"));
 
-/// Checks if the `help` command is being issued.
-///
-/// This runs before clap processing, because it needs to intercept the `help`
-/// command if a man page is available.
-///
-/// Returns `true` if help information was successfully displayed to the user.
-/// In this case, Cargo should exit.
-pub fn handle_embedded_help(config: &Config) -> bool {
-    match try_help(config) {
-        Ok(true) => true,
-        Ok(false) => false,
-        Err(e) => {
-            log::warn!("help failed: {:?}", e);
-            false
-        }
-    }
+pub fn cli() -> App {
+    subcommand("help")
+        .about("Displays help for a cargo subcommand")
+        .arg(Arg::new("SUBCOMMAND"))
 }
 
-fn try_help(config: &Config) -> CargoResult<bool> {
-    let mut args = std::env::args_os()
-        .skip(1)
-        .skip_while(|arg| arg.to_str().map_or(false, |s| s.starts_with('-')));
-    if !args
-        .next()
-        .map_or(false, |arg| arg.to_str() == Some("help"))
-    {
-        return Ok(false);
+pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
+    let subcommand = args.get_one::<String>("SUBCOMMAND");
+    if let Some(subcommand) = subcommand {
+        if !try_help(config, subcommand)? {
+            crate::execute_external_subcommand(config, subcommand, &[subcommand, "--help"])?;
+        }
+    } else {
+        let mut cmd = crate::cli::cli();
+        let _ = cmd.print_help();
     }
-    let subcommand = match args.next() {
-        Some(arg) => arg,
-        None => return Ok(false),
-    };
-    let subcommand = match subcommand.to_str() {
-        Some(s) => s,
-        None => return Ok(false),
-    };
+    Ok(())
+}
 
+fn try_help(config: &Config, subcommand: &str) -> CargoResult<bool> {
     let subcommand = match check_alias(config, subcommand) {
         // If this alias is more than a simple subcommand pass-through, show the alias.
         Some(argv) if argv.len() > 1 => {

src/bin/cargo/commands/mod.rs

@@ -13,6 +13,7 @@ pub fn builtin() -> Vec<App> {
         fix::cli(),
         generate_lockfile::cli(),
         git_checkout::cli(),
+        help::cli(),
         init::cli(),
         install::cli(),
         locate_project::cli(),
@@ -54,6 +55,7 @@ pub fn builtin_exec(cmd: &str) -> Option<fn(&mut Config, &ArgMatches) -> CliResu
         "fix" => fix::exec,
         "generate-lockfile" => generate_lockfile::exec,
         "git-checkout" => git_checkout::exec,
+        "help" => help::exec,
         "init" => init::exec,
         "install" => install::exec,
         "locate-project" => locate_project::exec,

src/bin/cargo/main.rs

@@ -1,7 +1,6 @@
 #![warn(rust_2018_idioms)] // while we're getting used to 2018
 #![allow(clippy::all)]
 
-use cargo::core::shell::Shell;
 use cargo::util::toml::StringOrVec;
 use cargo::util::CliError;
 use cargo::util::{self, closest_msg, command_prelude, CargoResult, CliResult, Config};
@@ -22,25 +21,17 @@ fn main() {
     #[cfg(not(feature = "pretty-env-logger"))]
     env_logger::init_from_env("CARGO_LOG");
 
-    let mut config = match Config::default() {
-        Ok(cfg) => cfg,
-        Err(e) => {
-            let mut shell = Shell::new();
-            cargo::exit_with_error(e.into(), &mut shell)
-        }
-    };
+    let mut config = cli::LazyConfig::new();
 
-    let result = match cargo::ops::fix_maybe_exec_rustc(&config) {
-        Ok(true) => Ok(()),
-        Ok(false) => {
-            let _token = cargo::util::job::setup();
-            cli::main(&mut config)
-        }
-        Err(e) => Err(CliError::from(e)),
+    let result = if let Some(lock_addr) = cargo::ops::fix_get_proxy_lock_addr() {
+        cargo::ops::fix_exec_rustc(config.get(), &lock_addr).map_err(|e| CliError::from(e))
+    } else {
+        let _token = cargo::util::job::setup();
+        cli::main(&mut config)
     };
 
     match result {
-        Err(e) => cargo::exit_with_error(e, &mut *config.shell()),
+        Err(e) => cargo::exit_with_error(e, &mut config.get_mut().shell()),
         Ok(()) => {}
     }
 }

src/cargo/ops/fix.rs

@@ -333,20 +333,23 @@ to prevent this issue from happening.
     Ok(())
 }
 
+/// Provide the lock address when running in proxy mode
+///
+/// Returns `None` if `fix` is not being run (not in proxy mode). Returns
+/// `Some(...)` if in `fix` proxy mode
+pub fn fix_get_proxy_lock_addr() -> Option<String> {
+    env::var(FIX_ENV).ok()
+}
+
 /// Entry point for `cargo` running as a proxy for `rustc`.
 ///
 /// This is called every time `cargo` is run to check if it is in proxy mode.
 ///
-/// Returns `false` if `fix` is not being run (not in proxy mode). Returns
-/// `true` if in `fix` proxy mode, and the fix was complete without any
-/// warnings or errors. If there are warnings or errors, this does not return,
+/// If there are warnings or errors, this does not return,
 /// and the process exits with the corresponding `rustc` exit code.
-pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> {
-    let lock_addr = match env::var(FIX_ENV) {
-        Ok(s) => s,
-        Err(_) => return Ok(false),
-    };
-
+///
+/// See [`fix_proxy_lock_addr`]
+pub fn fix_exec_rustc(config: &Config, lock_addr: &str) -> CargoResult<()> {
     let args = FixArgs::get()?;
     trace!("cargo-fix as rustc got file {:?}", args.file);
 
@@ -392,7 +395,7 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> {
         // any. If stderr is empty then there's no need for the final exec at
         // the end, we just bail out here.
         if output.status.success() && output.stderr.is_empty() {
-            return Ok(true);
+            return Ok(());
         }
 
         // Otherwise, if our rustc just failed, then that means that we broke the

src/cargo/ops/mod.rs

@@ -19,7 +19,7 @@ pub use self::cargo_read_manifest::{read_package, read_packages};
 pub use self::cargo_run::run;
 pub use self::cargo_test::{run_benches, run_tests, TestOptions};
 pub use self::cargo_uninstall::uninstall;
-pub use self::fix::{fix, fix_maybe_exec_rustc, FixOptions};
+pub use self::fix::{fix, fix_exec_rustc, fix_get_proxy_lock_addr, FixOptions};
 pub use self::lockfile::{load_pkg_lockfile, resolve_to_string, write_pkg_lockfile};
 pub use self::registry::HttpTimeout;
 pub use self::registry::{configure_http_handle, http_handle, http_handle_and_timeout};