After changing Arg.validator() to Arg.value_parser(), the behavior of the parsing failure changed

[hi-rustin]

The code:
https://github.com/rust-lang/rustup/blob/ef329e1c62fe06610cd695df7984108939c873fc/src/cli/rustup_mode.rs#L244

Before change:

rustup git:(master) ✗ RUSTUP_FORCE_ARG0='rustup' ./target/debug/rustup-init fsafas
rustup 1.25.2+242 (81fad9d0f 2023-03-28) dirty 1 modification
The Rust toolchain installer

USAGE:
    rustup-init [OPTIONS] [+toolchain] <SUBCOMMAND>

ARGS:
    <+toolchain>    release channel (e.g. +stable) or custom toolchain to set override

OPTIONS:
    -v, --verbose    Enable verbose output
    -q, --quiet      Disable progress output
    -h, --help       Print help information
    -V, --version    Print version information

SUBCOMMANDS:
    show           Show the active and installed toolchains or profiles
    update         Update Rust toolchains and rustup
    check          Check for updates to Rust toolchains and rustup
    default        Set the default toolchain
    toolchain      Modify or query the installed toolchains
    target         Modify a toolchain's supported targets
    component      Modify a toolchain's installed components
    override       Modify directory toolchain overrides
    run            Run a command with an environment configured for a given toolchain
    which          Display which binary will be run for a given command
    doc            Open the documentation for the current toolchain
    man            View the man page for a given command
    self           Modify the rustup installation
    set            Alter rustup settings
    completions    Generate tab-completion scripts for your shell
    help           Print this message or the help of the given subcommand(s)

DISCUSSION:
    Rustup installs The Rust Programming Language from the official
    release channels, enabling you to easily switch between stable,
    beta, and nightly compilers and keep them updated. It makes
    cross-compiling simpler with binary builds of the standard library
    for common platforms.

    If you are new to Rust consider running `rustup doc --book` to
    learn Rust.

After change:

➜  rustup git:(master) RUSTUP_FORCE_ARG0='rustup' ./target/debug/rustup-init fsafas
error: error: Invalid value "fsafas" for '<+toolchain>': Toolchain overrides must begin with '+'

For more information try --help
: Toolchain overrides must begin with '+'

How can I keep the original behavior? Thanks!

[epage]

I'm not seeing why that would make a difference / how the old one is supposed to function.

Could you provide a minimal reproduction case for the before and after so I can see what is trying to happen and provide guidance on how to fix it?

btw I thought elsewhere that + is handled before passing the args to clap? atm I can't find where I saw that...

[hi-rustin]

You can try https://github.com/hi-rustin/clap-bug-reproduce.

Use validator:

➜  clap-bug-reproduce git:(main) ✗ ./target/debug/clap-bug-reproduce fsfdsfs
error: 'clap-bug-reproduce' requires a subcommand but one was not provided

USAGE:
    clap-bug-reproduce [+toolchain] <SUBCOMMAND>

For more information try --help

Use value_parser:

➜  clap-bug-reproduce git:(main) ✗ ./target/debug/clap-bug-reproduce fsfdsfs
error: Invalid value "fsfdsfs" for '<+toolchain>': Toolchain overrides must begin with '+'

For more information try --help

[epage]

Seeing

error: 'clap-bug-reproduce' requires a subcommand but one was not provided

for the old code makes sense as to what is happening.

Before:

1. Parse command-line
2. Error for missing subcommand (or display help)
3. Run validators

After

1. Parse command-line

• Run value_parser

2. Error for missing subcommand (or display help)

For what rustup is telling clap, it is doing everything right. The problem is rustup is trying to make a value on the command-line be treated as a positional argument only if it starts with + and a subcommand otherwise.

We have some special processing for differentiating between values and flags, e.g. checking if it looks like a number of a flag. We have not generalized that yet so that a value parser can say whether to treat a value as something else. That would likely be a good amount of work especially for it to involve not just flags but subcommands.

Even with that, you wouldn't get the original behavior (showing the help) but instead get an invalid subcommand error. I actually think this is better because an error occurred but you aren't telling the user why but instead making them guess. For this reason, we removed SubcommandRequiredElseHelp but instead pushed people to use SubcommandRequired + ArgRequiredElseHelp in clap v4.

Personally, what I would recommend doing is add a toolchain_supported: bool to the cli function and remove the value_parser. On first run, you pass true. If the toolchain is missing a +, you then re-run cli and pass false, forcing an invalid subcommand error.