Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions doc/data/messages/m/match-class-bind-self/bad.py
Original file line number Diff line number Diff line change
@@ -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]
...
14 changes: 14 additions & 0 deletions doc/data/messages/m/match-class-bind-self/good.py
Original file line number Diff line number Diff line change
@@ -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):
...
1 change: 1 addition & 0 deletions doc/data/messages/m/match-class-bind-self/related.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- `Python documentation <https://docs.python.org/3/reference/compound_stmts.html#class-patterns>`_
12 changes: 12 additions & 0 deletions doc/data/messages/m/match-class-positional-attributes/bad.py
Original file line number Diff line number Diff line change
@@ -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]
...
12 changes: 12 additions & 0 deletions doc/data/messages/m/match-class-positional-attributes/good.py
Original file line number Diff line number Diff line change
@@ -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):
...
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- `Python documentation <https://docs.python.org/3/reference/compound_stmts.html#class-patterns>`_
6 changes: 6 additions & 0 deletions doc/user_guide/checkers/features.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions doc/user_guide/messages/messages_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions doc/whatsnew/fragments/10586.new_check
Original file line number Diff line number Diff line change
@@ -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
69 changes: 69 additions & 0 deletions pylint/checkers/match_statements_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Use '%s() as %s' instead",
"Use '%s() as %s' instead, if performance is a concern",

(This is a suggestion, the message is short, there's room for explanation in it, maybe ?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems Daniel merged it before I saw your comment 😄
Anyway, not sure we need an extra comment here. I did think about moving these checks to an extension (e.g. CodeStyle. However, there are clear performance benefits which is why I decided to keep them always enabled.

If we get reports about them, we could still consider amending the message or moving the check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry! I saw Pierre's approval and thought it was a comment from his earlier review.

Will be more careful next time!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, it was good to merge which is why I approved in the first place 😄

"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")
Expand Down Expand Up @@ -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."""
Expand All @@ -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 ["<self>"]
return None

match match_args:
Expand Down Expand Up @@ -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",
)
Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions tests/functional/ext/mccabe/mccabe.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check already making pylint better 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed most of the pylint code in #10580 already. That's why there are "only" two instances in a test case left here.

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"
Expand Down
40 changes: 40 additions & 0 deletions tests/functional/m/match_class_pattern.py
Original file line number Diff line number Diff line change
@@ -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:
Expand All @@ -18,13 +21,25 @@ 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:
case A(1): ...
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"""
Expand All @@ -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): ...
20 changes: 13 additions & 7 deletions tests/functional/m/match_class_pattern.txt
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions tests/functional/p/pattern_matching.py
Original file line number Diff line number Diff line change
@@ -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")
Expand Down
Loading