derive: flatten on Option fields

[epage]

Issue by lucab
Monday Apr 01, 2019 at 10:40 GMT
Originally opened as TeXitoi/structopt#175

---

I have a usecase where a bunch of structures are used by both serde_derive and structopt_derive in order to source cli-options and file-options in a uniform way. It works pretty well with #[structopt(flatten)] and other annotations, except when trying to flatten Option<_> fields.

That is, a simplified example is as following:

#[macro_use]
extern crate serde_derive;
#[macro_use]
extern crate structopt_derive;

#[derive(Deserialize, StructOpt)]
struct TopLevel {
    #[structopt(flatten)]
    section_a: Option<SectionA>,
}

#[derive(Deserialize, StructOpt)]
struct SectionA {
    #[structopt(long = "section_a.field" )]
    field: Option<String>,
}

This fails with:

the trait `structopt::StructOpt` is not implemented for `std::option::Option<SectionA>`

Approaching this from my own crate, I think I can't solve this alone due to orphan rules (both the StructOpt trait and the Option receiver are not originating in the crate). Thus I tried to add a blanket impl<T: StructOpt> StructOpt for Option<T: StructOpt> in structopt; that pushed the issue a bit forward, but then I ended up stuck in proc-macro logic.

@TeXitoi I'd like to know if you think it generally makes sense for structopt to take care of the Option case above, and if so adapting the derive logic to handle that.

[epage]

Comment by TeXitoi
Monday Apr 01, 2019 at 20:24 GMT

---

The problem is "what does it mean?"

Here, we have one field that is an option. Should structopt create a TopLevel { section_a: None } or a TopLevel { section_a: Some(SectionA { field: None }) }?

If we have 2 String fields, should the 2 fields be optional, but if you have one, you must have 2?

What about nested flatten?

[epage]

Comment by TimoFreiberg
Thursday Oct 17, 2019 at 13:05 GMT

---

I just encountered this problem too, and I think my case is more clearcut:

I have an optional filter argument and I want to add an additional second filter that can only be added if the first filter is set. I tried to represent this as following:

struct Params {
    // more fields ...
    #[structopt(flatten)
    range_filter: Option<RangeFilter>
}

#[derive(StructOpt)
struct RangeFilter {
    #[structopt(short = "b")
    bottom: String,
    #[structopt(short = "t")
    top: Option<String>
}

[epage]

Comment by CreepySkeleton
Thursday Oct 17, 2019 at 13:26 GMT

---

@TimoFreiberg How do you expect the top (-t) option should be handled? What this double-Option would mean?

I guess this should work for you just fine

struct Params {
    #[structopt(flatten)]
    range_filter: RangeFilter
}

#[derive(StructOpt)]
struct RangeFilter {
    #[structopt(short = "b")]
    bottom: Option<String>,
    #[structopt(short = "t")]
    top: Option<String>
}

[epage]

Comment by TimoFreiberg
Thursday Oct 17, 2019 at 14:30 GMT

---

top should not be set unless bottom is set. I handled it similar to your suggestion now, but I panic when only top is set.

[epage]

Comment by CreepySkeleton
Thursday Oct 17, 2019 at 16:07 GMT

---

@TimoFreiberg You can use clap::Arg::requires instead of panic:

#[derive(StructOpt)]
struct RangeFilter {
    #[structopt(short = "b")]
    bottom: Option<String>,
    #[structopt(short = "t", requires = "bottom")]
    top: Option<String>
}

[epage]

Comment by CreepySkeleton
Tuesday Dec 03, 2019 at 01:44 GMT

---

The problem is "what does it mean?"

Here, we have one field that is an option. Should structopt create a TopLevel { section_a: None } or a TopLevel { section_a: Some(SectionA { field: None }) }?

I propose to implement the first design variant since it would allow us to handle nested flatten quite naturally:

#[derive(StructOpt)]
struct TopLevel {
    #[structopt(flatten)]
    section_a: Option<SectionA>,
}

#[derive(StructOpt)]
struct SectionA {
    #[structopt(flatten)]
    inner_sect: Option<InnerSection>,
    
    #[structopt(long = "section_a.field")]
    field: String,
}

#[derive(StructOpt)]
struct InnerSection {
    #[structopt(long = "inner_section.field")]
    field: String,
}

• app %no args% => TopLevel { section_a: None }

• // app --section_a.field arg
  TopLevel { 
      section_a: Some(SectionA { 
          field: "arg",  
          inner_sect: None
      })
  }

• // app --section_a.field arg --inner_section.flag inner
  TopLevel { 
      section_a: Some(SectionA { 
          field: "arg",  
          inner_sect: Some( InnerSection {
              field: "inner
          })
      })
  }

cc @TeXitoi do you agree?

[epage]

Comment by TeXitoi
Tuesday Dec 03, 2019 at 08:42 GMT

---

What about app --inner_section.field foo?

[epage]

Comment by TeXitoi
Tuesday Dec 03, 2019 at 08:44 GMT

---

I'm afraid that implementing this will be very complicated to code, maintain and understand who to use.

[epage]

Comment by integrer
Friday Feb 07, 2020 at 22:05 GMT

---

In my case:

Given:
external/command1.rs:

#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct Command1 {
    // Some arguments
}

external/command2.rs:

#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct Command2 {
    // Some arguments
}

Need:
mycrate/command.rs:

#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct MyCommand {
    #[serde(flatten)]
    #[structopt(flatten)]
    com1: Command1,

    #[serde(flatten)]
    #[structopt(flatten, required_if("arg1"))]
    com2: Option<Command2>,

    #[structopt(short = "a", long)]
    arg1: bool,
}

In other words, in structure MyCommand arguments from Command1 required always, but arguments from Command2 required only if arg1 appears (i.e. true).

Of course we can create two separate structures for two options (with arg1 and without it), but it not consistent with DRY.

But good solution may be use flatten between our structures:

#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct MyCommand {
    #[serde(flatten)]
    #[structopt(flatten)]
    com1: Command1,
}

#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct MyCommandWithArg1 {
    #[serde(flatten)]
    #[structopt(flatten)]
    my_com: MyCommand,

    #[serde(flatten)]
    #[structopt(flatten)]
    com2: Command2,
}

In this case we can reuse functionality of MyCommand in MyCommandWithArg1.

[epage]

Comment by scottlamb
Thursday May 06, 2021 at 16:07 GMT

---

The problem is "what does it mean?"

My two cents: maybe match whatever serde does? The feature request opened with "I have a usecase where a bunch of structures are used by both serde_derive and structopt_derive", and consistency would help with that. I haven't verified serde has reasonable behavior, but off-hand I don't see any reason the two should behave differently.

fwiw, I ended up here after trying a simpler thing:

#[derive(StructOpt)]
struct Credentials {
    #[structopt(long)]
    username: String,

    #[structopt(long)]
    password: String,
}

#[derive(StructOpt)]
struct Opt {
    // ...

    #[structopt(flatten)]
    credentials: Option<Credentials>,

    // ...
}

which I'd expect to have similar behavior as the following, with a more structured representation.

#[derive(StructOpt)]
struct Opt {
    // ...

    #[structopt(long, requires="password")]
    username: Option<String>,

    #[structopt(long, requires="username")]
    password: Option<String>,

    // ...
}

Not a must-have; it'd just make my code more readable and avoid having an unreachable error case later on:

let credentials = match (opt.username, opt.password) {
    (Some(username), Some(password)) => Some(Credentials {
        username,
        password,
    }),
    (None, None) => None,
    _ => unreachable!(), // prevented by structopt requires
};

[epage]

Comment by TeXitoi
Thursday May 06, 2021 at 16:42 GMT

---

That would be really tricky to do this kind of things.

[epage]

Since this was discussed, update_from_args was added that makes all fields optional which has the appearance of allowing this to be implemented without requiring extra cross-derive communication that tends to be the bane of derive feature requests (see pretty much any A-derive issue). #3707 experimented with this.

1. In general, I feel hesitant of what corner cases might be lurking because that API call was not designed with that purpose in mind and there is a lot of nuanced behavior to update_from_args.
2. It is derive-only behavior so the rest of clap doesn't know, like help rendering
3. We'd need to make sure it works in all contexts, including when flattening other structs, pulling in subcommands, etc.

[epage]

This was released in v4.0.10