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

Issue:
Flatten structs causes "Argument group name must be unique" if has same name
### Please complete the following tasks

- [X] I have searched the [discussions](https://github.com/clap-rs/clap/discussions)
- [X] I have searched the [open](https://github.com/clap-rs/clap/issues) and [rejected](https://github.com/clap-rs/clap/issues?q=is%3Aissue+label%3AS-wont-fix+is%3Aclosed) issues

### Rust Version

rustc 1.64.0 (a55dd71d5 2022-09-19)

### Clap Version

4.0.1

### Minimal reproducible code

```rust
#[derive(clap::Parser, Debug)]
struct Config {
    #[arg(long)]
    pub value1: usize,

    #[clap(flatten)]
    pub config: sub::Config,
}

mod sub {

    #[derive(clap::Parser, Debug)]
    pub struct Config {
        #[arg(long)]
        pub value2: usize,
    }
}

fn main() {
    use clap::Parser;
    let config = Config::parse();

    println!("Hello: {config:?}");
}
```


### Steps to reproduce the bug with the above code

`cargo run`

### Actual Behaviour

The above causes panic on:

```
Argument group name must be unique

        'Config' is already in use', /home/repi/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-4.0.1/src/builder/debug_asserts.rs:284:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

due to the two `Config` structs having the same name. Maybe caused internally by these being placed in separate argument groups but with the same argument group names instead of being merged together to a single argument group with then name "Config"?

The code 

### Expected Behaviour

Would want this to work out of the box without having to rename the types

### Additional Context

_No response_

### Debug Output

```
[      clap::builder::command]  Command::_do_parse
[      clap::builder::command]  Command::_build: name="clap-tests"
[      clap::builder::command]  Command::_propagate:clap-tests
[      clap::builder::command]  Command::_check_help_and_version:clap-tests expand_help_tree=false
[      clap::builder::command]  Command::long_help_exists
[      clap::builder::command]  Command::_check_help_and_version: Building default --help
[      clap::builder::command]  Command::_propagate_global_args:clap-tests
[clap::builder::debug_asserts]  Command::_debug_asserts
[clap::builder::debug_asserts]  Arg::_debug_asserts:value1
[clap::builder::debug_asserts]  Arg::_debug_asserts:value2
[clap::builder::debug_asserts]  Arg::_debug_asserts:help
thread 'main' panicked at 'Command clap-tests: Argument group name must be unique

        'Config' is already in use', /home/repi/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-4.0.1/src/builder/debug_asserts.rs:284:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```


Discussion / hints:
This group was added so we could implement
- https://github.com/clap-rs/clap/issues/3165
- https://github.com/clap-rs/clap/issues/4211
- https://github.com/clap-rs/clap/issues/2621

When we implement https://github.com/clap-rs/clap/issues/3165, we should allow people to rename the implicit group which would be a work around for this that is less disruptive than renaming a type.

As for making this work automatically, I do not have any thoughts / plans for that.
Thanks. So to confirm, the only current solution is to rename the struct in the code so one doesn't have 2 or more structs with the same name if using flattening?
For now, yes.  If you  want, you can wait to upgrade until we implement #3165 to have another workaround.
While [upgrading to `0.4` in `cucumber` crate](https://github.com/cucumber-rs/cucumber/pull/230) we've stumbled into this issue, but just renaming structs doesn't work for our use case. 

In short, we use `Compose` generic struct like:

```rust
#[derive(Args, Debug)]
pub struct Compose<L: Args, R: Args> {
    #[clap(flatten)]
    pub left: L,
    #[clap(flatten)]
    pub right: R,
}
```

This `Compose` struct is used to fuse CLIs of different components (some of them may be user-defined). And most of those components doesn't add any new cli flags, so they use `Empty`:

```rust
#[derive(Args, Clone, Copy, Debug)]
pub struct Empty;
```

So we quickly get [`Empty is already in use` panic](https://github.com/cucumber-rs/cucumber/actions/runs/3158414239/jobs/5140421395#step:4:448). And I don't think that https://github.com/clap-rs/clap/issues/3165 is going to help solve this problem. 
Hmmm... what if?
1. Identify a group not by a type name, but by a `TypeId`, or at least canonical name like `::crate::module::Type`? This will auto-resolve any group naming collisions.
2. Regarding the `Empty` case, just allow collisions if a group has no args, because there can be no real collisions in this edge case. Or provide a special empty type in a `clap` crate for such purposes with the reserved group name, so the `clap` crate is aware of it.

Fix patch:
diff --git a/clap_derive/src/attr.rs b/clap_derive/src/attr.rs
index df2ab5685d5..f97ba7d0a7f 100644
--- a/clap_derive/src/attr.rs
+++ b/clap_derive/src/attr.rs
@@ -35,10 +35,7 @@ impl ClapAttr {
                 } else if attr.path.is_ident("command") {
                     Some(Sp::new(AttrKind::Command, attr.path.span()))
                 } else if attr.path.is_ident("group") {
-                    abort!(
-                        attr.path.span(),
-                        "`#[group()]` attributes are not supported yet"
-                    )
+                    Some(Sp::new(AttrKind::Group, attr.path.span()))
                 } else if attr.path.is_ident("arg") {
                     Some(Sp::new(AttrKind::Arg, attr.path.span()))
                 } else if attr.path.is_ident("value") {
@@ -202,6 +199,7 @@ pub enum AttrKind {
     Clap,
     StructOpt,
     Command,
+    Group,
     Arg,
     Value,
 }
@@ -212,6 +210,7 @@ impl AttrKind {
             Self::Clap => "clap",
             Self::StructOpt => "structopt",
             Self::Command => "command",
+            Self::Group => "group",
             Self::Arg => "arg",
             Self::Value => "value",
         }
diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs
index c136d21d16d..9c99c3fb516 100644
--- a/clap_derive/src/derives/args.rs
+++ b/clap_derive/src/derives/args.rs
@@ -315,11 +315,19 @@ pub fn gen_augment(
     let initial_app_methods = parent_item.initial_top_level_methods();
     let group_id = parent_item.ident().unraw().to_string();
     let final_app_methods = parent_item.final_top_level_methods();
+    let group_app_methods = if parent_item.skip_group() {
+        quote!()
+    } else {
+        quote!(
+            .group(clap::ArgGroup::new(#group_id).multiple(true))
+        )
+    };
     quote! {{
         #deprecations
         let #app_var = #app_var
             #initial_app_methods
-            .group(clap::ArgGroup::new(#group_id).multiple(true));
+            #group_app_methods
+            ;
         #( #args )*
         #app_var #final_app_methods
     }}
diff --git a/clap_derive/src/item.rs b/clap_derive/src/item.rs
index 758df901a37..8b60b1f8428 100644
--- a/clap_derive/src/item.rs
+++ b/clap_derive/src/item.rs
@@ -50,6 +50,7 @@ pub struct Item {
     next_help_heading: Option<Method>,
     is_enum: bool,
     is_positional: bool,
+    skip_group: bool,
     kind: Sp<Kind>,
 }
 
@@ -272,6 +273,7 @@ impl Item {
             next_help_heading: None,
             is_enum: false,
             is_positional: true,
+            skip_group: false,
             kind,
         }
     }
@@ -291,7 +293,7 @@ impl Item {
                         ),
                     });
                 }
-                AttrKind::Arg | AttrKind::Clap | AttrKind::StructOpt => {}
+                AttrKind::Group | AttrKind::Arg | AttrKind::Clap | AttrKind::StructOpt => {}
             }
             self.name = Name::Assigned(quote!(#arg));
         } else if name == "name" {
@@ -309,7 +311,11 @@ impl Item {
                         ),
                     });
                 }
-                AttrKind::Command | AttrKind::Value | AttrKind::Clap | AttrKind::StructOpt => {}
+                AttrKind::Group
+                | AttrKind::Command
+                | AttrKind::Value
+                | AttrKind::Clap
+                | AttrKind::StructOpt => {}
             }
             self.name = Name::Assigned(quote!(#arg));
         } else if name == "value_parser" {
@@ -330,6 +336,7 @@ impl Item {
                 continue;
             }
 
+            let actual_attr_kind = *attr.kind.get();
             let kind = match &attr.magic {
                 Some(MagicAttrName::FromGlobal) => {
                     if attr.value.is_some() {
@@ -373,7 +380,7 @@ impl Item {
                     let kind = Sp::new(Kind::Flatten, attr.name.clone().span());
                     Some(kind)
                 }
-                Some(MagicAttrName::Skip) => {
+                Some(MagicAttrName::Skip) if actual_attr_kind != AttrKind::Group => {
                     let expr = attr.value.clone();
                     let kind = Sp::new(
                         Kind::Skip(expr, self.kind.attr_kind()),
@@ -404,6 +411,8 @@ impl Item {
                     ));
                 }
 
+                (AttrKind::Group, AttrKind::Command) => {}
+
                 _ if attr.kind != expected_attr_kind => {
                     abort!(
                         attr.kind.span(),
@@ -799,6 +808,10 @@ impl Item {
                     self.env_casing = CasingStyle::from_lit(lit);
                 }
 
+                Some(MagicAttrName::Skip) if actual_attr_kind == AttrKind::Group => {
+                    self.skip_group = true;
+                }
+
                 None
                 // Magic only for the default, otherwise just forward to the builder
                 | Some(MagicAttrName::Short)
@@ -1025,6 +1038,10 @@ impl Item {
             .iter()
             .any(|m| m.name != "help" && m.name != "long_help")
     }
+
+    pub fn skip_group(&self) -> bool {
+        self.skip_group
+    }
 }
 
 #[derive(Clone)]
diff --git a/src/_derive/mod.rs b/src/_derive/mod.rs
index 000320eb50d..653f076ce02 100644
--- a/src/_derive/mod.rs
+++ b/src/_derive/mod.rs
@@ -4,6 +4,7 @@
 //! 2. [Attributes](#attributes)
 //!     1. [Terminology](#terminology)
 //!     2. [Command Attributes](#command-attributes)
+//!     2. [ArgGroup Attributes](#arggroup-attributes)
 //!     3. [Arg Attributes](#arg-attributes)
 //!     4. [ValueEnum Attributes](#valueenum-attributes)
 //!     5. [Possible Value Attributes](#possible-value-attributes)
@@ -28,6 +29,7 @@
 //! /// Doc comment
 //! #[derive(Parser)]
 //! #[command(CMD ATTRIBUTE)]
+//! #[group(GROUP ATTRIBUTE)]
 //! struct Cli {
 //!     /// Doc comment
 //!     #[arg(ARG ATTRIBUTE)]
@@ -46,6 +48,7 @@
 //! /// Doc comment
 //! #[derive(Args)]
 //! #[command(PARENT CMD ATTRIBUTE)]
+//! #[group(GROUP ATTRIBUTE)]
 //! struct Struct {
 //!     /// Doc comment
 //!     #[command(ARG ATTRIBUTE)]
@@ -173,6 +176,13 @@
 //! - `external_subcommand`: [`Command::allow_external_subcommand(true)`][crate::Command::allow_external_subcommands]
 //!   - Variant must be either `Variant(Vec<String>)` or `Variant(Vec<OsString>)`
 //!
+//! ### ArgGroup Attributes
+//!
+//! These correspond to the [`ArgGroup`][crate::ArgGroup] which is implicitly created for each
+//! `Args` derive.
+//!
+//! At the moment, only `#[group(skip)]` is supported
+//!
 //! ### Arg Attributes
 //!
 //! These correspond to a [`Arg`][crate::Arg].


Test patch:
diff --git a/tests/derive/groups.rs b/tests/derive/groups.rs
index edc2d93c91c..86548d3d3c6 100644
--- a/tests/derive/groups.rs
+++ b/tests/derive/groups.rs
@@ -21,3 +21,30 @@ fn test_safely_nest_parser() {
         Opt::try_parse_from(&["test", "--foo"]).unwrap()
     );
 }
+
+#[test]
+fn skip_group_avoids_duplicate_ids() {
+    #[derive(Parser, Debug)]
+    struct Opt {
+        #[command(flatten)]
+        first: Compose<Empty, Empty>,
+        #[command(flatten)]
+        second: Compose<Empty, Empty>,
+    }
+
+    #[derive(clap::Args, Debug)]
+    #[group(skip)]
+    pub struct Compose<L: clap::Args, R: clap::Args> {
+        #[clap(flatten)]
+        pub left: L,
+        #[clap(flatten)]
+        pub right: R,
+    }
+
+    #[derive(clap::Args, Clone, Copy, Debug)]
+    #[group(skip)]
+    pub struct Empty;
+
+    use clap::CommandFactory;
+    Opt::command().debug_assert();
+}