Add Y091: Protocol method arg should not be pos-or-kw (#442)

Fixes #441

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Shantanu <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Change Log
 
+## Unreleased
+
+New error codes:
+* Introduce Y091: Protocol method parameters should not be positional-or-keyword.
+
 ## 24.9.0
 
 Bugfixes

ERRORCODES.md

@@ -95,3 +95,4 @@ recommend only using `--extend-select`, never `--select`.
 | Code | Description | Code category
 |------|-------------|---------------
 | <a id="Y090" href="#Y090">Y090</a> | `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
+| <a id="Y091" href="#Y091">Y091</a> | Protocol methods should not have positional-or-keyword parameters. Usually, a positional-only parameter is better.

pyi.py

@@ -1742,7 +1742,9 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None:
         self.generic_visit(node)
         self._check_class_bases(node.bases)
         self.enclosing_class_ctx = old_context
+        self.check_class_pass_and_ellipsis(node)
 
+    def check_class_pass_and_ellipsis(self, node: ast.ClassDef) -> None:
         # empty class body should contain "..." not "pass"
         if len(node.body) == 1:
             statement = node.body[0]
@@ -2086,7 +2088,11 @@ def _check_class_method_for_bad_typevars(
         ):
             self._Y019_error(method, cls_typevar)
 
-    def check_self_typevars(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None:
+    def check_self_typevars(
+        self,
+        node: ast.FunctionDef | ast.AsyncFunctionDef,
+        decorator_names: Container[str],
+    ) -> None:
         pos_or_keyword_args = node.args.posonlyargs + node.args.args
 
         if not pos_or_keyword_args:
@@ -2100,12 +2106,6 @@ def check_self_typevars(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> N
         if not isinstance(first_arg_annotation, (ast.Name, ast.Subscript)):
             return
 
-        decorator_names = {
-            decorator.id
-            for decorator in node.decorator_list
-            if isinstance(decorator, ast.Name)
-        }
-
         if "classmethod" in decorator_names or node.name == "__new__":
             self._check_class_method_for_bad_typevars(
                 method=node,
@@ -2121,6 +2121,20 @@ def check_self_typevars(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> N
                 return_annotation=return_annotation,
             )
 
+    def check_protocol_param_kinds(
+        self,
+        node: ast.FunctionDef | ast.AsyncFunctionDef,
+        decorator_names: Container[str],
+    ) -> None:
+        if "staticmethod" in decorator_names:
+            relevant_params = node.args.args
+        else:
+            relevant_params = node.args.args[1:]  # exclude "self"
+        for pos_or_kw in relevant_params:
+            if pos_or_kw.arg.startswith("__"):
+                continue
+            self.error(pos_or_kw, Y091.format(arg=pos_or_kw.arg, method=node.name))
+
     @staticmethod
     def _is_positional_pre_570_argname(name: str) -> bool:
         # https://peps.python.org/pep-0484/#positional-only-arguments
@@ -2175,7 +2189,14 @@ def _visit_function(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None:
 
         self._check_pep570_syntax_used_where_applicable(node)
         if self.enclosing_class_ctx is not None:
-            self.check_self_typevars(node)
+            decorator_names = {
+                decorator.id
+                for decorator in node.decorator_list
+                if isinstance(decorator, ast.Name)
+            }
+            self.check_self_typevars(node, decorator_names)
+            if self.enclosing_class_ctx.is_protocol_class:
+                self.check_protocol_param_kinds(node, decorator_names)
 
     def visit_arg(self, node: ast.arg) -> None:
         if _is_NoReturn(node.annotation):
@@ -2413,5 +2434,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 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"
+)
 
-DISABLED_BY_DEFAULT = ["Y090"]
+DISABLED_BY_DEFAULT = ["Y090", "Y091"]

tests/protocol_arg.pyi

@@ -0,0 +1,14 @@
+# flags: --extend-select=Y091
+from typing import Protocol
+
+class P(Protocol):
+    def method1(self, arg: int) -> None: ...  # Y091 Argument "arg" to protocol method "method1" should probably not be positional-or-keyword. Make it positional-only, since usually you don't want to mandate a specific argument name
+    def method2(self, arg: str, /) -> None: ...
+    def method3(self, *, arg: str) -> None: ...
+    def method4(self, arg: int, /) -> None: ...
+    def method5(self, arg: int, /, *, foo: str) -> None: ...
+    # Ensure Y091 recognizes this as pos-only for the benefit of users still
+    # using the old syntax.
+    def method6(self, __arg: int) -> None: ...  # Y063 Use PEP-570 syntax to indicate positional-only arguments
+    @staticmethod
+    def smethod1(arg: int) -> None: ...  # Y091 Argument "arg" to protocol method "smethod1" should probably not be positional-or-keyword. Make it positional-only, since usually you don't want to mandate a specific argument name