typeshed icon indicating copy to clipboard operation
typeshed copied to clipboard

argparse: Forbid invalid `type` and `choices` combinations

Open srittau opened this issue 1 year ago • 10 comments


Deferred: See this discussion: https://discuss.python.org/t/constraining-generic-argument-types/56809

srittau avatar Jun 25 '24 16:06 srittau

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

pylint (https://github.com/pycqa/pylint)
+ pylint/config/arguments_manager.py:139: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "None"  [arg-type]
+ pylint/config/arguments_manager.py:150: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "None"  [arg-type]
+ pylint/config/arguments_manager.py:163: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "None"  [arg-type]
+ pylint/config/arguments_manager.py:174: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "None"  [arg-type]
+ pylint/config/arguments_manager.py:199: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "None"  [arg-type]

github-actions[bot] avatar Jun 25 '24 16:06 github-actions[bot]

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

pylint (https://github.com/pycqa/pylint)
- pylint/config/arguments_manager.py:136: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:147: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:160: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:171: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:196: error: Unused "type: ignore" comment  [unused-ignore]
+ pylint/config/arguments_manager.py:139: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "Iterable[str]"  [arg-type]
+ pylint/config/arguments_manager.py:150: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "Iterable[str]"  [arg-type]
+ pylint/config/arguments_manager.py:163: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "Iterable[str]"  [arg-type]
+ pylint/config/arguments_manager.py:174: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "Iterable[str]"  [arg-type]
+ pylint/config/arguments_manager.py:199: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "list[str] | None"; expected "Iterable[str]"  [arg-type]

mkosi (https://github.com/systemd/mkosi)
- mkosi/config.py:3257: error: Unused "type: ignore" comment  [unused-ignore]
+ mkosi/config.py:3146:5: error: No overload variant of "add_argument" of "_ActionsContainer" matches argument types "str", "str", "object", "Path", "str", "str"  [call-overload]
+ mkosi/config.py:3146:5: note: Possible overload variants:
+ mkosi/config.py:3146:5: note:     def add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: NoReturn = ..., choices: Iterable[str] = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3146:5: note:     def add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: FileType, choices: NoReturn = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3146:5: note:     def [_T] add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: Callable[[str], _T], choices: Iterable[_T] = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3146:5: note:     def add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: str, choices: Iterable[Any] = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3260:21: error: Argument "choices" to "add_argument" of "_ActionsContainer" has incompatible type "Optional[Any]"; expected "Iterable[str]"  [arg-type]

github-actions[bot] avatar Jun 25 '24 17:06 github-actions[bot]

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

pylint (https://github.com/pycqa/pylint)
- pylint/config/arguments_manager.py:136: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:147: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:160: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:171: error: Unused "type: ignore" comment  [unused-ignore]
- pylint/config/arguments_manager.py:196: error: Unused "type: ignore" comment  [unused-ignore]

mkosi (https://github.com/systemd/mkosi)
+ mkosi/config.py:3146:5: error: No overload variant of "add_argument" of "_ActionsContainer" matches argument types "str", "str", "object", "Path", "str", "str"  [call-overload]
+ mkosi/config.py:3146:5: note: Possible overload variants:
+ mkosi/config.py:3146:5: note:     def add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: FileType, choices: None = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3146:5: note:     def [_T] add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: Callable[[str], _T], choices: Optional[Iterable[_T]] = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3146:5: note:     def add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: str, choices: Optional[Iterable[Any]] = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action
+ mkosi/config.py:3146:5: note:     def add_argument(self, *name_or_flags: str, action: Union[str, type[Action]] = ..., nargs: Union[int, str, _SUPPRESS_T, None] = ..., const: Any = ..., default: Any = ..., type: NoReturn = ..., choices: Optional[Iterable[str]] = ..., required: bool = ..., help: Optional[str] = ..., metavar: Union[str, tuple[str, ...], None] = ..., dest: Optional[str] = ..., version: str = ..., **kwargs: Any) -> Action

github-actions[bot] avatar Jun 25 '24 17:06 github-actions[bot]

I'm unsure why the pyright regression tests fail. I can't see how the two ignored tests can match any of the overloads:

/home/runner/work/typeshed/typeshed/stdlib/@tests/test_cases/check_argparse.py:11:57 - error: Unnecessary "# type: ignore" comment (reportUnnecessaryTypeIgnoreComment)
  /home/runner/work/typeshed/typeshed/stdlib/@tests/test_cases/check_argparse.py:18:64 - error: Unnecessary "# type: ignore" comment (reportUnnecessaryTypeIgnoreComment)

srittau avatar Jun 25 '24 17:06 srittau

Regarding the primer output: pylint is actually using add_argument in an unsafe manner. It defines "argument" classes that have choices, which are list[str] and a type that can be a callable returning arbitrary values. This doesn't work at runtime, unless the callable returns a str.

The mkosi problem is mypy's overzealous base class finding behavior. I.e., mypy thinks that the type of parse_chdir if chdir else str is `object. Not much we can do about that.

srittau avatar Jun 25 '24 17:06 srittau

python/mypy#17427 should hopefully fix the issue with ternaries.

JelleZijlstra avatar Jun 25 '24 18:06 JelleZijlstra

(But I don't think we need to wait for that mypy PR.)

JelleZijlstra avatar Jun 25 '24 18:06 JelleZijlstra

@erictraut Could you have a look at https://github.com/python/typeshed/pull/12211/files? I'm not sure why pyright thinks the lines in question are okay. I don't think any potential should match, but maybe I'm overlooking something?

srittau avatar Jun 26 '24 09:06 srittau

I presume you're talking about the lines with # type: ignore comments next to them? For example: parser.add_argument("--foo", type=int, choices=[""])?

The second overload ('If type is a callable...') makes this OK. Here's a simplified version:

from typing import Callable, Iterable

def add_argument[_T](
    *name_or_flags: str, type: Callable[[str], _T], choices: Iterable[_T]
) -> _T: ...

x = add_argument("--foo", type=int, choices=[""])
reveal_type(x) # int | str

erictraut avatar Jun 26 '24 21:06 erictraut

That's interesting. mypy comes to a different conclusion (when using a slightly adapted example):

foo.py:10: error: Argument "type" to "add_argument" has incompatible type "type[int]"; expected "Callable[[str], str]"  [arg-type]
foo.py:11: note: Revealed type is "builtins.str"

mypy's interpretation (constraining _T based on each individual argument, instead of constraining it to the union of the arguments) is what I would have expected to happen. I guess this is a discrepancy that should be discussed separately. I've added the deferred status for now.

Edit: https://discuss.python.org/t/constraining-generic-argument-types/56809

srittau avatar Jun 27 '24 08:06 srittau

It seems that this is currently inexpressible in a way that's compatible with all type checkers.

srittau avatar Sep 12 '24 09:09 srittau