Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

argparse: Forbid invalid type and choices combinations #12211

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Jun 25, 2024

stdlib/argparse.pyi Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

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

@srittau
Copy link
Collaborator Author

srittau commented Jun 25, 2024

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
Copy link
Collaborator Author

srittau commented Jun 25, 2024

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.

@JelleZijlstra
Copy link
Member

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

@JelleZijlstra
Copy link
Member

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

@srittau
Copy link
Collaborator Author

srittau commented Jun 26, 2024

@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?

@erictraut
Copy link
Contributor

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

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Jun 27, 2024
@srittau
Copy link
Collaborator Author

srittau commented Jun 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants