argparse: mismatch between choices parsing and usage/error message

[cjerdonek]

BPO	16977
Nosy	@ericvsmith, @cjerdonek, @iritkatriel
Files	argparse.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-01-16.02:30:07.460>
labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
title = 'argparse: mismatch between choices parsing and usage/error message'
updated_at = <Date 2021-12-10.15:38:13.260>
user = 'https://github.com/cjerdonek'

bugs.python.org fields:

activity = <Date 2021-12-10.15:38:13.260>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2013-01-16.02:30:07.460>
creator = 'chris.jerdonek'
dependencies = []
files = ['28769']
hgrepos = []
issue_num = 16977
keywords = ['patch']
message_count = 13.0
messages = ['180068', '180069', '180136', '180153', '180187', '180197', '180208', '180216', '180218', '192254', '192295', '192715', '408213']
nosy_count = 5.0
nosy_names = ['eric.smith', 'chris.jerdonek', 'paul.j3', 'jeffknupp', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'test needed'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16977'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

• gh-61181: Fix support of choices with string value in argparse #124578
• [3.13] gh-61181: Fix support of choices with string value in argparse (GH-124578) #124755
• [3.12] gh-61181: Fix support of choices with string value in argparse (GH-124578) #124756

[cjerdonek]

When passing a string for the choices argument, argparse's usage and error messages differ from its behavior:

>>> p = argparse.ArgumentParser()
>>> p.add_argument('a', choices='abc')
>>> p.parse_args(['d'])
usage: [-h] {a,b,c}
: error: argument a: invalid choice: 'd' (choose from 'a', 'b', 'c')
>>> p.parse_args(['bc'])
Namespace(a='bc')

This is because argparse uses the "in" operator instead of sequence iteration to check whether an argument value is valid. Any resolution should also consider the behavior for string subclasses as well as perhaps bytes-like objects.

[cjerdonek]

I forgot to mention that argparse uses such cases as examples in its documentation (one of which was replaced in bddbaaf332d7).

[ericvsmith]

Isn't this really just an inappropriate use of a string instead of a list? If indeed this is in the documentation, it should be changed.

I still don't like:
>>> p.add_argument('a', choices=list('abc'))
but at least it would work.

This call to list() could be done internally, but I think passing in a string is a bad practice and argparse should not contain internal workarounds to cater to this usage.

If you're proposing that argparse should use sequence iteration instead of the "in" operator, I disagree with that solution.

[cjerdonek]

This wasn't just in the documentation. It was the *first* example of how to use the choices argument out of the two examples in that section (from the time Python first adopted argparse and before):

----
16.4.3.7. choices
Some command-line arguments should be selected from a restricted set of values. These can be handled by passing a container object as the choices keyword argument to add_argument(). When the command line is parsed, argument values will be checked, and an error message will be displayed if the argument was not one of the acceptable values:

>>> parser = argparse.ArgumentParser(prog='PROG')
>>> parser.add_argument('foo', choices='abc')
>>> parser.parse_args('c'.split())
Namespace(foo='c')
>>> parser.parse_args('X'.split())
usage: PROG [-h] {a,b,c}
PROG: error: argument foo: invalid choice: 'X' (choose from 'a', 'b', 'c')

(from http://hg.python.org/cpython/file/c0ddae67f4df/Doc/library/argparse.rst#l1021 )
----

So I think it's a bit late to say it's an inappropriate usage or bad practice.

To preserve backwards compatibility, I think we should use sequence iteration for strings, or equivalently apply "in" to iter(choices), set(choices), etc. when choices is a string. I don't think, however, that we should alter the choices attribute because that could break things for people:

>>> p = argparse.ArgumentParser()
>>> a = p.add_argument("letter", choices='abcd')
>>> a.choices
'abcd'

[cjerdonek]

There are also test cases with a string being passed for choices.

[jeffknupp]

Attached a patch. Rather than altering choices or making a special check for string instances, I just changed the if statement to

if action.choices is not None and value not in list(action.choices):

from

if action.choices is not None and value not in action.choices:

It has the added benefit of handling all sequence types correctly (rather than just strings). I tried to think of a case where this wouldn't work as expected, but wasn't able to.

[cjerdonek]

See bpo-16468 for why that won't work in general.

[jeffknupp]

The only time this would be an issue is for infinite sequences via range or a generator, which doesn't work anyway.

>>> p = argparse.ArgumentParser()
>>> a = p.add_argument('a', choices=itertools.count(0), type=int)
>>> p.parse_args(['10000'])
... hangs

Are there any other cases where coercing to a list wouldn't work?

[cjerdonek]

When choices isn't iterable but supports the "in" operator, e.g.

class MyChoices(object):
    def __contains__(self, item):
        return True

[paulj3]

test_argparse.py has some "choices='abc'" cases.

In those should "parser.parse_args(['--foo','bc'])" be considered a success or failure?

The underlying issue here is that while string iteration behaves like list iteration, string __contains__ looks for substrings, not just one character that matches. (String __contains__ also returns a TypeError if its argument is not a string.)

But other than removing poor examples in documentation and tests, I'm not sure this issue requires a change.

[paulj3]

Changing _check_value from:

    def _check_value(self, action, value):
        # converted value must be one of the choices (if specified)
        if action.choices is not None and value not in action.choices:
            ...

to

    def _check_value(self, action, value):
        # converted value must be one of the choices (if specified)
        if action.choices is not None:
            choices = action.choices
            if isinstance(choices, str):
                choices = list(choices)
            if value not in action.choices:
                ...

would correct the string search without affecting other types of choices.

[paulj3]

I just posted a patch to http://bugs.python.org/issue16468 that deals with this 'bc' in 'abc' issue.

[iritkatriel]

We could make the error message less wrong:

>>> p.parse_args(['d'])
usage: [-h] {a,b,c}
: error: argument a: invalid choice: 'd' (choose a value in 'abc')

% git diff
diff --git a/Lib/argparse.py b/Lib/argparse.py
index b44fa4f0f6..f03cc1f110 100644
--- a/Lib/argparse.py
+++ b/Lib/argparse.py
@@ -2499,8 +2499,8 @@ def _check_value(self, action, value):
         # converted value must be one of the choices (if specified)
         if action.choices is not None and value not in action.choices:
             args = {'value': value,
-                    'choices': ', '.join(map(repr, action.choices))}
-            msg = _('invalid choice: %(value)r (choose from %(choices)s)')
+                    'choices': repr(action.choices)}
+            msg = _('invalid choice: %(value)r (choose a value in %(choices)s)')
             raise ArgumentError(action, msg % args)
 
     #
```=======================

[serhiy-storchaka]

This can even be classified as a minor security issue. The current behavior is most likely not what most of users mean. It allows invalid data to pass the primary validation. Examples with string choices is present in the documentation and tests, so many users can use it unaware of the flaw.

#124578 converts the string choices value to list, adds tests, and updates examples.

There is a similar issue with bytes and bytearray, but it has much less chance to occur (you need a special type converter that can return int and bytes), so I leave it as is.