GeezerVison is showing github.com — argparse: mismatch between choices parsing and usage/error message · Issue #61181 · python/cpython
argparse: mismatch between choices parsing and usage/error message #61181Description
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
Activitycjerdonek commented on Jan 16, 2013 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 commented on Jan 16, 2013 I forgot to mention that argparse uses such cases as examples in its documentation (one of which was replaced in bddbaaf332d7). ericvsmith commented on Jan 17, 2013 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 commented on Jan 17, 2013 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 commented on Jan 18, 2013 There are also test cases with a string being passed for choices. jeffknupp commented on Jan 18, 2013 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 commented on Jan 18, 2013 See bpo-16468 for why that won't work in general. jeffknupp commented on Jan 18, 2013 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'])
... hangsAre there any other cases where coercing to a list wouldn't work? cjerdonek commented on Jan 18, 2013 When choices isn't iterable but supports the "in" operator, e.g.
class MyChoices(object):
def __contains__(self, item):
return Truepaulj3 commented on Jul 3, 2013 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 commented on Jul 4, 2013 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 commented on Jul 9, 2013 iritkatriel commented on Dec 10, 2021 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 commented on Sep 26, 2024 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.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|