argparse: subparser is required even with `required=False`

[pony1k]

Bug report

When adding a positional argument with nargs='+' and a subparsers after this, the subparsers is required even though it shouldn't be.

import argparse

parser = argparse.ArgumentParser()
parser.add_argument('foo', nargs='+')
subparsers = parser.add_subparsers(required=False)
sub1 = subparsers.add_parser('sub1')
sub1.add_argument('arg1')
sub2 = subparsers.add_parser('sub2')
sub2.add_argument('arg2')

parser.parse_args(['a','b','c'])
# usage: arg_parse_bug_minimal.py [-h] foo [foo ...] {sub1,sub2} ...
# arg_parse_bug_minimal.py: error: invalid choice: 'c' (choose from 'sub1', 'sub2')

In the above example, I would expect foo to consume all remaining arguments. In ArgumentParser._get_nargs_pattern I found this:

        # allow one argument followed by any number of options or arguments
        elif nargs == PARSER:
            nargs_pattern = '(-*A[-AO]*)'

The comment sais allow one argument, but the pattern actually requires one.

Your environment

• CPython versions tested on: python 3.10
• Operating system and architecture: OpenSuse Tumbleweed x86_64

[liam-gersten]

Hello, my name is Liam, and I’m part of a student software engineer group of 4 at Carnegie Mellon. We’ve been tasked with contributing to an open source project, and this issue seems like a great first start. Would you mind assigning this to me (my group)? Also, you might see the same comment on 1-2 other issues, since we’ll likely be working on more than just one for our assignment.

[hpaulj]

Liam, I don't think argparse issues are formally assigned to people, at least not on a regular basis. You are welcome to explore the issue, and propose a fix here, or formally as a push.

I commented on this issue recently on StackOverFlow

https://stackoverflow.com/questions/75930563/how-can-i-implement-an-argument-parser-with-a-variable-number-of-positional-argu

Subparsers is a positional argument. Normally if we define:

   parser.add_argument('foo', nargs='*')
   parser.add_argument('bar', choices=['A','B'])

then python prog x' will produce an 'invalid choice' error, because it allocates []tofoo, and x' to bar. If we provide 'a b c x', ['a','b','c'] go to foo, and again, the last string to bar. positional arguments are assigned by position, not value. The test against choices is done after assignment. The allocation isn't conditional on the choices match.

In contrast to my example bar, a subparser isn't 'required', but that's not because it has a '?' or '*' nargs, but because of a quirk in how it's required attribute is assigned, and later tested. There's a long history of bug/issues surrounding this.

In short, regardless of what it's required attribute is, a subparser argument will get the 'last' string, and test that against its choices. Whether that's a bug or not is debatable. Any attempt to change that behavior needs to carefully chosen, and thoroughly tested. It's won't be a trivial fix.

[hpaulj]

Some implementation details that we need to be aware of:

 class _SubParsersAction(Action):
       def __init__(...
              required=False, ...
              ,,,,
             super .... 
                   nargs=PARSER,

So while the subparsers Action gets a default/user defined required value, it also gets a argparse.PARSER nargs.

For an ordinary positional, required is set based on the nargs:

 def _get_positional_kwargs(self, dest, **kwargs):
     ...
    if kwargs.get('nargs') not in [OPTIONAL, ZERO_OR_MORE]:
        kwargs['required'] = True
    if kwargs.get('nargs') == ZERO_OR_MORE and 'default' not in kwargs:
        kwargs['required'] = True

option_strings are allocated to Actions by

 def _get_nargs_pattern(self, action):

    # allow one argument followed by any number of options or arguments
    elif nargs == PARSER:
        nargs_pattern = '(-*A[-AO]*)'

This is a lot like the ONE_OR_MORE pattern ' (-A[A-])' (+) (in fact its string value is '+...'), REMAINDER with a required first non-flag string. It's this pattern that determines how strings are allocated between the preceding '' positional and the subparser argument. It may be possible change this allocation to behave more like '?' or '' (optional), but this is such a fundamental part of parsing that any changes have to be done with considerable care.

argparse.PARSER is again check in:

def _get_values(self, action, arg_strings):
    ....
   # PARSER arguments convert all values, but check only the first
    elif action.nargs == PARSER:
        value = [self._get_value(action, v) for v in arg_strings]
        self._check_value(action, value[0])

It's check_value that compares the string with the choices (which for subparsers are the allowable parser names). Again this is a fundamental part of parsing, so changes could cause a lot of problems

The action.required attribute is not checked during parsing, until the end of

def _parse_known_args(self, arg_strings, namespace):

when it scans through all _actions and checks whether anything with a required True value is in the "has been seen" list.

Subparsers were required until this new means of checking for missing arguments was added (the previous code just complained that some, unnamed, arguments were missing). The current required check together with the default required=False is what made optional subparsers possible. Even so we had make a couple of more patches to allow the user to set the required attribute, and allow correct reporting of a missing subparser when the dest was not set.

In summary, to the main parser, subparsers are just another positional with a unique argparse.PARSER nargs. It is processed just a like all the other positionals. Nearly all the special subparser action is performed by the _SubParsersAction class itself. It's the class __call__ that passes all remaining argv strings to the new parser, and gets back a namespace.

[hpaulj]

Even if we make the subparsers narg behave more like the ? non-required positional it will have problems. With a sequence of *? positionals, the * is greedy, and leaves nothing for the ?.

In the linked StackOverFlow I show how changing the '*' to a flagged argument sort of works, but we still need to use some other flagged argument to terminate its list of strings.

As long as subparsers is handled a a positional, it will impossible to change the current behavior. Using a * argument is tricky - it has to be last, or its end has to be clearly marked. Another positional will not work.

I think this issue should be closed.

[serhiy-storchaka]

I think that the following patch can make a parser with required=False truly non-required.

diff --git a/Lib/argparse.py b/Lib/argparse.py
index 66192fb9283..48c41df299e 100644
--- a/Lib/argparse.py
+++ b/Lib/argparse.py
@@ -1192,6 +1192,8 @@ def _get_subactions(self):
         return self._choices_actions
 
     def __call__(self, parser, namespace, values, option_string=None):
+        if not values:
+            return
         parser_name = values[0]
         arg_strings = values[1:]
 
@@ -2069,7 +2071,7 @@ def consume_positionals(start_index):
                 args = arg_strings[start_index: start_index + arg_count]
                 # Strip out the first '--' if it is not in REMAINDER arg.
                 if action.nargs == PARSER:
-                    if arg_strings_pattern[start_index] == '-':
+                    if arg_count and arg_strings_pattern[start_index] == '-':
                         assert args[0] == '--'
                         args.remove('--')
                 elif action.nargs != REMAINDER:
@@ -2367,7 +2369,10 @@ def _get_nargs_pattern(self, action):
 
         # allow one argument followed by any number of options or arguments
         elif nargs == PARSER:
-            nargs_pattern = '(-*A[-AO]*)'
+            if action.required:
+                nargs_pattern = '(-*A[-AO]*)'
+            else:
+                nargs_pattern = '(-*(?:A[-AO]*)?)'
 
         # suppress action, like nargs=0
         elif nargs == SUPPRESS:
@@ -2512,7 +2517,8 @@ def _get_values(self, action, arg_strings):
         # PARSER arguments convert all values, but check only the first
         elif action.nargs == PARSER:
             value = [self._get_value(action, v) for v in arg_strings]
-            self._check_value(action, value[0])
+            if action.required or value:
+                self._check_value(action, value[0])
 
         # SUPPRESS argument does not put anything in the namespace
         elif action.nargs == SUPPRESS:

All tests passed with this change... and that's the issue. It does not make difference in normal cases, and I have no cases in which the difference would be desirable. The initial example would not work, because nargs='*' (as well as nargs='?') is greedy. So you should consider only cases with fixed number of positional arguments preceding the command. But they already work with the current code.

Adding a magic which would stop nargs='*' and nargs='?' when potential valid command is found requires too much efforts, and the resulting interface would be error prone. So I agree with @hpaul on closing this issue.