From 41be1bed0815d9940b3ac0f7e6f97bd46e2b3c43 Mon Sep 17 00:00:00 2001
From: Ed Page <eopage@gmail.com>
Date: Thu, 11 Aug 2022 14:50:58 -0500
Subject: [PATCH] fix(parser)!: Store args in a group, rather than values

Now that `Id` is public, we can have `ArgMatches` report them.  If we
have to choose one behavior, this is more universal.  The user can still
look up the values, this works with groups whose args have different
types, and this allows people to make decisions off of it when otherwise
there isn't enogh information.

Fixes #2317
Fixes #3748
---
 CHANGELOG.md             |  1 +
 src/builder/arg_group.rs | 11 +++++++++--
 src/parser/parser.rs     | 15 ++++++++++-----
 tests/builder/groups.rs  | 22 ++++++++++------------
 4 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5ab42a9ae20..0505bb5b53d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   built-in flags and be copied to all subcommands, instead disable
   the built-in flags (`Command::disable_help_flag`,
   `Command::disable_version_flag`) and mark the custom flags as `global(true)`.
+- Looking up a group in `ArgMatches` now returns the arg `Id`s, rather than the values.
 - Various `Arg`, `Command`, and `ArgGroup` calls were switched from accepting `&[]` to `[]` via `IntoIterator`
 - *(help)* Make `DeriveDisplayOrder` the default and removed the setting.  To sort help, set `next_display_order(None)` (#2808)
 - *(help)* Subcommand display order respects `Command::next_display_order` instead of `DeriveDisplayOrder` and using its own initial display order value (#2808)
diff --git a/src/builder/arg_group.rs b/src/builder/arg_group.rs
index ffc88c2c1ca..64efeb952c0 100644
--- a/src/builder/arg_group.rs
+++ b/src/builder/arg_group.rs
@@ -49,10 +49,10 @@ use crate::util::Id;
 /// let err = result.unwrap_err();
 /// assert_eq!(err.kind(), ErrorKind::ArgumentConflict);
 /// ```
-/// This next example shows a passing parse of the same scenario
 ///
+/// This next example shows a passing parse of the same scenario
 /// ```rust
-/// # use clap::{Command, arg, ArgGroup};
+/// # use clap::{Command, arg, ArgGroup, Id};
 /// let result = Command::new("cmd")
 ///     .arg(arg!(--"set-ver" <ver> "set the version manually").required(false))
 ///     .arg(arg!(--major           "auto increase major"))
@@ -66,6 +66,13 @@ use crate::util::Id;
 /// let matches = result.unwrap();
 /// // We may not know which of the args was used, so we can test for the group...
 /// assert!(matches.contains_id("vers"));
+/// // We can also ask the group which arg was used
+/// assert_eq!(matches
+///     .get_one::<Id>("vers")
+///     .expect("`vers` is required")
+///     .as_str(),
+///     "major"
+/// );
 /// // we could also alternatively check each arg individually (not shown here)
 /// ```
 /// [`ArgGroup::multiple(true)`]: ArgGroup::multiple()
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index bef9bac6146..d2355210be5 100644
--- a/src/parser/parser.rs
+++ b/src/parser/parser.rs
@@ -17,6 +17,7 @@ use crate::mkeymap::KeyType;
 use crate::output::fmt::Stream;
 use crate::output::{fmt::Colorizer, Usage};
 use crate::parser::features::suggestions;
+use crate::parser::AnyValue;
 use crate::parser::{ArgMatcher, SubCommand};
 use crate::parser::{Validator, ValueSource};
 use crate::util::Id;
@@ -1052,15 +1053,19 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
             let value_parser = arg.get_value_parser();
             let val = value_parser.parse_ref(self.cmd, Some(arg), &raw_val)?;
 
-            // Increment or create the group "args"
-            for group in self.cmd.groups_for_arg(&arg.id) {
-                matcher.add_val_to(&group, val.clone(), raw_val.clone());
-            }
-
             matcher.add_val_to(&arg.id, val, raw_val);
             matcher.add_index_to(&arg.id, self.cur_idx.get());
         }
 
+        // Increment or create the group "args"
+        for group in self.cmd.groups_for_arg(&arg.id) {
+            matcher.add_val_to(
+                &group,
+                AnyValue::new(arg.get_id().clone()),
+                OsString::from(arg.get_id().as_str()),
+            );
+        }
+
         Ok(())
     }
 
diff --git a/tests/builder/groups.rs b/tests/builder/groups.rs
index 4ff054b8e44..84cbc4acbe2 100644
--- a/tests/builder/groups.rs
+++ b/tests/builder/groups.rs
@@ -1,6 +1,6 @@
 use super::utils;
 
-use clap::{arg, error::ErrorKind, Arg, ArgAction, ArgGroup, Command};
+use clap::{arg, error::ErrorKind, Arg, ArgAction, ArgGroup, Command, Id};
 
 static REQ_GROUP_USAGE: &str = "error: The following required arguments were not provided:
     <base|--delete>
@@ -94,10 +94,7 @@ fn group_single_value() {
 
     let m = res.unwrap();
     assert!(m.contains_id("grp"));
-    assert_eq!(
-        m.get_one::<String>("grp").map(|v| v.as_str()).unwrap(),
-        "blue"
-    );
+    assert_eq!(m.get_one::<Id>("grp").map(|v| v.as_str()).unwrap(), "color");
 }
 
 #[test]
@@ -141,13 +138,7 @@ fn group_multi_value_single_arg() {
 
     let m = res.unwrap();
     assert!(m.contains_id("grp"));
-    assert_eq!(
-        &*m.get_many::<String>("grp")
-            .unwrap()
-            .map(|v| v.as_str())
-            .collect::<Vec<_>>(),
-        &["blue", "red", "green"]
-    );
+    assert_eq!(m.get_one::<Id>("grp").map(|v| v.as_str()).unwrap(), "color");
 }
 
 #[test]
@@ -234,6 +225,13 @@ fn required_group_multiple_args() {
     let m = result.unwrap();
     assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));
     assert!(*m.get_one::<bool>("color").expect("defaulted by clap"));
+    assert_eq!(
+        &*m.get_many::<Id>("req")
+            .unwrap()
+            .map(|v| v.as_str())
+            .collect::<Vec<_>>(),
+        &["flag", "color"]
+    );
 }
 
 #[test]