gh-53584: Prevent variable-nargs options from stealing required positional args by stephenfin · Pull Request #146513 · python/cpython · GitHub
Skip to content

gh-53584: Prevent variable-nargs options from stealing required positional args #146513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stephenfin wants to merge 2 commits into
base: main
Choose a base branch
from
+197 −0
Open

gh-53584: Prevent variable-nargs options from stealing required positional args #146513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2c4dfa
gh-53584: Add reproducers for greedy opts issue
stephenfin Mar 26, 2026
ec14116
gh-53584: Prevent variable-nargs options from stealing required posit…
stephenfin Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions Lib/argparse.py
View file
Open in desktop
101 changes: 101 additions & 0 deletions Lib/test/test_argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,17 @@ class TestOptionalsNargs3(ParserTestCase):
('-x a b c', NS(x=['a', 'b', 'c'])),
]

@savannahostrowski savannahostrowski Apr 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add at least one more test for something like ['--foo', '--', 'abc'] with nargs='?' and a required positional, I think.

@stephenfin stephenfin Apr 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll tack this on.

def test_does_not_steal_required_positional(self):
# https://github.com/python/cpython/issues/53584

@savannahostrowski savannahostrowski Apr 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove the GitHub issue comment on each test? It's a bit noisy.

@stephenfin stephenfin Apr 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. I was copying what I'd seen in some other tests here.

# Fixed integer nargs is never greedy: --foo takes exactly N args and
# leaves the remainder for positionals.
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs=2)
parser.add_argument('bar')
args = parser.parse_args(['--foo', 'a', 'b', 'c'])
self.assertEqual(args.foo, ['a', 'b'])
self.assertEqual(args.bar, 'c')


class TestOptionalsNargsOptional(ParserTestCase):
"""Tests specifying an Optional arg for an Optional"""
Expand All @@ -687,6 +698,48 @@ class TestOptionalsNargsOptional(ParserTestCase):
('-z 2', NS(w=None, x=None, y='spam', z=2)),
]

def test_does_not_steal_required_positional(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='?')
parser.add_argument('bar')
args = parser.parse_args(['--foo', 'abc'])
self.assertIsNone(args.foo)
self.assertEqual(args.bar, 'abc')

def test_does_not_steal_two_required_positionals(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='?')
parser.add_argument('bar')
parser.add_argument('bax')
args = parser.parse_args(['--foo', 'a', 'b'])
self.assertIsNone(args.foo)
self.assertEqual(args.bar, 'a')
self.assertEqual(args.bax, 'b')

def test_does_not_steal_two_required_positional_interspersed(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='?')
parser.add_argument('--bar', nargs='?')
parser.add_argument('baz')
parser.add_argument('bax')
args = parser.parse_args(['--foo', 'a', '--bar', 'b'])
self.assertEqual(args.baz, 'a')

@savannahostrowski savannahostrowski Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make sure that every test asserts both positionals and optionals? We should check to make sure that foo and bar ended up with the correct values. There are several tests that would benefit from this.

@stephenfin stephenfin Apr 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do.

self.assertEqual(args.bax, 'b')

def test_greedy_when_positional_also_optional(self):
# https://github.com/python/cpython/issues/53584
# When the following positional is also optional, --foo taking the arg
# is acceptable (both are optional, no unambiguous correct answer).
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='?')
parser.add_argument('bar', nargs='?')
args = parser.parse_args(['--foo', 'abc'])
self.assertEqual(args.foo, 'abc')
self.assertIsNone(args.bar)


class TestOptionalsNargsZeroOrMore(ParserTestCase):
"""Tests specifying args for an Optional that accepts zero or more"""
Expand All @@ -706,6 +759,15 @@ class TestOptionalsNargsZeroOrMore(ParserTestCase):
('-y a b', NS(x=None, y=['a', 'b'])),
]

def test_does_not_steal_required_positional(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='*')
parser.add_argument('bar')
args = parser.parse_args(['--foo', 'abc'])
self.assertEqual(args.foo, [])
self.assertEqual(args.bar, 'abc')


class TestOptionalsNargsOneOrMore(ParserTestCase):
"""Tests specifying args for an Optional that accepts one or more"""
Expand All @@ -723,6 +785,27 @@ class TestOptionalsNargsOneOrMore(ParserTestCase):
('-y a b', NS(x=None, y=['a', 'b'])),
]

def test_does_not_steal_required_positional(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='+')
parser.add_argument('bar')
args = parser.parse_args(['--foo', 'a', 'b'])
self.assertEqual(args.foo, ['a'])
self.assertEqual(args.bar, 'b')

def test_does_not_steal_interspersed_required_positional(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='+')
parser.add_argument('--bar', nargs='+')
parser.add_argument('baz')
parser.add_argument('bax')
args = parser.parse_args(['--foo', 'a', 'c', '--bar', 'b', 'd'])
self.assertEqual(args.foo, ['a'])
self.assertEqual(args.bar, ['b'])
self.assertEqual(args.baz, 'c')
self.assertEqual(args.bax, 'd')

class TestOptionalsChoices(ParserTestCase):
"""Tests specifying the choices for an Optional"""
Expand Down Expand Up @@ -6760,6 +6843,24 @@ def test_invalid_args(self):
parser = ErrorRaisingArgumentParser(prog='PROG')
self.assertRaises(ArgumentParserError, parser.parse_intermixed_args, ['a'])

def test_variable_nargs_option_before_positional(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='?')
parser.add_argument('bar')
args = parser.parse_intermixed_args(['--foo', 'abc'])
self.assertIsNone(args.foo)
self.assertEqual(args.bar, 'abc')

def test_variable_nargs_option_after_positional(self):
# https://github.com/python/cpython/issues/53584
parser = ErrorRaisingArgumentParser()
parser.add_argument('--foo', nargs='?')
parser.add_argument('bar')
args = parser.parse_intermixed_args(['abc', '--foo', 'xyz'])
self.assertEqual(args.foo, 'xyz')
self.assertEqual(args.bar, 'abc')


class TestIntermixedMessageContentError(TestCase):
# case where Intermixed gives different error message
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2026-03-27-12-54-44.gh-issue-53584.4mA7Wp.rst
View file
Open in desktop
Loading