From 13dfdb38709ded67d2bedafdea56306f04749bd9 Mon Sep 17 00:00:00 2001
From: Ed Page <eopage@gmail.com>
Date: Wed, 31 Jul 2024 15:19:29 -0500
Subject: [PATCH] fix(derive): Improve flattening-skipped-group assert

- Improves the error message
- Happens on initialization, rather than parse, making it so it will
  always show up and not just when certain parts of the CLI are
  exercised

Fixes #5609
---
 clap_derive/src/derives/args.rs | 11 ++++++++++-
 tests/derive/flatten.rs         |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs
index 4105608829d..d6fecc01970 100644
--- a/clap_derive/src/derives/args.rs
+++ b/clap_derive/src/derives/args.rs
@@ -228,8 +228,16 @@ pub(crate) fn gen_augment(
 
                 let next_help_heading = item.next_help_heading();
                 let next_display_order = item.next_display_order();
+                let flatten_group_assert = if matches!(**ty, Ty::Option) {
+                    quote_spanned! { kind.span()=>
+                        <#inner_type as clap::Args>::group_id().expect("cannot `#[flatten]` an `Option<Args>` with `#[group(skip)]");
+                    }
+                } else {
+                    quote! {}
+                };
                 if override_required {
                     Some(quote_spanned! { kind.span()=>
+                        #flatten_group_assert
                         let #app_var = #app_var
                             #next_help_heading
                             #next_display_order;
@@ -237,6 +245,7 @@ pub(crate) fn gen_augment(
                     })
                 } else {
                     Some(quote_spanned! { kind.span()=>
+                        #flatten_group_assert
                         let #app_var = #app_var
                             #next_help_heading
                             #next_display_order;
@@ -499,7 +508,7 @@ pub(crate) fn gen_constructor(fields: &[(&Field, Item)]) -> Result<TokenStream,
                         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`");
+                                    .expect("asserted during `Arg` creation");
                                 if #arg_matches.contains_id(group_id.as_str()) {
                                     Some(
                                         <#inner_type as clap::FromArgMatches>::from_arg_matches_mut(#arg_matches)?
diff --git a/tests/derive/flatten.rs b/tests/derive/flatten.rs
index 3242a6b63d0..95ad43e2bee 100644
--- a/tests/derive/flatten.rs
+++ b/tests/derive/flatten.rs
@@ -257,7 +257,7 @@ fn docstrings_ordering_with_multiple_clap_partial() {
 }
 
 #[test]
-#[should_panic = "#[arg(flatten)]`ed field type implements `Args::group_id"]
+#[should_panic = "cannot `#[flatten]` an `Option<Args>` with `#[group(skip)]"]
 fn flatten_skipped_group() {
     #[derive(clap::Parser, Debug)]
     struct Cli {