-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add additional checks for match class patterns #10587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
42b289d
279e8cc
3ce613a
a8e71f1
768c4c9
5502cd3
96cf94e
9ba694b
f420f28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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] | ||
... |
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): | ||
... |
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>`_ |
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] | ||
... |
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>`_ |
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 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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' 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", | ||||||
|
"Use keyword attributes instead of positional ones", | |
"Use keyword attributes instead of positional ones ('%s')", |
I think we can construct the expected here too. (And one day use it to autofix?)
cdce8p marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
cdce8p marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the sentry
results, I do wonder if this is to strict and we'd end up emitting too many false positives. self
matches also work for every subclass of the special builtin classes (as long as they don't implement __match_args__
). Most notably NamedTuple
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some special casing for tuple
that should handle it.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check already making pylint better 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
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:12:21:12:31:C:`__match_args__` must be a tuple of strings.:HIGH | ||
invalid-match-args-definition:15:21:15:29:D:`__match_args__` must be a tuple of strings.:HIGH | ||
too-many-positional-sub-patterns:26:13:26:20:f1:A expects 1 positional sub-patterns (given 2):INFERENCE | ||
too-many-positional-sub-patterns:28:13:28:23:f1:B expects 2 positional sub-patterns (given 3):INFERENCE | ||
too-many-positional-sub-patterns:30:13:30:22:f1:int expects 1 positional sub-patterns (given 2):INFERENCE | ||
multiple-class-sub-patterns:35:13:35:22:f2:Multiple sub-patterns for attribute x:INFERENCE | ||
multiple-class-sub-patterns:37:13:37:29:f2:Multiple sub-patterns for attribute x:INFERENCE | ||
undefined-variable:43:13:43:23:f2:Undefined variable 'NotDefined':UNDEFINED | ||
match-class-bind-self:48:17:48:18:f3:Use 'int() as y' instead:HIGH | ||
match-class-bind-self:51:17:51:18:f3:Use 'str() as y' instead:HIGH | ||
match-class-bind-self:54:19:54:20:f3:Use 'tuple() as y' instead:HIGH | ||
match-class-positional-attributes:63:13:63:17:f4:Use keyword attributes instead of positional ones:HIGH | ||
match-class-positional-attributes:65:13:65:20:f4:Use keyword attributes instead of positional ones:HIGH |
Uh oh!
There was an error while loading. Please reload this page.