bpo-45235: Fix argparse namespace overridden by subparsers default

[frostming]

This PR is another attempt to fix bpo-45235.
In this patch, setting the default values is postponed until parsing is done and if the value is absent in the namespace. Also added a test case to cover the regression issue.

As a side-effect, the default values of other arguments won't be seen during parsing(such as inside an action). But IMO it is an invalid use case since the value may be later overridden by a passed-in value.

https://bugs.python.org/issue45235

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@frostming

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[jiasli]

As a side-effect, the default values of other arguments won't be seen during parsing(such as inside an action). But IMO it is an invalid use case since the value may be later overridden by a passed-in value.

This is not true 🙁. We (Azure CLI) and IPython (commented by @tacaswell) both rely on the the default value to be there during an Action:

• Global Arguments stop working in Python 3.9.8 Azure/azure-cli#20269 (comment)
• bpo-45235: Fix argparse overrides namespace with subparser defaults #28420 (comment)

This looks like a BREAKING CHANGE to me. @rhettinger @ambv

[frostming]

As a side-effect, the default values of other arguments won't be seen during parsing(such as inside an action). But IMO it is an invalid use case since the value may be later overridden by a passed-in value.

This is not true 🙁. We (Azure CLI) and IPython (commented by @tacaswell) both rely on the the default value to be there during an Action:

• Global Arguments stop working in Python 3.9.8 Azure/azure-cli#20269 (comment)
• [bpo-45235: Fix argparse overrides namespace with subparser defaults bpo-45235: Fix argparse overrides namespace with subparser defaults #28420 (comment)](bpo-45235: Fix argparse overrides namespace with subparser defaults #28420 (comment))

This looks like a BREAKING CHANGE to me. @rhettinger @ambv

I meant relying on the default values before parsing is finished can be fragile since it depends on the parsing order of the arguments. Think of this example:

command [--foo=VALUE] [--callback]

Here --foo is an option with the default value of 5. And --callback is bound to an action that reads the value of foo on the Namespace object. If the default values are assigned before the parsing, it will see different values of foo:

command --foo=10 --callback   # callback sees foo=10
command --callback --foo=10   # callback sees foo=5

It is true that with the patch of this PR the callback action can by no means know the value of foo unless it is given before --callback. The default value only applies when all parsing is done and no value is given. This looks more explicit to me in the way that if you want to refer to the value of another option, you must give it before:

command --foo=10 --callback   # callback sees foo=10
command --callback --foo=10   # Error!
command --callback   # Error!

@jiasli Does it make more sense?

[jiasli]

The default value we are referring to in an Action doesn't necessarily come from an argument like --foo. It may come from set_defaults:

            command_parser.set_defaults(
                func=metadata,
                command=command_name,
                _cmd=metadata,

Thus, when calling self._parse_known_args, self._defaults will be in namespace.

If these 3 lines are moved down here, self._defaults won't be in namespace when calling self._parse_known_args, resulting in failure

  File "C:\Users\name\AppData\Local\Programs\Python\Python310\lib\argparse.py", line 2003, in consume_optional
    take_action(action, args, option_string)
  File "C:\Users\name\AppData\Local\Programs\Python\Python310\lib\argparse.py", line 1931, in take_action
    action(self, namespace, argument_values, option_string)
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\commands\arm.py", line 357, in __call__
    profile = Profile(cli_ctx=namespace._cmd.cli_ctx)  # pylint: disable=protected-access
AttributeError: 'Namespace' object has no attribute '_cmd'

I am open to whether this is the correct behavior, but it is definitely a BREAKING CHANGE given the behavior has been there for years. If this change is what Python committee want, it should be released in at least Python 3.12, not Python 3.10 or 3.11 which are already stabilized, per an email discussion with @gvanrossum:

Even in 3.11 such a breaking change would not be acceptable. They will have to figure out another way to provide the functionality.

[frostming]

This case makes sense to me. I admit that any change may be a breaking change to some people. This needs more discussion, it perhaps should happen on BPO I guess?

[dgw]

Yes, these are great concerns to get feedback from BPO-45235. Some backward compatibility concerns are already in that thread, and as maintainer of another project that was affected by the reverted change in Python 3.9.8, I'd love to see those get hashed out in the main thread instead of the line notes of this patch.

[iritkatriel]

Could you in the meantime submit patches for the unit tests we’re missing?

There are at least two - for the regression that happened and the one that was identified here.

[frostming]

they are included in the one added case, do you suggest to separate into two cases?

[rhettinger]

I'm going to step away from this one. The last attempt at a fix was catastrophic and should teach us that almost every nuance of the implementation is being relied upon. I think you're correct in saying "any change may be a breaking change to some people. " ISTM that there is no possible benefit that would be worth breaking some long deployed, stable, working command-line tools

[frostming]

In my second attempt #30219 the side effect is removed, it should be cleaner.

The issue itself is a valid one, but the first fix isn't that careful.

[MaxwellDupre]

I think it would help if you added some clarifying info to the Docs under subparsers, possibly with examples. I think it also helps to explain internal workings which guides users to correct usage, but that's entirely up to you. I dont think you need to generate too much more stuff, just add whats in the bug report and documentation that you added to the code.
I note the subparsers Docs has not changed since 3.7.

[StanFromIreland]

This has conflicts now.

[frostming]

This has conflicts now.

Resolved, thanks

[frostming]

Due to some implementation defects, I've decided not to continue with this PR and I have another fix at #30219 .

Thanks all for the kind reminders and discussions.