fix: add missing overloads for argparse.*parse_*_args

[kkirsche]

fix: #8538

This attempts to address issue #8538. Huge thank you to Akuli for their help in the issue. Hopefully I got this right, apologies if I messed anything up.

[kkirsche]

Yikes, I must have messed this up 😕 .

Maybe I should have left the original signature so that the overloads are just variants and the actual function has the correct signature?

[Akuli]

The overloads are correct (apart from bound=Namespace which seems unnecessary to me), but having them here means that everyone who overrides one of these methods in a subclass of ArgumentParser would have to copy/paste the overloads into their code.

I'm not sure what the best solution is. We could just leave the stubs as is: they would be incorrect, because the return type shouldn't always have Namespace, but that seems to cause less false positives than the overloads. Maybe return Namespace | Any so that you can at least do this without mypy errors?

parsed_args, remaining_args = parser.parse_known_args(namespace=MyNamespace())
assert isinstance(parsed_args, MyNamespace)

[kkirsche]

My MyPy integration in VSCode was saying that not having it bound was an error because of how the functions interact with each other, so I restricted the TypeVar to fix those errors

[kkirsche]

Yeah, the error I was seeing was:

Overloaded function signatures 1 and 2 overlap with incompatible return types

[Akuli]

The error is because without a bound, you could use None as your namespace. It wouldn't actually work though, because setting any attribute on None is an error. I don't think we have a way to express "any object that has at least one settable attribute", so we could just # type: ignore that error away if we decide to use overloads.

[kkirsche]

Since we are using the variable to represent a namespace, why would we choose to avoid using bound? Not opposed to removing it, I just don't understand the motivation

[Akuli]

There's no particular reason why the namespace would have to derive from argparse.Namespace. It can be any object that you can set attributes on, it's only used with setattr(). This is useful if you want to type-check the uses of your namespace.

class MyNamespace:
    foo: list[str]
    bar: int

args = parser.parse_args(namespace=MyNamespace())
print(args.foobar)  # error

If you inherit from argparse.Namespace (or just use argparse.Namespace), the def __getattr__(self, name: str) -> Any: ... we define for it would cause args.foobar to not be an error, and have type Any.

[kkirsche]

Ah, ok, thank you. The namespace concept with argparse has been an abstraction that hadn't made sense to me in the past with argparse, so I appreciate the information about that.

[kkirsche]

I'm not sure what the best solution is. We could just leave the stubs as is: they would be incorrect, because the return type shouldn't always have Namespace, but that seems to cause less false positives than the overloads. Maybe return Namespace | Any so that you can at least do this without mypy errors?

I think this may be too incorrect of a representation (as I understand it, a lot of people seem to be anti-union returns 😕 ), but I figure I'll ask just to ensure we've considered all options. What about something like this:

    def parse_known_args(
        self, args: Sequence[str] | None = ..., namespace: Namespace | _N | None = ...
    ) -> tuple[Namespace | _N, list[str]]: ...

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/config/argparsing.py:448: error: Incompatible return value type (got "Optional[Namespace]", expected "Namespace")  [return-value]

materialize (https://github.com/MaterializeInc/materialize)
+ misc/python/materialize/mzcompose/__init__.py:900: error: Signature of "parse_known_args" incompatible with supertype "ArgumentParser"
+ misc/python/materialize/mzcompose/__init__.py:900: note:      Superclass:
+ misc/python/materialize/mzcompose/__init__.py:900: note:          @overload
+ misc/python/materialize/mzcompose/__init__.py:900: note:          def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: None = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/mzcompose/__init__.py:900: note:          @overload
+ misc/python/materialize/mzcompose/__init__.py:900: note:          def [_N] parse_known_args(self, args: Optional[Sequence[str]], namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/mzcompose/__init__.py:900: note:          @overload
+ misc/python/materialize/mzcompose/__init__.py:900: note:          def [_N] parse_known_args(self, args: Optional[Sequence[str]] = ..., *, namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/mzcompose/__init__.py:900: note:      Subclass:
+ misc/python/materialize/mzcompose/__init__.py:900: note:          def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:657: error: Signature of "parse_known_args" incompatible with supertype "ArgumentParser"
+ misc/python/materialize/cli/mzcompose.py:657: note:      Superclass:
+ misc/python/materialize/cli/mzcompose.py:657: note:          @overload
+ misc/python/materialize/cli/mzcompose.py:657: note:          def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: None = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:657: note:          @overload
+ misc/python/materialize/cli/mzcompose.py:657: note:          def [_N] parse_known_args(self, args: Optional[Sequence[str]], namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:657: note:          @overload
+ misc/python/materialize/cli/mzcompose.py:657: note:          def [_N] parse_known_args(self, args: Optional[Sequence[str]] = ..., *, namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:657: note:      Subclass:
+ misc/python/materialize/cli/mzcompose.py:657: note:          def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:664: error: Incompatible return value type (got "Tuple[Optional[Namespace], List[str]]", expected "Tuple[Namespace, List[str]]")
+ misc/python/materialize/cli/mzcompose.py:668: error: Signature of "parse_known_args" incompatible with supertype "ArgumentParser"
+ misc/python/materialize/cli/mzcompose.py:668: note:      Superclass:
+ misc/python/materialize/cli/mzcompose.py:668: note:          @overload
+ misc/python/materialize/cli/mzcompose.py:668: note:          def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: None = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:668: note:          @overload
+ misc/python/materialize/cli/mzcompose.py:668: note:          def [_N] parse_known_args(self, args: Optional[Sequence[str]], namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:668: note:          @overload
+ misc/python/materialize/cli/mzcompose.py:668: note:          def [_N] parse_known_args(self, args: Optional[Sequence[str]] = ..., *, namespace: _N) -> Tuple[_N, List[str]]
+ misc/python/materialize/cli/mzcompose.py:668: note:      Subclass:
+ misc/python/materialize/cli/mzcompose.py:668: note:          def parse_known_args(self, args: Optional[Sequence[str]] = ..., namespace: Optional[Namespace] = ...) -> Tuple[Namespace, List[str]]
+ misc/python/materialize/cli/mzcompose.py:675: error: Incompatible return value type (got "Tuple[Optional[Namespace], List[str]]", expected "Tuple[Namespace, List[str]]")

[kkirsche]

Looks like removing bound introduces it's own set of problems, so it sounds like overloads are going to be rather noisy to downstream customers in a way that's (arguably) not going to improve the safety of those systems

[Akuli]

Yeah, it seems that we have multiple bad options to choose from. Feel free to try whatever you think might work and see what mypy_primer says :)

Arguably returning the correct type when a namespace is passed isn't very important. Instead of this:

args = parser.parse_args(namespace=MyNamespace())

You could just do this:

args = MyNamespace()
parser.parse_args(namespace=args)

Having to rewrite the code only to satisfy type checkers isn't great in general, but maybe it is the least bad option we have.

[Akuli]

a lot of people seem to be anti-union returns

Your suggestion was:

def parse_known_args(
    self, args: Sequence[str] | None = ..., namespace: Namespace | _N | None = ...
) -> tuple[Namespace | _N, list[str]]: ...

Here returning Namespace | _N doesn't have the usual disadvantages of returning a union. Because Namespace pretends to have any attribute whatsoever with its __getattr__, accessing an attribute of Foo on a Foo | Namespace shouldn't error. But accessing an attribute that doesn't exist on Foo would, because the attribute must exist on both union members. Overall it works a lot as if it was Foo, which is good. The one disadvantage is that you can't pass it to a function expecting a Foo as an argument, but even then it would be an improvement over the current situation.

There's one other problem with this definition. If you don't specify a namespace parameter, or you specify it as a Namespace or None, then _N is used only in the return type, and using a TypeVar only once is pointless. For a simpler example, consider this:

from __future__ import annotations
from typing import TypeVar

T = TypeVar("T")
def func(x: str | T) -> T: ...

reveal_type(func(1))     # Revealed type is "builtins.int*"
reveal_type(func("hi"))  # Revealed type is "<nothing>"

When you try to assign the result to a variable, mypy will then complain about needing a type annotation for it, as it doesn't know what the type is (it's T but there are no clues about what T should be solved to):

x = func("hi")  # error: Need type annotation for "x"

We probably don't want to require a type annotation for most uses of .parse_args(), it is usually called without specifying a namespace.