gh-53584: Prevent variable-nargs options from stealing required positional args#146513
gh-53584: Prevent variable-nargs options from stealing required positional args#146513stephenfin wants to merge 2 commits intopython:mainfrom
Conversation
Most of these are marked as expected fail for now, pending the fix. Signed-off-by: Stephen Finucane <[email protected]>
… positional args
Options with nargs='?', nargs='*', or nargs='+' were greedily consuming
argument strings that should have been reserved for required positional
arguments. For example:
parser.add_argument('--foo', nargs='?')
parser.add_argument('bar')
parser.parse_args(['--foo', 'abc']) # bar got nothing
argparse works by pattern-matching, with the bulk of its logic defined
in _parse_known_args(). That method encodes each argument string as a
single character ('O' for a recognised option flag, 'A' for everything
else, and '-' for strings following '--') which gives us a resulting
string pattern (e.g. 'OAOA'). We then "consume" arguments using this
string pattern, handling both positionals (via the consume_positionals()
closure) and optionals (via consume_optional()).
The bug arises in the latter. consume_optional() processes options by
building a regex based on the option's nargs (e.g. '-*A?-*' for
nargs='?') and then runs this regex against the remainder of the string
pattern (i.e. anything following the option flag). This is done via
_match_argument(). The regex will always stop at the next option flag
('O' token) but for non-fixed nargs values like '?' and '+' may greedily
consume every positional ('A' token) up to that point.
This fix works by manipulating the string pattern as part of our
optional consumption. Any 'A' tokens that are required by remaining
positionals are masked to 'O' to prevent the regex consuming them.
Masking will only consider tokens up to the next option flag ('O') and
it both accounts for what future options can absorb (to avoid masking
more than is necessary) and ensures that at least the minimum arguments
required for the optional are actually consumed.
In addition, we also handle the parse_intermixed_args() case. In
intermixed mode, positional-typed arguments already collected to the
left of the current option are also accounted for, since they will
satisfy positionals in the second-pass parse.
Signed-off-by: Stephen Finucane <[email protected]>
|
Hiya @stephenfin, shouldn't we pre calculate the required positional counts instead of re scanning them inside the optional loop? I am a little worried about |
There was a problem hiding this comment.
Thanks for taking a look at this. I've tested this very extensively and I think it looks pretty solid from a UX perspective.
From a performance perspective, I ran a stress-test benchmark (all nargs='?' options with a required positional) scaling from 10 to 100 options. Even at 100 options, it ran in under 1ms, and realistically, CLIs have far, far fewer options. IMO, this is not a practical concern, but we could explore some optimizations like precomputing the sorted option indices?
Aside from this, I will say that any bigger change to argparse makes me a bit uneasy. Mainly because people are likely working around this bug and this will be a silent behaviour change (i.e. existing programs will parse differently without any error or warning). We definitely need to add a What's New entry (not just a NEWS blurb) calling this out, so users upgrading to 3.15 are aware.
I also left a few comments below.
| parser.add_argument('baz') | ||
| parser.add_argument('bax') | ||
| args = parser.parse_args(['--foo', 'a', '--bar', 'b']) | ||
| self.assertEqual(args.baz, 'a') |
There was a problem hiding this comment.
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.
| ] | ||
|
|
||
| def test_does_not_steal_required_positional(self): | ||
| # https://github.com/python/cpython/issues/53584 |
There was a problem hiding this comment.
Can you please remove the GitHub issue comment on each test? It's a bit noisy.
There was a problem hiding this comment.
Will do. I was copying what I'd seen in some other tests here.
| extras_arg_count = sum( | ||
| 1 for c in extras_pattern if c == 'A' | ||
| ) | ||
| min_pos = max(0, min_pos - extras_arg_count) |
There was a problem hiding this comment.
| extras_arg_count = sum( | |
| 1 for c in extras_pattern if c == 'A' | |
| ) | |
| min_pos = max(0, min_pos - extras_arg_count) | |
| min_pos = max(0, min_pos - extras_pattern.count('A')) | |
| @@ -664,6 +664,17 @@ class TestOptionalsNargs3(ParserTestCase): | |||
| ('-x a b c', NS(x=['a', 'b', 'c'])), | |||
| ] | |||
|
|
|||
There was a problem hiding this comment.
We should add at least one more test for something like ['--foo', '--', 'abc'] with nargs='?' and a required positional, I think.
There was a problem hiding this comment.
Makes sense. I'll tack this on.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@savannahostrowski Thanks for the review. I'm trying to find a half day to come back and make the necessary changes for things you've called out, but in the interim...
🥳 This is something like my 5th attempt at this over the years 😅 (dating back to
Can do, though I should note that I'm already unhappy with how complicated this is (a necessary evil to avoid changing any APIs or general behavior) and would be nervous to increase that complexity further. I'll see what I can do.
It's a fair concern, but I should emphasise here that the existing greedy behavior always resulted in an error in the corner cases this bug focuses on. I've intentionally structured the PR with two commits to indicate this (in case you looked at the overall change rather than the per-commit view, the first commit adds tests with
Should I make this change here in the PR or can I do it in a follow-up PR? I know that changes can take some time to merge, and I'm nervous this will introduce a high likelihood of merge conflicts (since that file presumably changes quite often) and put me on merge/rebase treadmill. I also assume this request means you're thinking this is likely not a viable target for backporting? |
We usually do it in the same PR.
We usually merge, conflicts in what's new (if any) are easy to resolve, fear not. |
argparse supports options with variable numbers of args. gh-53584 tracks a long-standing bug where options with a non-fixed number of args (
nargs='?',nargs='*', ornargs='+') will greedily consume argument strings that should have been reserved for required positional arguments. For example:argparse works by pattern-matching, with the bulk of its logic defined in
_parse_known_args(). That method encodes each argument string as a single character ('O'for a recognised option flag,'A'for everything else, and'-'for strings following--) which gives us a resulting string pattern (e.g.'OAOA'). We then "consume" arguments using this string pattern, handling both positionals (via theconsume_positionals()closure) and optionals (viaconsume_optional()).The bug arises in the latter.
consume_optional()processes options by building a regex based on the option's nargs (e.g.'-*A?-*'fornargs='?') and then runs this regex against the remainder of the string pattern (i.e. anything following the option flag). This is done via_match_argument(). The regex will always stop at the next option flag ('O'token) but for non-fixed nargs values like'?'and'+'may greedily consume every positional ('A'token) up to that point.This fix works by manipulating the string pattern as part of our optional consumption. Any
'A'tokens that are required by remaining positionals are masked to'O'to prevent the regex consuming them. Masking will only consider tokens up to the next option flag ('O') and it both accounts for what future options can absorb (to avoid masking more than is necessary) and ensures that at least the minimum arguments required for the optional are actually consumed.In addition, we also handle the
parse_intermixed_args()case. In intermixed mode, positional-typed arguments already collected to the left of the current option are also accounted for, since they will satisfy positionals in the second-pass parse.Note that this is a rather gnarly issue, and I've done my best to avoid changing the API behavior of the module without layering on too much additional complexity, in the hope that this might actually be backportable. Hopefully my proposed approach is sound but I'm happy to iterate on this if there's something I've missed or there is a better way to do this.