Structopt: `flatten` on enums does not trigger error, while it should

[cecton]

Code

use structopt::StructOpt;

#[derive(Debug, StructOpt)]
enum BaseCli {
    Command1(Command1),
}

#[derive(Debug, StructOpt)]
struct Command1 {
    argument1: String,
}

#[derive(Debug, StructOpt)]
enum ExtendedCli {
    Command2(Command2),
}

#[derive(Debug, StructOpt)]
struct Command2 {
    argument2: String,
}

#[derive(Debug, StructOpt)]
struct GroupCli {
    #[structopt(flatten)]
    extended_cli: ExtendedCli,
    #[structopt(flatten)]
    base_cli: BaseCli,
}

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

Command:

cargo run -- command1 arg

Expected Result

The code is invalid. It should not compile since ExtendedCli and BaseCli cannot have both a value at the same time.

Actual Result

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:378:21
stack backtrace:
(...)
  15: core::option::Option<T>::unwrap
             at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libcore/macros.rs:41
  16: <test_struct_opt::ExtendedCli as structopt::StructOpt>::from_clap
             at src/main.rs:13
  17: <test_struct_opt::GroupCli as structopt::StructOpt>::from_clap
             at src/main.rs:25
  18: structopt::StructOpt::from_args
             at /home/cecile/.cargo/registry/src/github.com-1ecc6299db9ec823/structopt-0.3.7/src/lib.rs:942
  19: test_struct_opt::main
             at src/main.rs:32
(...)

[TeXitoi]

I don't think flatten is supposed to work on enum.

[TeXitoi]

it's complicated to detect several subcommand in the flatten tree...

[cecton]

Yes I can imagine... I'm just reporting the bug for history purpose (this is not blocking for me) but I don't think many people tried this.

[TeXitoi]

Of course, and thanks for reporting. My comments are more remarks on the bug than a critique to your bug report.

[cecton]

And thanks to you for your work on this great library

[CreepySkeleton]

it's complicated to detect several subcommand in the flatten tree...

It's not complicated, it's simply impossible 😄, at least on compile time. This is the reason why we have this check.

About flattening enums - I noticed the piece of code responsible for this while I was working on external subcommands and I was going to bring in up myself 😏

My opinion here - this is not documented (just checked), this is the work of subcommand, flatten is not supposed to work here. flatten is for structs, subcommand is for enums.

I'm changing the title because the issue is not about "this doesn't work" but rather "this is not supposed to work but it does".

[cecton]

I'm changing the title because the issue is not about "this doesn't work" but rather "this is not supposed to work but it does".

Exactly! :grin:

[TeXitoi]

You have a worst case: 2 StructOpt containing a subcommand, and another StructOpt that flatten these 2 StructOpt, kaboom. That's the same problem, but with enough layers to be much more complicated to handle :-/

[TeXitoi]

The only workaround for this would be to have a method fn contains_subcommand on the StructOpt internal trait, and forbidding (dynamically) using a struct that contains a subcommand on flatten.

[TeXitoi]

Or maybe this can be some kind of const fn with static assert?

[CreepySkeleton]

Related:

#[derive(StructOpt)]
struct Opt {
    // OK
    #[structopt(subcommand)]
    subcommand: Subcommand,

    // OK
    #[structopt(flatten)]
    options: Options,

    // fails with "Subcommand does not implement StructFlatten" 
    #[structopt(flatten)]
    flattenned_enum: Subcommand, 

    // fails with "Options does not implement StructSubcommand"
    #[structopt(subcommand)]
    subcommand_struct: Options,
}

#[derive(StructSubcommand)]
enum Subcommand {}

#[derive(StructFlatten)]
struct Options {}

This allows us to partially bypass the "no info across macro expansions" boundary.

Or maybe this can be some kind of const fn with static assert?

Let's see what my C++ SFINAE experience can get us to

[TeXitoi]

Not backcompat, and I think too complex for the user, 3 traits instead of 1.

[TeXitoi]

rust const fn is much more sane than SFINAE ;-)

[CreepySkeleton]

Not backcompat, and I think too complex for the user, 3 traits instead of 1.

Agreed on backcompat, subject to clap_derive.

About complexity - well, this is "one giant trait for everything, easy to use but easy to make mistake and poor error reporting in such cases" vs "fine grained control".

[pksunkara]

When we come across flatten, we know whether we are in a struct or enum. We can use generic parameters to auto-magic the traits. This way, we don't need 3 traits. (Atleast those won't be user-facing and internal only)

Generated code would be something like this:

struct Opt<T: IsStruct> {
    subcommand: Subcommand<T>,
    options: Options<T>,
    flattenned_enum: Subcommand<T>, 
    subcommand_struct: Options<T>,
}

enum Subcommand<T: IsEnum> {}

struct Options<T: IsStruct> {}

[CreepySkeleton]

hey @TeXitoi , we could make use of your idea:

The only workaround for this would be to have a method fn contains_subcommand on the StructOpt internal trait, and forbidding (dynamically) using a struct that contains a subcommand on flatten.

and insert a SFINAE-like const assert as described here. The error will look like

error[E0277]: the trait bound `Foo: FlattenedStructMustNotContainSubcommands` is not satisfied
 --> src/main.rs:8:10
  |
8 | #[derive(StructOpt)]
  |          ^^^^^^^^^ the trait `FlattenedStructMustNotContainSubcommands` is not implemented for `Foo`

I think there's no need to mention the downsides: SFINAE says it all.

[TeXitoi]

Not so great: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=160206d3d751d95e1bc1a2d81a81ce3b

[CreepySkeleton]

Yes, this is why I brought you the link to the advanced method: https://github.com/dtolnay/case-studies/blob/master/bitfield-assertion/README.md#solution

[TeXitoi]

Can you provide a playground link of your idea. That doesn't look so simple.

Better, but still less pretty than the bound message: https://play.rust-lang.org/?gist=962e9178f07a0a534229b6d6b0c37d22

[nbsdx]

I'm not sure if this belongs in this ticket, but I think it's related (at least as far as putting flatten on enum variants). I was trying to break up my subcommand arguments into more manageable blocks, and accidentally added flatten to the enum variants like in my example below. There are actually 2 bugs here as far as I can tell:

1. flattening the subcommand unit structs causes a panic in this cases where we have an optional subcommand followed by an optional argument
2. flattening the subcommand unit structs cause all arguments from all subcommands to be required.

Expected behavior is exhibited if the flatten attributes are removed. I can open a separate ticket for this if need be, or if it's just the result of improper usage then that's OK too :)

use structopt::StructOpt;

#[derive(StructOpt, Debug)]
struct Opts {
    #[structopt(subcommand)]
    command: Option<Subcommand>,

    // Comment this out for the second bug, leave in for the first.
    thing: Option<String>
}

#[derive(StructOpt, Debug)]
enum Subcommand {
    #[structopt(flatten)]
    Cmd1(args::Cmd1),

    #[structopt(flatten)]
    Cmd2(args::Cmd2),
}

pub mod args {
    #[derive(structopt::StructOpt, Debug)]
    pub struct Cmd1 {
        pub dir: String,
    }

    #[derive(structopt::StructOpt, Debug)]
    pub struct Cmd2 {
        pub user: String,
        pub pass: String,
    }
}

fn main() {
    let args = Opts::from_args();
    println!("{:#?}", args);
}

Panic stack:

structopt-subcommand-flatten $ RUST_BACKTRACE=full cargo run cmd1
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/structopt-subcommand-flatten cmd1`
thread 'main' panicked at 'Found positional argument which is not required with a lower index than a required positional argument: "thing" index 1', /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/parser.rs:612:21
stack backtrace:
   0:     0x55c225ee9774 - backtrace::backtrace::libunwind::trace::h65597d255cb1398b
                               at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1:     0x55c225ee9774 - backtrace::backtrace::trace_unsynchronized::hd4f479d7150ec4a0
                               at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2:     0x55c225ee9774 - std::sys_common::backtrace::_print_fmt::h015072984a2b172c
                               at src/libstd/sys_common/backtrace.rs:77
   3:     0x55c225ee9774 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h6df05d3335f32194
                               at src/libstd/sys_common/backtrace.rs:61
   4:     0x55c225f0436c - core::fmt::write::h1f444f4312eb6c27
                               at src/libcore/fmt/mod.rs:1028
   5:     0x55c225ee7927 - std::io::Write::write_fmt::h8d147888220078ef
                               at src/libstd/io/mod.rs:1412
   6:     0x55c225eebb7e - std::sys_common::backtrace::_print::h8a6df0fa81d6af62
                               at src/libstd/sys_common/backtrace.rs:65
   7:     0x55c225eebb7e - std::sys_common::backtrace::print::h6f05b4733407e509
                               at src/libstd/sys_common/backtrace.rs:50
   8:     0x55c225eebb7e - std::panicking::default_hook::{{closure}}::h0d0a23bd02315dd8
                               at src/libstd/panicking.rs:188
   9:     0x55c225eeb871 - std::panicking::default_hook::h8d15a9aecb4efac6
                               at src/libstd/panicking.rs:205
  10:     0x55c225eec21b - std::panicking::rust_panic_with_hook::hbe174577402a475d
                               at src/libstd/panicking.rs:464
  11:     0x55c225eebdbe - std::panicking::continue_panic_fmt::h4d855dad868accf3
                               at src/libstd/panicking.rs:373
  12:     0x55c225eebcff - std::panicking::begin_panic_fmt::ha0f013e3301a9528
                               at src/libstd/panicking.rs:328
  13:     0x55c225e20d6f - clap::app::parser::Parser::verify_positionals::he0b2141f9a12b069
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/parser.rs:612
  14:     0x55c225e1c307 - clap::app::parser::Parser::app_debug_asserts::hc8bdd9c8d0ecf938
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/parser.rs:139
  15:     0x55c225e23547 - clap::app::parser::Parser::get_matches_with::h5d277a46b8993774
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/parser.rs:854
  16:     0x55c225d92ddf - clap::app::App::get_matches_from_safe_borrow::hfa398c4b6e59a188
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/mod.rs:1629
  17:     0x55c225d92324 - clap::app::App::get_matches_from::h1df89b533dcacf19
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/mod.rs:1509
  18:     0x55c225d9226e - clap::app::App::get_matches::hcf8b35734a49e073
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/app/mod.rs:1451
  19:     0x55c225d52712 - structopt::StructOpt::from_args::ha8c1500f162d5c3a
                               at /home/neil/.cargo/registry/src/github.com-1ecc6299db9ec823/structopt-0.3.9/src/lib.rs:1073
  20:     0x55c225d52c71 - structopt_subcommand_flatten::main::he03349ed602dd76b
                               at src/main.rs:34
  21:     0x55c225d53960 - std::rt::lang_start::{{closure}}::hef08e0231a87b2ef
                               at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/rt.rs:61
  22:     0x55c225eebc43 - std::rt::lang_start_internal::{{closure}}::h6ea535ec5c50fc3e
                               at src/libstd/rt.rs:48
  23:     0x55c225eebc43 - std::panicking::try::do_call::h631c6408dfccc6f5
                               at src/libstd/panicking.rs:287
  24:     0x55c225eedafa - __rust_maybe_catch_panic
                               at src/libpanic_unwind/lib.rs:78
  25:     0x55c225eec6fd - std::panicking::try::hab539b2d1255d635
                               at src/libstd/panicking.rs:265
  26:     0x55c225eec6fd - std::panic::catch_unwind::hd5e0a26424bd7f34
                               at src/libstd/panic.rs:396
  27:     0x55c225eec6fd - std::rt::lang_start_internal::h3bdc4c7d98181bf9
                               at src/libstd/rt.rs:47
  28:     0x55c225d53939 - std::rt::lang_start::h8dbd766d6a0ad56c
                               at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/libstd/rt.rs:61
  29:     0x55c225d5373a - main
  30:     0x7fa43c161e5b - __libc_start_main
  31:     0x55c225d4f17a - _start
  32:                0x0 - <unknown>

[CreepySkeleton]

it's just the result of improper usage

Yep, it is. I agree that the error messages should be more clear in this case, but there's almost nothing we can do without drastic changes to the internal architecture. We're working on improving the situation in clap_derive, see https://github.com/clap-rs/clap/pull/1681#issuecomment-584840564

[nbsdx]

it's just the result of improper usage

Yep, it is. I agree that the error messages should be more clear in this case, but there's almost nothing we can do without drastic changes to the internal architecture.

The more I read up on the topic the more I figured this was the case :) Thanks for taking the time to clarify!

[cecton]

Inactive, closing