Weird bool behaviour when not defined as a flag parameter

[therealprof]

I've been (incorrectly) using bool arguments without any option under the assumption that the mandatory value is actually parsed and turned into a proper bool according to what was passed on the CLI but I just found out that it is always true. This makes bool parameters which are not flags rather useless; IMHO they either need to be really parsed or there should be a compiler error saying: You've been trying to use a bool parameter which is not a flag and hence will be always true after evaluation which is probably not what you want.

[TeXitoi]

Can you give an example of what you were doing, to be sure I understand correctly?

[therealprof]

Sure:

In
https://github.com/axiros/rusp/blob/16c8a04e74f4440acc7090a8370695fd22fb4c3f/src/main.rs#L127
we had this send_resp parameter which is supposed to be a mandatory one but no matter what you specified on the command line at the position of this parameter it would always pass in true throughout the application which is of course not the intended behaviour; there's no need to bother the user to pass an additional parameter if it always has the same value. 😅

[TeXitoi]

So, an error saying "bool without short or long is meaningless"?

[therealprof]

That would work. Or we could have proper bool parsing. 😅

[TeXitoi]

Which results do you expect from the linked code? Passing true or false on the command line?

[therealprof]

@TeXitoi Yes, exactly that.

[CreepySkeleton]

Idea:

• ship a set of standard parsers along with structopt, say in structopt::standard_parsers. This module has from_str, from_os_str, try_from_str, try_from_os_str submodules, each of them contains a set of standard functions (parsers).
• expand parse with additional syntax, like parse(from_str::true_or_false) that generates

  .validator(|s| {
      ::structopt::standard_parsers::from_str::true_or_false(&s)
          .map(|_: bool| ())
          .map_err(|e| (&e).to_string())
  })

This approach would allow us to support a lot of types almost out of box, like AtomicBool, BTreeSet, whatever with no black magic at all

[TeXitoi]

See also #79

[CreepySkeleton]

@therealprof bool implements FromStr so you can use a type alias to prevent structpt's special casing

type Flag = bool;

#[derive(Structopt)]
struct Opt {
    #[structopt(parse(from_str))]
    flag: Flag
}

[therealprof]

Cool!

[therealprof]

@CreepySkeleton

... but it does not work:

error[E0277]: the trait bound `bool: std::convert::From<&str>` is not satisfied
   --> src/main.rs:167:20
    |
167 |         send_resp: Flag,
    |                    ^^^^ the trait `std::convert::From<&str>` is not implemented for `bool`
    |
    = note: required by `std::convert::From::from`

error[E0277]: the trait bound `bool: std::convert::From<&str>` is not satisfied
   --> src/main.rs:166:27
    |
166 |         #[structopt(parse(from_str))]
    |                           ^^^^^^^^ the trait `std::convert::From<&str>` is not implemented for `bool`
    |
    = note: required by `std::convert::From::from`

[TeXitoi]

Can you give an example? This is working:

use structopt::StructOpt;

type Bool = bool;

#[derive(StructOpt, Debug)]
struct Opt {
    verbose: Bool,
}

fn main() {
    let opt = Opt::from_args();
    println!("{:?}", opt);
}

[TeXitoi]

Ha, That's try_from_str, not from_str, and this is the default, so not needed.

[therealprof]

So use the typedef but get rid of the custom parser?

[TeXitoi]

That's proc macro, so no type information, just checking that the type of the field is stringly bool. So if it's not bool, that's an unknown type for structopt.

[therealprof]

That works.

[CreepySkeleton]

Could someone please explain what this issue is for? bool is special for a reason, this is not going to change, workaround for this specific use-case is present. Should we close this?

[therealprof]

Is this mentioned in the documentation somewhere?

[CreepySkeleton]

@therealprof Fair enough, it should be.

[therealprof]

I'm working on a PR to add it to the README.

[CreepySkeleton]

@therealprof Err, I'm working on it too 😄 . OK, go make you PR and I'll adjust mine to it.

Btw, documentation goes to src/lib.rs, not into README

[therealprof]

I was about to add it to the "basic" example. Seems to be useful and confusing enough to warrant appearance on the front page.

[CreepySkeleton]

@therealprof This is what you think but it doesn't look like. You see, this issue has been here for ~5 months and it got no 👍 likes at all, so I would say it's not really common use-case.

We all think our use case is special but this is oftentimes wrong.

I think a separate file in examples folder is enough.

[therealprof]

I disagree. Just because no one chimed in on this use case here doesn't mean it's not relevant. I haven't even noticed that what I intended to do didn't even work until a lot later...

I've used boolean parameters at least as often if not more often in non-Rust projects than flags.

On the contrary I not quite happy about the attitude that because you see this as totally and completely obvious it might not be a stumbling block for others...

Anyway I'll leave it up to the maintainers to figure out relevance of this and whether they'd like to apply it to the basic example or not.

[CreepySkeleton]

@therealprof

Just because no one chimed in on this use case here doesn't mean it's not relevant

I never said that it's not relevant, I said it's not common. I was trying to show you the reason I consider it a corner case by this like-counting. If someone out there thinks I'm wrong I'd be happy to listen.

On the contrary I not quite happy about the attitude that because you see this as totally and completely obvious it might not be a stumbling block for others...

Please don't ascribe me words I've never said, I genially apologize if the wording was not clear enough.
What I meant is - while this behavior might be surprising for a newcomer we already have an example of this special casing in the readme.

    // A flag, true if used in the command line. Note doc comment will
    // be used for the help message of the flag. The name of the
    // argument will be, by default, based on the name of the field.
    /// Activate debug mode
    #[structopt(short, long)]
    debug: bool,

What you're asking for is a way to avoid this special casing, i.e just the contrary.

I haven't even noticed that what I intended to do didn't even work until a lot later...

I've used boolean parameters at least as often if not more often in non-Rust projects than flags.

structopt is originally build with this "bool means flag" behavior in mind, there's a list of special types in the doc. It's only natural for newcomers to take some time getting used to a new library, all we can do is to make common cases simple and provide fallbacks for corner cases.

Based on my subjective experience your case is a corner one. Based on your subjective experience you're case is important enough to be mentioned and I agree, but I don't agree that it should be mentioned on the front page, only common cases go there.

Of course we can't apply judgement based only on subjective opinions, we [maintainers] need to be objective.
"I use it a lot" is subjective, not objective. To be objective opinions must be common, and this is where "like-counting" (or voting, if you prefer it that way) comes in. Your case haven't received a single upvote and this is why I consider it not common.

Anyway I'll leave it up to the maintainers to figure out relevance of this and whether they'd like to apply it to the basic example or not.

I am one of the maintainer here, you can ask/argue with me If you have any other arguments, but please try to be objective.

@TeXitoi what do you think about it?

[therealprof]

If someone out there thinks I'm wrong I'd be happy to listen.

I for one am. I don't think "like counting" is representative for anything unless you're expressly polling for likes which is kind of nonsense...

What I meant is - while this behavior might be surprising for a newcomer we already have an example of this special casing in the readme.

It is absolutely not clear what that means. In hindsight I do understand what is happening but just from reading this example it is definitely not and the documentation doesn't explicitly mention it either. If no one came up with the "type Bool = bool;" suggestion I'd have never figured this out at all.

only common cases go there.

I do think there should be an example of every basic type in this basic example, true boolean parameters are such a basic type just like the others are.

[CreepySkeleton]

I for one am. I don't think "like counting" is representative for anything unless you're expressly polling for likes which is kind of nonsense...

This is the best we've got. It's not like "pooling for likes", it's about "is there many people interested in the topic". We can't put every single neat trick in the readme or main doc page or it would grow so large so it'd become unusable/unclear. We have to choose and filter out only simple and common use cases, there are examples for anything else.

Also, the very fact that it's only you (one man) and maintainers are participating in this discussion and no likes involved represents unpopularity of the topic quite clearly.

And by the way, about the "nonsense" thing: there is rust-embedded/wg#351 , it was done via like pooling and you were among people behind this RFC 😄

the documentation doesn't explicitly mention it either.

There's a table of special types in the doc along with description of every one of them. If you think this isn't clear enough I'm open to proposals, but I really don't think populating corner cases' explanations in the basic example would help the matter.

I do think there should be an example of every basic type in this basic example

There is such an example of bool field in the readme. The thing is, **bool means flag in structopt **, there are plenty examples of that.

You want to use bool not as flag, this is not on par with structopt's design, so you were forced to use a custom parser which is intended way to do that. I suggested you use type alias so you could delegate to default FromStr parser for bool and get rid of an explicit custom parser, that's all. A neat trick, no more, no less.

"true"/"false" parsing does not come out of box with structopt. It's not "basic" at all.

If no one came up with the "type Bool = bool;" suggestion I'd have never figured this out at all.

Well, you misunderstood the doc/example which explicitly shows you that bool field is a flag. Who do you think is to blame here? :)

[CreepySkeleton]

Actually, we could support "true"/"false" out of box parsing when bool is used as positional parameter.
cc @TeXitoi @therealprof

[TeXitoi]

First, please calm down. We are non native English speakers from different cultures, and we might interpret wrongly the tone of the others. It was the Care Bears interlude.

The README is an advertisement, or a quickstart guide. Not everything is present inside it. For example, subcommands, that are used a lot, are not present in it. So the README is not really appropriate to talk about that.

About common usage of parameters/options taking true or false, I must admit that I was surprised by your issue, as no one asked about that before (and I've seen very strange question in this repository). I can't find any well known command line tool taking such parameters. If you know someones, I'd be interested in looking at it.

Now, that's important to have solution to allow this use case, and document it as the bool case is quite central in structopt. It can be particularly tricky to do that in a non error-prone way.

I propose this as a solution:

The bool case with long or short stay as is, as it is the most used behavior, and exists as is since the very first version of structopt. Changing it will be a big breaking change that seems irrelevant.

The option taking bool will be documented like that (already working):

use structopt::StructOpt;

#[derive(StructOpt, Debug)]
struct Opt {
    #[structopt(long, parse(try_from_str))]
    verbose: bool,
}

fn main() {
    let opt = Opt::from_args();
    println!("{:?}", opt);
}

That's, IMHO, much better that the type alias workaround.

This parameter version must fail to compile with, as message, something like "Using bool as a parameter might not be what you want. Maybe you want to create a flag? In this case, add long or short. If you really want a boolean parameter, please use parse(try_from_str).

Fail to compile (but compile today):

use structopt::StructOpt;

#[derive(StructOpt, Debug)]
struct Opt {
    verbose: bool,
}

fn main() {
    let opt = Opt::from_args();
    println!("{:?}", opt);
}

Compile with the expected behavior (and is already working today):

use structopt::StructOpt;

#[derive(StructOpt, Debug)]
struct Opt {
    #[structopt(parse(try_from_str))]
    verbose: bool,
}

fn main() {
    let opt = Opt::from_args();
    println!("{:?}", opt);
}

For documentation, I propose an example, and a note in the documentation around parse(try_from_str) saying that it can be used on bool to parse true and false on the command line.

What do you think?

[CreepySkeleton]

What do you think?

I think Care Bears are adorable.

As regards to the proposal - I'm 100% agree. @therealprof Does this proposal suit your needs (and do you agree Care Bears are cherish?)?

[therealprof]

@TeXitoi Sounds good. However I thought that's how I implemented originally and it didn't work as intended.

Will have to retest when I find the time.

Re examples: Traditionally GNU projects tend to use --[no]-foo but often they'll accept --foo=true or --foo=false as well, however that depends on the particular implementation and a lot do not explicitly mention what they actually support.

Two popular examples I happen to know from the top of my head which support it are atom and VSCode.

More sophisticated getopt implementations also do support both boolean flags and boolean options, e.g.:
https://dlang.org/phobos/std_getopt.html
https://godoc.org/github.com/pborman/getopt#BoolVar
https://github.com/JorgeBucaran/getopts

It's just a matter of preference.

[TeXitoi]

Your buggy version was the one that is specked as "doesn't compile".

[therealprof]

I tried a lot of things. I'm note quite certain anymore whether try_from_str was one of them. Let me try that right now.

[therealprof]

Okay, works a treat. Going to change my implemtations, thanks!