python / cpython

The Python programming language
https://www.python.org
Other
62.15k stars 29.86k forks source link

argparse behavior when a 'store' arg and a 'store_const' arg both store to the same dest is undefined and inconsistent #121829

Open AdamWill opened 1 month ago

AdamWill commented 1 month ago

Bug report

Bug description:

import argparse

parser1 = argparse.ArgumentParser()
parser1.add_argument("--squashfs-only", action="store_const", const="squashfs", dest="rootfs_type")
parser1.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs")
print(parser1.parse_args())
parser2 = argparse.ArgumentParser()
parser2.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs")
parser2.add_argument("--squashfs-only", action="store_const", const="squashfs", dest="rootfs_type")
print(parser2.parse_args())

Note the script adds the same two arguments to each parser, but in both possible orders. Output for me, with Python 3.11 and Python 3.13:

Namespace(rootfs_type=None)
Namespace(rootfs_type='squashfs')

AFAICS nothing in argparse docs indicates what the intended behavior in this case is, and it seems surprising and possibly not optimal that the actual behavior differs depending on what order the args are specified in.

This is a real-world issue - because of this, https://github.com/weldr/lorax/commit/01dd27a97c334c823903f7779d1b589cf288cc92 broke lorax (the Fedora Linux distribution's script for building installer images). In that case, the expected behavior was that the value should be "squashfs" if neither arg was specified on the command line, but because the args were added in the parser1 order (--squashfs-only first), it is actually None.

CPython versions tested on:

3.11, 3.13

Operating systems tested on:

Linux

AdamWill commented 1 month ago

In fixing this, I found it's weirdly hard to tell argparse "accept this old argument so people's scripts don't break, but do nothing at all with it". There is no "null" action, and making your own seems weirdly difficult (I tried it for an hour and gave up). In the end I went with a PR that simply keeps --squashfs-only as a store_true and ignores it, but that seems inelegant.

serhiy-storchaka commented 1 month ago

The code sets the default value from the first action. I think this should not be surprising. The default value for the "--squashfs-only" option was None, and adding the "--rootfs-type" have not changed this.

You can see the same behavior for two "store" actions with different defaults:

import argparse

parser1 = argparse.ArgumentParser()
parser1.add_argument("--foo", action="store", default="foo", dest="dest")
parser1.add_argument("--bar", action="store", default="bar", dest="dest")
print(parser1.parse_args())
parser2 = argparse.ArgumentParser()
parser2.add_argument("--bar", action="store", default="bar", dest="dest")
parser2.add_argument("--foo", action="store", default="foo", dest="dest")
print(parser2.parse_args())

Output:

Namespace(dest='foo')
Namespace(dest='bar')

The conflict between different defaults should be solved in some way, and choosing the first one is not worse than any other way. Changing this now will break existing code which relies on this.

In fixing this, I found it's weirdly hard to tell argparse "accept this old argument so people's scripts don't break, but do nothing at all with it". There is no "null" action, and making your own seems weirdly difficult (I tried it for an hour and gave up). In the end I went with a PR that simply keeps --squashfs-only as a store_true and ignores it, but that seems inelegant.

I do not that "null" action is what you need here. --rootfs-type=erofs --squashfs-only should be equivalent to --rootfs-type=erofs --rootfs-type=squashfs, i.e. set rootfs_type to "squashfs", not to "erofs".

There are a number of options to solve your problem. You can change the order of add_argument(). You can add explicit default for "--squashfs-only". You can use set_defaults() to set defaults instead of specifying them in add_argument().

AdamWill commented 1 month ago

I do not that "null" action is what you need here. --rootfs-type=erofs --squashfs-only should be equivalent to --rootfs-type=erofs --rootfs-type=squashfs, i.e. set rootfs_type to "squashfs", not to "erofs".

That's kinda getting into the weeds, but as it happens in this case we actually want --rootfs-type to win. Anyhow, we've solved it downstream already. I figured it was worth mentioning upstream, but I guess your explanation makes sense.