argparse: positional with type=int, default=SUPPRESS raise ValueError

@n8falke

BPO	36078
Nosy	@n8falke, @yangyangxcf

^{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 2019-02-22.14:18:34.731>
labels = ['3.8', 'type-bug', 'library']
title = 'argparse: positional with type=int, default=SUPPRESS raise ValueError'
updated_at = <Date 2020-03-16.16:20:47.351>
user = 'https://github.com/n8falke'

bugs.python.org fields:

activity = <Date 2020-03-16.16:20:47.351>
actor = 'paul.j3'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-02-22.14:18:34.731>
creator = 'n8falke'
dependencies = []
files = []
hgrepos = []
issue_num = 36078
keywords = []
message_count = 9.0
messages = ['336314', '336320', '336340', '336347', '336524', '359955', '360224', '364328', '364335']
nosy_count = 4.0
nosy_names = ['paul.j3', 'n8falke', 'fhsxfhsx', 'jeyekomon']
pr_nums = []
priority = 'high'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36078'
versions = ['Python 3.8']

Linked PRs

Example source:
from argparse import ArgumentParser, SUPPRESS
==============

parser = ArgumentParser()
parser.add_argument('i', nargs='?', type=int, default=SUPPRESS)
args = parser.parse_args([])

==============
results in:
error: argument integer: invalid int value: '==SUPPRESS=='

Expected: args = Namespace()

In Lib/argparse.py:
line 2399 in _get_value: result = type_func(arg_string)
with arg_string = SUPPRESS = '==SUPPRESS=='

called by ... line 1836 in take_action: argument_values = self._get_values(action, argument_strings)
which is done before checking for SUPPRESS in line 1851:
if argument_values is not SUPPRESS:
action(...)

Some more details:
The problem is not the order of assignment in take_action:
Defaults have been set by:
def parse_known_args(self, args=None, namespace=None):
...
# add any action defaults that aren't present
for action in self._actions:
if action.dest is not SUPPRESS:
if not hasattr(namespace, action.dest):
if action.default is not SUPPRESS:
setattr(namespace, action.dest, action.default)

Assignment without argument should not happen, like the example shows:
==============

from argparse import ArgumentParser
parser = ArgumentParser()
parser.add_argument('i', action="count", default=42)
args = parser.parse_args([])
print(repr(args))
==============
Namespace(i=43)

==============

Defaults are handled into two stages.

At the start of parsing defaults are added to the Namespace.

At the end of parsing intact defaults are evaluated with 'type'.

But a nargs='?' positional gets special handling. It matches an empty string, so it is always 'seen'. If its default is not None, that default is put in the Namespace instead of the matching empty list.

It's this special default handling that lets us use a ?-positional in a mutually exclusive group.

I suspect the error arises from this special default handling, but I'll have to look at the code to verify the details.

By defining a custom 'type' function:

    def foo(astr):
        if astr is argparse.SUPPRESS:
            raise KeyError
        return astr

I get the full traceback

1831 def take_action(action, argument_strings, option_string=None):
1832 seen_actions.add(action)
-> 1833 argument_values = self._get_values(action, argument_strings)

and in '_get_values' the error is produced when it calls '_get_value' (which runs the 'type' function):

        # optional argument produces a default when not present
        if not arg_strings and action.nargs == OPTIONAL:
            if action.option_strings:
                value = action.const
            else:
                value = action.default
            if isinstance(value, str):
 -->            value = self._get_value(action, value)
                self._check_value(action, value)
  
It identifies this as an OPTIONAL action that has received an empty argument list, and assigns it the action.default.

ZERO_OR_MORE * also gets the action.default, but without a _get_value() call. That default can be SUPPRESSed by the test at the end of take_action.

A couple of fixes come to mind:

add a SUPPRESS test at the start of take_action
add a SUPPRESS test to _get_values block I quote above, maybe bypassing the _get_value call

There is a unittest case of a suppressed optional positional; it just doesn't also test for a failed type.

class TestDefaultSuppress(ParserTestCase):
    """Test actions with suppressed defaults"""

    argument_signatures = [
        Sig('foo', nargs='?', default=argparse.SUPPRESS)

I'm inclined go with the second choice, but the alternatives need to be throughly tested.

In the mean time, an 'int' type could be replaced with one that is SUPPRESS knowledgeable:

    def bar(astr):
        if astr is argparse.SUPPRESS:
            return astr
        else:
            return int(astr)

Note that this use of action.default is different from the normal default handling at the start of parse_known_args (and the end of _parse_known_args). It's specifically for positionals that will always be 'seen' (because an empty argument strings list satisfies their nargs).

Thanks for so fast looking into this.

Good idea to use the workaround with a own conversion function. I'll use this for now.

To see what's happening, I tried a own Action with print in __call__ and a own conversion function with printing. I found following workflow:

direct assignment of unconverted default value (if not SUPPRESS, in parse_known_args)
conversion of argument string into given type
call to Action.__call__ which sets the converted value
only in edge cases: Convert default into given type and set in target

When there is no option there is only:
default | arg, narg = '?' | --opt | arg, narg = '*'
-----------------------------------------------------
None | 1), 3) | 1) | 1), 2) with []
SUPPRESS | 2)! | |
str | 1), 2), 3) | 1) | 1), 2)
not str* | 1), 3) | 1), 4) | 1), 2)

*can be int, float or other calss

It gets more complex the deeper I get into the source...

Yes, your second choice has probably less side effects.

I ran into the same issue and looked into the code, and found it more complicated than I thought. The more I went on, more issues occur. I wonder if I should open a new issue, but I will first comment here. If you feel like this should be a new issue, I will open one then. And I apologize in advance for possible vaguenesses in this comment because I modified it several times as I explored the code and found more issues. (also because of my poor English):)

It seems the issue happens only on positional arguments but not optional ones. Empty optional arguments will not call take_action and default values are handled and converted after consuming all arguments.

It also leads to inconsistancy between positional and optional arguments behaviour. Positional arguments always go through take_action, but optional arguments don't if an argument doesn't appear.

This inconsistancy causes another I think is strange behaviour,

    parser = ArgumentParser()
    parser.add_argument('i', action='count')
    parser.parse_args([])

got

    Namespace(i=1)

On the other hand, in _get_values function, _check_value is called to handle choices=, but there is no such guard for optional arguments, which means,

    parser = ArgumentParser()
    parser.add_argument('-i', nargs='?', type=int, default='2', choices=[1])
    parser.parse_args([])

doesn't raise an error.

Besides Paul's two instructive solutions, I think it better to make both sides behave the same. However, I found things seem not that simple.

First, ZERO_OR_MORE, no default value, positional arguments have required=True by default, but

    parser.add_argument('foo', nargs='*')
    parser.parse_args([])

got no problems. So it at least appears not required. (The document says required is only for optionals, so I guess it's just a implementation level but not a user level thing)

Second, the last case above gives

    Namespace(foo=[])

which seems logically incorrect or at least controversial, because the default is not set and you give no arguments, how does this list come? The document says nothing about the case (it's true it's a very corner one) and it also differs from the optional arguments case which gives

    Namespace(foo=None)

A walk around which doesn't change it is possible and I've written a patch fixing it.
And I'm not sure what we usually do if I propose to make them give the same result, is a PEP needed or I just raise a discussion about it? The change might break current code.

This is a complicated issue that needs a lot of thought and testing before we make any changes.

While all Actions have the 'required' attribute, the programmer can only set it for optionals. _get_positional_kwargs() will raise an error if the programmer tries to set it for a positional. For a positional its value is determined by the nargs value.

The distinction between positionals and optionals occurs through out argparse, so we shouldn't put much effort (if any) into making their handling more uniform. (The fundamental distinction between the two is whether the action.option_strings list is empty or not.)

A fundamental difference in parsing is that an optional's Action is called only if the flag is used. A positional's Action is always called.

_parse_known_args starts with a list of positionals

    positionals = self._get_positional_actions()

The nested consume_positionals function removes actions from this list.

Earlier versions raised an error at the end of parsing if this list was not empty. In the current version that's been replace by the 'required_actions' test (which tests both positionals and optionals). It was this change over that resulted in the bug/feature that subparsers (a positionals Action) are no longer required (by default).

Positionals with nargs='?' and '*' pose an extra challenge. They are, in a sense, no longer 'required'. But they will always be 'seen' because their nargs is satisfied by an empty list of values. But that would overwrite any 'default' in the Namespace. So there's the added step in _get_values of (re)inserting the action.default. And it's the handling of that 'default' that gives rise to the current issue.

These two positionals can also be used in a mutually_exclusive_group. To handle that 'take_action' has to maintain both the 'seen_actions' set and the 'seen_non_default_actions' set.

I found what appears to be a very similar issue so instead of creating a new issue I will place it here as a comment.

The following code:

==
import argparse

parser = argparse.ArgumentParser()
parser.add_argument('letter', choices=['a', 'b', 'c'], default=argparse.SUPPRESS, nargs='?')
args = parser.parse_args([])
==

results in this error:

==
usage: pok.py [-h] [{a,b,c}]
pok.py: error: argument letter: invalid choice: '==SUPPRESS==' (choose from 'a', 'b', 'c')

You are right, this part of the same issue.

_get_value() tests '==SUPPRESS==' both for type and choices.

pythongh-80259: Fix conflict between type and default=SUPPRESS in arg…

This is something similar to #53834 (especially the case reported in #80259 (comment)). In any case #124519 fixes all cases.

I would say that this is rather a simple issue. The bug occurs only because argparse.SUPPRESS is a string. If it would implemented as not a string (and this is another way to fix this issue), there would be no bug.

gh-80259: Fix conflict between type and default=SUPPRESS in argparse (G…

pythongh-80259: Fix conflict between type and default=SUPPRESS in arg…

[3.12] gh-80259: Fix conflict between type and default=SUPPRESS in ar…

[3.13] gh-80259: Fix conflict between type and default=SUPPRESS in ar…

Navigation Menu

Repository navigation

argparse: positional with type=int, default=SUPPRESS raise ValueError #80259

Linked PRs

==
usage: pok.py [-h] [{a,b,c}]
pok.py: error: argument letter: invalid choice: '==SUPPRESS==' (choose from 'a', 'b', 'c')

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

argparse: positional with type=int, default=SUPPRESS raise ValueError #80259

Description

Linked PRs

Activity

n8falke commented on Feb 22, 2019

n8falke commented on Feb 22, 2019

paulj3 commented on Feb 22, 2019

paulj3 commented on Feb 22, 2019

n8falke commented on Feb 25, 2019

yangyangxcf commented on Jan 14, 2020

paulj3 commented on Jan 18, 2020

jeyekomon commented on Mar 16, 2020

== usage: pok.py [-h] [{a,b,c}] pok.py: error: argument letter: invalid choice: '==SUPPRESS==' (choose from 'a', 'b', 'c')

paulj3 commented on Mar 16, 2020

serhiy-storchaka commented on Sep 25, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

==
usage: pok.py [-h] [{a,b,c}]
pok.py: error: argument letter: invalid choice: '==SUPPRESS==' (choose from 'a', 'b', 'c')