Skip to content

Conversation

@srittau
Copy link
Collaborator

@srittau srittau commented Jun 25, 2024

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
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

@srittau srittau added reason: inexpressible Closed, because this can't be expressed within the current type system and removed status: deferred Issue or PR deferred until some precondition is fixed labels Sep 12, 2024
@srittau
Copy link
Collaborator Author

srittau commented Sep 12, 2024

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

@srittau srittau closed this Sep 12, 2024
@srittau srittau deleted the argparse branch September 12, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reason: inexpressible Closed, because this can't be expressed within the current type system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants