diff --git a/doc/data/messages/m/match-class-bind-self/bad.py b/doc/data/messages/m/match-class-bind-self/bad.py new file mode 100644 index 0000000000..a2919b72ff --- /dev/null +++ b/doc/data/messages/m/match-class-bind-self/bad.py @@ -0,0 +1,14 @@ +class Book: + __match_args__ = ("title", "year") + + def __init__(self, title, year): + self.title = title + self.year = year + + +def func(item: Book): + match item: + case Book(title=str(title)): # [match-class-bind-self] + ... + case Book(year=int(year)): # [match-class-bind-self] + ... diff --git a/doc/data/messages/m/match-class-bind-self/good.py b/doc/data/messages/m/match-class-bind-self/good.py new file mode 100644 index 0000000000..df2eceda98 --- /dev/null +++ b/doc/data/messages/m/match-class-bind-self/good.py @@ -0,0 +1,14 @@ +class Book: + __match_args__ = ("title", "year") + + def __init__(self, title, year): + self.title = title + self.year = year + + +def func(item: Book): + match item: + case Book(title=str() as title): + ... + case Book(year=int() as year): + ... diff --git a/doc/data/messages/m/match-class-bind-self/related.rst b/doc/data/messages/m/match-class-bind-self/related.rst new file mode 100644 index 0000000000..f10342b564 --- /dev/null +++ b/doc/data/messages/m/match-class-bind-self/related.rst @@ -0,0 +1 @@ +- `Python documentation `_ diff --git a/doc/data/messages/m/match-class-positional-attributes/bad.py b/doc/data/messages/m/match-class-positional-attributes/bad.py new file mode 100644 index 0000000000..69013b3f56 --- /dev/null +++ b/doc/data/messages/m/match-class-positional-attributes/bad.py @@ -0,0 +1,12 @@ +class Book: + __match_args__ = ("title", "year") + + def __init__(self, title, year): + self.title = title + self.year = year + + +def func(item: Book): + match item: + case Book("abc", 2000): # [match-class-positional-attributes] + ... diff --git a/doc/data/messages/m/match-class-positional-attributes/good.py b/doc/data/messages/m/match-class-positional-attributes/good.py new file mode 100644 index 0000000000..93c567bbf1 --- /dev/null +++ b/doc/data/messages/m/match-class-positional-attributes/good.py @@ -0,0 +1,12 @@ +class Book: + __match_args__ = ("title", "year") + + def __init__(self, title, year): + self.title = title + self.year = year + + +def func(item: Book): + match item: + case Book(title="abc", year=2000): + ... diff --git a/doc/data/messages/m/match-class-positional-attributes/related.rst b/doc/data/messages/m/match-class-positional-attributes/related.rst new file mode 100644 index 0000000000..f10342b564 --- /dev/null +++ b/doc/data/messages/m/match-class-positional-attributes/related.rst @@ -0,0 +1 @@ +- `Python documentation `_ diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index f0da6353b4..ef2424522d 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -694,6 +694,12 @@ Match Statements checker Messages :multiple-class-sub-patterns (E1904): *Multiple sub-patterns for attribute %s* Emitted when there is more than one sub-pattern for a specific attribute in a class pattern. +:match-class-bind-self (R1905): *Use '%s() as %s' instead* + Match class patterns are faster if the name binding happens for the whole + pattern and any lookup for `__match_args__` can be avoided. +:match-class-positional-attributes (R1906): *Use keyword attributes instead of positional ones (%s)* + Keyword attributes are more explicit and slightly faster since CPython can + skip the `__match_args__` lookup. Method Args checker diff --git a/doc/user_guide/messages/messages_overview.rst b/doc/user_guide/messages/messages_overview.rst index f71738c83c..5b1378474e 100644 --- a/doc/user_guide/messages/messages_overview.rst +++ b/doc/user_guide/messages/messages_overview.rst @@ -518,6 +518,8 @@ All messages in the refactor category: refactor/inconsistent-return-statements refactor/literal-comparison refactor/magic-value-comparison + refactor/match-class-bind-self + refactor/match-class-positional-attributes refactor/no-classmethod-decorator refactor/no-else-break refactor/no-else-continue diff --git a/doc/whatsnew/fragments/10586.new_check b/doc/whatsnew/fragments/10586.new_check new file mode 100644 index 0000000000..4d1e2ae17d --- /dev/null +++ b/doc/whatsnew/fragments/10586.new_check @@ -0,0 +1,8 @@ +Add additional checks for suboptimal uses of class patterns in :keyword:`match`. + +* :ref:`match-class-bind-self` is emitted if a name is bound to ``self`` instead of + using an ``as`` pattern. +* :ref:`match-class-positional-attributes` is emitted if a class pattern has positional + attributes when keywords could be used. + +Refs #10586 diff --git a/pylint/checkers/match_statements_checker.py b/pylint/checkers/match_statements_checker.py index 70788803e8..011fb3235a 100644 --- a/pylint/checkers/match_statements_checker.py +++ b/pylint/checkers/match_statements_checker.py @@ -19,6 +19,23 @@ from pylint.lint import PyLinter +# List of builtin classes which match self +# https://docs.python.org/3/reference/compound_stmts.html#class-patterns +MATCH_CLASS_SELF_NAMES = { + "builtins.bool", + "builtins.bytearray", + "builtins.bytes", + "builtins.dict", + "builtins.float", + "builtins.frozenset", + "builtins.int", + "builtins.list", + "builtins.set", + "builtins.str", + "builtins.tuple", +} + + class MatchStatementChecker(BaseChecker): name = "match_statements" msgs = { @@ -46,6 +63,19 @@ class MatchStatementChecker(BaseChecker): "Emitted when there is more than one sub-pattern for a specific " "attribute in a class pattern.", ), + "R1905": ( + "Use '%s() as %s' instead", + "match-class-bind-self", + "Match class patterns are faster if the name binding happens " + "for the whole pattern and any lookup for `__match_args__` " + "can be avoided.", + ), + "R1906": ( + "Use keyword attributes instead of positional ones (%s)", + "match-class-positional-attributes", + "Keyword attributes are more explicit and slightly faster " + "since CPython can skip the `__match_args__` lookup.", + ), } @only_required_for_messages("invalid-match-args-definition") @@ -86,6 +116,26 @@ def visit_match(self, node: nodes.Match) -> None: confidence=HIGH, ) + @only_required_for_messages("match-class-bind-self") + def visit_matchas(self, node: nodes.MatchAs) -> None: + match node: + case nodes.MatchAs( + parent=nodes.MatchClass(cls=nodes.Name() as cls_name, patterns=[_]), + name=nodes.AssignName(name=name), + pattern=None, + ): + inferred = safe_infer(cls_name) + if ( + isinstance(inferred, nodes.ClassDef) + and inferred.qname() in MATCH_CLASS_SELF_NAMES + ): + self.add_message( + "match-class-bind-self", + node=node, + args=(cls_name.name, name), + confidence=HIGH, + ) + @staticmethod def get_match_args_for_class(node: nodes.NodeNG) -> list[str] | None: """Infer __match_args__ from class name.""" @@ -95,6 +145,8 @@ def get_match_args_for_class(node: nodes.NodeNG) -> list[str] | None: try: match_args = inferred.getattr("__match_args__") except astroid.exceptions.NotFoundError: + if inferred.qname() in MATCH_CLASS_SELF_NAMES: + return [""] return None match match_args: @@ -124,6 +176,7 @@ def check_duplicate_sub_patterns( attrs.add(name) @only_required_for_messages( + "match-class-positional-attributes", "multiple-class-sub-patterns", "too-many-positional-sub-patterns", ) @@ -144,6 +197,22 @@ def visit_matchclass(self, node: nodes.MatchClass) -> None: ) return + inferred = safe_infer(node.cls) + if not ( + isinstance(inferred, nodes.ClassDef) + and ( + inferred.qname() in MATCH_CLASS_SELF_NAMES + or "tuple" in inferred.basenames + ) + ): + attributes = [f"'{attr}'" for attr in match_args[: len(node.patterns)]] + self.add_message( + "match-class-positional-attributes", + node=node, + args=(", ".join(attributes),), + confidence=INFERENCE, + ) + for i in range(len(node.patterns)): name = match_args[i] self.check_duplicate_sub_patterns(name, node, attrs=attrs, dups=dups) diff --git a/tests/functional/ext/mccabe/mccabe.py b/tests/functional/ext/mccabe/mccabe.py index 9cb69735f0..851fb6db7f 100644 --- a/tests/functional/ext/mccabe/mccabe.py +++ b/tests/functional/ext/mccabe/mccabe.py @@ -327,9 +327,9 @@ def nested_match_case(data): # [too-complex] match data: case {"type": "user", "data": user_data}: match user_data: # Nested match adds complexity - case {"name": str(name)}: + case {"name": str() as name}: return f"User: {name}" - case {"id": int(user_id)}: + case {"id": int() as user_id}: return f"User ID: {user_id}" case _: return "Unknown user format" diff --git a/tests/functional/m/match_class_pattern.py b/tests/functional/m/match_class_pattern.py index 0e1cd3826a..3b227c9fdc 100644 --- a/tests/functional/m/match_class_pattern.py +++ b/tests/functional/m/match_class_pattern.py @@ -1,4 +1,7 @@ # pylint: disable=missing-docstring,unused-variable,too-few-public-methods +# pylint: disable=match-class-positional-attributes + +from typing import NamedTuple # -- Check __match_args__ definitions -- class A: @@ -18,6 +21,12 @@ def f(self): __match_args__ = ["x"] +class Result(NamedTuple): + # inherits from tuple -> match self + x: int + y: int + + def f1(x): """Check too many positional sub-patterns""" match x: @@ -25,6 +34,12 @@ def f1(x): case A(1, 2): ... # [too-many-positional-sub-patterns] case B(1, 2): ... case B(1, 2, 3): ... # [too-many-positional-sub-patterns] + case int(1): ... + case int(1, 2): ... # [too-many-positional-sub-patterns] + case tuple(1): ... + case tuple(1, 2): ... # [too-many-positional-sub-patterns] + case tuple((1, 2)): ... + case Result(1, 2): ... def f2(x): """Check multiple sub-patterns for attribute""" @@ -38,3 +53,28 @@ def f2(x): # If class name is undefined, we can't get __match_args__ case NotDefined(1, x=1): ... # [undefined-variable] + +def f3(x): + """Check class pattern with name binding to self.""" + match x: + case int(y): ... # [match-class-bind-self] + case int() as y: ... + case int(2 as y): ... + case str(y): ... # [match-class-bind-self] + case str() as y: ... + case str("Hello" as y): ... + case tuple(y, 2): ... # pylint: disable=too-many-positional-sub-patterns + case tuple((y, 2)): ... + +def f4(x): + """Check for positional attributes if keywords could be used.""" + # pylint: enable=match-class-positional-attributes + match x: + case int(2): ... + case bool(True): ... + case A(1): ... # [match-class-positional-attributes] + case A(x=1): ... + case B(1, 2): ... # [match-class-positional-attributes] + case B(x=1, y=2): ... + case Result(1, 2): ... + case Result(x=1, y=2): ... diff --git a/tests/functional/m/match_class_pattern.txt b/tests/functional/m/match_class_pattern.txt index c77d5d2891..2ab0727468 100644 --- a/tests/functional/m/match_class_pattern.txt +++ b/tests/functional/m/match_class_pattern.txt @@ -1,7 +1,13 @@ -invalid-match-args-definition:11:21:11:31:C:`__match_args__` must be a tuple of strings.:HIGH -invalid-match-args-definition:14:21:14:29:D:`__match_args__` must be a tuple of strings.:HIGH -too-many-positional-sub-patterns:25:13:25:20:f1:A expects 1 positional sub-patterns (given 2):INFERENCE -too-many-positional-sub-patterns:27:13:27:23:f1:B expects 2 positional sub-patterns (given 3):INFERENCE -multiple-class-sub-patterns:32:13:32:22:f2:Multiple sub-patterns for attribute x:INFERENCE -multiple-class-sub-patterns:34:13:34:29:f2:Multiple sub-patterns for attribute x:INFERENCE -undefined-variable:40:13:40:23:f2:Undefined variable 'NotDefined':UNDEFINED +invalid-match-args-definition:14:21:14:31:C:`__match_args__` must be a tuple of strings.:HIGH +invalid-match-args-definition:17:21:17:29:D:`__match_args__` must be a tuple of strings.:HIGH +too-many-positional-sub-patterns:34:13:34:20:f1:A expects 1 positional sub-patterns (given 2):INFERENCE +too-many-positional-sub-patterns:36:13:36:23:f1:B expects 2 positional sub-patterns (given 3):INFERENCE +too-many-positional-sub-patterns:38:13:38:22:f1:int expects 1 positional sub-patterns (given 2):INFERENCE +too-many-positional-sub-patterns:40:13:40:24:f1:tuple expects 1 positional sub-patterns (given 2):INFERENCE +multiple-class-sub-patterns:47:13:47:22:f2:Multiple sub-patterns for attribute x:INFERENCE +multiple-class-sub-patterns:49:13:49:29:f2:Multiple sub-patterns for attribute x:INFERENCE +undefined-variable:55:13:55:23:f2:Undefined variable 'NotDefined':UNDEFINED +match-class-bind-self:60:17:60:18:f3:Use 'int() as y' instead:HIGH +match-class-bind-self:63:17:63:18:f3:Use 'str() as y' instead:HIGH +match-class-positional-attributes:75:13:75:17:f4:Use keyword attributes instead of positional ones ('x'):INFERENCE +match-class-positional-attributes:77:13:77:20:f4:Use keyword attributes instead of positional ones ('x', 'y'):INFERENCE diff --git a/tests/functional/p/pattern_matching.py b/tests/functional/p/pattern_matching.py index 4a5884812b..d590748208 100644 --- a/tests/functional/p/pattern_matching.py +++ b/tests/functional/p/pattern_matching.py @@ -1,4 +1,5 @@ # pylint: disable=missing-docstring,invalid-name,too-few-public-methods +# pylint: disable=match-class-positional-attributes class Point2D: __match_args__ = ("x", "y")