Display order of subcommands' pre-existing args is not merged/updated

[kornelski]

Please complete the following tasks

• I have searched the discussions
• I have searched the open and rejected issues

Rust Version

rustc 1.69.0 (84c898d65 2023-04-16)

Clap Version

4.2.1

Minimal reproducible code

 Command::new("top")
        .arg(Arg::new("A").long("A").global(true))
        .arg(Arg::new("B").long("B").global(true))
        .arg(Arg::new("C").long("C").global(true))
        .subcommand(Command::new("sub")
            .arg(Arg::new("D").long("D"))
            .arg(Arg::new("E").long("E"))
            .arg(Arg::new("F").long("F")));

Steps to reproduce the bug with the above code

cargo run -- sub --help

Actual Behaviour

Options:
      --A <A>
      --D <D>
      --B <B>
      --E <E>
      --C <C>
      --F <F>

Expected Behaviour

Options:
      --A <A>
      --B <B>
      --C <C>
      --D <D>
      --E <E>
      --F <F>

Additional Context

Cargo is affected by this. cargo build -h displays:

      --keep-going              Do not abort the build as soon as there is an error (unstable)
      --frozen                  Require Cargo.lock and cache are up to date
      --lib                     Build only this package's library
      --bin [<NAME>]            Build only the specified binary
      --locked                  Require Cargo.lock is up to date
      --bins                    Build all binaries
      --offline                 Run without accessing the network
      --config <KEY=VALUE>      Override a configuration value
      --example [<NAME>]        Build only the specified example
      --examples                Build all examples

Note that --locked inserted between --bin and --bins. Cargo defines lib, bin, bins, example, examples one after another. But global args happen to be numbered the same and get interspersed with unrelated commands.

I believe it's because each command has its own disp_order numbering starting from zero, and subcommand_internal resets the number for future args, but doesn't update order of existing args to give them their unique number range. This means that global args from the parent numbered from zero are sorted together with subcommands' args numbered from zero.

Debug Output

No response

[epage]

I'm wondering what would be the right way to solve this. We are setting the display order when calling Command::arg and the subcommand doesn't know about the next display order from the parent command. When propagating global arguments, I'd rather not make guesses as to whether the user intended their display order or not and change things.

A workaround for this would be calling Command::next_display_order on subcommands before adding any arguments.

[kornelski]

To me the interspersed A/D/B/E/C/F order looks wrong, and I can't imagine users actually wanting that behavior. It is a logical consequence of the implementation, but not a feature to desire.

Adding all of subcommand's args (in their subcommand's order) after args existing in the parent seems sensible to me. It'd be analogous to adding these args directly to the parent without subcommand's indirection.

I think the fix should be, when adding a subcommand to a parent:

1. Add parent's latest disp_order value to every arg's order in the subcommand. This will "move" args of the subcommand to where they'd be added now in the parent, while preserving their relative order.
2. Then increase parent's disp_order to be after all of subcommand's args (take max of updated subcommand's order values + 1), so that newly added args in the parent appear after the subcommand.

This would work well for a typical usage with parent.subcommand(Command).

This leaves find_subcommand_mut with potentially interspersed behavior if users add subcommand, then find it, and then add args to it. That would still intersperse newly added args. I hope this usage is not common. Perhaps it simply could be documented to require managing next_display_order explicitly. Alternatively, in the algorithm above change step two from using max + 1 to max + sufficiently_large_value_for_anybody. That feels a bit arbitrary and less elegant, but 64-bit integers are plentiful and the just-in-case gap for new subcommand args could be large enough for all practical purposes.

[epage]

To me the interspersed A/D/B/E/C/F order looks wrong, and I can't imagine users actually wanting that behavior. It is a logical consequence of the implementation, but not a feature to desire.

To be clear, I never said it does look right. I also disagree that this is a consequence of implementation but is instead a matter of trade offs.

Adding all of subcommand's args (in their subcommand's order) after args existing in the parent seems sensible to me. It'd be analogous to adding these args directly to the parent without subcommand's indirection.

FYI I would consider this a breaking change so this would not be able to go in until clap v5, if we go this route. The reason is what I had hinted at before as to why I was not considering this route: a user can be setting the display order, explicitly with Arg::display_order or implicitly with Command::next_display_order to be getting the behavior they want and us changing the order could undo the careful design people do. While not likely, this would also prevent users from intermixing global flags in with their command's flags.

We could try to track intent (what was done explicitly) but that is adding a lot of extra complexity to try to out-guess the user.

This leaves find_subcommand_mut with potentially interspersed behavior if users add subcommand, then find it, and then add args to it.

Globals are passed down while building the Command. I expect a lot of clap to not work if you muck with things after the Command is built, so I'm not too worried about this case. I would love to address this in the type system but the builder and built forms are large with so much overlap, I've been hesitant.