From 7a0f1103c26ea8cf0ce572fe04a53982cc6f6ab0 Mon Sep 17 00:00:00 2001 From: Sebastian Rittau Date: Fri, 29 Aug 2025 16:31:17 +0200 Subject: [PATCH 1/7] Y092: Detect pseudo-protocols Closes: #508 --- CHANGELOG.md | 9 +++++++-- ERRORCODES.md | 3 ++- flake8_pyi/errors.py | 3 ++- flake8_pyi/visitor.py | 26 ++++++++++++++++++++++++-- tests/pseudo_protocols.pyi | 29 +++++++++++++++++++++++++++++ 5 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 tests/pseudo_protocols.pyi diff --git a/CHANGELOG.md b/CHANGELOG.md index 9199be5..4ce4223 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,15 @@ # Change Log +## Unreleased + +New error codes: +* Y092: Pseudo-protocols should not be used as argument types. + ## 5.5.0 New error codes: -* Introduce Y067: Don't use `Incomplete | None = None`. -* Introduce Y091: Protocol method parameters should not be positional-or-keyword. +* Y067: Don't use `Incomplete | None = None`. +* Y091: Protocol method parameters should not be positional-or-keyword. Other changes: * Y011/Y015 will now allow all defaults that include an attribute access, diff --git a/ERRORCODES.md b/ERRORCODES.md index eae5916..148fa3c 100644 --- a/ERRORCODES.md +++ b/ERRORCODES.md @@ -97,4 +97,5 @@ recommend only using `--extend-select`, never `--select`. | Code | Description | Code category |------|-------------|--------------- | 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`". | Correctness -| Y091 | Protocol methods should not have positional-or-keyword parameters. Usually, a positional-only parameter is better. +| Y091 | Protocol methods should not have positional-or-keyword parameters. Usually, a positional-only parameter is better. | Correctness +| Y092 | Pseudo-protocols `Sequence`, `Mapping`, or `MutableMapping` should not be used as argument types. Using a protocol is preferred. | Correctness diff --git a/flake8_pyi/errors.py b/flake8_pyi/errors.py index 239d76e..e9af456 100644 --- a/flake8_pyi/errors.py +++ b/flake8_pyi/errors.py @@ -136,5 +136,6 @@ class Error(NamedTuple): 'Y091 Argument "{arg}" to protocol method "{method}" should probably not be positional-or-keyword. ' "Make it positional-only, since usually you don't want to mandate a specific argument name" ) +Y092 = 'Y092 Don\'t use pseudo-protocol "{arg}" as parameter type. Use a protocol instead.' -DISABLED_BY_DEFAULT = ["Y090", "Y091"] +DISABLED_BY_DEFAULT = ["Y090", "Y091", "Y092"] diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index e2fd4b2..421d2a3 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -138,6 +138,8 @@ def all_equal(iterable: Iterable[object]) -> bool: } ) +PSEUDO_PROTOCOLS = {"Sequence", "Mapping", "MutableMapping"} + def _ast_node_for(string: str) -> ast.AST: """Helper function for doctests.""" @@ -447,6 +449,18 @@ def _is_bad_TypedDict(node: ast.Call) -> bool: return True +def _contains_pseudo_protocol(node: ast.expr | None) -> str | None: + """Return the name of the pseudo-protocol if found, else None.""" + if isinstance(node, ast.Subscript): + return _contains_pseudo_protocol(node.value) or _contains_pseudo_protocol(node.slice) + if _is_pep_604_union(node): + return _contains_pseudo_protocol(node.left) or _contains_pseudo_protocol(node.right) + for name in PSEUDO_PROTOCOLS: + if _is_object(node, name, from_=_TYPING_OR_COLLECTIONS_ABC): + return name + return None + + def _is_assignment_which_must_have_a_value( target_name: str | None, *, in_class: bool ) -> bool: @@ -747,6 +761,13 @@ def _is_valid_default_value_with_annotation( return False +def _is_pep_604_union(node: ast.AST | None) -> TypeGuard[ast.BinOp]: + return ( + isinstance(node, ast.BinOp) + and isinstance(node.op, ast.BitOr) + ) + + def _is_valid_pep_604_union_member(node: ast.expr) -> bool: return _is_None(node) or isinstance(node, (ast.Name, ast.Attribute, ast.Subscript)) @@ -754,8 +775,7 @@ def _is_valid_pep_604_union_member(node: ast.expr) -> bool: def _is_valid_pep_604_union(node: ast.expr) -> TypeGuard[ast.BinOp]: """Does `node` represent a valid PEP-604 union (e.g. `int | str`)?""" return ( - isinstance(node, ast.BinOp) - and isinstance(node.op, ast.BitOr) + _is_pep_604_union(node) and ( _is_valid_pep_604_union_member(node.left) or _is_valid_pep_604_union(node.left) @@ -2100,6 +2120,8 @@ def visit_arg(self, node: ast.arg) -> None: self.error(node, errors.Y050) if _is_Incomplete(node.annotation): self.error(node, errors.Y065.format(what=f'parameter "{node.arg}"')) + if proto := _contains_pseudo_protocol(node.annotation): + self.error(node, errors.Y092.format(arg=proto)) with self.visiting_arg.enabled(): self.generic_visit(node) diff --git a/tests/pseudo_protocols.pyi b/tests/pseudo_protocols.pyi new file mode 100644 index 0000000..81914a2 --- /dev/null +++ b/tests/pseudo_protocols.pyi @@ -0,0 +1,29 @@ +# flags: --extend-select=Y092 + +# Tests for pseudo-protocols like `Sequence`, `Mapping`, or `MutableMapping` +# imported from collections.abc. +# +# We're explicitly not testing for imports from typing as that should already +# trigger Y022 (import from collections.abc instead from typing). + +import collections.abc +from collections.abc import Iterable, Mapping, MutableMapping, Sequence +from collections.abc import Mapping as MyMapping + +def test_sequence(seq: Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_mapping(mapping: Mapping[str, int]) -> None: ... # Y092 Don't use pseudo-protocol "Mapping" as parameter type. Use a protocol instead. +def test_mutable_mapping(mapping: MutableMapping[str, int]) -> None: ... # Y092 Don't use pseudo-protocol "MutableMapping" as parameter type. Use a protocol instead. +def test_import_alias(mapping: MyMapping[str, int]) -> None: ... # TODO: import aliases are currently not supported. +def test_plain(seq: Sequence) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_union(arg: Sequence[int] | int) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_nested(arg: list[Sequence[int]]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_full_type(seq: collections.abc.Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. + +x: Sequence[int] # ok +def test_iterable(it: Iterable[str]) -> None: ... # ok +def test_as_return_type() -> Sequence[int]: ... # ok + +class Foo: + x: Sequence[int] # ok + + def test_method(self, seq: Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. From d55518d808523ec3cfc973ef91172474d0846693 Mon Sep 17 00:00:00 2001 From: Sebastian Rittau Date: Fri, 29 Aug 2025 16:48:47 +0200 Subject: [PATCH 2/7] Allow multiple errors in one annotation --- flake8_pyi/visitor.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index 421d2a3..8e1714f 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -449,18 +449,6 @@ def _is_bad_TypedDict(node: ast.Call) -> bool: return True -def _contains_pseudo_protocol(node: ast.expr | None) -> str | None: - """Return the name of the pseudo-protocol if found, else None.""" - if isinstance(node, ast.Subscript): - return _contains_pseudo_protocol(node.value) or _contains_pseudo_protocol(node.slice) - if _is_pep_604_union(node): - return _contains_pseudo_protocol(node.left) or _contains_pseudo_protocol(node.right) - for name in PSEUDO_PROTOCOLS: - if _is_object(node, name, from_=_TYPING_OR_COLLECTIONS_ABC): - return name - return None - - def _is_assignment_which_must_have_a_value( target_name: str | None, *, in_class: bool ) -> bool: @@ -2120,8 +2108,7 @@ def visit_arg(self, node: ast.arg) -> None: self.error(node, errors.Y050) if _is_Incomplete(node.annotation): self.error(node, errors.Y065.format(what=f'parameter "{node.arg}"')) - if proto := _contains_pseudo_protocol(node.annotation): - self.error(node, errors.Y092.format(arg=proto)) + self._check_pseudo_protocol(node.annotation) with self.visiting_arg.enabled(): self.generic_visit(node) @@ -2138,6 +2125,16 @@ def visit_arguments(self, node: ast.arguments) -> None: if node.kwarg is not None: self.visit(node.kwarg) + def _check_pseudo_protocol(self, node: ast.expr | None) -> None: + if isinstance(node, ast.Subscript): + self._check_pseudo_protocol(node.value) + self._check_pseudo_protocol(node.slice) + if _is_pep_604_union(node): + self._check_pseudo_protocol(node.left) + self._check_pseudo_protocol(node.right) + if isinstance(node, ast.Name) and node.id in PSEUDO_PROTOCOLS: + self.error(node, errors.Y092.format(arg=node.id)) + def check_arg_default(self, arg: ast.arg, default: ast.expr | None) -> None: self.visit(arg) if default is not None: From 8d3d8a67dbc53b87056a748cfef34cd45464528b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 29 Aug 2025 14:49:10 +0000 Subject: [PATCH 3/7] [pre-commit.ci] auto fixes from pre-commit.com hooks --- flake8_pyi/errors.py | 4 +++- flake8_pyi/visitor.py | 5 +---- tests/pseudo_protocols.pyi | 9 +++++++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/flake8_pyi/errors.py b/flake8_pyi/errors.py index e9af456..2aef086 100644 --- a/flake8_pyi/errors.py +++ b/flake8_pyi/errors.py @@ -136,6 +136,8 @@ class Error(NamedTuple): 'Y091 Argument "{arg}" to protocol method "{method}" should probably not be positional-or-keyword. ' "Make it positional-only, since usually you don't want to mandate a specific argument name" ) -Y092 = 'Y092 Don\'t use pseudo-protocol "{arg}" as parameter type. Use a protocol instead.' +Y092 = ( + 'Y092 Don\'t use pseudo-protocol "{arg}" as parameter type. Use a protocol instead.' +) DISABLED_BY_DEFAULT = ["Y090", "Y091", "Y092"] diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index 8e1714f..7e69deb 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -750,10 +750,7 @@ def _is_valid_default_value_with_annotation( def _is_pep_604_union(node: ast.AST | None) -> TypeGuard[ast.BinOp]: - return ( - isinstance(node, ast.BinOp) - and isinstance(node.op, ast.BitOr) - ) + return isinstance(node, ast.BinOp) and isinstance(node.op, ast.BitOr) def _is_valid_pep_604_union_member(node: ast.expr) -> bool: diff --git a/tests/pseudo_protocols.pyi b/tests/pseudo_protocols.pyi index 81914a2..dd513e9 100644 --- a/tests/pseudo_protocols.pyi +++ b/tests/pseudo_protocols.pyi @@ -7,8 +7,13 @@ # trigger Y022 (import from collections.abc instead from typing). import collections.abc -from collections.abc import Iterable, Mapping, MutableMapping, Sequence -from collections.abc import Mapping as MyMapping +from collections.abc import ( + Iterable, + Mapping, + Mapping as MyMapping, + MutableMapping, + Sequence, +) def test_sequence(seq: Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. def test_mapping(mapping: Mapping[str, int]) -> None: ... # Y092 Don't use pseudo-protocol "Mapping" as parameter type. Use a protocol instead. From 097c8780e9610eb1a24d24ed5fda6c7a64c95482 Mon Sep 17 00:00:00 2001 From: Sebastian Rittau Date: Fri, 29 Aug 2025 16:57:33 +0200 Subject: [PATCH 4/7] Fix --- flake8_pyi/visitor.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index 7e69deb..6bd48e9 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -2123,14 +2123,17 @@ def visit_arguments(self, node: ast.arguments) -> None: self.visit(node.kwarg) def _check_pseudo_protocol(self, node: ast.expr | None) -> None: + if node is None: + return if isinstance(node, ast.Subscript): self._check_pseudo_protocol(node.value) self._check_pseudo_protocol(node.slice) if _is_pep_604_union(node): self._check_pseudo_protocol(node.left) self._check_pseudo_protocol(node.right) - if isinstance(node, ast.Name) and node.id in PSEUDO_PROTOCOLS: - self.error(node, errors.Y092.format(arg=node.id)) + for name in PSEUDO_PROTOCOLS: + if _is_object(node, name, from_=_TYPING_OR_COLLECTIONS_ABC): + self.error(node, errors.Y092.format(arg=name)) def check_arg_default(self, arg: ast.arg, default: ast.expr | None) -> None: self.visit(arg) From a7996195b49b695906fe191e067f42d5c9928463 Mon Sep 17 00:00:00 2001 From: Sebastian Rittau Date: Wed, 3 Sep 2025 12:09:31 +0200 Subject: [PATCH 5/7] Add more pseudo-protocols --- ERRORCODES.md | 2 +- flake8_pyi/visitor.py | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/ERRORCODES.md b/ERRORCODES.md index 148fa3c..d54dd4f 100644 --- a/ERRORCODES.md +++ b/ERRORCODES.md @@ -98,4 +98,4 @@ recommend only using `--extend-select`, never `--select`. |------|-------------|--------------- | 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`". | Correctness | Y091 | Protocol methods should not have positional-or-keyword parameters. Usually, a positional-only parameter is better. | Correctness -| Y092 | Pseudo-protocols `Sequence`, `Mapping`, or `MutableMapping` should not be used as argument types. Using a protocol is preferred. | Correctness +| Y092 | Pseudo-protocols like `Sequence` or `Mapping` should not be used as argument types. Using a protocol is preferred. | Correctness diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index 6bd48e9..041ef6f 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -138,7 +138,14 @@ def all_equal(iterable: Iterable[object]) -> bool: } ) -PSEUDO_PROTOCOLS = {"Sequence", "Mapping", "MutableMapping"} +PSEUDO_PROTOCOLS = { + "Sequence", + "MutableSequence", + "Mapping", + "MutableMapping", + "Set", + "MutableSet" +} def _ast_node_for(string: str) -> ast.AST: From 06fc42427b9d8d858bfdf13cd6911af80615416c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 3 Sep 2025 10:09:43 +0000 Subject: [PATCH 6/7] [pre-commit.ci] auto fixes from pre-commit.com hooks --- flake8_pyi/visitor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index 041ef6f..b6ec918 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -144,7 +144,7 @@ def all_equal(iterable: Iterable[object]) -> bool: "Mapping", "MutableMapping", "Set", - "MutableSet" + "MutableSet", } From 5eebca6018717a733c99fc26a95a81ac4568e725 Mon Sep 17 00:00:00 2001 From: Sebastian Rittau Date: Tue, 30 Sep 2025 18:51:39 +0200 Subject: [PATCH 7/7] Y092 -> Y093 (since Y092 was used previously) --- ERRORCODES.md | 2 +- flake8_pyi/errors.py | 6 +++--- flake8_pyi/visitor.py | 2 +- tests/pseudo_protocols.pyi | 18 +++++++++--------- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ERRORCODES.md b/ERRORCODES.md index c2f8be6..a15c8ff 100644 --- a/ERRORCODES.md +++ b/ERRORCODES.md @@ -98,4 +98,4 @@ recommend only using `--extend-select`, never `--select`. |------|-------------|--------------- | 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`". | Correctness | Y091 | Protocol methods should not have positional-or-keyword parameters. Usually, a positional-only parameter is better. | Correctness -| Y092 | Pseudo-protocols like `Sequence` or `Mapping` should not be used as argument types. Using a protocol is preferred. | Correctness +| Y093 | Pseudo-protocols like `Sequence` or `Mapping` should not be used as argument types. Using a protocol is preferred. | Correctness diff --git a/flake8_pyi/errors.py b/flake8_pyi/errors.py index cdb2563..0f90c67 100644 --- a/flake8_pyi/errors.py +++ b/flake8_pyi/errors.py @@ -138,8 +138,8 @@ class Error(NamedTuple): 'Y091 Argument "{arg}" to protocol method "{method}" should probably not be positional-or-keyword. ' "Make it positional-only, since usually you don't want to mandate a specific argument name" ) -Y092 = ( - 'Y092 Don\'t use pseudo-protocol "{arg}" as parameter type. Use a protocol instead.' +Y093 = ( + 'Y093 Don\'t use pseudo-protocol "{arg}" as parameter type. Use a protocol instead.' ) -DISABLED_BY_DEFAULT = ["Y090", "Y091", "Y092"] +DISABLED_BY_DEFAULT = ["Y090", "Y091", "Y093"] diff --git a/flake8_pyi/visitor.py b/flake8_pyi/visitor.py index daf99eb..7368aee 100644 --- a/flake8_pyi/visitor.py +++ b/flake8_pyi/visitor.py @@ -2148,7 +2148,7 @@ def _check_pseudo_protocol(self, node: ast.expr | None) -> None: self._check_pseudo_protocol(node.right) for name in PSEUDO_PROTOCOLS: if _is_object(node, name, from_=_TYPING_OR_COLLECTIONS_ABC): - self.error(node, errors.Y092.format(arg=name)) + self.error(node, errors.Y093.format(arg=name)) def check_arg_default(self, arg: ast.arg, default: ast.expr | None) -> None: self.visit(arg) diff --git a/tests/pseudo_protocols.pyi b/tests/pseudo_protocols.pyi index dd513e9..66a7a60 100644 --- a/tests/pseudo_protocols.pyi +++ b/tests/pseudo_protocols.pyi @@ -1,4 +1,4 @@ -# flags: --extend-select=Y092 +# flags: --extend-select=Y093 # Tests for pseudo-protocols like `Sequence`, `Mapping`, or `MutableMapping` # imported from collections.abc. @@ -15,14 +15,14 @@ from collections.abc import ( Sequence, ) -def test_sequence(seq: Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. -def test_mapping(mapping: Mapping[str, int]) -> None: ... # Y092 Don't use pseudo-protocol "Mapping" as parameter type. Use a protocol instead. -def test_mutable_mapping(mapping: MutableMapping[str, int]) -> None: ... # Y092 Don't use pseudo-protocol "MutableMapping" as parameter type. Use a protocol instead. +def test_sequence(seq: Sequence[int]) -> None: ... # Y093 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_mapping(mapping: Mapping[str, int]) -> None: ... # Y093 Don't use pseudo-protocol "Mapping" as parameter type. Use a protocol instead. +def test_mutable_mapping(mapping: MutableMapping[str, int]) -> None: ... # Y093 Don't use pseudo-protocol "MutableMapping" as parameter type. Use a protocol instead. def test_import_alias(mapping: MyMapping[str, int]) -> None: ... # TODO: import aliases are currently not supported. -def test_plain(seq: Sequence) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. -def test_union(arg: Sequence[int] | int) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. -def test_nested(arg: list[Sequence[int]]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. -def test_full_type(seq: collections.abc.Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_plain(seq: Sequence) -> None: ... # Y093 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_union(arg: Sequence[int] | int) -> None: ... # Y093 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_nested(arg: list[Sequence[int]]) -> None: ... # Y093 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. +def test_full_type(seq: collections.abc.Sequence[int]) -> None: ... # Y093 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. x: Sequence[int] # ok def test_iterable(it: Iterable[str]) -> None: ... # ok @@ -31,4 +31,4 @@ def test_as_return_type() -> Sequence[int]: ... # ok class Foo: x: Sequence[int] # ok - def test_method(self, seq: Sequence[int]) -> None: ... # Y092 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead. + def test_method(self, seq: Sequence[int]) -> None: ... # Y093 Don't use pseudo-protocol "Sequence" as parameter type. Use a protocol instead.