Skip to content

Use match statement in checkers (1) #10514

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 31, 2025
Merged

Use match statement in checkers (1) #10514

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffed4b5
Use match statement in checkers (1)
cdce8p Aug 22, 2025
ed93da5
Revert mypy disable
cdce8p Aug 29, 2025
3b460ee
Revert questionable tuple match
cdce8p Aug 29, 2025
b7938b1
Use better variable names
cdce8p Aug 29, 2025
e734457
Remove unnecessary variable binding
cdce8p Aug 29, 2025
b3d1654
Add back code comment
cdce8p Aug 29, 2025
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
4 changes: 2 additions & 2 deletions pylint/checkers/match_statements_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ def visit_match(self, node: nodes.Match) -> None:
"""
for idx, case in enumerate(node.cases):
match case.pattern:
case nodes.MatchAs(pattern=None, name=nodes.AssignName()) if (
case nodes.MatchAs(pattern=None, name=nodes.AssignName(name=n)) if (
Copy link
Member Author

Choose a reason for hiding this comment

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

If we bind variables, what should the variable name be? A short-form like n here or better name? Maybe both is also an option?

Not sure where I picked this up from but using short-forms has somewhat grown on me. It's especially useful if we bind multiple variables in one case. Say nodes.Subscript(slice=s, value=v). Furthermore it might help reduce the ambiguity if the keyword name is used for the variable name as well, e.g. name=name. It also reminds me a bit of indices in loops with also just use i, j, so it might feel natural to look for the name in the match case.

Then again just name might still be better in the case body.

--
Another factor could be if it's just a generic name or a descriptive one. As example, one of the other example in this PR uses next_if_node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally prefer more descriptive names, especially since match ... case can become quite long and I quickly lose context in the Github review view.

But if others prefer shorthand, that is also fine. I don't feel strongly.

idx < len(node.cases) - 1
):
self.add_message(
"bare-name-capture-pattern",
node=case.pattern,
args=case.pattern.name.name,
args=n,
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though case.pattern.name.name is correctly narrowed by the match case, both pyright and mypy have problems inferring the type correctly. This can be resolved / worked around if the name is bound in the match case.

Note: Mypy will always infer it as Any since astroid is untyped, but even it it wasn't wouldn't be able to infer case .pattern.name.name probably at the moment.

confidence=HIGH,
)

Expand Down
35 changes: 18 additions & 17 deletions pylint/extensions/_check_docs_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,23 +133,24 @@ def possible_exc_types(node: nodes.NodeNG) -> set[nodes.ClassDef]:
except astroid.InferenceError:
pass
else:
target = _get_raise_target(node)
if isinstance(target, nodes.ClassDef):
exceptions = [target]
elif isinstance(target, nodes.FunctionDef):
for ret in target.nodes_of_class(nodes.Return):
if ret.value is None:
continue
if ret.frame() != target:
# return from inner function - ignore it
continue

val = utils.safe_infer(ret.value)
if val and utils.inherit_from_std_ex(val):
if isinstance(val, nodes.ClassDef):
exceptions.append(val)
elif isinstance(val, astroid.Instance):
exceptions.append(val.getattr("__class__")[0])
match target := _get_raise_target(node):
case nodes.ClassDef():
exceptions = [target]
case nodes.FunctionDef():
for ret in target.nodes_of_class(nodes.Return):
if ret.value is None:
continue
if ret.frame() != target:
# return from inner function - ignore it
continue

val = utils.safe_infer(ret.value)
if val and utils.inherit_from_std_ex(val):
match val:
case nodes.ClassDef():
exceptions.append(val)
case astroid.Instance():
exceptions.append(val.getattr("__class__")[0])

try:
return {
Expand Down
13 changes: 7 additions & 6 deletions pylint/extensions/code_style.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,12 +309,13 @@ def _check_ignore_assignment_expr_suggestion(
if isinstance(node.test, nodes.Compare):
next_if_node: nodes.If | None = None
next_sibling = node.next_sibling()
if len(node.orelse) == 1 and isinstance(node.orelse[0], nodes.If):
# elif block
next_if_node = node.orelse[0]
elif isinstance(next_sibling, nodes.If):
# separate if block
next_if_node = next_sibling
match (node.orelse, next_sibling):
case [[nodes.If() as next_if_node], _]:
# elif block
pass
case [_, nodes.If() as next_if_node]:
# separate if block
pass
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about this one. It works 🤷🏻‍♂️ There are a few things to note though.

Only variable binding - no body
So far I've seen quite a few cases where it might make sense to "abuse" match to just do the variable binding. I considered using ... as body to make it more obvious that's what's happing but that doesn't look good / feels wrong. Maybe just pass is still the best option.

Performance
For the match to work here, I matched each case against a constructed sequence. The plain if-elif is probably a bit faster though I haven't profiled it yet. Ideally something like a match expression would be added to Python at some point. Something like

if node.orelse match [nodes.If() as next_if_node]:
    pass
elif isinstance(next_sibling, nodes.If):
    ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'm a bit on the fence on this one as well. The original code isn't much better, but the explicit matching on a tuple only to then disregard 50% of the tuple in both cases feels a bit "meh"... But I can't come up with a better suggestion, so fine to merge for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted this one. No need to force it with the match statement. If still works fine.


if ( # pylint: disable=too-many-boolean-expressions
next_if_node is not None
Expand Down
13 changes: 7 additions & 6 deletions pylint/extensions/for_any_all.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,13 @@ def _build_suggested_string(node: nodes.For, final_return_bool: bool) -> str:
loop_iter = node.iter.as_string()
test_node = next(node.body[0].get_children())

if isinstance(test_node, nodes.UnaryOp) and test_node.op == "not":
# The condition is negated. Advance the node to the operand and modify the suggestion
test_node = test_node.operand
suggested_function = "all" if final_return_bool else "not all"
else:
suggested_function = "not any" if final_return_bool else "any"
match test_node:
case nodes.UnaryOp(op="not"):
# The condition is negated. Advance the node to the operand and modify the suggestion
test_node = test_node.operand
suggested_function = "all" if final_return_bool else "not all"
case _:
suggested_function = "not any" if final_return_bool else "any"

test = test_node.as_string()
return f"{suggested_function}({test} for {loop_var} in {loop_iter})"
Expand Down
52 changes: 26 additions & 26 deletions pylint/extensions/private_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,14 +147,15 @@ def _populate_type_annotations(
if isinstance(usage_node, nodes.AssignName) and isinstance(
usage_node.parent, (nodes.AnnAssign, nodes.Assign)
):
assign_parent = usage_node.parent
if isinstance(assign_parent, nodes.AnnAssign):
name_assignments.append(assign_parent)
private_name = self._populate_type_annotations_annotation(
usage_node.parent.annotation, all_used_type_annotations
)
elif isinstance(assign_parent, nodes.Assign):
name_assignments.append(assign_parent)
match assign_parent := usage_node.parent:
case nodes.AnnAssign():
name_assignments.append(assign_parent)
private_name = self._populate_type_annotations_annotation(
assign_parent.annotation,
all_used_type_annotations,
)
case nodes.Assign():
name_assignments.append(assign_parent)

if isinstance(usage_node, nodes.FunctionDef):
self._populate_type_annotations_function(
Expand Down Expand Up @@ -194,24 +195,23 @@ def _populate_type_annotations_annotation(
"""Handles the possibility of an annotation either being a Name, i.e. just type,
or a Subscript e.g. `Optional[type]` or an Attribute, e.g. `pylint.lint.linter`.
"""
if isinstance(node, nodes.Name) and node.name not in all_used_type_annotations:
all_used_type_annotations[node.name] = True
return node.name # type: ignore[no-any-return]
if isinstance(node, nodes.Subscript): # e.g. Optional[List[str]]
# slice is the next nested type
self._populate_type_annotations_annotation(
node.slice, all_used_type_annotations
)
# value is the current type name: could be a Name or Attribute
return self._populate_type_annotations_annotation(
node.value, all_used_type_annotations
)
if isinstance(node, nodes.Attribute):
# An attribute is a type like `pylint.lint.pylinter`. node.expr is the next level
# up, could be another attribute
return self._populate_type_annotations_annotation(
node.expr, all_used_type_annotations
)
match node:
case nodes.Name(name=n) if n not in all_used_type_annotations:
all_used_type_annotations[n] = True
return n # type: ignore[no-any-return]
case nodes.Subscript(slice=s, value=v):
# slice is the next nested type
self._populate_type_annotations_annotation(s, all_used_type_annotations)
# value is the current type name: could be a Name or Attribute
return self._populate_type_annotations_annotation(
v, all_used_type_annotations
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the use of s and v here as I mentioned earlier. Would replacing s with slice / v with value be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For me it would!

Copy link
Member Author

@cdce8p cdce8p Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Replaced most variable names with their long form. Incidentally not s since slice is also a builtin name.

Update
It might even make more sense to just continue using node.slice and node.value here. Though I haven't tested it yet, it could be a small performance improvement as we can skip hasattr checks which are implicit in the match statement.

case nodes.Attribute(expr=e):
# An attribute is a type like `pylint.lint.pylinter`. node.expr is the next level
# up, could be another attribute
return self._populate_type_annotations_annotation(
e, all_used_type_annotations
)
return None

@staticmethod
Expand Down
5 changes: 5 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ scripts_are_modules = true
warn_unused_ignores = true
show_error_codes = true
enable_error_code = "ignore-without-code"
disable_error_code = [
# TODO: remove once match false-positives are fixed, probably in v1.18
# https://github.com/python/mypy/pull/19708
"has-type",
]
strict = true
# TODO: Remove this once pytest has annotations
disallow_untyped_decorators = false
Expand Down
Loading