Unexpected elements in usage

[elszben]

extern crate clap;
use clap::{Arg,App};

fn main() {
    println!("Hello, world!");

    App::new("alma"
    ).arg(
       Arg::with_name("arg1").short("c").takes_value(true).help("stuff").required(true)
     ).arg(
       Arg::with_name("arg2").short("d").takes_value(true).help("stuff").required(true)
     ).arg(
       Arg::with_name("arg3").short("e").takes_value(true).help("stuff").required(true)
     ).arg(
      Arg::with_name("arg4").short("f").takes_value(true).help("stuff").required(true)
    ).get_matches();
}

This program with arguments -c gd -d fdf generates a really weird, unexpected error message:

Hello, world!
error: The following required arguments were not supplied:
       '-c <arg1>'
       '-d <arg2>'
       '-e <arg3>'
       '-f <arg4>'

USAGE:
       claptest -c <arg1> -d <arg2> -e <arg3> -f <arg4> -d <arg2> -c <arg1>

For more information try --help

The argument c and d were supplied but the program lists them as not supplied. I think this is plain wrong but I don't know what the design was, let's say that this is working as intended.
However the USAGE text lists -c and -d twice and I can't come up with a situation where this is actually useful or makes sense.

I've tried to figure out why the second fault happens and added some debug code into clap. It seems that create_usage() creates a list of args from known required args and it just appends the current matches to the list but does not remove duplicates. No idea how to fix this elegantly while also keeping the order (is that even necessary?). This context specific usage generation seems weird to me:) The library is quite nice otherwise.

[WildCryptoFox]

@elszben Interesting...

What do you get when you give it each argument exactly once? Does it run as expected?
It does run as expected with all arguments.

[elszben]

I am not sure I understand your request. I did give each argument exactly once. The command line arguments are these during the test: -c gd -d fdf

Are you asking if it works if I supply the correct arguments? It seems to work in that case.

[WildCryptoFox]

@elszben I meant for -c c -d d -e e -f f which runs. Yes the correct arguments.

[WildCryptoFox]

@elszben Okay. So two issues here. Sorry. I mis-read something somewhere. Okay.

• The following required arguments were not supplied doesn't exclude what was supplied.
• USAGE is poisoned with two lists. The expected required arguments and the supplied arguments (oddly also in reversed order).

[WildCryptoFox]

I don't have the time to patch this right now, quickly providing line numbers and suggestions for anyone with such time

• src/app/app.rs:2792 needs to filter away what was supplied.
• src/app/app.rs:2694 needs to show the expected usage, not the poisoned extended usage.

[kbknapp]

Good catch @james-darkfox

@elszben thanks for taking the time to file this, once it's fixed we'll report back.

[kbknapp]

Once this issue is fixed we'll put out v1.4.3

[kbknapp]

v1.4.3 is out on crates.io which fixes this issue

[elszben]

Thank you! I'll check it when I have time.

[elszben]

I had time to check the solution and it does not solve all faults described in this bug report (at least not in all cases). The supplied arguments are correctly filtered out of the required args. The USAGE text is sometimes filtered and sometimes not. The original program invoked with -c gd -d fdf -g shows this output:

Hello, world!
error: The argument '-g' isn't valid

USAGE:
        claptest.exe -c <arg1> -d <arg2> -e <arg3> -f <arg4> -d <arg2> -c <arg1>

In this case the solution is not applied and the USAGE text still contains the supplied arguments twice.

Another interesting bug is that if I call the program with a valid argument but "accidentally" concatenate the argument and its value then the error states that the -c argument is not valid. This is clearly wrong, the -c is a valid argument, it is even stated in the USAGE text.

PS D:\rust\claptest> cargo run --  -cdfdf
 Running `target\debug\claptest.exe -cdfdf`
Hello, world!
error: The argument '-c' isn't valid

USAGE:
        claptest.exe -c <arg1> -d <arg2> -e <arg3> -f <arg4>

[kbknapp]

Thanks for letting us know! I'll start re-investigating now.

[kbknapp]

@elszben #305 should take care of this for you. Also, as a bi-product of solving this issue clap now supports the -Lvalue style options...so huge thanks for filing this 👍

Once that PR merges test out master, and if it truly fixes your scenario let us know and we'll put out 1.4.4 😉

[elszben]

Great, thanks:)

[elszben]

I've tried it and it works on all cases I've previously reported. However there are still cases when it does not:) I did not even try to understand how clap works internally but based on this bug report it seems to me that this functionality is copied a few times, maybe it would be worth to change that? Just an idea. Anyway, here is the not correct case:

PS D:\rust\claptest> cargo run -- -c alma -f asd -f asda
     Running `target\debug\claptest.exe -c alma -f asd -f asda`
Hello, world!
error: The argument '-f' was supplied more than once, but does not support multiple values

USAGE:
        claptest.exe -c <arg1> -d <arg2> -e <arg3> -f <arg4> -c <arg1> -f <arg4>

For more information try --help

[WildCryptoFox]

@elszben This problem is due to the terribly organic way this library has been written. This makes testing fairly difficult as there are too many edge cases. I started a re-write in attempt to counter these types of problems. This re-write is being tracked in #259 - just in case you're interested; It is NOT yet ready unless you have VERY simple needs (No usage string yet 👻).

[..] this functionality is copied a few times, maybe it would be worth to change that?

No doubt about it. 1.x includes more duplicated code. A good refactor might have good value, however, completing the re-write would be better future-value. If the 2.0 plan goes through as expected, it'll directly replace the current clap. But again, there is nothing wrong with improving the current and working clap.

[kbknapp]

@elszben I apologize! We've been slowly refactoring away cases like this because it's like @james-darkfox said, edge cases pop up from everywhere! :)

I'm going to make a quick change to this copy-paste-pasta which should eliminate this issue entirely as there will be only a single instance of determining what goes in the usage string...not the many that there are now.

Once it's done, I'd be very interested if you find another case of duplication! Thanks again for taking the time to file and test these! 👍

[elszben]

No need to apologize, and there is certainly no need to hurry, I have all the time in the world:) I don't need a quick fix, just make it correct:)

[kbknapp]

@elszben hopefully #312 finally does it 😉

[elszben]

It mostly works but now it also crashes:)

Example:

PS D:\rust\claptest> cargo run -- -c sdsd -d df -g sfds
     Running `target\debug\claptest.exe -c sdsd -d df -g sfds`
Hello, world!
thread '<main>' panicked at 'index out of bounds: the len is 1 but the index is 1', D:\clap\src\app\app.rs:1574

This is from v1.4.4 branch.

p.s.: This is so funny, it just does not want to work:)

[kbknapp]

I've added some assertions to ensure that doesn't happen again. Once #314 merges I'm going to ahead an put out 1.4.5 since it fixes a crash.

@elszben Do you have your full test list? Just so we can ensure we're catching them all...and then we'll also add those to the actual tests 😄

Also, I'm leaving this open until we know for sure this issue is gone 😜

[kbknapp]

1.4.5 is out

[elszben]

Unfortunately I do not have a list, originally I only had a single use case I wanted to be covered then I just came up with more cases that broke clap. I believe they are all included in this bug but here is a "complete" list:

1. arguments must be accepted even if provided in random order (believe it or not this was the use case that failed on my previous choice of opt parser and then I switched to clap)
2. missing arguments must be properly printed as missing
3. provided arguments cannot be printed as missing
4. usage string must contain all arguments (provided arguments cannot corrupt usage string)
5. short argument concatenated with its value must be accepted as well (this is now a feature in clap, I think).
6. incorrectly providing an argument multiple times cannot break usage string (probably covered by 4.?)
7. invalid argument with or without value must be correctly handled

[elszben]

Interesting "feature" in 1.4.5.

 PS D:\rust\claptest> cargo run -- -c asd -d sd -e fdsf -dsdfdsf
 Hello, world!
 error: The argument '-dsdfdsf' was supplied more than once, but does not support multiple values

 USAGE:
         claptest.exe [FLAGS] -c <arg1> -d <arg2> -e <arg3> -f <arg4>

If an argument is provided twice and one of them is concatenated with its value then the argument name is printed with its value:)

[kbknapp]

That's the intended functionality. In that particular use case (using -Lvalue style args) I can see how it's somewhat surprising, but typically (or so I would assume) it would appear as something like The argument '--opt value' was supplied more than once

It'd be possible for us to change it so that it only shows the argument, without the value, but I'd imagine that's somewhat bike shedding as the main goal is to alert the user to an error and where to find the error. 😄

[elszben]

Well, it is up to you, it does not hurt the usability in my use case:) I consider this bug to be fixed then. Thanks for all the help:)