Rewatch cli help: do not show build command options in the root help (#7715)

* Rewatch cli help: do not show build command options in the root help

* Satisfy clippy

* CHANGELOG

# Conflicts:
#	CHANGELOG.md

# Conflicts:
#	CHANGELOG.md

* Text + Formatting

* Fix implementation

* Added tests

* Bug fix

* Simplify and satisfy clippy

* Fix non-utf8 error

* Satisfy clippy

* rewatch: harden default command parsing

* rewatch: add baseline clap behavior tests

* add test for "--"

* add comments

* Move default arg handling to cli module

* format

* Move env::args_os().collect into cli.rs

* Add comment regarding workaround

Author: cknitt

CHANGELOG.md

@@ -26,6 +26,8 @@
 
 #### :nail_care: Polish
 
+- Rewatch cli: do not show build command options in the root help. https://github.com/rescript-lang/rescript/pull/7715
+
 #### :house: Internal
 
 # 12.0.0-beta.13

rewatch/src/cli.rs

@@ -1,6 +1,19 @@
-use std::{ffi::OsString, ops::Deref};
-
-use clap::{Args, Parser, Subcommand};
+// We currently use https://docs.rs/clap/latest/clap/ v4 for command line parsing.
+// However, it does not fully fit our use case as it does not support default commands,
+// but we want to default to the "build" command if no other command is specified.
+//
+// Various workarounds exist, but each with its own drawbacks.
+// The workaround implemented here (injecting "build" into the args at the right place
+// and then parsing again if no other command matches at the first parse attempt)
+// avoids flattening all build command options into the root help, but requires careful
+// handling of edge cases regarding global flags.
+// Correctness is ensured by a comprehensive test suite.
+//
+// However, we may want to revisit the decision to use clap after the v12 release.
+
+use std::{env, ffi::OsString, ops::Deref};
+
+use clap::{Args, CommandFactory, Parser, Subcommand, error::ErrorKind};
 use clap_verbosity_flag::InfoLevel;
 use regex::Regex;
 
@@ -20,8 +33,14 @@ pub enum FileExtension {
 
 /// ReScript - Fast, Simple, Fully Typed JavaScript from the Future
 #[derive(Parser, Debug)]
+// The shipped binary is `rescript.exe` everywhere, but users invoke it as `rescript` (e.g.
+// via `npm run rescript`). Without forcing `bin_name`, clap would print `rescript.exe` in help,
+// which leaks the packaging detail into the CLI UX.
+#[command(name = "rescript", bin_name = "rescript")]
 #[command(version)]
-#[command(args_conflicts_with_subcommands = true)]
+#[command(
+    after_help = "Note: If no command is provided, the build command is run by default. See `rescript help build` for more information."
+)]
 pub struct Cli {
     /// Verbosity:
     /// -v -> Debug
@@ -35,10 +54,121 @@ pub struct Cli {
 
     /// The command to run. If not provided it will default to build.
     #[command(subcommand)]
-    pub command: Option<Command>,
+    pub command: Command,
+}
 
-    #[command(flatten)]
-    pub build_args: BuildArgs,
+/// Parse argv from the current process while treating `build` as the implicit default subcommand
+/// when clap indicates the user omitted one. This keeps the top-level help compact while still
+/// supporting bare `rescript …` invocations that expect to run the build.
+pub fn parse_with_default() -> Result<Cli, clap::Error> {
+    // Use `args_os` so non-UTF bytes still reach clap for proper error reporting on platforms that
+    // allow arbitrary argv content.
+    let raw_args: Vec<OsString> = env::args_os().collect();
+    parse_with_default_from(&raw_args)
+}
+
+/// Parse the provided argv while applying the implicit `build` defaulting rules.
+pub fn parse_with_default_from(raw_args: &[OsString]) -> Result<Cli, clap::Error> {
+    match Cli::try_parse_from(raw_args) {
+        Ok(cli) => Ok(cli),
+        Err(err) => {
+            if should_default_to_build(&err, raw_args) {
+                let fallback_args = build_default_args(raw_args);
+                Cli::try_parse_from(&fallback_args)
+            } else {
+                Err(err)
+            }
+        }
+    }
+}
+
+fn should_default_to_build(err: &clap::Error, args: &[OsString]) -> bool {
+    match err.kind() {
+        ErrorKind::MissingSubcommand
+        | ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand
+        | ErrorKind::UnknownArgument
+        | ErrorKind::InvalidSubcommand => {
+            let first_non_global = first_non_global_arg(args);
+            match first_non_global {
+                Some(arg) => !is_known_subcommand(arg),
+                None => true,
+            }
+        }
+        _ => false,
+    }
+}
+
+fn is_global_flag(arg: &OsString) -> bool {
+    matches!(
+        arg.to_str(),
+        Some(
+            "-v" | "-vv"
+                | "-vvv"
+                | "-vvvv"
+                | "-q"
+                | "-qq"
+                | "-qqq"
+                | "-qqqq"
+                | "--verbose"
+                | "--quiet"
+                | "-h"
+                | "--help"
+                | "-V"
+                | "--version"
+        )
+    )
+}
+
+fn first_non_global_arg(args: &[OsString]) -> Option<&OsString> {
+    args.iter().skip(1).find(|arg| !is_global_flag(arg))
+}
+
+fn is_known_subcommand(arg: &OsString) -> bool {
+    let Some(arg_str) = arg.to_str() else {
+        return false;
+    };
+
+    Cli::command().get_subcommands().any(|subcommand| {
+        subcommand.get_name() == arg_str || subcommand.get_all_aliases().any(|alias| alias == arg_str)
+    })
+}
+
+fn build_default_args(raw_args: &[OsString]) -> Vec<OsString> {
+    // Preserve clap's global flag handling semantics by keeping `-v/-q/-h/-V` in front of the
+    // inserted `build` token while leaving the rest of the argv untouched. This mirrors clap's own
+    // precedence rules so the second parse sees an argument layout it would have produced if the
+    // user had typed `rescript build …` directly.
+    let mut result = Vec::with_capacity(raw_args.len() + 1);
+    if raw_args.is_empty() {
+        return vec![OsString::from("build")];
+    }
+
+    let mut globals = Vec::new();
+    let mut others = Vec::new();
+    let mut saw_double_dash = false;
+
+    for arg in raw_args.iter().skip(1) {
+        if !saw_double_dash {
+            if arg == "--" {
+                saw_double_dash = true;
+                others.push(arg.clone());
+                continue;
+            }
+
+            if is_global_flag(arg) {
+                globals.push(arg.clone());
+                continue;
+            }
+        }
+
+        others.push(arg.clone());
+    }
+
+    result.push(raw_args[0].clone());
+    result.extend(globals);
+    result.push(OsString::from("build"));
+    result.extend(others);
+    result
 }
 
 #[derive(Args, Debug, Clone)]
@@ -136,6 +266,144 @@ pub struct BuildArgs {
     pub warn_error: Option<String>,
 }
 
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use clap::error::ErrorKind;
+    use log::LevelFilter;
+
+    fn parse(args: &[&str]) -> Result<Cli, clap::Error> {
+        let raw_args: Vec<OsString> = args.iter().map(OsString::from).collect();
+        parse_with_default_from(&raw_args)
+    }
+
+    // Default command behaviour.
+    #[test]
+    fn no_subcommand_defaults_to_build() {
+        let cli = parse(&["rescript"]).expect("expected default build command");
+        assert!(matches!(cli.command, Command::Build(_)));
+    }
+
+    #[test]
+    fn defaults_to_build_with_folder_shortcut() {
+        let cli = parse(&["rescript", "someFolder"]).expect("expected build command");
+
+        match cli.command {
+            Command::Build(build_args) => assert_eq!(build_args.folder.folder, "someFolder"),
+            other => panic!("expected build command, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn trailing_global_flag_is_treated_as_global() {
+        let cli = parse(&["rescript", "my-project", "-v"]).expect("expected build command");
+
+        assert_eq!(cli.verbose.log_level_filter(), LevelFilter::Debug);
+        match cli.command {
+            Command::Build(build_args) => assert_eq!(build_args.folder.folder, "my-project"),
+            other => panic!("expected build command, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn double_dash_keeps_following_args_positional() {
+        let cli = parse(&["rescript", "--", "-v"]).expect("expected build command");
+
+        assert_eq!(cli.verbose.log_level_filter(), LevelFilter::Info);
+        match cli.command {
+            Command::Build(build_args) => assert_eq!(build_args.folder.folder, "-v"),
+            other => panic!("expected build command, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn unknown_subcommand_help_uses_global_help() {
+        let err = parse(&["rescript", "xxx", "--help"]).expect_err("expected global help");
+        assert_eq!(err.kind(), ErrorKind::DisplayHelp);
+    }
+
+    // Build command specifics.
+    #[test]
+    fn build_help_shows_subcommand_help() {
+        let err = parse(&["rescript", "build", "--help"]).expect_err("expected subcommand help");
+        assert_eq!(err.kind(), ErrorKind::DisplayHelp);
+        let rendered = err.to_string();
+        assert!(
+            rendered.contains("Usage: rescript build"),
+            "unexpected help: {rendered:?}"
+        );
+        assert!(!rendered.contains("Usage: rescript [OPTIONS] <COMMAND>"));
+    }
+
+    #[test]
+    fn build_allows_global_verbose_flag() {
+        let cli = parse(&["rescript", "build", "-v"]).expect("expected build command");
+        assert_eq!(cli.verbose.log_level_filter(), LevelFilter::Debug);
+        assert!(matches!(cli.command, Command::Build(_)));
+    }
+
+    #[test]
+    fn build_option_is_parsed_normally() {
+        let cli = parse(&["rescript", "build", "--no-timing"]).expect("expected build command");
+
+        match cli.command {
+            Command::Build(build_args) => assert!(build_args.no_timing),
+            other => panic!("expected build command, got {other:?}"),
+        }
+    }
+
+    // Subcommand flag handling.
+    #[test]
+    fn respects_global_flag_before_subcommand() {
+        let cli = parse(&["rescript", "-v", "watch"]).expect("expected watch command");
+
+        assert!(matches!(cli.command, Command::Watch(_)));
+    }
+
+    #[test]
+    fn invalid_option_for_subcommand_does_not_fallback() {
+        let err = parse(&["rescript", "watch", "--no-timing"]).expect_err("expected watch parse failure");
+        assert_eq!(err.kind(), ErrorKind::UnknownArgument);
+    }
+
+    // Version/help flag handling.
+    #[test]
+    fn version_flag_before_subcommand_displays_version() {
+        let err = parse(&["rescript", "-V", "build"]).expect_err("expected version display");
+        assert_eq!(err.kind(), ErrorKind::DisplayVersion);
+    }
+
+    #[test]
+    fn version_flag_after_subcommand_is_rejected() {
+        let err = parse(&["rescript", "build", "-V"]).expect_err("expected unexpected argument");
+        assert_eq!(err.kind(), ErrorKind::UnknownArgument);
+    }
+
+    #[test]
+    fn global_help_flag_shows_help() {
+        let err = parse(&["rescript", "--help"]).expect_err("expected clap help error");
+        assert_eq!(err.kind(), ErrorKind::DisplayHelp);
+        let rendered = err.to_string();
+        assert!(rendered.contains("Usage: rescript [OPTIONS] <COMMAND>"));
+    }
+
+    #[test]
+    fn global_version_flag_shows_version() {
+        let err = parse(&["rescript", "--version"]).expect_err("expected clap version error");
+        assert_eq!(err.kind(), ErrorKind::DisplayVersion);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn non_utf_argument_returns_error() {
+        use std::os::unix::ffi::OsStringExt;
+
+        let args = vec![OsString::from("rescript"), OsString::from_vec(vec![0xff])];
+        let err = parse_with_default_from(&args).expect_err("expected clap to report invalid utf8");
+        assert_eq!(err.kind(), ErrorKind::InvalidUtf8);
+    }
+}
+
 #[derive(Args, Clone, Debug)]
 pub struct WatchArgs {
     #[command(flatten)]
@@ -181,7 +449,7 @@ impl From<BuildArgs> for WatchArgs {
 
 #[derive(Subcommand, Clone, Debug)]
 pub enum Command {
-    /// Build the project
+    /// Build the project (default command)
     Build(BuildArgs),
     /// Build, then start a watcher
     Watch(WatchArgs),

rewatch/src/main.rs

@@ -1,22 +1,21 @@
 use anyhow::Result;
-use clap::Parser;
 use log::LevelFilter;
 use std::{io::Write, path::Path};
 
 use rescript::{build, cli, cmd, format, lock, watcher};
 
 fn main() -> Result<()> {
-    let args = cli::Cli::parse();
+    let cli = cli::parse_with_default().unwrap_or_else(|err| err.exit());
 
-    let log_level_filter = args.verbose.log_level_filter();
+    let log_level_filter = cli.verbose.log_level_filter();
 
     env_logger::Builder::new()
         .format(|buf, record| writeln!(buf, "{}:\n{}", record.level(), record.args()))
         .filter_level(log_level_filter)
         .target(env_logger::fmt::Target::Stdout)
         .init();
 
-    let mut command = args.command.unwrap_or(cli::Command::Build(args.build_args));
+    let mut command = cli.command;
 
     if let cli::Command::Build(build_args) = &command {
         if build_args.watch {