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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+65 −57
Open

Use match statement in checkers (1) #10514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Comment on lines -312 to +318
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.


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
)
Comment on lines +202 to +208
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!

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 @@ -207,6 +207,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",
Comment on lines +211 to +213
Copy link
Member Author

Choose a reason for hiding this comment

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

Mypy errors if we don't disable has-type:

pylint/extensions/private_import.py:199: error: Cannot determine type of "n"  [has-type]
pylint/extensions/private_import.py:200: error: Cannot determine type of "n"  [has-type]
pylint/extensions/private_import.py:201: error: Cannot determine type of "n"  [has-type]
pylint/extensions/private_import.py:201: note: Error code "has-type" not covered by "type: ignore" comment
pylint/extensions/private_import.py:204: error: Cannot determine type of "s"  [has-type]
pylint/extensions/private_import.py:207: error: Cannot determine type of "v"  [has-type]
pylint/extensions/private_import.py:213: error: Cannot determine type of "e"  [has-type]
pylint/checkers/match_statements_checker.py:46: error: Cannot determine type of "n"  [has-type]

Basically every time we do a variable binding inside a match case and use set variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels dangerous to disable. Normally it means you're doing something very estoric that you shouldn't be doing anyway.

In this case that is obviously not true but we shouldn't forget to remove it once we can!

Copy link
Member Author

Choose a reason for hiding this comment

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

The fix is already merged python/mypy#19708. Just not sure when the next release will be though.

]
strict = true
# TODO: Remove this once pytest has annotations
disallow_untyped_decorators = false
Expand Down
Loading