From 24eeed65a26bcc953ba1a524baedc4cef92159bf Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH] Added an -S/--strict-longopts option to argparse. This flag disables a very surprising and confusing feature I found in the code of wgetopt.rs: the ability to abbreviate the names of long options and the ability to parse long options with a single "-". This commit addresses #7341, but unlike pull request #11220, it does so in a backwards compatible way: one must use the new -S/--strict-longotps flag to disable the old legacy behaviour. Unlike pull request #11220 however, this flag only applies to ``argparse``, and not to any builtins used by fish. Note that forcing the flag -S/--strict-longotps on (i.e. in src/wgetopt.rs, replacing both uses of `self.strict_long_opts` with `true`), does not cause any of the current test cases to fail. However, third-party fish scripts may be depending on the current behaviour. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 15 +++++++++++++++ share/completions/argparse.fish | 2 ++ src/builtins/argparse.rs | 6 +++++- src/wgetopt.rs | 16 ++++++++++++---- tests/checks/argparse.fish | 30 ++++++++++++++++++++++++++++++ 6 files changed, 65 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b38d70076..d1d5313e8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -46,6 +46,7 @@ Scripting improvements - ``argparse`` options can now be marked to be deleted from ``$argv_opts`` (by adding a ``&`` at the end of the option spec, before a ``!`` if present). There is now also a corresponding ``-d`` / ``--delete`` option to ``fish_opt``. - ``argparse --ignore-unknown`` now removes preceding known short options from groups containing unknown options (e.g. when parsing ``-abc``, if ``a`` is known but ``b`` is not, then ``$argv`` will contain ``-bc``). - ``argparse`` now has an ``-u`` / ``--move-unknown`` option that works like ``--ignore-unknown``, but unknown options (and their arguments) are moved from ``$argv`` to ``$argv_opts``. whereas ``--ignore-unknown`` keeps them in ``$argv``. +- ``argparse`` now has an ``-S`` / ``--strict-longopts`` option that forbids abbreviating long options or passing them with a single dash (e.g. if there is a long option called ``foo``, ``--fo`` and ``--foo`` won't match it). Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index c007ce5ab..d6e234fe4 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -50,6 +50,21 @@ The following ``argparse`` options are available. They must appear before all *O **-i** or **--ignore-unknown** Deprecated. This is like **--move-unknown**, except that unknown options and their arguments are kept in ``$argv`` and not moved to ``$argv_opts``. Unlike **--move-unknown**, this option makes it impossible to distinguish between an unknown option and non-option argument that starts with a ``-`` (since any ``--`` seperator in ``$argv`` will be removed). +**-S** or **--strict-longopts** + This makes the parsing of long options more strict. In particular, *without* this flag, if ``long`` is a known long option flag, ``--long`` and ``--long=`` can be abbreviated as: + + - ``-long`` and ``-long=``, but *only* if there is no short flag ``l``. + + - ``--lo`` and ``--lo=``, but *only* if there is no other long flag that starts with ``lo``. Similarly with any other non-empty prefix of ``long``. + + - ``-lo`` and ``-lo=`` (i.e. combining the above two). + + With the ``--strict-longopts`` flag, the above three are parse errors: one must use the syntax ``--long`` or ``--long=`` to use a long option called ``long``. + + This flag has no effect on the parsing of unknown options (which are parsed as if this flag is on). + + This option may be on all the time in the future, so do not rely on the behaviour without it. + **-s** or **--stop-nonopt** Causes scanning the arguments to stop as soon as the first non-option argument is seen. Among other things, this is useful to implement subcommands that have their own options. diff --git a/share/completions/argparse.fish b/share/completions/argparse.fish index c76421b2f..fd419fc23 100644 --- a/share/completions/argparse.fish +++ b/share/completions/argparse.fish @@ -72,5 +72,7 @@ complete --command argparse --short-option i --long-option ignore-unknown \ complete --command argparse --short-option u --long-option move-unknown \ -n '! __fish_seen_argument --short i --long ignore-unknown' \ --description 'Move unknown options into \$argv_opts' +complete --command argparse --short-option S --long-option strict-longopts \ + --description 'Pass long options strictly' complete --command argparse --short-option s --long-option stop-nonopt \ --description 'Exit on subcommand' diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 847b60c35..6e2caac14 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -60,6 +60,7 @@ enum UnknownHandling { #[derive(Default)] struct ArgParseCmdOpts<'args> { unknown_handling: UnknownHandling, + strict_long_opts: bool, print_help: bool, stop_nonopt: bool, min_args: usize, @@ -83,13 +84,14 @@ fn new() -> Self { } } -const SHORT_OPTIONS: &wstr = L!("+hn:siux:N:X:"); +const SHORT_OPTIONS: &wstr = L!("+hn:siux:SN:X:"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("stop-nonopt"), ArgType::NoArgument, 's'), wopt(L!("ignore-unknown"), ArgType::NoArgument, 'i'), wopt(L!("move-unknown"), ArgType::NoArgument, 'u'), wopt(L!("name"), ArgType::RequiredArgument, 'n'), wopt(L!("exclusive"), ArgType::RequiredArgument, 'x'), + wopt(L!("strict-longopts"), ArgType::NoArgument, 'S'), wopt(L!("help"), ArgType::NoArgument, 'h'), wopt(L!("min-args"), ArgType::RequiredArgument, 'N'), wopt(L!("max-args"), ArgType::RequiredArgument, 'X'), @@ -532,6 +534,7 @@ fn parse_cmd_opts<'args>( UnknownHandling::Move } } + 'S' => opts.strict_long_opts = true, // Just save the raw string here. Later, when we have all the short and long flag // definitions we'll parse these strings into a more useful data structure. 'x' => opts.raw_exclusive_flags.push(w.woptarg.unwrap()), @@ -873,6 +876,7 @@ fn argparse_parse_flags<'args>( populate_option_strings(opts, &mut short_options, &mut long_options); let mut w = WGetopter::new(&short_options, &long_options, args); + w.strict_long_opts = opts.strict_long_opts; while let Some((opt, longopt_idx)) = w.next_opt_indexed() { let is_long_flag = longopt_idx.is_some(); let retval: BuiltinResult = match opt { diff --git a/src/wgetopt.rs b/src/wgetopt.rs index 42cefe626..2511563ac 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -139,6 +139,9 @@ pub struct WGetopter<'opts, 'args, 'argarray> { pub last_nonopt: usize, /// Prevents redundant initialization. initialized: bool, + // Makes parsing long options more strict (long options must have their full name specified, + // and cannot be given with a single hyphen). + pub strict_long_opts: bool, /// This will be populated with the elements of the original args that were interpreted /// as options and arguments to options pub argv_opts: Vec>, @@ -162,6 +165,7 @@ pub fn new( first_nonopt: 0, last_nonopt: 0, initialized: false, + strict_long_opts: false, argv_opts: Vec::new(), } } @@ -438,6 +442,9 @@ fn find_matching_long_opt(&self, name_end: usize) -> LongOptMatch<'opts> { index = i; exact = true; break; + } else if self.strict_long_opts { + // It didn't match exactly, continue + continue; } else if opt.is_none() { // The option begins with the matching text, but is not // exactly equal. @@ -531,9 +538,9 @@ fn wgetopt_inner(&mut self, longopt_index: &mut usize) -> Option { } // We set things up so that `-f` is parsed as a short-named option if there - // is a valid option to match it to, otherwise we parse it as a long-named - // option. We also make sure that `-fu` is *not* parsed as `-f` with - // an arg `u`. + // is a valid option to match it to, otherwise, if self.strict_long_opts is false, + // we parse it as a long-named option. We also make sure that `-fu` is *not* parsed as + // `-f` with an arg `u`. if !self.longopts.is_empty() && self.wopt_index < self.argv.len() { let arg = self.argv[self.wopt_index]; @@ -541,7 +548,8 @@ fn wgetopt_inner(&mut self, longopt_index: &mut usize) -> Option { // matches options like `--foo` arg.char_at(0) == '-' && arg.char_at(1) == '-' // matches options like `-f` if `f` is not a valid shortopt. - || !self.shortopts.as_char_slice().contains(&arg.char_at(1)); + || (!self.strict_long_opts + && !self.shortopts.as_char_slice().contains(&arg.char_at(1))); if try_long { let retval = self.handle_long_opt(longopt_index); diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 4d2c21d9f..740ae38be 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -645,5 +645,35 @@ argparse a/alpha -- --banna=value argparse a/alpha -- --alpha=value --banna=value # CHECKERR: argparse: --alpha=value: option does not take an argument +# Check behaviour without -S/--strict-longopts option +begin + argparse long valu=+ -- --lon -long -lon -valu=3 -valu 4 -val=3 -val 4 + set -lL + # CHECK: _flag_long '--long' '--long' '--long' + # CHECK: _flag_valu '3' '4' '3' '4' + # CHECK: argv + # CHECK: argv_opts '--lon' '-long' '-lon' '-valu=3' '-valu' '4' '-val=3' '-val' '4' + argparse amb ambig -- -am + # CHECKERR: argparse: -am: unknown option + argparse a ambig -- -ambig + # CHECKERR: argparse: -ambig: unknown option + argparse long -- -long3 + # CHECKERR: argparse: -long3: unknown option +end + +# Check behaviour with -S/--strict-longopts option +begin + argparse -S long -- --lon + # CHECKERR: argparse: --lon: unknown option + argparse -S long -- -long + # CHECKERR: argparse: -long: unknown option + argparse --strict-longopts long -- -lon + # CHECKERR: argparse: -lon: unknown option + argparse -S value=+ -- -valu=3 + # CHECKERR: argparse: -valu=3: unknown option + argparse --strict-longopts value=+ -- -valu 4 + # CHECKERR: argparse: -valu: unknown option +end + # Check that the argparse's are properly wrapped in begin blocks set -l