Repository: clap-rs/clap
Pull request #3765 (2022-05-27 18:47:14)

fix(parser): Make behavior more consistent
Overall this is refactoring to slowly prepare the parser for customizable responses to arguments needed for #3405.  Some of these fixed bugs along the way while others just cleaned up the parser, making it easier to understand what is going on.

The line between a bug fix and a breaking change is awkward at times.  The part that seems most questionable to me is changing how we count occurrences for positional arguments.  This gets us closer to the documented behavior and makes the parser more consistent.  I'm trying to stop short of #3763 because I worry that resolving that would be too much of a change for the 3.x line since it ties into validation.


Patch:
diff --git a/examples/README.md b/examples/README.md
index 2034ab80..42ba8899 100644
--- a/examples/README.md
+++ b/examples/README.md
@@ -33,7 +33,7 @@
 ## Contributing
 
 New examples:
-- Building: They must be added to [Cargo.toml](../Cargo.toml) with the appropriate `required-features`.
+- Building: They must be added to [Cargo.toml](../../Cargo.toml) with the appropriate `required-features`.
 - Testing: Ensure there is a markdown file with [trycmd](https://docs.rs/trycmd) syntax
 - Link the `.md` file from here
 
diff --git a/src/builder/action.rs b/src/builder/action.rs
new file mode 100644
index 00000000..71da9ccc
--- /dev/null
+++ b/src/builder/action.rs
@@ -0,0 +1,8 @@
+/// Behavior of arguments when they are encountered while parsing
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub(crate) enum Action {
+    StoreValue,
+    Flag,
+    Help,
+    Version,
+}
diff --git a/src/builder/arg.rs b/src/builder/arg.rs
index 49cb8a96..46676adb 100644
--- a/src/builder/arg.rs
+++ b/src/builder/arg.rs
@@ -18,6 +18,7 @@ use yaml_rust::Yaml;
 
 // Internal
 use crate::builder::usage_parser::UsageParser;
+use crate::builder::Action;
 use crate::builder::ArgPredicate;
 use crate::util::{Id, Key};
 use crate::PossibleValue;
@@ -63,6 +64,7 @@ pub struct Arg<'help> {
     pub(crate) name: &'help str,
     pub(crate) help: Option<&'help str>,
     pub(crate) long_help: Option<&'help str>,
+    pub(crate) action: Option<Action>,
     pub(crate) value_parser: Option<super::ValueParser>,
     pub(crate) blacklist: Vec<Id>,
     pub(crate) settings: ArgFlags,
@@ -4528,6 +4530,12 @@ impl<'help> Arg<'help> {
         self.is_set(ArgSettings::AllowInvalidUtf8)
     }
 
+    /// Behavior when parsing the argument
+    pub(crate) fn get_action(&self) -> &super::Action {
+        const DEFAULT: super::Action = super::Action::StoreValue;
+        self.action.as_ref().unwrap_or(&DEFAULT)
+    }
+
     /// Configured parser for argument values
     ///
     /// # Example
diff --git a/src/builder/command.rs b/src/builder/command.rs
index b54012bd..0a7ce146 100644
--- a/src/builder/command.rs
+++ b/src/builder/command.rs
@@ -4115,6 +4115,10 @@ impl<'help> App<'help> {
             let mut pos_counter = 1;
             let self_override = self.is_set(AppSettings::AllArgsOverrideSelf);
             let hide_pv = self.is_set(AppSettings::HidePossibleValues);
+            let auto_help =
+                !self.is_set(AppSettings::NoAutoHelp) && !self.is_disable_help_flag_set();
+            let auto_version =
+                !self.is_set(AppSettings::NoAutoVersion) && !self.is_disable_version_flag_set();
             for a in self.args.args_mut() {
                 // Fill in the groups
                 for g in &a.groups {
@@ -4141,6 +4145,24 @@ impl<'help> App<'help> {
                     a.overrides.push(self_id);
                 }
                 a._build();
+                // HACK: Setting up action at this level while auto-help / disable help flag is
+                // required.  Otherwise, most of this won't be needed because when we can break
+                // compat, actions will reign supreme (default to `Store`)
+                if a.action.is_none() {
+                    if a.get_id() == "help" && auto_help {
+                        let action = super::Action::Help;
+                        a.action = Some(action);
+                    } else if a.get_id() == "version" && auto_version {
+                        let action = super::Action::Version;
+                        a.action = Some(action);
+                    } else if a.is_takes_value_set() {
+                        let action = super::Action::StoreValue;
+                        a.action = Some(action);
+                    } else {
+                        let action = super::Action::Flag;
+                        a.action = Some(action);
+                    }
+                }
                 if a.is_positional() && a.index.is_none() {
                     a.index = Some(pos_counter);
                     pos_counter += 1;
diff --git a/src/builder/mod.rs b/src/builder/mod.rs
index 9b009ea3..10e8bdff 100644
--- a/src/builder/mod.rs
+++ b/src/builder/mod.rs
@@ -3,6 +3,7 @@
 #[macro_use]
 mod macros;
 
+mod action;
 mod app_settings;
 mod arg;
 mod arg_group;
@@ -53,6 +54,7 @@ pub use command::App;
 #[cfg(feature = "regex")]
 pub use self::regex::RegexRef;
 
+pub(crate) use action::Action;
 pub(crate) use arg::display_arg_val;
 pub(crate) use arg_predicate::ArgPredicate;
 pub(crate) use value_parser::ValueParserInner;
diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs
index bb26bc3f..e162a16f 100644
--- a/src/parser/arg_matcher.rs
+++ b/src/parser/arg_matcher.rs
@@ -11,9 +11,6 @@ use crate::parser::{ArgMatches, MatchedArg, SubCommand, ValueSource};
 use crate::util::Id;
 use crate::INTERNAL_ERROR_MSG;
 
-// Third party
-use indexmap::map::Entry;
-
 #[derive(Debug, Default)]
 pub(crate) struct ArgMatcher(ArgMatches);
 
@@ -141,6 +138,7 @@ impl ArgMatcher {
         let ma = self.entry(id).or_insert(MatchedArg::new_arg(arg));
         debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id()));
         ma.set_source(source);
+        ma.new_val_group();
     }
 
     pub(crate) fn start_custom_group(&mut self, id: &Id, source: ValueSource) {
@@ -151,6 +149,7 @@ impl ArgMatcher {
         let ma = self.entry(id).or_insert(MatchedArg::new_group());
         debug_assert_eq!(ma.type_id(), None);
         ma.set_source(source);
+        ma.new_val_group();
     }
 
     pub(crate) fn start_occurrence_of_arg(&mut self, arg: &Arg) {
@@ -160,6 +159,7 @@ impl ArgMatcher {
         debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id()));
         ma.set_source(ValueSource::CommandLine);
         ma.inc_occurrences();
+        ma.new_val_group();
     }
 
     pub(crate) fn start_occurrence_of_group(&mut self, id: &Id) {
@@ -168,6 +168,7 @@ impl ArgMatcher {
         debug_assert_eq!(ma.type_id(), None);
         ma.set_source(ValueSource::CommandLine);
         ma.inc_occurrences();
+        ma.new_val_group();
     }
 
     pub(crate) fn start_occurrence_of_external(&mut self, cmd: &crate::Command) {
@@ -184,46 +185,19 @@ impl ArgMatcher {
         );
         ma.set_source(ValueSource::CommandLine);
         ma.inc_occurrences();
+        ma.new_val_group();
     }
 
-    pub(crate) fn add_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString, append: bool) {
-        if append {
-            self.append_val_to(arg, val, raw_val);
-        } else {
-            self.push_val_to(arg, val, raw_val);
-        }
-    }
-
-    fn push_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) {
-        // We will manually inc occurrences later(for flexibility under
-        // specific circumstances, like only add one occurrence for flag
-        // when we met: `--flag=one,two`).
-        let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
-        ma.push_val(val, raw_val);
-    }
-
-    fn append_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) {
+    pub(crate) fn add_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) {
         let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
         ma.append_val(val, raw_val);
     }
 
-    pub(crate) fn new_val_group(&mut self, arg: &Id) {
-        let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
-        ma.new_val_group();
-    }
-
     pub(crate) fn add_index_to(&mut self, arg: &Id, idx: usize) {
         let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
         ma.push_index(idx);
     }
 
-    pub(crate) fn has_val_groups(&mut self, arg: &Id) -> bool {
-        match self.entry(arg) {
-            Entry::Occupied(e) => e.get().has_val_groups(),
-            Entry::Vacant(_) => false,
-        }
-    }
-
     pub(crate) fn needs_more_vals(&self, o: &Arg) -> bool {
         debug!("ArgMatcher::needs_more_vals: o={}", o.name);
         if let Some(ma) = self.get(&o.id) {
diff --git a/src/parser/matches/matched_arg.rs b/src/parser/matches/matched_arg.rs
index fa9f7664..b2c527ea 100644
--- a/src/parser/matches/matched_arg.rs
+++ b/src/parser/matches/matched_arg.rs
@@ -87,11 +87,6 @@ impl MatchedArg {
         self.indices.push(index)
     }
 
-    #[cfg(test)]
-    pub(crate) fn raw_vals(&self) -> Iter<Vec<OsString>> {
-        self.raw_vals.iter()
-    }
-
     #[cfg(feature = "unstable-grouped")]
     pub(crate) fn vals(&self) -> Iter<Vec<AnyValue>> {
         self.vals.iter()
@@ -118,11 +113,6 @@ impl MatchedArg {
         self.raw_vals_flatten().next()
     }
 
-    pub(crate) fn push_val(&mut self, val: AnyValue, raw_val: OsString) {
-        self.vals.push(vec![val]);
-        self.raw_vals.push(vec![raw_val]);
-    }
-
     pub(crate) fn new_val_group(&mut self) {
         self.vals.push(vec![]);
         self.raw_vals.push(vec![]);
@@ -151,10 +141,6 @@ impl MatchedArg {
         self.vals.iter().flatten().count() == 0
     }
 
-    pub(crate) fn has_val_groups(&self) -> bool {
-        !self.vals.is_empty()
-    }
-
     pub(crate) fn check_explicit(&self, predicate: ArgPredicate) -> bool {
         if self.source == Some(ValueSource::DefaultValue) {
             return false;
@@ -244,38 +230,4 @@ mod tests {
         m.append_val(AnyValue::new(String::from("ccc")), "ccc".into());
         assert_eq!(m.first_raw(), Some(&OsString::from("bbb")));
     }
-
-    #[test]
-    fn test_grouped_vals_push_and_append() {
-        let mut m = MatchedArg::new_group();
-        m.push_val(AnyValue::new(String::from("aaa")), "aaa".into());
-        m.new_val_group();
-        m.append_val(AnyValue::new(String::from("bbb")), "bbb".into());
-        m.append_val(AnyValue::new(String::from("ccc")), "ccc".into());
-        m.new_val_group();
-        m.append_val(AnyValue::new(String::from("ddd")), "ddd".into());
-        m.push_val(AnyValue::new(String::from("eee")), "eee".into());
-        m.new_val_group();
-        m.append_val(AnyValue::new(String::from("fff")), "fff".into());
-        m.append_val(AnyValue::new(String::from("ggg")), "ggg".into());
-        m.append_val(AnyValue::new(String::from("hhh")), "hhh".into());
-        m.append_val(AnyValue::new(String::from("iii")), "iii".into());
-
-        let vals: Vec<&Vec<OsString>> = m.raw_vals().collect();
-        assert_eq!(
-            vals,
-            vec![
-                &vec![OsString::from("aaa")],
-                &vec![OsString::from("bbb"), OsString::from("ccc"),],
-                &vec![OsString::from("ddd")],
-                &vec![OsString::from("eee")],
-                &vec![
-                    OsString::from("fff"),
-                    OsString::from("ggg"),
-                    OsString::from("hhh"),
-                    OsString::from("iii"),
-                ]
-            ]
-        )
-    }
 }
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index 14e4c223..3e2b2282 100644
--- a/src/parser/parser.rs
+++ b/src/parser/parser.rs
@@ -8,6 +8,7 @@ use std::{
 use clap_lex::RawOsStr;
 
 // Internal
+use crate::builder::Action;
 use crate::builder::AppSettings as AS;
 use crate::builder::{Arg, Command};
 use crate::error::Error as ClapError;
@@ -182,8 +183,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                             && !self.cmd.is_disable_help_subcommand_set()
                         {
                             self.parse_help_subcommand(raw_args.remaining(&mut args_cursor))?;
+                            unreachable!("`parse_help_subcommand` always errors");
+                        } else {
+                            subcmd_name = Some(sc_name.to_owned());
                         }
-                        subcmd_name = Some(sc_name.to_owned());
                         break;
                     }
                 }
@@ -253,12 +256,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                                 Usage::new(self.cmd).create_usage_no_title(&used),
                             ))
                         }
-                        ParseResult::HelpFlag => {
-                            return Err(self.help_err(true, Stream::Stdout));
-                        }
-                        ParseResult::VersionFlag => {
-                            return Err(self.version_err(true));
-                        }
                         ParseResult::MaybeHyphenValue => {
                             // Maybe a hyphen value, do nothing.
                         }
@@ -336,12 +333,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                                 Usage::new(self.cmd).create_usage_with_title(&[]),
                             ));
                         }
-                        ParseResult::HelpFlag => {
-                            return Err(self.help_err(false, Stream::Stdout));
-                        }
-                        ParseResult::VersionFlag => {
-                            return Err(self.version_err(false));
-                        }
                         ParseResult::MaybeHyphenValue => {
                             // Maybe a hyphen value, do nothing.
                         }
@@ -358,7 +349,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                         &self.cmd[id],
                         arg_os.to_value_os(),
                         matcher,
-                        true,
                         trailing_values,
                     )?;
                     parse_state = match parse_result {
@@ -385,15 +375,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                     trailing_values = true;
                 }
 
-                // Increase occurrence no matter if we are appending, occurrences
-                // of positional argument equals to number of values rather than
-                // the number of value groups.
-                self.start_occurrence_of_arg(matcher, p);
-                // Creating new value group rather than appending when the arg
-                // doesn't have any value. This behaviour is right because
-                // positional arguments are always present continuously.
-                let append = self.has_val_groups(matcher, p);
-                self.add_val_to_arg(p, arg_os.to_value_os(), matcher, append, trailing_values)?;
+                if !p.is_multiple_values_set() || !matcher.contains(&p.id) {
+                    self.start_occurrence_of_arg(matcher, p);
+                }
+                let _ignored_result =
+                    self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?;
+                debug!(
+                    "Parser::get_matches_with: Ignoring state {:?}; positionals do their own thing",
+                    _ignored_result
+                );
 
                 // Only increment the positional counter if it doesn't allow multiples
                 if !p.is_multiple() {
@@ -426,7 +416,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                 for raw_val in raw_args.remaining(&mut args_cursor) {
                     let val = external_parser.parse_ref(self.cmd, None, raw_val)?;
                     let external_id = &Id::empty_hash();
-                    sc_m.add_val_to(external_id, val, raw_val.to_os_string(), false);
+                    sc_m.add_val_to(external_id, val, raw_val.to_os_string());
                 }
 
                 matcher.subcommand(SubCommand {
@@ -436,8 +426,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                 });
 
                 #[cfg(feature = "env")]
-                self.add_env(matcher, trailing_values)?;
-                self.add_defaults(matcher, trailing_values)?;
+                self.add_env(matcher)?;
+                self.add_defaults(matcher)?;
                 return Validator::new(self.cmd).validate(parse_state, matcher);
             } else {
                 // Start error processing
@@ -456,8 +446,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         }
 
         #[cfg(feature = "env")]
-        self.add_env(matcher, trailing_values)?;
-        self.add_defaults(matcher, trailing_values)?;
+        self.add_env(matcher)?;
+        self.add_defaults(matcher)?;
         Validator::new(self.cmd).validate(parse_state, matcher)
     }
 
@@ -586,7 +576,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
     fn parse_help_subcommand(
         &self,
         cmds: impl Iterator<Item = &'cmd OsStr>,
-    ) -> ClapResult<ParseResult> {
+    ) -> ClapResult<std::convert::Infallible> {
         debug!("Parser::parse_help_subcommand");
 
         let mut cmd = self.cmd.clone();
@@ -695,74 +685,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         Ok(())
     }
 
-    // Retrieves the names of all args the user has supplied thus far, except required ones
-    // because those will be listed in self.required
-    fn check_for_help_and_version_str(&self, arg: &RawOsStr) -> Option<ParseResult> {
-        debug!("Parser::check_for_help_and_version_str");
-        debug!(
-            "Parser::check_for_help_and_version_str: Checking if --{:?} is help or version...",
-            arg
-        );
-
-        if let Some(help) = self.cmd.find(&Id::help_hash()) {
-            if let Some(h) = help.long {
-                if arg == h && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_flag_set()
-                {
-                    debug!("Help");
-                    return Some(ParseResult::HelpFlag);
-                }
-            }
-        }
-
-        if let Some(version) = self.cmd.find(&Id::version_hash()) {
-            if let Some(v) = version.long {
-                if arg == v
-                    && !self.is_set(AS::NoAutoVersion)
-                    && !self.cmd.is_disable_version_flag_set()
-                {
-                    debug!("Version");
-                    return Some(ParseResult::VersionFlag);
-                }
-            }
-        }
-
-        debug!("Neither");
-        None
-    }
-
-    fn check_for_help_and_version_char(&self, arg: char) -> Option<ParseResult> {
-        debug!("Parser::check_for_help_and_version_char");
-        debug!(
-            "Parser::check_for_help_and_version_char: Checking if -{} is help or version...",
-            arg
-        );
-
-        if let Some(help) = self.cmd.find(&Id::help_hash()) {
-            if let Some(h) = help.short {
-                if arg == h && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_flag_set()
-                {
-                    debug!("Help");
-                    return Some(ParseResult::HelpFlag);
-                }
-            }
-        }
-
-        if let Some(version) = self.cmd.find(&Id::version_hash()) {
-            if let Some(v) = version.short {
-                if arg == v
-                    && !self.is_set(AS::NoAutoVersion)
-                    && !self.cmd.is_disable_version_flag_set()
-                {
-                    debug!("Version");
-                    return Some(ParseResult::VersionFlag);
-                }
-            }
-        }
-
-        debug!("Neither");
-        None
-    }
-
     fn parse_long_arg(
         &mut self,
         matcher: &mut ArgMatcher,
@@ -804,30 +726,37 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                 "Parser::parse_long_arg: Found valid opt or flag '{}'",
                 opt.to_string()
             );
-            Some(opt)
+            Some((long_arg, opt))
         } else if self.cmd.is_infer_long_args_set() {
-            self.cmd.get_arguments().find(|a| {
-                a.long.map_or(false, |long| long.starts_with(long_arg))
-                    || a.aliases
-                        .iter()
-                        .any(|(alias, _)| alias.starts_with(long_arg))
+            self.cmd.get_arguments().find_map(|a| {
+                if let Some(long) = a.long {
+                    if long.starts_with(long_arg) {
+                        return Some((long, a));
+                    }
+                }
+                a.aliases
+                    .iter()
+                    .find_map(|(alias, _)| alias.starts_with(long_arg).then(|| (*alias, a)))
             })
         } else {
             None
         };
 
-        if let Some(opt) = opt {
+        if let Some((long_arg, opt)) = opt {
             *valid_arg_found = true;
             if opt.is_takes_value_set() {
                 debug!(
-                    "Parser::parse_long_arg: Found an opt with value '{:?}'",
-                    &long_value
+                    "Parser::parse_long_arg({:?}): Found an opt with value '{:?}'",
+                    long_arg, &long_value
                 );
                 let has_eq = long_value.is_some();
-                self.parse_opt(long_value, opt, matcher, trailing_values, has_eq)
+                self.parse_opt_value(long_value, opt, matcher, trailing_values, has_eq)
             } else if let Some(rest) = long_value {
                 let required = self.cmd.required_graph();
-                debug!("Parser::parse_long_arg: Got invalid literal `{:?}`", rest);
+                debug!(
+                    "Parser::parse_long_arg({:?}): Got invalid literal `{:?}`",
+                    long_arg, rest
+                );
                 let used: Vec<Id> = matcher
                     .arg_names()
                     .filter(|&n| {
@@ -843,13 +772,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                     used,
                     arg: opt.to_string(),
                 })
-            } else if let Some(parse_result) =
-                self.check_for_help_and_version_str(RawOsStr::from_str(long_arg))
-            {
-                Ok(parse_result)
             } else {
-                debug!("Parser::parse_long_arg: Presence validated");
-                Ok(self.parse_flag(opt, matcher))
+                debug!("Parser::parse_long_arg({:?}): Presence validated", long_arg);
+                self.parse_flag(Identifier::Long(long_arg), opt, matcher)
             }
         } else if let Some(sc_name) = self.possible_long_flag_subcommand(long_arg) {
             Ok(ParseResult::FlagSubCommand(sc_name.to_string()))
@@ -946,10 +871,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                 );
                 *valid_arg_found = true;
                 if !opt.is_takes_value_set() {
-                    if let Some(parse_result) = self.check_for_help_and_version_char(c) {
-                        return Ok(parse_result);
-                    }
-                    ret = self.parse_flag(opt, matcher);
+                    ret = self.parse_flag(Identifier::Short(c), opt, matcher)?;
                     continue;
                 }
 
@@ -975,7 +897,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                 } else {
                     (val, false)
                 };
-                match self.parse_opt(val, opt, matcher, trailing_values, has_eq)? {
+                match self.parse_opt_value(val, opt, matcher, trailing_values, has_eq)? {
                     ParseResult::AttachedValueNotConsumed => continue,
                     x => return Ok(x),
                 }
@@ -1003,7 +925,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         Ok(ret)
     }
 
-    fn parse_opt(
+    fn parse_opt_value(
         &self,
         attached_value: Option<&RawOsStr>,
         opt: &Arg<'help>,
@@ -1012,12 +934,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         has_eq: bool,
     ) -> ClapResult<ParseResult> {
         debug!(
-            "Parser::parse_opt; opt={}, val={:?}, has_eq={:?}",
+            "Parser::parse_opt_value; opt={}, val={:?}, has_eq={:?}",
             opt.name, attached_value, has_eq
         );
-        debug!("Parser::parse_opt; opt.settings={:?}", opt.settings);
+        debug!("Parser::parse_opt_value; opt.settings={:?}", opt.settings);
 
-        debug!("Parser::parse_opt; Checking for val...");
+        debug!("Parser::parse_opt_value; Checking for val...");
         // require_equals is set, but no '=' is provided, try throwing error.
         if opt.is_require_equals_set() && !has_eq {
             if opt.min_vals == Some(0) {
@@ -1025,13 +947,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                 self.start_occurrence_of_arg(matcher, opt);
                 // We assume this case is valid: require equals, but min_vals == 0.
                 if !opt.default_missing_vals.is_empty() {
-                    debug!("Parser::parse_opt: has default_missing_vals");
-                    self.add_multiple_vals_to_arg(
-                        opt,
-                        opt.default_missing_vals.iter().map(OsString::from),
-                        matcher,
-                        false,
-                    )?;
+                    debug!("Parser::parse_opt_value: has default_missing_vals");
+                    for v in opt.default_missing_vals.iter() {
+                        let _ignored_result =
+                            self.add_val_to_arg(opt, &RawOsStr::new(v), matcher, trailing_values)?;
+                        if _ignored_result != ParseResult::ValuesDone {
+                            debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _ignored_result);
+                        }
+                    }
                 };
                 if attached_value.is_some() {
                     Ok(ParseResult::AttachedValueNotConsumed)
@@ -1046,15 +969,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
             }
         } else if let Some(v) = attached_value {
             self.start_occurrence_of_arg(matcher, opt);
-            self.add_val_to_arg(opt, v, matcher, false, trailing_values)?;
-            Ok(ParseResult::ValuesDone)
+            let mut val_result = self.add_val_to_arg(opt, v, matcher, trailing_values)?;
+            if val_result != ParseResult::ValuesDone {
+                debug!("Parser::parse_opt_value: Overriding state {:?}; no values accepted after attached", val_result);
+                val_result = ParseResult::ValuesDone;
+            }
+            Ok(val_result)
         } else {
-            debug!("Parser::parse_opt: More arg vals required...");
+            debug!("Parser::parse_opt_value: More arg vals required...");
             self.start_occurrence_of_arg(matcher, opt);
-            matcher.new_val_group(&opt.id);
-            for group in self.cmd.groups_for_arg(&opt.id) {
-                matcher.new_val_group(&group);
-            }
             Ok(ParseResult::Opt(opt.id.clone()))
         }
     }
@@ -1064,7 +987,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         arg: &Arg<'help>,
         val: &RawOsStr,
         matcher: &mut ArgMatcher,
-        append: bool,
         trailing_values: bool,
     ) -> ClapResult<ParseResult> {
         debug!("Parser::add_val_to_arg; arg={}, val={:?}", arg.name, val);
@@ -1073,59 +995,35 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
             trailing_values,
             self.cmd.is_dont_delimit_trailing_values_set()
         );
-        if !(trailing_values && self.cmd.is_dont_delimit_trailing_values_set()) {
-            if let Some(delim) = arg.val_delim {
-                let terminator = arg.terminator.map(OsStr::new);
-                let vals = val
-                    .split(delim)
-                    .map(|x| x.to_os_str().into_owned())
-                    .take_while(|val| Some(val.as_os_str()) != terminator);
-                self.add_multiple_vals_to_arg(arg, vals, matcher, append)?;
-                // If there was a delimiter used or we must use the delimiter to
-                // separate the values or no more vals is needed, we're not
-                // looking for more values.
-                return if val.contains(delim)
-                    || arg.is_require_value_delimiter_set()
-                    || !matcher.needs_more_vals(arg)
-                {
-                    Ok(ParseResult::ValuesDone)
-                } else {
-                    Ok(ParseResult::Opt(arg.id.clone()))
-                };
-            }
+
+        let mut delim = arg.val_delim;
+        if trailing_values && self.cmd.is_dont_delimit_trailing_values_set() {
+            delim = None;
         }
-        if let Some(t) = arg.terminator {
-            if t == val {
-                return Ok(ParseResult::ValuesDone);
+        match delim {
+            Some(delim) if val.contains(delim) => {
+                let vals = val.split(delim).map(|x| x.to_os_str().into_owned());
+                for raw_val in vals {
+                    if Some(raw_val.as_os_str()) == arg.terminator.map(OsStr::new) {
+                        return Ok(ParseResult::ValuesDone);
+                    }
+                    self.add_single_val_to_arg(arg, raw_val, matcher)?;
+                }
+                // Delimited values are always considered the final value
+                Ok(ParseResult::ValuesDone)
             }
-        }
-        self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher, append)?;
-        if matcher.needs_more_vals(arg) {
-            Ok(ParseResult::Opt(arg.id.clone()))
-        } else {
-            Ok(ParseResult::ValuesDone)
-        }
-    }
-
-    fn add_multiple_vals_to_arg(
-        &self,
-        arg: &Arg<'help>,
-        raw_vals: impl Iterator<Item = OsString>,
-        matcher: &mut ArgMatcher,
-        append: bool,
-    ) -> ClapResult<()> {
-        // If not appending, create a new val group and then append vals in.
-        if !append {
-            matcher.new_val_group(&arg.id);
-            for group in self.cmd.groups_for_arg(&arg.id) {
-                matcher.new_val_group(&group);
+            _ if Some(val) == arg.terminator.map(RawOsStr::from_str) => Ok(ParseResult::ValuesDone),
+            _ => {
+                self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher)?;
+                if arg.is_require_value_delimiter_set() {
+                    Ok(ParseResult::ValuesDone)
+                } else if matcher.needs_more_vals(arg) {
+                    Ok(ParseResult::Opt(arg.id.clone()))
+                } else {
+                    Ok(ParseResult::ValuesDone)
+                }
             }
         }
-        for raw_val in raw_vals {
-            self.add_single_val_to_arg(arg, raw_val, matcher, true)?;
-        }
-
-        Ok(())
     }
 
     fn add_single_val_to_arg(
@@ -1133,7 +1031,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         arg: &Arg<'help>,
         raw_val: OsString,
         matcher: &mut ArgMatcher,
-        append: bool,
     ) -> ClapResult<()> {
         debug!("Parser::add_single_val_to_arg: adding val...{:?}", raw_val);
 
@@ -1148,26 +1045,47 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
 
         // 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(), append);
+            matcher.add_val_to(&group, val.clone(), raw_val.clone());
         }
 
-        matcher.add_val_to(&arg.id, val, raw_val, append);
+        matcher.add_val_to(&arg.id, val, raw_val);
         matcher.add_index_to(&arg.id, self.cur_idx.get());
 
         Ok(())
     }
 
-    fn has_val_groups(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool {
-        matcher.has_val_groups(&arg.id)
-    }
-
-    fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult {
+    fn parse_flag(
+        &self,
+        ident: Identifier,
+        arg: &Arg<'help>,
+        matcher: &mut ArgMatcher,
+    ) -> ClapResult<ParseResult> {
         debug!("Parser::parse_flag");
+        match arg.get_action() {
+            Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()),
+            Action::Flag => {
+                self.start_occurrence_of_arg(matcher, arg);
+                matcher.add_index_to(&arg.id, self.cur_idx.get());
 
-        self.start_occurrence_of_arg(matcher, flag);
-        matcher.add_index_to(&flag.id, self.cur_idx.get());
-
-        ParseResult::ValuesDone
+                Ok(ParseResult::ValuesDone)
+            }
+            Action::Help => {
+                let use_long = match ident {
+                    Identifier::Long(_) => true,
+                    Identifier::Short(_) => false,
+                };
+                debug!("Help: use_long={}", use_long);
+                Err(self.help_err(use_long, Stream::Stdout))
+            }
+            Action::Version => {
+                let use_long = match ident {
+                    Identifier::Long(_) => true,
+                    Identifier::Short(_) => false,
+                };
+                debug!("Version: use_long={}", use_long);
+                Err(self.version_err(use_long))
+            }
+        }
     }
 
     fn remove_overrides(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) {
@@ -1193,17 +1111,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
     }
 
     #[cfg(feature = "env")]
-    fn add_env(&mut self, matcher: &mut ArgMatcher, trailing_values: bool) -> ClapResult<()> {
+    fn add_env(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> {
         debug!("Parser::add_env");
         use crate::util::str_to_bool;
 
+        let trailing_values = false; // defaults are independent of the commandline
         for arg in self.cmd.get_arguments() {
             // Use env only if the arg was absent among command line args,
             // early return if this is not the case.
-            if matcher
-                .get(&arg.id)
-                .map_or(false, |arg| arg.get_occurrences() != 0)
-            {
+            if matcher.contains(&arg.id) {
                 debug!("Parser::add_env: Skipping existing arg `{}`", arg);
                 continue;
             }
@@ -1218,17 +1134,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                         val, trailing_values
                     );
                     self.start_custom_arg(matcher, arg, ValueSource::EnvVariable);
-                    self.add_val_to_arg(arg, &val, matcher, false, trailing_values)?;
+                    let _ignored_result =
+                        self.add_val_to_arg(arg, &val, matcher, trailing_values)?;
+                    if _ignored_result != ParseResult::ValuesDone {
+                        debug!("Parser::add_env: Ignoring state {:?}; env variables are outside of the parse loop", _ignored_result);
+                    }
                 } else {
-                    // Early return on `HelpFlag` or `VersionFlag`.
-                    match self.check_for_help_and_version_str(&val) {
-                        Some(ParseResult::HelpFlag) => {
-                            return Err(self.help_err(true, Stream::Stdout));
-                        }
-                        Some(ParseResult::VersionFlag) => {
-                            return Err(self.version_err(true));
-                        }
-                        _ => {
+                    match arg.get_action() {
+                        Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()),
+                        Action::Flag => {
                             debug!("Parser::add_env: Found a flag with value `{:?}`", val);
                             let predicate = str_to_bool(val.to_str_lossy());
                             debug!("Parser::add_env: Found boolean literal `{:?}`", predicate);
@@ -1237,6 +1151,13 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                                 matcher.add_index_to(&arg.id, self.cur_idx.get());
                             }
                         }
+                        // Early return on `Help` or `Version`.
+                        Action::Help => {
+                            return Err(self.help_err(true, Stream::Stdout));
+                        }
+                        Action::Version => {
+                            return Err(self.version_err(true));
+                        }
                     }
                 }
             }
@@ -1245,31 +1166,69 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         Ok(())
     }
 
-    fn add_defaults(&mut self, matcher: &mut ArgMatcher, trailing_values: bool) -> ClapResult<()> {
+    fn add_defaults(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> {
         debug!("Parser::add_defaults");
 
         for o in self.cmd.get_opts() {
             debug!("Parser::add_defaults:iter:{}:", o.name);
-            self.add_default_value(o, matcher, trailing_values)?;
+            self.add_default_value(o, matcher)?;
         }
 
         for p in self.cmd.get_positionals() {
             debug!("Parser::add_defaults:iter:{}:", p.name);
-            self.add_default_value(p, matcher, trailing_values)?;
+            self.add_default_value(p, matcher)?;
         }
 
         Ok(())
     }
 
-    fn add_default_value(
-        &self,
-        arg: &Arg<'help>,
-        matcher: &mut ArgMatcher,
-        trailing_values: bool,
-    ) -> ClapResult<()> {
+    fn add_default_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult<()> {
+        let trailing_values = false; // defaults are independent of the commandline
+
+        if !arg.default_missing_vals.is_empty() {
+            debug!(
+                "Parser::add_default_value:iter:{}: has default missing vals",
+                arg.name
+            );
+            match matcher.get(&arg.id) {
+                Some(ma) if ma.all_val_groups_empty() => {
+                    debug!(
+                        "Parser::add_default_value:iter:{}: has no user defined vals",
+                        arg.name
+                    );
+                    // The flag occurred, we just want to add the val groups
+                    self.start_custom_arg(matcher, arg, ValueSource::CommandLine);
+                    for v in arg.default_missing_vals.iter() {
+                        let _ignored_result =
+                            self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?;
+                        if _ignored_result != ParseResult::ValuesDone {
+                            debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _ignored_result);
+                        }
+                    }
+                }
+                None => {
+                    debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name);
+                    // do nothing
+                }
+                _ => {
+                    debug!(
+                        "Parser::add_default_value:iter:{}: has user defined vals",
+                        arg.name
+                    );
+                    // do nothing
+                }
+            }
+        } else {
+            debug!(
+                "Parser::add_default_value:iter:{}: doesn't have default missing vals",
+                arg.name
+            );
+            // do nothing
+        }
+
         if !arg.default_vals_ifs.is_empty() {
             debug!("Parser::add_default_value: has conditional defaults");
-            if matcher.get(&arg.id).is_none() {
+            if !matcher.contains(&arg.id) {
                 for (id, val, default) in arg.default_vals_ifs.iter() {
                     let add = if let Some(a) = matcher.get(id) {
                         match val {
@@ -1285,13 +1244,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                     if add {
                         if let Some(default) = default {
                             self.start_custom_arg(matcher, arg, ValueSource::DefaultValue);
-                            self.add_val_to_arg(
+                            let _ignored_result = self.add_val_to_arg(
                                 arg,
                                 &RawOsStr::new(default),
                                 matcher,
-                                false,
                                 trailing_values,
                             )?;
+                            if _ignored_result != ParseResult::ValuesDone {
+                                debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _ignored_result);
+                            }
                         }
                         return Ok(());
                     }
@@ -1301,83 +1262,29 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
             debug!("Parser::add_default_value: doesn't have conditional defaults");
         }
 
-        fn process_default_vals(arg: &Arg<'_>, default_vals: &[&OsStr]) -> Vec<OsString> {
-            if let Some(delim) = arg.val_delim {
-                let mut vals = vec![];
-                for val in default_vals {
-                    let val = RawOsStr::new(val);
-                    for val in val.split(delim) {
-                        vals.push(val.to_os_str().into_owned());
-                    }
-                }
-                vals
-            } else {
-                default_vals.iter().map(OsString::from).collect()
-            }
-        }
-
         if !arg.default_vals.is_empty() {
             debug!(
                 "Parser::add_default_value:iter:{}: has default vals",
                 arg.name
             );
-            if matcher.get(&arg.id).is_some() {
+            if matcher.contains(&arg.id) {
                 debug!("Parser::add_default_value:iter:{}: was used", arg.name);
             // do nothing
             } else {
                 debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name);
 
                 self.start_custom_arg(matcher, arg, ValueSource::DefaultValue);
-                self.add_multiple_vals_to_arg(
-                    arg,
-                    process_default_vals(arg, &arg.default_vals).into_iter(),
-                    matcher,
-                    false,
-                )?;
-            }
-        } else {
-            debug!(
-                "Parser::add_default_value:iter:{}: doesn't have default vals",
-                arg.name
-            );
-
-            // do nothing
-        }
-
-        if !arg.default_missing_vals.is_empty() {
-            debug!(
-                "Parser::add_default_value:iter:{}: has default missing vals",
-                arg.name
-            );
-            match matcher.get(&arg.id) {
-                Some(ma) if ma.all_val_groups_empty() => {
-                    debug!(
-                        "Parser::add_default_value:iter:{}: has no user defined vals",
-                        arg.name
-                    );
-                    self.start_custom_arg(matcher, arg, ValueSource::DefaultValue);
-                    self.add_multiple_vals_to_arg(
-                        arg,
-                        process_default_vals(arg, &arg.default_missing_vals).into_iter(),
-                        matcher,
-                        false,
-                    )?;
-                }
-                None => {
-                    debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name);
-                    // do nothing
-                }
-                _ => {
-                    debug!(
-                        "Parser::add_default_value:iter:{}: has user defined vals",
-                        arg.name
-                    );
-                    // do nothing
+                for v in arg.default_vals.iter() {
+                    let _ignored_result =
+                        self.add_val_to_arg(arg, &RawOsStr::new(v), matcher, trailing_values)?;
+                    if _ignored_result != ParseResult::ValuesDone {
+                        debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _ignored_result);
+                    }
                 }
             }
         } else {
             debug!(
-                "Parser::add_default_value:iter:{}: doesn't have default missing vals",
+                "Parser::add_default_value:iter:{}: doesn't have default vals",
                 arg.name
             );
 
@@ -1493,6 +1400,7 @@ pub(crate) enum ParseState {
 
 /// Recoverable Parsing results.
 #[derive(Debug, PartialEq, Clone)]
+#[must_use]
 enum ParseResult {
     FlagSubCommand(String),
     Opt(Id),
@@ -1518,8 +1426,10 @@ enum ParseResult {
     },
     /// No argument found e.g. parser is given `-` when parsing a flag.
     NoArg,
-    /// This is a Help flag.
-    HelpFlag,
-    /// This is a version flag.
-    VersionFlag,
+}
+
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+enum Identifier<'f> {
+    Short(char),
+    Long(&'f str),
 }


Test patch:
diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs
index 0700df67..bacbe1ff 100644
--- a/tests/builder/default_missing_vals.rs
+++ b/tests/builder/default_missing_vals.rs
@@ -1,4 +1,4 @@
-use clap::{arg, Arg, ArgMatches, Command};
+use clap::{arg, Arg, Command};
 
 #[test]
 fn opt_missing() {
@@ -20,6 +20,10 @@ fn opt_missing() {
         "auto"
     );
     assert_eq!(m.occurrences_of("color"), 0);
+    assert_eq!(
+        m.value_source("color").unwrap(),
+        clap::ValueSource::DefaultValue
+    );
 }
 
 #[test]
@@ -42,6 +46,10 @@ fn opt_present_with_missing_value() {
         "always"
     );
     assert_eq!(m.occurrences_of("color"), 1);
+    assert_eq!(
+        m.value_source("color").unwrap(),
+        clap::ValueSource::CommandLine
+    );
 }
 
 #[test]
@@ -64,6 +72,10 @@ fn opt_present_with_value() {
         "never"
     );
     assert_eq!(m.occurrences_of("color"), 1);
+    assert_eq!(
+        m.value_source("color").unwrap(),
+        clap::ValueSource::CommandLine
+    );
 }
 
 #[test]
@@ -85,6 +97,10 @@ fn opt_present_with_empty_value() {
         ""
     );
     assert_eq!(m.occurrences_of("color"), 1);
+    assert_eq!(
+        m.value_source("color").unwrap(),
+        clap::ValueSource::CommandLine
+    );
 }
 
 //## `default_value`/`default_missing_value` non-interaction checks
@@ -134,44 +150,96 @@ fn default_missing_value_flag_value() {
         Arg::new("flag")
             .long("flag")
             .takes_value(true)
+            .default_value("false")
             .default_missing_value("true"),
     );
 
-    fn flag_value(m: ArgMatches) -> bool {
-        match m.get_one::<String>("flag").map(|v| v.as_str()) {
-            None => false,
-            Some(x) => x.parse().expect("non boolean value"),
-        }
-    }
+    let m = cmd.clone().try_get_matches_from(&["test"]).unwrap();
+    assert!(m.is_present("flag"));
+    assert_eq!(
+        m.get_one::<String>("flag").map(|v| v.as_str()),
+        Some("false")
+    );
+    assert_eq!(m.occurrences_of("flag"), 0);
+    assert_eq!(
+        m.value_source("flag").unwrap(),
+        clap::ValueSource::DefaultValue
+    );
 
+    let m = cmd
+        .clone()
+        .try_get_matches_from(&["test", "--flag"])
+        .unwrap();
+    assert!(m.is_present("flag"));
     assert_eq!(
-        flag_value(cmd.clone().try_get_matches_from(&["test"]).unwrap()),
-        false
+        m.get_one::<String>("flag").map(|v| v.as_str()),
+        Some("true")
     );
+    assert_eq!(m.occurrences_of("flag"), 1);
     assert_eq!(
-        flag_value(
-            cmd.clone()
-                .try_get_matches_from(&["test", "--flag"])
-                .unwrap()
-        ),
-        true
+        m.value_source("flag").unwrap(),
+        clap::ValueSource::CommandLine
     );
+
+    let m = cmd
+        .clone()
+        .try_get_matches_from(&["test", "--flag=true"])
+        .unwrap();
+    assert!(m.is_present("flag"));
+    assert_eq!(
+        m.get_one::<String>("flag").map(|v| v.as_str()),
+        Some("true")
+    );
+    assert_eq!(m.occurrences_of("flag"), 1);
     assert_eq!(
-        flag_value(
-            cmd.clone()
-                .try_get_matches_from(&["test", "--flag=true"])
-                .unwrap()
-        ),
-        true
+        m.value_source("flag").unwrap(),
+        clap::ValueSource::CommandLine
     );
+
+    let m = cmd.try_get_matches_from(&["test", "--flag=false"]).unwrap();
+    assert!(m.is_present("flag"));
+    assert_eq!(
+        m.get_one::<String>("flag").map(|v| v.as_str()),
+        Some("false")
+    );
+    assert_eq!(m.occurrences_of("flag"), 1);
+    assert_eq!(
+        m.value_source("flag").unwrap(),
+        clap::ValueSource::CommandLine
+    );
+}
+
+#[test]
+fn delimited_missing_value() {
+    let cmd = Command::new("test").arg(
+        Arg::new("flag")
+            .long("flag")
+            .default_value("one,two")
+            .default_missing_value("three,four")
+            .min_values(0)
+            .value_delimiter(',')
+            .require_equals(true),
+    );
+
+    let m = cmd.clone().try_get_matches_from(["test"]).unwrap();
+    assert_eq!(
+        m.get_many::<String>("flag")
+            .unwrap()
+            .map(|s| s.as_str())
+            .collect::<Vec<_>>(),
+        vec!["one", "two"]
+    );
+    assert_eq!(m.occurrences_of("flag"), 0);
+
+    let m = cmd.try_get_matches_from(["test", "--flag"]).unwrap();
     assert_eq!(
-        flag_value(
-            cmd.clone()
-                .try_get_matches_from(&["test", "--flag=false"])
-                .unwrap()
-        ),
-        false
+        m.get_many::<String>("flag")
+            .unwrap()
+            .map(|s| s.as_str())
+            .collect::<Vec<_>>(),
+        vec!["three", "four"]
     );
+    assert_eq!(m.occurrences_of("flag"), 1);
 }
 
 #[cfg(debug_assertions)]
diff --git a/tests/builder/default_vals.rs b/tests/builder/default_vals.rs
index 67848473..501ffcd3 100644
--- a/tests/builder/default_vals.rs
+++ b/tests/builder/default_vals.rs
@@ -831,3 +831,49 @@ fn missing_with_value_delimiter() {
         ["value1", "value2", "value3", "value4", "value5"]
     );
 }
+
+#[test]
+fn default_independent_of_trailing() {
+    let cmd = Command::new("test")
+        .dont_delimit_trailing_values(true)
+        .arg(Arg::new("pos").required(true))
+        .arg(
+            Arg::new("flag")
+                .long("flag")
+                .default_value("one,two")
+                .value_delimiter(','),
+        );
+
+    // Baseline behavior
+    let m = cmd
+        .clone()
+        .try_get_matches_from(vec!["program", "here"])
+        .unwrap();
+    assert_eq!(
+        m.get_one::<String>("pos").map(|v| v.as_str()).unwrap(),
+        "here"
+    );
+    assert_eq!(
+        m.get_many::<String>("flag")
+            .unwrap()
+            .map(|v| v.as_str())
+            .collect::<Vec<_>>(),
+        ["one", "two"]
+    );
+
+    // Trailing-values behavior should match the baseline
+    let m = cmd
+        .try_get_matches_from(vec!["program", "--", "here"])
+        .unwrap();
+    assert_eq!(
+        m.get_one::<String>("pos").map(|v| v.as_str()).unwrap(),
+        "here"
+    );
+    assert_eq!(
+        m.get_many::<String>("flag")
+            .unwrap()
+            .map(|v| v.as_str())
+            .collect::<Vec<_>>(),
+        ["one", "two"]
+    );
+}
diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs
index 2029a4df..c775e359 100644
--- a/tests/builder/multiple_values.rs
+++ b/tests/builder/multiple_values.rs
@@ -385,7 +385,7 @@ fn positional() {
     let m = m.unwrap();
 
     assert!(m.is_present("pos"));
-    assert_eq!(m.occurrences_of("pos"), 3);
+    assert_eq!(m.occurrences_of("pos"), 1);
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -409,7 +409,7 @@ fn positional_exact_exact() {
     let m = m.unwrap();
 
     assert!(m.is_present("pos"));
-    assert_eq!(m.occurrences_of("pos"), 3);
+    assert_eq!(m.occurrences_of("pos"), 1);
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -457,7 +457,7 @@ fn positional_min_exact() {
     let m = m.unwrap();
 
     assert!(m.is_present("pos"));
-    assert_eq!(m.occurrences_of("pos"), 3);
+    assert_eq!(m.occurrences_of("pos"), 1);
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -487,7 +487,7 @@ fn positional_min_more() {
     let m = m.unwrap();
 
     assert!(m.is_present("pos"));
-    assert_eq!(m.occurrences_of("pos"), 4);
+    assert_eq!(m.occurrences_of("pos"), 1);
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -507,7 +507,7 @@ fn positional_max_exact() {
     let m = m.unwrap();
 
     assert!(m.is_present("pos"));
-    assert_eq!(m.occurrences_of("pos"), 3);
+    assert_eq!(m.occurrences_of("pos"), 1);
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -527,7 +527,7 @@ fn positional_max_less() {
     let m = m.unwrap();
 
     assert!(m.is_present("pos"));
-    assert_eq!(m.occurrences_of("pos"), 2);
+    assert_eq!(m.occurrences_of("pos"), 1);
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -1139,7 +1139,7 @@ fn low_index_positional() {
     let m = m.unwrap();
 
     assert!(m.is_present("files"));
-    assert_eq!(m.occurrences_of("files"), 3);
+    assert_eq!(m.occurrences_of("files"), 1);
     assert!(m.is_present("target"));
     assert_eq!(m.occurrences_of("target"), 1);
     assert_eq!(
@@ -1176,7 +1176,7 @@ fn low_index_positional_in_subcmd() {
     let sm = m.subcommand_matches("test").unwrap();
 
     assert!(sm.is_present("files"));
-    assert_eq!(sm.occurrences_of("files"), 3);
+    assert_eq!(sm.occurrences_of("files"), 1);
     assert!(sm.is_present("target"));
     assert_eq!(sm.occurrences_of("target"), 1);
     assert_eq!(
@@ -1212,7 +1212,7 @@ fn low_index_positional_with_option() {
     let m = m.unwrap();
 
     assert!(m.is_present("files"));
-    assert_eq!(m.occurrences_of("files"), 3);
+    assert_eq!(m.occurrences_of("files"), 1);
     assert!(m.is_present("target"));
     assert_eq!(m.occurrences_of("target"), 1);
     assert_eq!(
@@ -1250,7 +1250,7 @@ fn low_index_positional_with_flag() {
     let m = m.unwrap();
 
     assert!(m.is_present("files"));
-    assert_eq!(m.occurrences_of("files"), 3);
+    assert_eq!(m.occurrences_of("files"), 1);
     assert!(m.is_present("target"));
     assert_eq!(m.occurrences_of("target"), 1);
     assert_eq!(
@@ -1498,6 +1498,7 @@ fn values_per_occurrence_named() {
             .collect::<Vec<_>>(),
         ["val1", "val2"]
     );
+    assert_eq!(m.occurrences_of("pos"), 1);
 
     let m = a.try_get_matches_from_mut(vec![
         "myprog", "--pos", "val1", "val2", "--pos", "val3", "val4",
@@ -1513,6 +1514,7 @@ fn values_per_occurrence_named() {
             .collect::<Vec<_>>(),
         ["val1", "val2", "val3", "val4"]
     );
+    assert_eq!(m.occurrences_of("pos"), 2);
 }
 
 #[test]
@@ -1535,6 +1537,7 @@ fn values_per_occurrence_positional() {
             .collect::<Vec<_>>(),
         ["val1", "val2"]
     );
+    assert_eq!(m.occurrences_of("pos"), 1);
 
     let m = a.try_get_matches_from_mut(vec!["myprog", "val1", "val2", "val3", "val4"]);
     let m = match m {
@@ -1548,4 +1551,36 @@ fn values_per_occurrence_positional() {
             .collect::<Vec<_>>(),
         ["val1", "val2", "val3", "val4"]
     );
+    //assert_eq!(m.occurrences_of("pos"), 2);  // Fails, we don't recognize this as two occurrences
+}
+
+// Theoretically we could support this but we aren't tracking occurrence boundaries for positionals
+#[test]
+#[should_panic = "When using a positional argument with .multiple_values(true) that is *not the last* positional argument, the last positional argument (i.e. the one with the highest index) *must* have .required(true) or .last(true) set."]
+fn positional_parser_advances() {
+    let m = Command::new("multiple_values")
+        .arg(Arg::new("pos1").number_of_values(2))
+        .arg(Arg::new("pos2").number_of_values(2))
+        .try_get_matches_from(vec!["myprog", "val1", "val2", "val3", "val4"]);
+
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    let m = m.unwrap();
+
+    assert_eq!(m.occurrences_of("pos1"), 1);
+    assert_eq!(
+        m.get_many::<String>("pos1")
+            .unwrap()
+            .map(|v| v.as_str())
+            .collect::<Vec<_>>(),
+        ["val1", "val2"]
+    );
+
+    assert_eq!(m.occurrences_of("pos2"), 1);
+    assert_eq!(
+        m.get_many::<String>("pos2")
+            .unwrap()
+            .map(|v| v.as_str())
+            .collect::<Vec<_>>(),
+        ["val3", "val4"]
+    );
 }