Fix error reporting in main and move all arg validation into fn args::parse (#585)

* Fix reporting parsing error from `args::parse`
   Report it in `args::parse` by using `Args::command().error(...).exit()`
   instead of returning `BinstallError`.
* Rm unused variant `BinstallError::OverrideOptionUsedWithMultiInstall`
* Refactor: Move `strategies` validation into `args::parse`
* Rm unused variant `BinstallError::InvalidStrategies`
* Add new unit test `args::test::verify_cli`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

crates/bin/src/args.rs

@@ -7,14 +7,14 @@ use std::{
 };
 
 use binstalk::{
-    errors::BinstallError,
     helpers::remote::tls::Version,
     manifests::cargo_toml_binstall::PkgFmt,
     ops::resolve::{CrateName, VersionReqExt},
 };
-use clap::{Parser, ValueEnum};
+use clap::{error::ErrorKind, CommandFactory, Parser, ValueEnum};
 use log::LevelFilter;
 use semver::VersionReq;
+use strum::EnumCount;
 use strum_macros::EnumCount;
 
 #[derive(Debug, Parser)]
@@ -39,10 +39,10 @@ pub struct Args {
     /// If duplicate names are provided, the last one (and their version requirement)
     /// is kept.
     #[clap(
-    help_heading = "Package selection",
-    value_name = "crate[@version]",
-    required_unless_present_any = ["version", "help"],
-)]
+        help_heading = "Package selection",
+        value_name = "crate[@version]",
+        required_unless_present_any = ["version", "help"],
+    )]
     pub crate_names: Vec<CrateName>,
 
     /// Package version to install.
@@ -309,7 +309,7 @@ pub enum Strategy {
     Compile,
 }
 
-pub fn parse() -> Result<Args, BinstallError> {
+pub fn parse() -> Args {
     // Filter extraneous arg when invoked by cargo
     // `cargo run -- --help` gives ["target/debug/cargo-binstall", "--help"]
     // `cargo binstall --help` gives ["/home/ryan/.cargo/bin/cargo-binstall", "binstall", "--help"]
@@ -335,6 +335,9 @@ pub fn parse() -> Result<Args, BinstallError> {
         opts.log_level = LevelFilter::Off;
     }
 
+    // Ensure no conflict
+    let mut command = Args::command();
+
     if opts.crate_names.len() > 1 {
         let option = if opts.version_req.is_some() {
             "version"
@@ -345,9 +348,95 @@ pub fn parse() -> Result<Args, BinstallError> {
         };
 
         if !option.is_empty() {
-            return Err(BinstallError::OverrideOptionUsedWithMultiInstall { option });
+            command
+                .error(
+                    ErrorKind::ArgumentConflict,
+                    format_args!(
+                        r#"override option used with multi package syntax.
+You cannot use --{option} and specify multiple packages at the same time. Do one or the other."#
+                    ),
+                )
+                .exit();
         }
     }
 
-    Ok(opts)
+    // Check strategies for duplicates
+    let mut new_dup_strategy_err = || {
+        command.error(
+            ErrorKind::TooManyValues,
+            "--strategies should not contain duplicate strategy",
+        )
+    };
+
+    if opts.strategies.len() > Strategy::COUNT {
+        // If len of strategies is larger than number of variants of Strategy,
+        // then there must be duplicates by pigeon hole principle.
+        new_dup_strategy_err().exit()
+    }
+
+    // Whether specific variant of Strategy is present
+    let mut is_variant_present = [false; Strategy::COUNT];
+
+    for strategy in &opts.strategies {
+        let index = *strategy as u8 as usize;
+        if is_variant_present[index] {
+            new_dup_strategy_err().exit()
+        } else {
+            is_variant_present[index] = true;
+        }
+    }
+
+    // Default strategies if empty
+    if opts.strategies.is_empty() {
+        opts.strategies = vec![
+            Strategy::CrateMetaData,
+            Strategy::QuickInstall,
+            Strategy::Compile,
+        ];
+    }
+
+    // Filter out all disabled strategies
+    if !opts.disable_strategies.is_empty() {
+        // Since order doesn't matter, we can sort it and remove all duplicates
+        // to speedup checking.
+        opts.disable_strategies.sort_unstable();
+        opts.disable_strategies.dedup();
+
+        // disable_strategies.len() <= Strategy::COUNT, of which is faster
+        // to just use [Strategy]::contains rather than
+        // [Strategy]::binary_search
+        opts.strategies
+            .retain(|strategy| !opts.disable_strategies.contains(strategy));
+
+        if opts.strategies.is_empty() {
+            command
+                .error(ErrorKind::TooFewValues, "You have disabled all strategies")
+                .exit()
+        }
+
+        // Free disable_strategies as it will not be used again.
+        opts.disable_strategies = Vec::new();
+    }
+
+    // Ensure that Strategy::Compile is specified as the last strategy
+    if opts.strategies[..(opts.strategies.len() - 1)].contains(&Strategy::Compile) {
+        command
+            .error(
+                ErrorKind::InvalidValue,
+                "Compile strategy must be the last one",
+            )
+            .exit()
+    }
+
+    opts
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn verify_cli() {
+        Args::command().debug_assert()
+    }
 }

crates/bin/src/entry.rs

@@ -17,7 +17,6 @@ use binstalk_manifests::{
 use crates_io_api::AsyncClient as CratesIoApiClient;
 use log::LevelFilter;
 use miette::{miette, Result, WrapErr};
-use strum::EnumCount;
 use tokio::task::block_in_place;
 use tracing::{debug, error, info, warn};
 
@@ -28,9 +27,21 @@ use crate::{
 };
 
 pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -> Result<()> {
-    // Compute strategies
-    let (resolvers, cargo_install_fallback) =
-        compute_resolvers(args.strategies, args.disable_strategies)?;
+    // Compute Resolvers
+    let mut cargo_install_fallback = false;
+
+    let resolvers: Vec<_> = args
+        .strategies
+        .into_iter()
+        .filter_map(|strategy| match strategy {
+            Strategy::CrateMetaData => Some(GhCrateMeta::new as Resolver),
+            Strategy::QuickInstall => Some(QuickInstall::new as Resolver),
+            Strategy::Compile => {
+                cargo_install_fallback = true;
+                None
+            }
+        })
+        .collect();
 
     // Compute paths
     let (install_path, cargo_roots, metadata, temp_dir) =
@@ -183,79 +194,6 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
     })
 }
 
-/// Return (resolvers, cargo_install_fallback)
-fn compute_resolvers(
-    mut strategies: Vec<Strategy>,
-    mut disable_strategies: Vec<Strategy>,
-) -> Result<(Vec<Resolver>, bool), BinstallError> {
-    let dup_strategy_err =
-        BinstallError::InvalidStrategies("--strategies should not contain duplicate strategy");
-
-    if strategies.len() > Strategy::COUNT {
-        // If len of strategies is larger than number of variants of Strategy,
-        // then there must be duplicates by pigeon hole principle.
-        return Err(dup_strategy_err);
-    }
-
-    // Whether specific variant of Strategy is present
-    let mut is_variant_present = [false; Strategy::COUNT];
-
-    for strategy in &strategies {
-        let index = *strategy as u8 as usize;
-        if is_variant_present[index] {
-            return Err(dup_strategy_err);
-        } else {
-            is_variant_present[index] = true;
-        }
-    }
-
-    // Default strategies if empty
-    if strategies.is_empty() {
-        strategies = vec![
-            Strategy::CrateMetaData,
-            Strategy::QuickInstall,
-            Strategy::Compile,
-        ];
-    }
-
-    // Filter out all disabled strategies
-    if !disable_strategies.is_empty() {
-        // Since order doesn't matter, we can sort it and remove all duplicates
-        // to speedup checking.
-        disable_strategies.sort_unstable();
-        disable_strategies.dedup();
-
-        // disable_strategies.len() <= Strategy::COUNT, of which is faster
-        // to just use [T]::contains rather than [T]::binary_search
-        strategies.retain(|strategy| !disable_strategies.contains(strategy));
-
-        if strategies.is_empty() {
-            return Err(BinstallError::InvalidStrategies(
-                "You have disabled all strategies",
-            ));
-        }
-    }
-
-    let cargo_install_fallback = *strategies.last().unwrap() == Strategy::Compile;
-
-    if cargo_install_fallback {
-        strategies.pop().unwrap();
-    }
-
-    let resolvers = strategies
-        .into_iter()
-        .map(|strategy| match strategy {
-            Strategy::CrateMetaData => Ok(GhCrateMeta::new as Resolver),
-            Strategy::QuickInstall => Ok(QuickInstall::new as Resolver),
-            Strategy::Compile => Err(BinstallError::InvalidStrategies(
-                "Compile strategy must be the last one",
-            )),
-        })
-        .collect::<Result<Vec<_>, BinstallError>>()?;
-
-    Ok((resolvers, cargo_install_fallback))
-}
-
 /// Return (install_path, cargo_roots, metadata, temp_dir)
 fn compute_paths_and_load_manifests(
     roots: Option<PathBuf>,

crates/bin/src/main.rs

@@ -18,10 +18,7 @@ fn main() -> MainExit {
     // This must be the very first thing to happen
     let jobserver_client = LazyJobserverClient::new();
 
-    let args = match args::parse() {
-        Ok(args) => args,
-        Err(err) => return MainExit::Error(err),
-    };
+    let args = args::parse();
 
     if args.version {
         println!("{}", env!("CARGO_PKG_VERSION"));

crates/binstalk/src/errors.rs

@@ -205,28 +205,6 @@ pub enum BinstallError {
     )]
     SuperfluousVersionOption,
 
-    /// An override option is used when multiple packages are to be installed.
-    ///
-    /// This is raised when more than one package name is provided and any of:
-    ///
-    /// - `--version`
-    /// - `--manifest-path`
-    /// - `--bin-dir`
-    /// - `--pkg-fmt`
-    /// - `--pkg-url`
-    ///
-    /// is provided.
-    ///
-    /// - Code: `binstall::conflict::overrides`
-    /// - Exit: 85
-    #[error("override option used with multi package syntax")]
-    #[diagnostic(
-        severity(error),
-        code(binstall::conflict::overrides),
-        help("You cannot use --{option} and specify multiple packages at the same time. Do one or the other.")
-    )]
-    OverrideOptionUsedWithMultiInstall { option: &'static str },
-
     /// No binaries were found for the crate.
     ///
     /// When installing, either the binaries are specified in the crate's Cargo.toml, or they're
@@ -304,14 +282,6 @@ pub enum BinstallError {
     #[diagnostic(severity(error), code(binstall::SourceFilePath))]
     EmptySourceFilePath,
 
-    /// Invalid strategies configured.
-    ///
-    /// - Code: `binstall::strategies`
-    /// - Exit: 93
-    #[error("Invalid strategies configured: {0}")]
-    #[diagnostic(severity(error), code(binstall::strategies))]
-    InvalidStrategies(&'static str),
-
     /// Fallback to `cargo-install` is disabled.
     ///
     /// - Code: `binstall::no_fallback_to_cargo_install`
@@ -344,7 +314,6 @@ impl BinstallError {
             VersionParse { .. } => 80,
             VersionMismatch { .. } => 82,
             SuperfluousVersionOption => 84,
-            OverrideOptionUsedWithMultiInstall { .. } => 85,
             UnspecifiedBinaries => 86,
             NoViableTargets => 87,
             BinFileNotFound(_) => 88,
@@ -352,7 +321,6 @@ impl BinstallError {
             DuplicateSourceFilePath { .. } => 90,
             InvalidSourceFilePath { .. } => 91,
             EmptySourceFilePath => 92,
-            InvalidStrategies(..) => 93,
             NoFallbackToCargoInstall => 94,
             CrateContext(context) => context.err.exit_number(),
         };