From f86e3bac9f396855885c7b37533d4bd58a5282a4 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 1 Oct 2024 13:16:05 +0300 Subject: [PATCH 1/3] gh-85935: Check for nargs=0 for positional arguments in argparse Raise ValueError in add_argument() if either explicit nargs=0 or action that does not consume arguments (like 'store_const' or 'store_true') is specified for positional argument. --- Lib/argparse.py | 10 ++- Lib/test/test_argparse.py | 80 +++++++++++-------- ...4-10-01-13-11-53.gh-issue-85935.CTwJUy.rst | 4 + 3 files changed, 58 insertions(+), 36 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst diff --git a/Lib/argparse.py b/Lib/argparse.py index 874f271959c4fe..5d7ecef55013d0 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -1426,11 +1426,17 @@ def add_argument(self, *args, **kwargs): kwargs['default'] = self.argument_default # create the action object, and add it to the parser + action_name = kwargs.get('action') action_class = self._pop_action_class(kwargs) if not callable(action_class): raise ValueError('unknown action "%s"' % (action_class,)) action = action_class(**kwargs) + # raise an error if action for positional argument does not + # consume arguments + if not action.option_strings and action.nargs == 0: + raise ValueError(f'action {action_name!r} is not valid for positional arguments') + # raise an error if the action type is not callable type_func = self._registry_get('type', action.type, action.type) if not callable(type_func): @@ -1534,7 +1540,9 @@ def _get_positional_kwargs(self, dest, **kwargs): # mark positional arguments as required if at least one is # always required nargs = kwargs.get('nargs') - if nargs not in [OPTIONAL, ZERO_OR_MORE, REMAINDER, SUPPRESS, 0]: + if nargs == 0: + raise ValueError('nargs for positionals must be != 0') + if nargs not in [OPTIONAL, ZERO_OR_MORE, REMAINDER, SUPPRESS]: kwargs['required'] = True # return the keyword arguments with no option strings diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index a972ed0cc9053b..2b0d83d29a88c1 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -5262,15 +5262,15 @@ def custom_formatter(prog): class TestInvalidArgumentConstructors(TestCase): """Test a bunch of invalid Argument constructors""" - def assertTypeError(self, *args, **kwargs): + def assertTypeError(self, *args, errmsg=None, **kwargs): parser = argparse.ArgumentParser() - self.assertRaises(TypeError, parser.add_argument, - *args, **kwargs) + self.assertRaisesRegex(TypeError, errmsg, parser.add_argument, + *args, **kwargs) - def assertValueError(self, *args, **kwargs): + def assertValueError(self, *args, errmsg=None, **kwargs): parser = argparse.ArgumentParser() - self.assertRaises(ValueError, parser.add_argument, - *args, **kwargs) + self.assertRaisesRegex(ValueError, errmsg, parser.add_argument, + *args, **kwargs) def test_invalid_keyword_arguments(self): self.assertTypeError('-x', bar=None) @@ -5280,8 +5280,9 @@ def test_invalid_keyword_arguments(self): def test_missing_destination(self): self.assertTypeError() - for action in ['append', 'store']: - self.assertTypeError(action=action) + for action in ['store', 'append', 'extend']: + with self.subTest(action=action): + self.assertTypeError(action=action) def test_invalid_option_strings(self): self.assertValueError('--') @@ -5298,10 +5299,8 @@ def test_invalid_action(self): self.assertValueError('-x', action='foo') self.assertValueError('foo', action='baz') self.assertValueError('--foo', action=('store', 'append')) - parser = argparse.ArgumentParser() - with self.assertRaises(ValueError) as cm: - parser.add_argument("--foo", action="store-true") - self.assertIn('unknown action', str(cm.exception)) + self.assertValueError('--foo', action="store-true", + errmsg='unknown action') def test_multiple_dest(self): parser = argparse.ArgumentParser() @@ -5314,39 +5313,50 @@ def test_multiple_dest(self): def test_no_argument_actions(self): for action in ['store_const', 'store_true', 'store_false', 'append_const', 'count']: - for attrs in [dict(type=int), dict(nargs='+'), - dict(choices=['a', 'b'])]: - self.assertTypeError('-x', action=action, **attrs) + with self.subTest(action=action): + for attrs in [dict(type=int), dict(nargs='+'), + dict(choices=['a', 'b'])]: + with self.subTest(attrs=attrs): + self.assertTypeError('-x', action=action, **attrs) + self.assertTypeError('x', action=action, **attrs) + self.assertValueError('x', action=action, + errmsg=f"action '{action}' is not valid for positional arguments") + self.assertTypeError('-x', action=action, nargs=0) + self.assertValueError('x', action=action, nargs=0, + errmsg='nargs for positionals must be != 0') def test_no_argument_no_const_actions(self): # options with zero arguments for action in ['store_true', 'store_false', 'count']: + with self.subTest(action=action): + # const is always disallowed + self.assertTypeError('-x', const='foo', action=action) - # const is always disallowed - self.assertTypeError('-x', const='foo', action=action) - - # nargs is always disallowed - self.assertTypeError('-x', nargs='*', action=action) + # nargs is always disallowed + self.assertTypeError('-x', nargs='*', action=action) def test_more_than_one_argument_actions(self): - for action in ['store', 'append']: - - # nargs=0 is disallowed - self.assertValueError('-x', nargs=0, action=action) - self.assertValueError('spam', nargs=0, action=action) - - # const is disallowed with non-optional arguments - for nargs in [1, '*', '+']: - self.assertValueError('-x', const='foo', - nargs=nargs, action=action) - self.assertValueError('spam', const='foo', - nargs=nargs, action=action) + for action in ['store', 'append', 'extend']: + with self.subTest(action=action): + # nargs=0 is disallowed + action_name = 'append' if action == 'extend' else action + self.assertValueError('-x', nargs=0, action=action, + errmsg=f'nargs for {action_name} actions must be != 0') + self.assertValueError('spam', nargs=0, action=action, + errmsg='nargs for positionals must be != 0') + + # const is disallowed with non-optional arguments + for nargs in [1, '*', '+']: + self.assertValueError('-x', const='foo', + nargs=nargs, action=action) + self.assertValueError('spam', const='foo', + nargs=nargs, action=action) def test_required_const_actions(self): for action in ['store_const', 'append_const']: - - # nargs is always disallowed - self.assertTypeError('-x', nargs='+', action=action) + with self.subTest(action=action): + # nargs is always disallowed + self.assertTypeError('-x', nargs='+', action=action) def test_parsers_action_missing_params(self): self.assertTypeError('command', action='parsers') diff --git a/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst b/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst new file mode 100644 index 00000000000000..1938431751749f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst @@ -0,0 +1,4 @@ +:meth:`argparse.ArgumentParser.add_argument` now raises exception if +:ref:`action` that does not consume arguments (like 'store_const' or +'strore_true') or explicit ``nargs=0`` are specified for positional +arguments. From b27771b65365031d0960f779e04543f614f81f47 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 11 Oct 2024 10:46:52 +0300 Subject: [PATCH 2/3] Update Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst Co-authored-by: Savannah Ostrowski --- .../Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst b/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst index 1938431751749f..f81593610c236f 100644 --- a/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst +++ b/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst @@ -1,4 +1,4 @@ -:meth:`argparse.ArgumentParser.add_argument` now raises exception if -:ref:`action` that does not consume arguments (like 'store_const' or -'strore_true') or explicit ``nargs=0`` are specified for positional +:meth:`argparse.ArgumentParser.add_argument` now raises an exception if +an :ref:`action` that does not consume arguments (like 'store_const' or +'store_true') or explicit ``nargs=0`` are specified for positional arguments. From ffed4fdbb9c3fbaecf8d6e814d431ec51d0baef7 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 11 Oct 2024 11:42:28 +0300 Subject: [PATCH 3/3] Update 2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst --- .../next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst b/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst index f81593610c236f..553f206bf26337 100644 --- a/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst +++ b/Misc/NEWS.d/next/Library/2024-10-01-13-11-53.gh-issue-85935.CTwJUy.rst @@ -1,4 +1,4 @@ -:meth:`argparse.ArgumentParser.add_argument` now raises an exception if +:meth:`argparse.ArgumentParser.add_argument` now raises an exception if an :ref:`action` that does not consume arguments (like 'store_const' or 'store_true') or explicit ``nargs=0`` are specified for positional arguments.