Repository: clap-rs/clap (language: rust)

Issue:
derive: flatten on Option fields
<a href="https://github.com/lucab"><img src="https://avatars.githubusercontent.com/u/98086?v=4" align="left" width="96" height="96" hspace="10"></img></a> **Issue by [lucab](https://github.com/lucab)**
_Monday Apr 01, 2019 at 10:40 GMT_
_Originally opened as https://github.com/TeXitoi/structopt/issues/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:
```rust
#[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.



Discussion / hints:
<a href="https://github.com/TeXitoi"><img src="https://avatars.githubusercontent.com/u/5787066?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [TeXitoi](https://github.com/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?

<a href="https://github.com/TimoFreiberg"><img src="https://avatars.githubusercontent.com/u/5281645?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [TimoFreiberg](https://github.com/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>
}
```

<a href="https://github.com/CreepySkeleton"><img src="https://avatars.githubusercontent.com/u/50968528?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [CreepySkeleton](https://github.com/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
```rust
struct Params {
    #[structopt(flatten)]
    range_filter: RangeFilter
}

#[derive(StructOpt)]
struct RangeFilter {
    #[structopt(short = "b")]
    bottom: Option<String>,
    #[structopt(short = "t")]
    top: Option<String>
}
```

<a href="https://github.com/TimoFreiberg"><img src="https://avatars.githubusercontent.com/u/5281645?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [TimoFreiberg](https://github.com/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.

<a href="https://github.com/CreepySkeleton"><img src="https://avatars.githubusercontent.com/u/50968528?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [CreepySkeleton](https://github.com/CreepySkeleton)**
_Thursday Oct 17, 2019 at 16:07 GMT_

----

@TimoFreiberg You can use [`clap::Arg::requires`](https://docs.rs/clap/2.33.0/clap/struct.Arg.html#method.requires) instead of panic:
```rust
#[derive(StructOpt)]
struct RangeFilter {
    #[structopt(short = "b")]
    bottom: Option<String>,
    #[structopt(short = "t", requires = "bottom")]
    top: Option<String>
}
```

<a href="https://github.com/CreepySkeleton"><img src="https://avatars.githubusercontent.com/u/50968528?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [CreepySkeleton](https://github.com/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: 
```rust
#[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 }` 
* ```rust
  // app --section_a.field arg
  TopLevel { 
      section_a: Some(SectionA { 
          field: "arg",  
          inner_sect: None
      })
  }
  ```
* ```rust
  // 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?

<a href="https://github.com/TeXitoi"><img src="https://avatars.githubusercontent.com/u/5787066?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [TeXitoi](https://github.com/TeXitoi)**
_Tuesday Dec 03, 2019 at 08:42 GMT_

----

What about `app --inner_section.field foo`?

<a href="https://github.com/TeXitoi"><img src="https://avatars.githubusercontent.com/u/5787066?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [TeXitoi](https://github.com/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.

<a href="https://github.com/integrer"><img src="https://avatars.githubusercontent.com/u/31435980?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [integrer](https://github.com/integrer)**
_Friday Feb 07, 2020 at 22:05 GMT_

----

In my case:

**Given:**
`external/command1.rs`:
```rust
#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct Command1 {
    // Some arguments
}
```
`external/command2.rs`:
```rust
#[derive(StructOpt, Debug, Serialize, Deserialize)]
pub struct Command2 {
    // Some arguments
}
```
**Need:**
`mycrate/command.rs`:
```rust
#[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:
```rust
#[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`.

<a href="https://github.com/scottlamb"><img src="https://avatars.githubusercontent.com/u/8949524?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [scottlamb](https://github.com/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:

```rust
#[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.

```rust
#[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:

```rust
let credentials = match (opt.username, opt.password) {
    (Some(username), Some(password)) => Some(Credentials {
        username,
        password,
    }),
    (None, None) => None,
    _ => unreachable!(), // prevented by structopt requires
};
```

<a href="https://github.com/TeXitoi"><img src="https://avatars.githubusercontent.com/u/5787066?v=4" align="left" width="48" height="48" hspace="10"></img></a> **Comment by [TeXitoi](https://github.com/TeXitoi)**
_Thursday May 06, 2021 at 16:42 GMT_

----

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

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.

Fix patch:
diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs
index 21786505a82..f49883ce3bb 100644
--- a/clap_derive/src/derives/args.rs
+++ b/clap_derive/src/derives/args.rs
@@ -202,8 +202,12 @@ pub fn gen_augment(
                         #implicit_methods;
                 })
             }
-            Kind::Flatten => {
-                let ty = &field.ty;
+            Kind::Flatten(ty) => {
+                let inner_type = match (**ty, sub_type(&field.ty)) {
+                    (Ty::Option, Some(sub_type)) => sub_type,
+                    _ => &field.ty,
+                };
+
                 let next_help_heading = item.next_help_heading();
                 let next_display_order = item.next_display_order();
                 if override_required {
@@ -211,14 +215,14 @@ pub fn gen_augment(
                         let #app_var = #app_var
                             #next_help_heading
                             #next_display_order;
-                        let #app_var = <#ty as clap::Args>::augment_args_for_update(#app_var);
+                        let #app_var = <#inner_type as clap::Args>::augment_args_for_update(#app_var);
                     })
                 } else {
                     Some(quote_spanned! { kind.span()=>
                         let #app_var = #app_var
                             #next_help_heading
                             #next_display_order;
-                        let #app_var = <#ty as clap::Args>::augment_args(#app_var);
+                        let #app_var = <#inner_type as clap::Args>::augment_args(#app_var);
                     })
                 }
             }
@@ -350,7 +354,7 @@ pub fn gen_augment(
             .iter()
             .filter(|(_field, item)| {
                 let kind = item.kind();
-                matches!(*kind, Kind::Flatten)
+                matches!(*kind, Kind::Flatten(_))
             })
             .count();
         if 0 < possible_group_members_len {
@@ -410,7 +414,6 @@ pub fn gen_constructor(fields: &[(&Field, Item)]) -> TokenStream {
                             }
                         }
                     },
-                    Ty::Vec |
                     Ty::Other => {
                         quote_spanned! { kind.span()=>
                             #field_name: {
@@ -418,6 +421,7 @@ pub fn gen_constructor(fields: &[(&Field, Item)]) -> TokenStream {
                             }
                         }
                     },
+                    Ty::Vec |
                     Ty::OptionOption |
                     Ty::OptionVec => {
                         abort!(
@@ -429,8 +433,42 @@ pub fn gen_constructor(fields: &[(&Field, Item)]) -> TokenStream {
                 }
             }
 
-            Kind::Flatten => quote_spanned! { kind.span()=>
-                #field_name: clap::FromArgMatches::from_arg_matches_mut(#arg_matches)?
+            Kind::Flatten(ty) => {
+                let inner_type = match (**ty, sub_type(&field.ty)) {
+                    (Ty::Option, Some(sub_type)) => sub_type,
+                    _ => &field.ty,
+                };
+                match **ty {
+                    Ty::Other => {
+                        quote_spanned! { kind.span()=>
+                            #field_name: <#inner_type as clap::FromArgMatches>::from_arg_matches_mut(#arg_matches)?
+                        }
+                    },
+                    Ty::Option => {
+                        quote_spanned! { kind.span()=>
+                            #field_name: {
+                                let group_id = <#inner_type as clap::Args>::group_id()
+                                    .expect("`#[arg(flatten)]`ed field type implements `Args::group_id`");
+                                if #arg_matches.contains_id(group_id.as_str()) {
+                                    Some(
+                                        <#inner_type as clap::FromArgMatches>::from_arg_matches_mut(#arg_matches)?
+                                    )
+                                } else {
+                                    None
+                                }
+                            }
+                        }
+                    },
+                    Ty::Vec |
+                    Ty::OptionOption |
+                    Ty::OptionVec => {
+                        abort!(
+                            ty.span(),
+                            "{} types are not supported for flatten",
+                            ty.as_str()
+                        );
+                    }
+                }
             },
 
             Kind::Skip(val, _) => match val {
@@ -506,9 +544,36 @@ pub fn gen_updater(fields: &[(&Field, Item)], use_self: bool) -> TokenStream {
                 }
             }
 
-            Kind::Flatten => quote_spanned! { kind.span()=> {
-                    #access
-                    clap::FromArgMatches::update_from_arg_matches_mut(#field_name, #arg_matches)?;
+            Kind::Flatten(ty) => {
+                let inner_type = match (**ty, sub_type(&field.ty)) {
+                    (Ty::Option, Some(sub_type)) => sub_type,
+                    _ => &field.ty,
+                };
+
+                let updater = quote_spanned! { ty.span()=>
+                    <#inner_type as clap::FromArgMatches>::update_from_arg_matches_mut(#field_name, #arg_matches)?;
+                };
+
+                let updater = match **ty {
+                    Ty::Option => quote_spanned! { kind.span()=>
+                        if let Some(#field_name) = #field_name.as_mut() {
+                            #updater
+                        } else {
+                            *#field_name = Some(<#inner_type as clap::FromArgMatches>::from_arg_matches_mut(
+                                #arg_matches
+                            )?);
+                        }
+                    },
+                    _ => quote_spanned! { kind.span()=>
+                        #updater
+                    },
+                };
+
+                quote_spanned! { kind.span()=>
+                    {
+                        #access
+                        #updater
+                    }
                 }
             },
 
diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs
index 1989e82fded..b437e42c076 100644
--- a/clap_derive/src/derives/subcommand.rs
+++ b/clap_derive/src/derives/subcommand.rs
@@ -173,7 +173,7 @@ fn gen_augment(
                     Some(subcommand)
                 }
 
-                Kind::Flatten => match variant.fields {
+                Kind::Flatten(_) => match variant.fields {
                     Unnamed(FieldsUnnamed { ref unnamed, .. }) if unnamed.len() == 1 => {
                         let ty = &unnamed[0];
                         let deprecations = if !override_required {
@@ -363,7 +363,7 @@ fn gen_has_subcommand(variants: &[(&Variant, Item)]) -> TokenStream {
         })
         .partition(|(_, item)| {
             let kind = item.kind();
-            matches!(&*kind, Kind::Flatten)
+            matches!(&*kind, Kind::Flatten(_))
         });
 
     let subcommands = variants.iter().map(|(_variant, item)| {
@@ -464,7 +464,7 @@ fn gen_from_arg_matches(variants: &[(&Variant, Item)]) -> TokenStream {
         })
         .partition(|(_, item)| {
             let kind = item.kind();
-            matches!(&*kind, Kind::Flatten)
+            matches!(&*kind, Kind::Flatten(_))
         });
 
     let subcommands = variants.iter().map(|(variant, item)| {
@@ -571,7 +571,7 @@ fn gen_update_from_arg_matches(variants: &[(&Variant, Item)]) -> TokenStream {
         })
         .partition(|(_, item)| {
             let kind = item.kind();
-            matches!(&*kind, Kind::Flatten)
+            matches!(&*kind, Kind::Flatten(_))
         });
 
     let subcommands = variants.iter().map(|(variant, item)| {
diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs
index 68af9d62836..b9f64da6499 100644
--- a/clap_derive/src/item.rs
+++ b/clap_derive/src/item.rs
@@ -148,7 +148,7 @@ impl Item {
         }
 
         match &*res.kind {
-            Kind::Flatten => {
+            Kind::Flatten(_) => {
                 if res.has_explicit_methods() {
                     abort!(
                         res.kind.span(),
@@ -224,7 +224,7 @@ impl Item {
         }
 
         match &*res.kind {
-            Kind::Flatten => {
+            Kind::Flatten(_) => {
                 if res.has_explicit_methods() {
                     abort!(
                         res.kind.span(),
@@ -383,7 +383,12 @@ impl Item {
                         let expr = attr.value_or_abort();
                         abort!(expr, "attribute `{}` does not accept a value", attr.name);
                     }
-                    let kind = Sp::new(Kind::Flatten, attr.name.clone().span());
+                    let ty = self
+                        .kind()
+                        .ty()
+                        .cloned()
+                        .unwrap_or_else(|| Sp::new(Ty::Other, self.kind.span()));
+                    let kind = Sp::new(Kind::Flatten(ty), attr.name.clone().span());
                     Some(kind)
                 }
                 Some(MagicAttrName::Skip) if actual_attr_kind != AttrKind::Group => {
@@ -902,10 +907,10 @@ impl Item {
         match (self.kind.get(), kind.get()) {
             (Kind::Arg(_), Kind::FromGlobal(_))
             | (Kind::Arg(_), Kind::Subcommand(_))
-            | (Kind::Arg(_), Kind::Flatten)
+            | (Kind::Arg(_), Kind::Flatten(_))
             | (Kind::Arg(_), Kind::Skip(_, _))
             | (Kind::Command(_), Kind::Subcommand(_))
-            | (Kind::Command(_), Kind::Flatten)
+            | (Kind::Command(_), Kind::Flatten(_))
             | (Kind::Command(_), Kind::Skip(_, _))
             | (Kind::Command(_), Kind::ExternalSubcommand)
             | (Kind::Value, Kind::Skip(_, _)) => {
@@ -1142,7 +1147,7 @@ pub enum Kind {
     Value,
     FromGlobal(Sp<Ty>),
     Subcommand(Sp<Ty>),
-    Flatten,
+    Flatten(Sp<Ty>),
     Skip(Option<AttrValue>, AttrKind),
     ExternalSubcommand,
 }
@@ -1155,7 +1160,7 @@ impl Kind {
             Self::Value => "value",
             Self::FromGlobal(_) => "from_global",
             Self::Subcommand(_) => "subcommand",
-            Self::Flatten => "flatten",
+            Self::Flatten(_) => "flatten",
             Self::Skip(_, _) => "skip",
             Self::ExternalSubcommand => "external_subcommand",
         }
@@ -1168,7 +1173,7 @@ impl Kind {
             Self::Value => AttrKind::Value,
             Self::FromGlobal(_) => AttrKind::Arg,
             Self::Subcommand(_) => AttrKind::Command,
-            Self::Flatten => AttrKind::Command,
+            Self::Flatten(_) => AttrKind::Command,
             Self::Skip(_, kind) => *kind,
             Self::ExternalSubcommand => AttrKind::Command,
         }
@@ -1176,10 +1181,12 @@ impl Kind {
 
     pub fn ty(&self) -> Option<&Sp<Ty>> {
         match self {
-            Self::Arg(ty) | Self::Command(ty) | Self::FromGlobal(ty) | Self::Subcommand(ty) => {
-                Some(ty)
-            }
-            Self::Value | Self::Flatten | Self::Skip(_, _) | Self::ExternalSubcommand => None,
+            Self::Arg(ty)
+            | Self::Command(ty)
+            | Self::Flatten(ty)
+            | Self::FromGlobal(ty)
+            | Self::Subcommand(ty) => Some(ty),
+            Self::Value | Self::Skip(_, _) | Self::ExternalSubcommand => None,
         }
     }
 }


Test patch:
diff --git a/tests/derive/flatten.rs b/tests/derive/flatten.rs
index d4330f02543..7ef6ad86cd8 100644
--- a/tests/derive/flatten.rs
+++ b/tests/derive/flatten.rs
@@ -255,3 +255,46 @@ fn docstrings_ordering_with_multiple_clap_partial() {
 
     assert!(short_help.contains("This is the docstring for Flattened"));
 }
+
+#[test]
+fn optional_flatten() {
+    #[derive(Parser, Debug, PartialEq, Eq)]
+    struct Opt {
+        #[command(flatten)]
+        source: Option<Source>,
+    }
+
+    #[derive(clap::Args, Debug, PartialEq, Eq)]
+    struct Source {
+        crates: Vec<String>,
+        #[arg(long)]
+        path: Option<std::path::PathBuf>,
+        #[arg(long)]
+        git: Option<String>,
+    }
+
+    assert_eq!(
+        Opt { source: None },
+        Opt::try_parse_from(&["test"]).unwrap()
+    );
+    assert_eq!(
+        Opt {
+            source: Some(Source {
+                crates: vec!["serde".to_owned()],
+                path: None,
+                git: None,
+            }),
+        },
+        Opt::try_parse_from(&["test", "serde"]).unwrap()
+    );
+    assert_eq!(
+        Opt {
+            source: Some(Source {
+                crates: Vec::new(),
+                path: Some("./".into()),
+                git: None,
+            }),
+        },
+        Opt::try_parse_from(&["test", "--path=./"]).unwrap()
+    );
+}