Use match statement in checkers (6) #10552

cdce8p · 2025-09-14T21:25:59Z

No description provided.

codecov · 2025-09-14T21:39:34Z

Codecov Report

❌ Patch coverage is 97.39583% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.93%. Comparing base (e694467) to head (adbae7b).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
pylint/checkers/design_analysis.py	60.00%	2 Missing ⚠️
pylint/checkers/refactoring/refactoring_checker.py	98.26%	2 Missing ⚠️
pylint/checkers/classes/class_checker.py	93.75%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (97.39%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10552      +/-   ##
==========================================
+ Coverage   95.90%   95.93%   +0.03%     
==========================================
  Files         176      176              
  Lines       19458    19454       -4     
==========================================
+ Hits        18661    18663       +2     
+ Misses        797      791       -6

Files with missing lines	Coverage Δ
pylint/checkers/deprecated.py	`99.07% <100.00%> (ø)`
pylint/checkers/newstyle.py	`96.07% <100.00%> (ø)`
...int/checkers/refactoring/recommendation_checker.py	`96.96% <100.00%> (+0.43%)`	⬆️
pylint/checkers/typecheck.py	`96.29% <100.00%> (+0.11%)`	⬆️
pylint/checkers/utils.py	`95.96% <100.00%> (ø)`
pylint/extensions/code_style.py	`100.00% <100.00%> (ø)`
pylint/extensions/consider_ternary_expression.py	`100.00% <100.00%> (ø)`
pylint/extensions/dict_init_mutate.py	`100.00% <100.00%> (ø)`
pylint/extensions/private_import.py	`100.00% <100.00%> (+0.82%)`	⬆️
pylint/extensions/typing.py	`97.88% <100.00%> (+0.03%)`	⬆️
... and 3 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Pierre-Sassoulas

The fact that it impact performance make me cautious and less eager to merge this kind of refactor. Don't get me wrong there's some banger where a structural pattern matching on astroids.Nodes transform 15 lines into 3... But we should definitely not try to force match everywhere and in particular for string matching I see negative value. (I didn't review everything, and stopped at the refactoring checker)

pylint/checkers/deprecated.py

Pierre-Sassoulas · 2025-09-21T06:07:42Z

pylint/checkers/refactoring/refactoring_checker.py

-        # "if <compare node>"
-        if not isinstance(node.test, nodes.Compare):
-            return False
-
-        # Does not have a single statement in the guard's body
-        if len(node.body) != 1:
-            return False
-
-        # Look for a single variable assignment on the LHS and a subscript on RHS
-        stmt = node.body[0]
-        if not (
-            isinstance(stmt, nodes.Assign)
-            and len(node.body[0].targets) == 1
-            and isinstance(node.body[0].targets[0], nodes.AssignName)
-            and isinstance(stmt.value, nodes.Subscript)
-        ):
-            return False
+        match node:
+            case nodes.If(
+                test=nodes.Compare() as test,
+                body=[
+                    nodes.Assign(
+                        targets=[nodes.AssignName()],
+                        value=nodes.Subscript(slice=slice_value, value=value),
+                    )
+                ],
+            ):
+                # "if <compare node>"
+                # Does not have a single statement in the guard's body
+                # Look for a single variable assignment on the LHS and a subscript on RHS
+                pass
+            case _:
+                return False


Pierre-Sassoulas · 2025-09-21T06:10:05Z

pylint/checkers/refactoring/refactoring_checker.py

+        match call_name:
+            case "any" | "all":
+                self.add_message(
+                    "use-a-generator",
+                    node=node,
+                    args=(call_name, inside_comp),
+                )
+            case _:
+                self.add_message(
+                    "consider-using-generator",
+                    node=node,
+                    args=(call_name, inside_comp),
+                )


Matching on string is always going to impact performance and having an if/elif is not worse... In fact I would argue it's better since the indentation is worse for match.

This particular example should actually be faster with match since the if used to create an inline set every time.

if call_name in {"any", "all"}: ...

Yes, performance for match is partially a concern, but it's really limited to the class and mapping patterns. All other ones are pretty close to there if equivalents if not even faster as the match subject just get's copied on the stack and doesn't need to be reloaded every time.

I don't know if we should go back to if call_name == "any" or call_name == "all": (it's not unreadable enough to warrant a decrease in perf) just for performances.

But I don't see benefits in readability of match agaisnt if x == "string" / elif x == "otherstring" to be honest, and there's going to be a lot of churn involved if we do the same thing for the whole codebase,

But I don't see benefits in readability of match agaisnt if x == "string" / elif x == "otherstring" to be honest, and there's going to be a lot of churn involved if we do the same thing for the whole codebase,

It can look quite nice, especially for more than two case, IMO. An example from an earlier (merged) PR, or just search for case "

pylint/pylint/checkers/format.py

Lines 354 to 373 in 47861c3

match token[1]:

case ",":

# This is a tuple, which is always acceptable.

return

case "and" | "or":

# 'and' and 'or' are the only boolean operators with lower precedence

# than 'not', so parens are only required when they are found.

found_and_or = True

case "yield":

# A yield inside an expression must always be in parentheses,

# quit early without error.

return

case "for":

# A generator expression always has a 'for' token in it, and

# the 'for' token is only legal inside parens when it is in a

# generator expression. The parens are necessary here, so bail

# without an error.

return

case "else":

# A generator expression can have an 'else' token in it.

Overall the churn also isn't as bad as one might think. The pattern isn't that common after all.

cdce8p · 2025-09-21T09:18:55Z

The fact that it impact performance make me cautious and less eager to merge this kind of refactor. Don't get me wrong there's some banger where a structural pattern matching on astroids.Nodes transform 15 lines into 3... But we should definitely not try to force match everywhere [...]

I do share your concerns. Unfortunately, even with my CPython PRs, I likely won't be able to fix the situation for 3.10-3.13. At best one of them might get backported to 3.14.1. That being said, the more I work with match especially in the context of pylint, the harder it gets to write the corresponding if statements. I just saw that yesterday as I was working on some more match checks. To me that just shows how much more natural and easier to understand they are compared to multiline if statements.

What I'm trying to say is that the performance impact is hopefully temporary but the readability and maintainability improvements are definitely real.

--
Btw my goal for the CPython PRs is that the class pattern (with only keyword arguments) will actually be faster than the comparable if statement (for Python 3.15). Unless I've missed something drastic, that should actually be archivable. I've already seen it in initial benchmarks.

Pierre-Sassoulas · 2025-09-21T11:22:16Z

I agree with changing to match for astroid nodes. But let's not take a lot of resource to use match on string either. 3.13 is going to last 4 years and there's a lot of things that we can do that are more valuable than rewriting old pylint code (making match faster in cpython, or creating new pylint check using match from the get go being some of them 😄 ). I had a look at your PR in cpython, great stuff !

cdce8p · 2025-09-21T11:35:39Z

pylint/checkers/refactoring/refactoring_checker.py

+        match name:
+            case "dict":


I reverted the call_name one but left this in place. With just match name, case "dict": ... and case "set": ... I think match is an improvement.

pylint/checkers/refactoring/refactoring_checker.py

github-actions · 2025-09-21T13:01:38Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit adbae7b

Use match statement in checkers (6)

b47a490

cdce8p added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry labels Sep 14, 2025

This comment has been minimized.

Sign in to view

cdce8p added 2 commits September 20, 2025 22:59

Small cleanup

a90b370

Merge remote-tracking branch 'upstream/main' into match-explore-6

6bcec48

This comment has been minimized.

Sign in to view

cdce8p requested a review from Pierre-Sassoulas September 20, 2025 21:49

Pierre-Sassoulas reviewed Sep 21, 2025

View reviewed changes

Revert match for strings

3f6ca36

cdce8p commented Sep 21, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Sep 21, 2025

View reviewed changes

pylint/checkers/refactoring/refactoring_checker.py Outdated Show resolved Hide resolved

pylint/checkers/refactoring/refactoring_checker.py Outdated Show resolved Hide resolved

pylint/checkers/refactoring/refactoring_checker.py Outdated Show resolved Hide resolved

Pierre-Sassoulas previously approved these changes Sep 21, 2025

View reviewed changes

Code review

adbae7b

cdce8p dismissed Pierre-Sassoulas’s stale review via adbae7b September 21, 2025 12:35

Pierre-Sassoulas approved these changes Sep 21, 2025

View reviewed changes

cdce8p merged commit a96dc99 into pylint-dev:main Sep 21, 2025
43 of 44 checks passed

cdce8p deleted the match-explore-6 branch September 21, 2025 14:23

cdce8p temporarily deployed to Backport September 21, 2025 14:23 — with GitHub Actions Inactive

	match token[1]:
	case ",":
	# This is a tuple, which is always acceptable.
	return
	case "and" \| "or":
	# 'and' and 'or' are the only boolean operators with lower precedence
	# than 'not', so parens are only required when they are found.
	found_and_or = True
	case "yield":
	# A yield inside an expression must always be in parentheses,
	# quit early without error.
	return
	case "for":
	# A generator expression always has a 'for' token in it, and
	# the 'for' token is only legal inside parens when it is in a
	# generator expression. The parens are necessary here, so bail
	# without an error.
	return
	case "else":
	# A generator expression can have an 'else' token in it.

Uh oh!

Use match statement in checkers (6) #10552

Use match statement in checkers (6) #10552

Uh oh!

Conversation

cdce8p commented Sep 14, 2025

Uh oh!

codecov bot commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p commented Sep 21, 2025

Uh oh!

Pierre-Sassoulas commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdce8p Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 21, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 14, 2025 •

edited

Loading

Pierre-Sassoulas commented Sep 21, 2025 •

edited

Loading