Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External subcommands that alias built-in subccommands cause panics #3263

Closed
2 tasks done
epage opened this issue Jan 5, 2022 · 2 comments · Fixed by #3703
Closed
2 tasks done

External subcommands that alias built-in subccommands cause panics #3263

epage opened this issue Jan 5, 2022 · 2 comments · Fixed by #3703
Labels
A-derive Area: #[derive]` macro API A-docs Area: documentation, including docs.rs, readme, examples, etc... A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@epage
Copy link
Member

epage commented Jan 5, 2022

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.57.0 (f1edd0429 2021-11-29)

Clap Version

master

Minimal reproducible code

use clap::{app_from_crate, arg, App, AppSettings};

fn main() {
    let matches = app_from_crate!()
        .global_setting(AppSettings::PropagateVersion)
        .global_setting(AppSettings::UseLongFormatForHelpSubcommand)
        .setting(AppSettings::SubcommandRequiredElseHelp)
        .setting(AppSettings::AllowExternalSubcommands)
        .subcommand(
            App::new("add")
                .about("Adds files to myapp")
                .arg(arg!([NAME])),
        )
        .get_matches();

    match matches.subcommand() {
        Some(("add", sub_matches)) => println!(
            "'myapp add' was used, name is: {:?}",
            sub_matches.value_of("NAME")
        ),
        _ => unreachable!(),
    }
}

Steps to reproduce the bug with the above code

See below

Actual Behaviour

$ cargo run -- add name
   Compiling test-clap v0.1.0 (/home/epage/src/personal/test-clap)
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
     Running `target/debug/test-clap add name`
'myapp add' was used, name is: Some("name")

$ cargo run -- -- add name
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/test-clap -- add name`
thread 'main' panicked at '`NAME` is not a name of an argument or a group.
Make sure you're using the name of the argument itself and not the name of short or long flags.', /home/epage/src/personal/clap/src/parse/matches/arg_matches.rs:119:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This happens because -- is escaping the command so it becomes an external subcommand that aliases the built-in. A simple match does not catch this and we then try to access "NAME" in an ArgMatches meant for external subcommands (only "" exists)

Expected Behaviour

Its a bit unclear. cargo just passes the ArgMatches along, warning about the contents, like:

$ cargo run -- -- check --invalid_argument -some-other-argument
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/cargo -- check --invalid_argument -some-other-argument`
warning: trailing arguments after built-in command `check` are ignored: `--invalid_argument -some-other-argument`
    Checking cargo v0.60.0 (/home/epage/src/personal/cargo)
^C  Building [=======================> ] 169/171: cargo                                                           

So with the above problem, that'd look like:

$ cargo run -- add name
   Compiling test-clap v0.1.0 (/home/epage/src/personal/test-clap)
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
     Running `target/debug/test-clap add name`
'myapp add' was used, name is: Some("name")

$ cargo run -- -- add name
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/test-clap -- add name`
'myapp add' was used, name is: None

Additional Context

Ran into this when porting cargo to clap3 with a test failure from cargo_alias_config::weird_check. cargo takes the approach of running the escaped command but ignoring the arguments. This means we pass an external subcommand's ArgMatches into a function assuming its getting checks ArgMatches and panics when it accesses an "undefined" field.

Builder users can detect that its an external subcommand by checking for values_of("") but derive users can't do that.

You have to even know to do that.

Once you do that, the question is what is next. Pass it through? Error? Fallback to the built-in like cargo? Seems like this should be the users choice. The challenge is with the fallback. They now need an ArgMatches that has the same shape (ie won't panic) when passing to that part of their program.

Debug Output

No response

@epage epage added the C-bug Category: Updating dependencies label Jan 5, 2022
@epage epage added this to the 3.x milestone Jan 5, 2022
@epage
Copy link
Member Author

epage commented Jan 5, 2022

So my thoughts

  • Call this out in in the AllowExternalSubcommands documentation
  • Add an extra check to the derive to ensure we actually populate external subcommands.

Unsure what I'll do for cargo to maintain behavior.

@epage
Copy link
Member Author

epage commented Jan 5, 2022

A big problem with detecting this is the fact that we only populate "" in ArgMatches if there are arguments for the external subcommand. The mere presence of "" causes cargo to issue a warning so others, in theory, could be relying on that and it'd be breaking to start populating it.

Without a way to detect this until 4.0:

  • We can't fix the derive until then
  • We are limited in what we can say in the docs.

So the choice is go straight for 4.0 already or apply a surgical hack to unblock cargo until 4.0.

I'm leaning towards the surgical hack.

@epage epage added A-derive Area: #[derive]` macro API A-docs Area: documentation, including docs.rs, readme, examples, etc... A-parsing Area: Parser's logic and needs it changed somehow. E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change. labels Jan 5, 2022
@epage epage modified the milestones: 3.x, 4.0 Jan 5, 2022
epage added a commit to epage/clap that referenced this issue Jan 5, 2022
This is a surgical workaround for clap-rs#3263.  It makes `cargo` pass tests!
epage added a commit to epage/clap that referenced this issue Jan 5, 2022
This is a surgical workaround for clap-rs#3263.  It makes `cargo` pass tests!
epage added a commit to epage/clap that referenced this issue Jan 5, 2022
This is a surgical workaround for clap-rs#3263.  It makes `cargo` pass tests!
epage added a commit to epage/cargo that referenced this issue Jan 6, 2022
- One parser change found by `cargo_config::includes` is that clap 2
  would ignore any values after a `=` for flags.
  `cargo config --show-origin` is a flag but the test passed `--show-origin=yes` which
  happens to give the desired result for that test but is the same as
  `--show-origin=no` or `--show-origin=alien-invasion`.
- The parser now panics when accessing an undefined attribute but clap
  takes advantage of that for sharing code across commands that have
  different subsets of arguments defined.  I've extended clap so we can
  "look before you leap" and put the checks at the argument calls to
  start off with so its very clear what is tenuously shared.  This
  allows us to go in either direction in the future, either addressing
  how we are sharing between commands or by moving this down into the
  extension methods and pretending this clap feature doesn't exist
- On that topic, a test found clap-rs/clap#3263.  For now, there is a
  hack in clap.  Depending on how we fix that in clap for clap 4.0, we
  might need to re-address things in cargo.
- `value_of_os` now requires setting `allow_invalid_utf8`, otherwise it
  asserts.  To help catch this, I updated the argument definitions
  associated with lookups reported by:
  - `rg 'values?_os' src/`
  - `rg 'values?_of_os' src/`
- clap now reports `2` for usage errors, so we had to bypass clap's
  `exit` call to keep the same exit code.

BREAKING CHANGE: API now uses clap3
bors added a commit to rust-lang/cargo that referenced this issue Jan 11, 2022
Port cargo to clap3

### What does this PR try to resolve?

This moves cargo to the latest major version of clap.

This supersedes #10259  and #10262

### How should we test and review this PR?

For testing, I mostly relied on existing tests.  I did manually validate that `cargo run <non-escaped command args>` behaved the same between both
```console
$ cargo run release --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.22s
     Running `target/debug/cargo-release release --help`
cargo-release 0.18.8
...

$ cargo run --manifest-path ../cargo/Cargo.toml -- run release --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `/home/epage/src/personal/cargo/target/debug/cargo run release --help`
    Finished dev [unoptimized + debuginfo] target(s) in 1.31s
     Running `target/debug/cargo-release release --help`
cargo-release 0.18.8
...
```

For reviewing, I split out each deprecation resolution into a separate commit so its easy to focus on more mechanical changes (most of the deprecation fixes) from interesting ones (the port, the `Arg::multiple` deprecation)

### Additional information

- One parser change found by `cargo_config::includes` is that clap 2
  would ignore any values after a `=` for flags.
  `cargo config --show-origin` is a flag but the test passed `--show-origin=yes` which
  happens to give the desired result for that test but is the same as
  `--show-origin=no` or `--show-origin=alien-invasion`.
- `ArgMatches` now panics when accessing an undefined argument but clap
  takes advantage of that for sharing code across commands that have
  different subsets of arguments defined.  I've extended clap so we can
  "look before you leap" and put the checks at the argument calls to
  start off with so its very clear what is tenuously shared.  This
  allows us to go in either direction in the future, either addressing
  how we are sharing between commands or by moving this down into the
  extension methods and pretending this clap feature doesn't exist
- On that topic, a test found clap-rs/clap#3263.  For now, there is a
  hack in clap.  Depending on how we fix that in clap for clap 4.0, we
  might need to re-address things in cargo.
- `value_of_os` now requires setting `allow_invalid_utf8`, otherwise it
  asserts.  To help catch this, I updated the argument definitions
  associated with lookups reported by:
  - `rg 'values?_os' src/`
  - `rg 'values?_of_os' src/`
- clap now reports `2` for usage errors, so we had to bypass clap's
  `exit` call to keep the same exit code.
- `cargo vendor --sync` did not use `multi_opt` and so it has both
  multiple occurrences **and** multiple values.  If we want to deprecate
  this, we'll need `unstable-grouped` to be stablized (or pin our clap
  version) and ensure each group has only 1 value.
epage added a commit to epage/clap that referenced this issue May 6, 2022
This allows distinguishing external subcommands from built-in
subcommands which can especially be confusing when escaping subcommands.

Fixes clap-rs#3263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API A-docs Area: documentation, including docs.rs, readme, examples, etc... A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant