From 24857935d0b25a7bb5a1aa3af09638cf16eba337 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Mon, 10 Jul 2023 13:07:32 +0100 Subject: [PATCH 1/4] Warn on `Callable[, None]` and `Callable[, Any]` --- CHANGELOG.md | 21 +++++++++++++-------- ERRORCODES.md | 1 + pyi.py | 17 +++++++++++++++-- tests/bad_callbacks.pyi | 28 ++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 tests/bad_callbacks.pyi diff --git a/CHANGELOG.md b/CHANGELOG.md index a2b5965a..7d1c7dd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,14 +2,19 @@ ## Unreleased -* Introduce Y090, which warns if you have an annotation such as `tuple[int]` or - `Tuple[int]`. These mean "a tuple of length 1, in which the sole element is - of type `int`". This is sometimes what you want, but more usually you'll want - `tuple[int, ...]`, which means "a tuple of arbitrary (possibly 0) length, in - which all elements are of type `int`". - - This error code is disabled by default due to the risk of false-positive - errors. To enable it, use the `--extend-select=Y090` option. +* Introduce several codes which are disabled by default due to the risk of + false-positive errors. To enable these lints, use the + `--extend-select={code1,code2,...}` option. + * Y090, which warns if you have an annotation such as `tuple[int]` or + `Tuple[int]`. These mean "a tuple of length 1, in which the sole element is + of type `int`". This is sometimes what you want, but more usually you'll want + `tuple[int, ...]`, which means "a tuple of arbitrary (possibly 0) length, in + which all elements are of type `int`". + * Y091, which warns if `Callable[, Any]` or + `Callable[, None]` is used as an argument annotation in a + function or method. Most of the time, the returned object from callbacks like + these is ignored at runtime, so the annotation you're probably looking for is + `Callable[, object]`. ## 23.6.0 diff --git a/ERRORCODES.md b/ERRORCODES.md index 7a06c6b2..673d075f 100644 --- a/ERRORCODES.md +++ b/ERRORCODES.md @@ -75,3 +75,4 @@ recommend only using `--extend-select`, never `--select`. | Code | Description |------|------------ | Y090 | `tuple[int]` means "a tuple of length 1, in which the sole element is of type `int`". Consider using `tuple[int, ...]` instead, which means "a tuple of arbitrary (possibly 0) length, in which all elements are of type `int`". +| Y091 | `Callable[, None]` or `Callable[, Any]` is generally a mistake in the context of a parameter annotation for a function or method. The returned value from a callback is generally ignored at runtime, so `Callable[, object]` is usually a better annotation in this kind of situation. diff --git a/pyi.py b/pyi.py index 0fc5dc4b..8669ef5a 100644 --- a/pyi.py +++ b/pyi.py @@ -1369,8 +1369,9 @@ def _Y090_error(self, node: ast.Subscript) -> None: def visit_Subscript(self, node: ast.Subscript) -> None: subscripted_object = node.value + interesting_modules = _TYPING_MODULES | {"builtins", "collections.abc"} subscripted_object_name = _get_name_of_class_if_from_modules( - subscripted_object, modules=_TYPING_MODULES | {"builtins"} + subscripted_object, modules=interesting_modules ) self.visit(subscripted_object) if subscripted_object_name == "Literal": @@ -1398,6 +1399,14 @@ def _visit_slice_tuple(self, node: ast.Tuple, parent: str | None) -> None: self.visit(elt) else: self.visit(node) + elif parent == "Callable": + self.visit(node) + if self.visiting_arg.active and len(node.elts) == 2: + return_annotation = node.elts[1] + if _is_None(return_annotation): + self.error(node, Y091.format(bad_return="None")) + elif _is_Any(return_annotation): + self.error(node, Y091.format(bad_return="Any")) else: self.visit(node) @@ -2129,5 +2138,9 @@ def parse_options(options: argparse.Namespace) -> None: '"a tuple of length 1, in which the sole element is of type {typ!r}". ' 'Perhaps you meant "{new}"?' ) +Y091 = ( + 'Y091 "Callable" in argument annotations ' + 'should generally have "object" for the second parameter, not "{bad_return}"' +) -DISABLED_BY_DEFAULT = ["Y090"] +DISABLED_BY_DEFAULT = ["Y090", "Y091"] diff --git a/tests/bad_callbacks.pyi b/tests/bad_callbacks.pyi new file mode 100644 index 00000000..c2ce5e89 --- /dev/null +++ b/tests/bad_callbacks.pyi @@ -0,0 +1,28 @@ +# flags: --extend-select=Y091 + +import collections.abc +import typing +from collections.abc import Callable +from typing import Any +from typing_extensions import TypeAlias + +def bad( + a: Callable[[], None], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "None" + b: Callable[..., Any], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "Any" + c: typing.Callable[[str, int], typing.Any], # Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax) # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "Any" + d: collections.abc.Callable[[None], None], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "None" + e: int | str | tuple[Callable[[], None], int, str], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "None" +) -> None: ... + +def good( + a: Callable[[], object], + b: Callable[..., object], + c: typing.Callable, # Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax) + d: collections.abc.Callable[[None], object], +) -> None: ... + +def also_good() -> Callable[[], None]: ... + +_TypeAlias: TypeAlias = collections.abc.Callable[[int, str], Any] +x: _TypeAlias +y: typing.Callable[[], None] # Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax) From c8a32b0bc29f95291a277c1b56bd51174982e522 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 10 Jul 2023 12:14:07 +0000 Subject: [PATCH 2/4] [pre-commit.ci] auto fixes from pre-commit.com hooks --- tests/bad_callbacks.pyi | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/bad_callbacks.pyi b/tests/bad_callbacks.pyi index c2ce5e89..dd4c44c7 100644 --- a/tests/bad_callbacks.pyi +++ b/tests/bad_callbacks.pyi @@ -4,6 +4,7 @@ import collections.abc import typing from collections.abc import Callable from typing import Any + from typing_extensions import TypeAlias def bad( From 9c80fd2b6579b694db775814df19291d6b6e8ef0 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 10 Jul 2023 17:09:23 +0100 Subject: [PATCH 3/4] Update pyi.py Co-authored-by: Jelle Zijlstra --- pyi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyi.py b/pyi.py index 8669ef5a..0478fa98 100644 --- a/pyi.py +++ b/pyi.py @@ -2140,7 +2140,7 @@ def parse_options(options: argparse.Namespace) -> None: ) Y091 = ( 'Y091 "Callable" in argument annotations ' - 'should generally have "object" for the second parameter, not "{bad_return}"' + 'should generally have "object" as the return type, not "{bad_return}"' ) DISABLED_BY_DEFAULT = ["Y090", "Y091"] From 47cd29183b80008b052c5e1b588d6aaeec9efb2d Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Mon, 10 Jul 2023 17:11:23 +0100 Subject: [PATCH 4/4] Update tests --- tests/bad_callbacks.pyi | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/bad_callbacks.pyi b/tests/bad_callbacks.pyi index dd4c44c7..0661d5d2 100644 --- a/tests/bad_callbacks.pyi +++ b/tests/bad_callbacks.pyi @@ -8,11 +8,11 @@ from typing import Any from typing_extensions import TypeAlias def bad( - a: Callable[[], None], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "None" - b: Callable[..., Any], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "Any" - c: typing.Callable[[str, int], typing.Any], # Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax) # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "Any" - d: collections.abc.Callable[[None], None], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "None" - e: int | str | tuple[Callable[[], None], int, str], # Y091 "Callable" in argument annotations should generally have "object" for the second parameter, not "None" + a: Callable[[], None], # Y091 "Callable" in argument annotations should generally have "object" as the return type, not "None" + b: Callable[..., Any], # Y091 "Callable" in argument annotations should generally have "object" as the return type, not "Any" + c: typing.Callable[[str, int], typing.Any], # Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax) # Y091 "Callable" in argument annotations should generally have "object" as the return type, not "Any" + d: collections.abc.Callable[[None], None], # Y091 "Callable" in argument annotations should generally have "object" as the return type, not "None" + e: int | str | tuple[Callable[[], None], int, str], # Y091 "Callable" in argument annotations should generally have "object" as the return type, not "None" ) -> None: ... def good(