Drop the build system's clap-era argument rewriting

changes/cli-command-first.breaking.md

@@ -0,0 +1 @@
+- `rescript` needs the command before its options and folder, as in `rescript build -v ./pkg`, and reports an unknown command even when `--help` or `--version` follows it.

rewatch-ocaml/cli.ml

@@ -287,104 +287,46 @@ let root =
       ~version:("rescript " ^ Rewatch_version.version)
       ~doc:"Fast, Simple, Fully Typed JavaScript from the Future" ~man
   in
-  Cmd.group info ~default:(build_term ~watch:false)
-    [build; watch; clean; format; compiler_args; help]
+  let commands = [build; watch; clean; format; compiler_args; help] in
+  (* Options before a command name make Cmdliner read the name as the folder of
+     the default build, so [rescript -v watch] would build a folder named
+     watch. *)
+  let default =
+    Term.term_result ~usage:true
+      (let+ command = build_term ~watch:false in
+       match command with
+       | Build {folder}
+         when List.exists (fun cmd -> Cmd.name cmd = folder) commands ->
+         Error
+           (`Msg
+              (Printf.sprintf
+                 "options go after the command: rescript %s [OPTION]… (write \
+                  ./%s for a folder named %s)"
+                 folder folder folder))
+       | command -> Ok command)
+  in
+  Cmd.group info ~default commands
 
 type evaluation = Run of command | Exit of int
 
 exception Parse_error of string
 exception Help
 exception Version
 
-(* Bare project folders must select the default build command, even though
-   Cmdliner otherwise treats them as unknown commands. Move global options
-   behind the selected command and expand short help/version clusters because
-   Cmdliner's standard display options only provide long names. A root display
-   request takes precedence over otherwise invalid implicit-build arguments. *)
+(* Cmdliner's standard display options only have long names. Accept -h and -V
+   as aliases, and print help as plain text instead of through a pager, up to a
+   "--" that ends the options. *)
 let normalize_argv argv =
-  let is_short_global_cluster argument =
-    let length = String.length argument in
-    length > 1
-    && argument.[0] = '-'
-    && argument.[1] <> '-'
-    && String.for_all
-         (function
-           | 'v' | 'q' | 'h' | 'V' -> true
-           | _ -> false)
-         (String.sub argument 1 (length - 1))
-  in
-  let short_cluster_contains character argument =
-    is_short_global_cluster argument
-    && String.contains_from argument 1 character
-  in
-  let is_global = function
-    | "--verbose" | "--quiet" | "--help" | "--version" -> true
-    | argument ->
-      is_short_global_cluster argument
-      || String.starts_with ~prefix:"--help=" argument
-  in
-  let requests_help argument =
-    argument = "--help"
-    || String.starts_with ~prefix:"--help=" argument
-    || short_cluster_contains 'h' argument
-  in
-  let requests_version argument =
-    argument = "--version" || short_cluster_contains 'V' argument
-  in
-  let is_command = function
-    | "build" | "watch" | "clean" | "format" | "compiler-args" | "help" -> true
-    | _ -> false
-  in
-  let rec normalize_display_options = function
+  let rec normalize = function
     | [] -> []
     | "--" :: rest -> "--" :: rest
-    | argument :: rest when String.starts_with ~prefix:"--help=" argument ->
-      argument :: normalize_display_options rest
-    | argument :: rest when requests_help argument ->
-      "--help=plain" :: normalize_display_options rest
-    | argument :: rest when requests_version argument ->
-      "--version" :: normalize_display_options rest
-    | argument :: rest -> argument :: normalize_display_options rest
-  in
-  let explicit_command arguments =
-    let rec loop globals = function
-      | [] | "--" :: _ -> None
-      | argument :: rest when is_global argument ->
-        loop (argument :: globals) rest
-      | command :: rest when is_command command ->
-        Some (List.rev globals, command, rest)
-      | _ -> None
-    in
-    loop [] arguments
-  in
-  let partition_implicit arguments =
-    let rec loop globals others = function
-      | [] -> (List.rev globals, List.rev others)
-      | "--" :: rest -> (List.rev globals, List.rev_append others ("--" :: rest))
-      | argument :: rest when is_global argument ->
-        loop (argument :: globals) others rest
-      | argument :: rest -> loop globals (argument :: others) rest
-    in
-    loop [] [] arguments
+    | ("-h" | "--help") :: rest -> "--help=plain" :: normalize rest
+    | "-V" :: rest -> "--version" :: normalize rest
+    | argument :: rest -> argument :: normalize rest
   in
   match Array.to_list argv with
   | [] -> argv
-  | executable :: arguments ->
-    let routed =
-      match explicit_command arguments with
-      | Some (globals, command, rest) ->
-        executable :: command :: (globals @ rest)
-      | None ->
-        let globals, others = partition_implicit arguments in
-        if
-          List.exists
-            (fun argument ->
-              requests_help argument || requests_version argument)
-            globals
-        then executable :: globals
-        else executable :: "build" :: (globals @ others)
-    in
-    Array.of_list (normalize_display_options routed)
+  | executable :: arguments -> Array.of_list (executable :: normalize arguments)
 
 (* Evaluates the command line and returns Cmdliner's result with the
    diagnostics it wrote, or [Error] for arguments that are not UTF-8. Help is

rewatch-ocaml/tests/check_command_validation.sh

@@ -474,7 +474,7 @@ run_build_output_case() {
   build_project="$work/$name-build"
   cp -R "$fixture" "$build_project"
   if [ "$mode" = quiet ]; then
-    run_rescript -q build "$build_project"
+    run_rescript build -q "$build_project"
   elif [ "$mode" = forced-color ]; then
     CLICOLOR_FORCE=1 run_rescript build "$build_project"
   else
@@ -582,12 +582,15 @@ run_build_output_case quiet-warning 0 \
   "$root/rewatch-ocaml/tests/warning-replay" quiet
 
 run_case build-subcommand-version accept build --version
-run_case clustered-global-version accept -vV build
-run_case clustered-global-verbosity accept -vvvvv build "$project"
-run_case clustered-global-version-before-help accept -Vh build
-run_case clustered-global-help-before-version accept -hV build
-run_case clustered-subcommand-help-before-version accept build -hV
-run_case clustered-subcommand-version-before-help accept build -Vh
+run_case short-help accept -h
+run_case short-version accept -V
+run_case clustered-verbosity accept build -vvvvv "$project"
+run_case options-before-command exit2 -v build
+require_error_contains options-before-command \
+  'options go after the command: rescript build'
+run_case folder-named-like-command reject -v ./build
+require_error_contains folder-named-like-command \
+  "Project folder './build' does not exist."
 "$rescript" build --help=groff >"$work/help-groff.out"
 if ! grep -F '.\" Pipe this output to groff' \
   "$work/help-groff.out" >/dev/null; then
@@ -597,11 +600,17 @@ if ! grep -F '.\" Pipe this output to groff' \
 fi
 checked=$((checked + 1))
 run_case implicit-conflicting-verbosity exit2 -v -q
-require_error_contains implicit-conflicting-verbosity 'Usage: rescript build'
+require_error_contains implicit-conflicting-verbosity \
+  'verbose cannot be used together with --quiet'
 run_case build-conflicting-verbosity exit2 build --verbose --quiet
-run_case unknown-command reject bogus-command
-require_error_contains unknown-command \
-  "Project folder 'bogus-command' does not exist. Run 'rescript --help' for the available commands."
+# Cmdliner quotes the command name when colors are off and styles it
+# otherwise, so the name is checked apart from the message.
+for display in "" --help -V; do
+  name=unknown-command${display:+-$display}
+  run_case "$name" exit2 bogus-command ${display:+"$display"}
+  require_error_contains "$name" 'unknown command'
+  require_error_contains "$name" 'bogus-command'
+done
 run_case filter-perl-quoting accept build --filter '\QA.res\E' "$project"
 run_case filter-invalid exit2 build --filter '(' "$project"
 require_error_contains filter-invalid 'invalid regular expression'
@@ -989,7 +998,7 @@ if [ "$rescript_status" -ne 0 ] || \
 fi
 check_output_snapshot clean-redirected
 checked=$((checked + 1))
-run_rescript -q clean "$project"
+run_rescript clean -q "$project"
 if [ "$rescript_status" -ne 0 ] || \
   [ -s "$work/rescript.out" ] || [ -s "$work/rescript.err" ]; then
   echo "Quiet redirected clean emitted output" >&2
@@ -1122,7 +1131,7 @@ wait_for_file "$work/watch-dependency-recovery/src/A.js"
 terminate_and_wait "$dependency_recovery_pid" "dependency-recovery watcher"
 checked=$((checked + 1))
 
-"$rescript" -v watch "$work/watch-dependency-install" \
+"$rescript" watch -v "$work/watch-dependency-install" \
   >"$work/watch-dependency-install.out" \
   2>"$work/watch-dependency-install.err" &
 dependency_install_pid=$!
@@ -1150,7 +1159,7 @@ checked=$((checked + 1))
 
 directory_link "$work/watch-dependency-fallback/packages/dep" \
   "$work/node_modules/dep"
-"$rescript" -v watch "$work/watch-dependency-fallback" \
+"$rescript" watch -v "$work/watch-dependency-fallback" \
   >"$work/watch-dependency-fallback.out" \
   2>"$work/watch-dependency-fallback.err" &
 dependency_fallback_pid=$!
@@ -1256,7 +1265,7 @@ checked=$((checked + 1))
 quiet_watch_project="$work/quiet-watch"
 quiet_watch_marker="$quiet_watch_project/after-build.log"
 REWATCH_WATCH_FILTER_MARKER="$quiet_watch_marker" \
-  "$rescript" -q watch \
+  "$rescript" watch -q \
     --after-build "node $command_work/watch-filter-marker.js" \
     "$quiet_watch_project" \
     >"$work/quiet-watch.out" 2>"$work/quiet-watch.err" &

rewatch-ocaml/tests/check_interactive_output.sh

@@ -183,7 +183,7 @@ if $windows_posix_shell; then
 fi
 
 echo "Checking quiet interactive builds..."
-run_in_terminal "$work/quiet.tty" -q build "$(command_path "$work/project")"
+run_in_terminal "$work/quiet.tty" build -q "$(command_path "$work/project")"
 if tr '\r' '\n' <"$work/quiet.tty" \
   | grep -E '(Cleaned|Parsed|Parsing\.\.\.|Compiled|Compiling\.\.\.|Finished .*compilation)' >/dev/null; then
   echo "A quiet interactive build emitted progress" >&2
@@ -216,7 +216,7 @@ if ! cmp -s "$work/expected-clean" "$work/clean.phases"; then
 fi
 
 echo "Checking quiet interactive cleans..."
-run_in_terminal "$work/clean-quiet.tty" -q clean "$(command_path "$work/project")"
+run_in_terminal "$work/clean-quiet.tty" clean -q "$(command_path "$work/project")"
 if tr '\r' '\n' <"$work/clean-quiet.tty" | grep -E '(Cleaning|Cleaned)' >/dev/null; then
   echo "A quiet interactive clean emitted progress" >&2
   cat "$work/clean-quiet.tty" >&2

rewatch-ocaml/tests/check_verbose_output.sh

@@ -1,5 +1,5 @@
 #!/bin/bash
-# Checks the DEBUG and TRACE events of `rescript -v build` and `-vv build`
+# Checks the DEBUG and TRACE events of `rescript build -v` and `build -vv`
 # against snapshots/verbose_output. Only the event lines are compared, sorted,
 # so parallel compilation does not reorder them.
 set -eu
@@ -45,7 +45,7 @@ capture() {
   project="$work/$level"
   (
     cd "$project"
-    "$rescript" "-$level" build >"$work/$name.stdout" 2>"$work/$name.stderr"
+    "$rescript" build "-$level" >"$work/$name.stdout" 2>"$work/$name.stderr"
   )
   if [ -s "$work/$name.stderr" ]; then
     echo "$name unexpectedly wrote verbose build output to stderr" >&2

rewatch-ocaml/tests/run.sh

@@ -1077,7 +1077,7 @@ test ! -f "$watch_basic/src/New.js"
 # A shallow watch on the nearest existing ancestor must advance one directory
 # at a time until a configured nested source root exists. The slash in the JSON
 # path also exercises native separator normalization on Windows.
-"$port" -v watch "$missing_nested_source" \
+"$port" watch -v "$missing_nested_source" \
   >"$missing_nested_source/watch.log" 2>&1 &
 missing_nested_source_pid=$!
 background_pids="$background_pids $missing_nested_source_pid"
@@ -1725,7 +1725,7 @@ awk 'BEGIN { for (i = 1; i <= 200000; i++) printf "let v%d = (%d, \"%d\")\n", i,
   >"$second_interrupt/src/Slow.res"
 
 # A second build waits for the first one's build lock and then runs.
-"$port" -vv build "$second_interrupt" >"$second_interrupt/first.log" 2>&1 &
+"$port" build -vv "$second_interrupt" >"$second_interrupt/first.log" 2>&1 &
 lock_first_pid=$!
 background_pids="$background_pids $lock_first_pid"
 # Logged when the parsed module is handed to the compiler.
@@ -1759,7 +1759,7 @@ if ! $windows_posix_shell; then
   rm "$second_interrupt/src/Slow.js"
   mkdir -p "$second_interrupt/tmp"
   TMPDIR="$second_interrupt/tmp" \
-    "$port" -vv build "$second_interrupt" >"$second_interrupt/build.log" 2>&1 &
+    "$port" build -vv "$second_interrupt" >"$second_interrupt/build.log" 2>&1 &
   second_interrupt_pid=$!
   background_pids="$background_pids $second_interrupt_pid"
   # Logged when the parsed module is handed to the compiler.
@@ -1860,7 +1860,7 @@ if ! $windows_posix_shell; then
     expected_status=${interrupt_signal#*:}
     interrupt_build="$work/interrupt-build-$signal_name"
     slow_project "$interrupt_build"
-    "$port" -vv build "$interrupt_build" >"$interrupt_build/build.log" 2>&1 &
+    "$port" build -vv "$interrupt_build" >"$interrupt_build/build.log" 2>&1 &
     interrupt_build_pid=$!
     background_pids="$background_pids $interrupt_build_pid"
     if ! wait_for_text "$interrupt_build/build.log" 'compile dirty: Slow'; then
@@ -1886,7 +1886,7 @@ fi
 # watcher's compiler work like a signal, before the queued dependent.
 lock_interrupt="$work/lock-interrupt"
 slow_project "$lock_interrupt"
-"$port" -vv watch "$lock_interrupt" >"$lock_interrupt/watch.log" 2>&1 &
+"$port" watch -vv "$lock_interrupt" >"$lock_interrupt/watch.log" 2>&1 &
 lock_interrupt_pid=$!
 background_pids="$background_pids $lock_interrupt_pid"
 if ! wait_for_text "$lock_interrupt/watch.log" 'compile dirty: Slow'; then
@@ -1909,7 +1909,7 @@ fi
 # becoming the baseline of the next one.
 atomic_save="$work/atomic-save-rebuild"
 slow_project "$atomic_save"
-"$port" -vv watch "$atomic_save" >"$atomic_save/watch.log" 2>&1 &
+"$port" watch -vv "$atomic_save" >"$atomic_save/watch.log" 2>&1 &
 atomic_save_pid=$!
 background_pids="$background_pids $atomic_save_pid"
 if ! wait_for_text "$atomic_save/watch.log" 'Finished initial compilation' 1 1200; then

rewatch/tests/watch/06-watch-missing-source-folder.sh

@@ -26,7 +26,7 @@ node -e "
 "
 
 # Start watcher and capture output (use -vv to get error logs)
-rewatch_bg -vv watch > rewatch.log 2>&1 &
+rewatch_bg watch -vv > rewatch.log 2>&1 &
 success "Watcher Started"
 
 # Wait for initial build to complete

tests/build_tests/cli_help/input.js

@@ -369,12 +369,11 @@ await test(["-h"], { stdout: cliHelp, stderr: "", status: 0 });
 // Shows cli help with -h arg
 await test(["help"], { stdout: cliHelp, stderr: "", status: 0 });
 
-// Exits with cli help with unknown command
-// Exits with build usage on unknown args
+// Exits with cli usage on unknown args
 await test(["--foo"], {
   stdout: "",
   stderr:
-    "Usage: rescript build [--help] [OPTION]… [FOLDER]\n" +
+    "Usage: rescript [--help] [COMMAND] …\n" +
     "rescript: unknown option --foo. Did you mean -f?\n",
   status: 2,
 });