Make sure that ArgMatches methods panic on unknown arg, not return false

src/build/app/mod.rs

@@ -2382,12 +2382,13 @@ impl<'help> App<'help> {
 
     fn _do_parse(&mut self, it: &mut Input) -> ClapResult<ArgMatches> {
         debug!("App::_do_parse");
-        let mut matcher = ArgMatcher::default();
 
         // If there are global arguments, or settings we need to propagate them down to subcommands
         // before parsing in case we run into a subcommand
         self._build();
 
+        let mut matcher = ArgMatcher::new(self);
+
         // do the real parsing
         let mut parser = Parser::new(self);
         if let Err(error) = parser.get_matches_with(&mut matcher, it) {

src/build/arg/mod.rs

@@ -4267,7 +4267,7 @@ impl<'help> Arg<'help> {
     ///     ]);
     ///
     /// assert!(res.is_ok());
-    /// assert_eq!(res.unwrap().value_of("config"), None);
+    /// assert_eq!(res.unwrap().value_of("cfg"), Some(""));
     /// ```
     ///
     /// By adding this setting, we can forbid empty values.

src/parse/arg_matcher.rs

@@ -3,7 +3,7 @@ use std::{collections::HashMap, ffi::OsString, mem, ops::Deref};
 
 // Internal
 use crate::{
-    build::{Arg, ArgSettings},
+    build::{App, Arg, ArgSettings},
     parse::{ArgMatches, MatchedArg, SubCommand, ValueType},
     util::Id,
 };
@@ -16,12 +16,27 @@ pub(crate) struct ArgMatcher(pub(crate) ArgMatches);
 
 impl Deref for ArgMatcher {
     type Target = ArgMatches;
+
     fn deref(&self) -> &Self::Target {
         &self.0
     }
 }
 
 impl ArgMatcher {
+    pub(crate) fn new(app: &App) -> Self {
+        ArgMatcher(ArgMatches {
+            #[cfg(debug_assertions)]
+            valid_args: {
+                let args = app.args.args().map(|a| a.id.clone());
+                let groups = app.groups.iter().map(|g| g.id.clone());
+                args.chain(groups).collect()
+            },
+            #[cfg(debug_assertions)]
+            valid_subcommands: app.subcommands.iter().map(|sc| sc.id.clone()).collect(),
+            ..Default::default()
+        })
+    }
+
     pub(crate) fn into_inner(self) -> ArgMatches {
         self.0
     }

src/parse/matches/arg_matches.rs

@@ -72,18 +72,51 @@ pub(crate) struct SubCommand {
 /// }
 /// ```
 /// [`App::get_matches`]: crate::App::get_matches()
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, Default, PartialEq, Eq)]
 pub struct ArgMatches {
+    #[cfg(debug_assertions)]
+    pub(crate) valid_args: Vec<Id>,
+    #[cfg(debug_assertions)]
+    pub(crate) valid_subcommands: Vec<Id>,
     pub(crate) args: IndexMap<Id, MatchedArg>,
     pub(crate) subcommand: Option<Box<SubCommand>>,
 }
 
-impl Default for ArgMatches {
+// Private methods
-    fn default() -> Self {
+impl ArgMatches {
-        ArgMatches {
+    #[inline]
-            args: IndexMap::new(),
+    fn get_arg(&self, arg: &Id) -> Option<&MatchedArg> {
-            subcommand: None,
+        #[cfg(debug_assertions)]
+        {
+            if *arg != Id::empty_hash() && !self.valid_args.contains(arg) {
+                panic!(
+                    "`'{:?}' is not a name of an argument or a group.\n\
+                     Make sure you're using the name of the argument itself \
+                     and not the name of short or long flags.",
+                    arg
+                );
+            }
         }
+
+        self.args.get(arg)
+    }
+
+    #[inline]
+    fn get_subcommand(&self, id: &Id) -> Option<&SubCommand> {
+        #[cfg(debug_assertions)]
+        {
+            if *id != Id::empty_hash() && !self.valid_subcommands.contains(id) {
+                panic!("'{:?}' is not a name of a subcommand.", id);
+            }
+        }
+
+        if let Some(ref sc) = self.subcommand {
+            if sc.id == *id {
+                return Some(sc);
+            }
+        }
+
+        None
     }
 }
 
@@ -121,7 +154,7 @@ impl ArgMatches {
     /// [`default_value`]: crate::Arg::default_value()
     /// [`occurrences_of`]: crate::ArgMatches::occurrences_of()
     pub fn value_of<T: Key>(&self, id: T) -> Option<&str> {
-        if let Some(arg) = self.args.get(&Id::from(id)) {
+        if let Some(arg) = self.get_arg(&Id::from(id)) {
             if let Some(v) = arg.first() {
                 return Some(v.to_str().expect(INVALID_UTF8));
             }
@@ -161,7 +194,7 @@ impl ArgMatches {
     /// [`occurrences_of`]: ArgMatches::occurrences_of()
     /// [`Arg::values_of_lossy`]: ArgMatches::values_of_lossy()
     pub fn value_of_lossy<T: Key>(&self, id: T) -> Option<Cow<'_, str>> {
-        if let Some(arg) = self.args.get(&Id::from(id)) {
+        if let Some(arg) = self.get_arg(&Id::from(id)) {
             if let Some(v) = arg.first() {
                 return Some(v.to_string_lossy());
             }
@@ -204,8 +237,7 @@ impl ArgMatches {
     /// [`occurrences_of`]: ArgMatches::occurrences_of()
     /// [`ArgMatches::values_of_os`]: ArgMatches::values_of_os()
     pub fn value_of_os<T: Key>(&self, id: T) -> Option<&OsStr> {
-        self.args
+        self.get_arg(&Id::from(id))
-            .get(&Id::from(id))
             .and_then(|arg| arg.first().map(OsString::as_os_str))
     }
 
@@ -235,7 +267,7 @@ impl ArgMatches {
     /// ```
     /// [`Iterator`]: std::iter::Iterator
     pub fn values_of<T: Key>(&self, id: T) -> Option<Values> {
-        self.args.get(&Id::from(id)).map(|arg| {
+        self.get_arg(&Id::from(id)).map(|arg| {
             fn to_str_slice(o: &OsString) -> &str {
                 o.to_str().expect(INVALID_UTF8)
             }
@@ -259,8 +291,7 @@ impl ArgMatches {
             arg.vals()
                 .map(|g| g.iter().map(|x| x.to_str().expect(INVALID_UTF8)).collect())
         };
-        self.args
+        self.get_arg(&Id::from(id))
-            .get(&Id::from(id))
             .map(arg_values)
             .map(|iter| GroupedValues { iter })
     }
@@ -293,7 +324,7 @@ impl ArgMatches {
     /// assert_eq!(itr.next(), None);
     /// ```
     pub fn values_of_lossy<T: Key>(&self, id: T) -> Option<Vec<String>> {
-        self.args.get(&Id::from(id)).map(|arg| {
+        self.get_arg(&Id::from(id)).map(|arg| {
             arg.vals_flatten()
                 .map(|v| v.to_string_lossy().into_owned())
                 .collect()
@@ -338,7 +369,7 @@ impl ArgMatches {
             o
         }
 
-        self.args.get(&Id::from(id)).map(|arg| OsValues {
+        self.get_arg(&Id::from(id)).map(|arg| OsValues {
             iter: arg.vals_flatten().map(to_str_slice),
         })
     }
@@ -551,6 +582,10 @@ impl ArgMatches {
     /// [`occurrences_of`]: ArgMatches::occurrences_of()
     pub fn is_present<T: Key>(&self, id: T) -> bool {
         let id = Id::from(id);
+
+        #[cfg(debug_assertions)]
+        self.get_arg(&id);
+
         self.args.contains_key(&id)
     }
 
@@ -594,7 +629,7 @@ impl ArgMatches {
     /// assert_eq!(m.occurrences_of("flag"), 1);
     /// ```
     pub fn occurrences_of<T: Key>(&self, id: T) -> u64 {
-        self.args.get(&Id::from(id)).map_or(0, |a| a.occurs)
+        self.get_arg(&Id::from(id)).map_or(0, |a| a.occurs)
     }
 
     /// Gets the starting index of the argument in respect to all other arguments. Indices are
@@ -727,7 +762,7 @@ impl ArgMatches {
     /// ```
     /// [delimiter]: crate::Arg::value_delimiter()
     pub fn index_of<T: Key>(&self, name: T) -> Option<usize> {
-        if let Some(arg) = self.args.get(&Id::from(name)) {
+        if let Some(arg) = self.get_arg(&Id::from(name)) {
             if let Some(i) = arg.get_index(0) {
                 return Some(i);
             }
@@ -807,7 +842,7 @@ impl ArgMatches {
     /// [`ArgMatches::index_of`]: ArgMatches::index_of()
     /// [delimiter]: Arg::value_delimiter()
     pub fn indices_of<T: Key>(&self, id: T) -> Option<Indices<'_>> {
-        self.args.get(&Id::from(id)).map(|arg| Indices {
+        self.get_arg(&Id::from(id)).map(|arg| Indices {
             iter: arg.indices(),
         })
     }
@@ -844,12 +879,7 @@ impl ArgMatches {
     /// [`Subcommand`]: crate::Subcommand
     /// [`App`]: crate::App
     pub fn subcommand_matches<T: Key>(&self, id: T) -> Option<&ArgMatches> {
-        if let Some(ref s) = self.subcommand {
+        self.get_subcommand(&id.into()).map(|sc| &sc.matches)
-            if s.id == id.into() {
-                return Some(&s.matches);
-            }
-        }
-        None
     }
 
     /// Because [`Subcommand`]s are essentially "sub-[`App`]s" they have their own [`ArgMatches`]

src/parse/parser.rs

@@ -702,7 +702,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                 };
 
                 // Collect the external subcommand args
-                let mut sc_m = ArgMatcher::default();
+                let mut sc_m = ArgMatcher::new(self.app);
 
                 while let Some((v, _)) = it.next() {
                     if !self.is_set(AS::AllowInvalidUtf8ForExternalSubcommands)
@@ -993,7 +993,6 @@ impl<'help, 'app> Parser<'help, 'app> {
         let partial_parsing_enabled = self.is_set(AS::IgnoreErrors);
 
         if let Some(sc) = self.app.subcommands.iter_mut().find(|s| s.name == sc_name) {
-            let mut sc_matcher = ArgMatcher::default();
             // Display subcommand name, short and long in usage
             let mut sc_names = sc.name.clone();
             let mut flag_subcmd = false;
@@ -1030,6 +1029,8 @@ impl<'help, 'app> Parser<'help, 'app> {
             // Ensure all args are built and ready to parse
             sc._build();
 
+            let mut sc_matcher = ArgMatcher::new(sc);
+
             debug!("Parser::parse_subcommand: About to parse sc={}", sc.name);
 
             {

tests/arg_matcher_assertions.rs

@@ -0,0 +1,37 @@
+use clap::{App, Arg};
+
+#[test]
+#[cfg(debug_assertions)]
+#[should_panic = "'f' is not a name of an argument or a group."]
+fn arg_matches_if_present_wrong_arg() {
+    let m = App::new("test")
+        .arg(Arg::new("flag").short('f'))
+        .get_matches_from(&["test", "-f"]);
+
+    assert!(m.is_present("flag"));
+    m.is_present("f");
+}
+
+#[test]
+#[cfg(debug_assertions)]
+#[should_panic = "'o' is not a name of an argument or a group."]
+fn arg_matches_value_of_wrong_arg() {
+    let m = App::new("test")
+        .arg(Arg::new("opt").short('o').takes_value(true))
+        .get_matches_from(&["test", "-o", "val"]);
+
+    assert_eq!(m.value_of("opt"), Some("val"));
+    m.value_of("o");
+}
+
+#[test]
+#[cfg(debug_assertions)]
+#[should_panic = "'seed' is not a name of a subcommand."]
+fn arg_matches_subcommand_matches_wrong_sub() {
+    let m = App::new("test")
+        .subcommand(App::new("speed"))
+        .get_matches_from(&["test", "speed"]);
+
+    assert!(m.subcommand_matches("speed").is_some());
+    m.subcommand_matches("seed");
+}

tests/fixtures/app.yaml

@@ -47,7 +47,7 @@ args:
             - positional2
     - option3:
         short: O
-        long: Option
+        long: option3
         about: tests options with specific value sets
         takes_value: true
         possible_values:

tests/global_args.rs

@@ -94,5 +94,4 @@ fn global_arg_available_in_subcommand() {
 
     assert!(m.is_present("global"));
     assert!(m.subcommand_matches("ping").unwrap().is_present("global"));
-    assert!(!m.subcommand_matches("ping").unwrap().is_present("not"));
 }

tests/help.rs

@@ -41,7 +41,7 @@ OPTIONS:
         --multvals <one> <two>       Tests multiple values, not mult occs
         --multvalsmo <one> <two>     Tests multiple values, and mult occs
     -o, --option <opt>...            tests options
-    -O, --Option <option3>           specific vals [possible values: fast, slow]
+    -O, --option3 <option3>          specific vals [possible values: fast, slow]
     -V, --version                    Print version information
 
 SUBCOMMANDS:

tests/tests.rs

@@ -150,7 +150,7 @@ pub fn check_complex_output(args: &str, out: &str) {
         .unwrap();
     }
 
-    let _ = match matches.value_of("Option3").unwrap_or("") {
+    let _ = match matches.value_of("option3").unwrap_or("") {
         "fast" => writeln!(w, "option3 present quickly"),
         "slow" => writeln!(w, "option3 present slowly"),
         _ => writeln!(w, "option3 NOT present"),

tests/utils.rs

@@ -70,7 +70,7 @@ pub fn complex_app() -> App<'static> {
                 .conflicts_with("option")
                 .requires("positional2"),
             Arg::from("[positional2] 'tests positionals with exclusions'"),
-            Arg::from("-O --Option [option3] 'specific vals'").possible_values(opt3_vals),
+            Arg::from("-O --option3 [option3] 'specific vals'").possible_values(opt3_vals),
             Arg::from("[positional3]... 'tests specific values'").possible_values(pos3_vals),
             Arg::from("--multvals [one] [two] 'Tests multiple values, not mult occs'"),
             Arg::from("--multvalsmo... [one] [two] 'Tests multiple values, and mult occs'"),