Use match statement in checkers (3) #10529

cdce8p · 2025-08-29T22:52:02Z

To review I'd suggest to use Hide whitespace.

codecov · 2025-08-29T23:05:21Z

Codecov Report

❌ Patch coverage is 98.43137% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.86%. Comparing base (e737d0c) to head (8ee42d1).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
pylint/checkers/base/basic_checker.py	96.07%	2 Missing ⚠️
pylint/checkers/classes/class_checker.py	95.65%	1 Missing ⚠️
pylint/checkers/classes/special_methods_checker.py	92.85%	1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (98.43%) 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   #10529      +/-   ##
==========================================
+ Coverage   95.84%   95.86%   +0.02%     
==========================================
  Files         177      177              
  Lines       19284    19308      +24     
==========================================
+ Hits        18483    18510      +27     
+ Misses        801      798       -3

Files with missing lines	Coverage Δ
pylint/checkers/base/basic_error_checker.py	`95.41% <100.00%> (ø)`
pylint/checkers/base/docstring_checker.py	`97.70% <100.00%> (+0.05%)`	⬆️
pylint/checkers/base/name_checker/checker.py	`98.69% <100.00%> (+0.01%)`	⬆️
...heckers/refactoring/implicit_booleaness_checker.py	`100.00% <100.00%> (ø)`
pylint/checkers/refactoring/refactoring_checker.py	`98.18% <100.00%> (+0.12%)`	⬆️
pylint/checkers/classes/class_checker.py	`93.85% <95.65%> (+0.12%)`	⬆️
pylint/checkers/classes/special_methods_checker.py	`96.04% <92.85%> (+0.59%)`	⬆️
pylint/checkers/base/basic_checker.py	`98.08% <96.07%> (+<0.01%)`	⬆️

... and 7 files 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

Yeah, this looks amazing.

pylint/checkers/base/basic_checker.py

Pierre-Sassoulas · 2025-08-30T06:34:22Z

pylint/checkers/base/basic_error_checker.py

-        # Check *a, *b = ...
-        assign_target = node.targets[0]
-        # Check *a = b


Those comments were stangely placed, good job fixing them.

That's one of the things I noticed with the match statement. It forces you to think about conditions in a more linear way compared to if statements which often improves the code itself.

pylint/checkers/base/docstring_checker.py

Pierre-Sassoulas · 2025-08-30T06:38:14Z

pylint/checkers/classes/class_checker.py

-                if isinstance(decorator, nodes.Attribute):
-                    if self._check_functools_or_not(decorator):
+                match decorator:
+                    case nodes.Attribute(attrname="getter" | "setter" | "deleter"):


So elegant !

Pierre-Sassoulas · 2025-08-30T06:41:09Z

pylint/checkers/classes/special_methods_checker.py

-        elif isinstance(node, nodes.ClassDef):
-            metaclass = node.metaclass()
-            if metaclass and isinstance(metaclass, nodes.ClassDef):
+            case bases.Instance():
                try:
-                    metaclass.local_attr(NEXT_METHOD)
+                    node.local_attr(NEXT_METHOD)
                    return True
                except astroid.NotFoundError:
                    pass
+            case nodes.ClassDef():
+                metaclass = node.metaclass()
+                if metaclass and isinstance(metaclass, nodes.ClassDef):
+                    try:
+                        metaclass.local_attr(NEXT_METHOD)
+                        return True
+                    except astroid.NotFoundError:
+                        pass


Not sure if this one is better as we had to duplicate some code. Maybe. (+0)

The diff itself looks worse than it really is. The partial duplication is there already. I just replaced the if statements with case ... and indented the individual blocks.

Pierre-Sassoulas · 2025-08-30T06:46:22Z

pylint/checkers/refactoring/refactoring_checker.py

+                return any(
+                    self._is_node_return_ended(_child) for _child in all_but_handler
+                ) and all(self._is_node_return_ended(_child) for _child in handlers)
+            case nodes.Assert(test=nodes.Const(value=value)) if not value:


We could even simplify this a bit further since we only really care about assert False. This should work

-case nodes.Assert(test=nodes.Const(value=value)): +case nodes.Assert(test=nodes.Const(value=False)): ...

Will do that in a followup as it's technically a (minor) behavior change and not just refactoring.

Pierre-Sassoulas

We might want to add the missing coverage to be safe.

cdce8p · 2025-08-30T18:48:37Z

We might want to add the missing coverage to be safe.

Thanks actually just a result of how coverage evaluates the ast. For else: return only the return line is marked as uncovered, whereas for case _: return both lines are. So the code coverage didn't really change.

Pierre-Sassoulas · 2025-08-30T19:11:18Z

I know that the code was not covered by automated test before the refactor, but the old code was tested by time since release and billions of use on very diverse codebases. The refactor with the new match statement isn't, so I don't think we can trust it as much as the old code without adding automated tests.

We might realize that the default match case is impossible to reach and remove it or pragma no-cover it because it's very hard to automatically test but still need to be there.

cdce8p · 2025-08-30T19:32:33Z

The refactor with the new match statement isn't, so I don't think we can trust it as much as the old code without adding automated tests.

Might be my own bias speaking here. After converting a lot of if statements, I think the transformations in this PR are quite safe. I've actually excluded the unsafe / questionable ones for latter.

We might realize that the default match case is impossible to reach and remove it or pragma no-cover it because it's very hard to automatically test but still need to be there.

After looking at the code again, the uncovered else case shouldn't ever be reached as we exclude all nodes which parent isn't DictComp, ListComp or SetComp earlier. ~~Will go ahead and remove it.~~ Will add a pragma: no cover for it.

Pierre-Sassoulas · 2025-08-30T20:01:12Z

The refactor with the new match statement isn't, so I don't think we can trust it as much as the old code without adding automated tests.

Might be my own bias speaking here. After converting a lot of if statements, I think the transformations in this PR are quite safe. I've actually excluded the unsafe / questionable ones for latter.

Yeah, you know match better than me and it's probably safe, but (in my youth... 😛) I've been absolutely certain that a refactor was safe and didn't need new tests and then destroyed the productions of less impactful projects :)

cdce8p · 2025-08-30T20:23:53Z

I've been absolutely certain that a refactor was safe and didn't need new tests and then destroyed the productions of less impactful projects :)

True, happened to all of us at some point 😅

The primer runs do help to feel better about it though. Yes, it could be that there are one or two regressions hiding in the match PRs. However those could easily be fixed later, similar to the *_ I added earlier.

github-actions · 2025-08-31T21:59:17Z

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

This comment was generated for commit 8ee42d1

Use match statement in checkers (3)

ba06e43

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

This comment has been minimized.

Sign in to view

Pierre-Sassoulas previously approved these changes Aug 30, 2025

View reviewed changes

Use optimized if stmt in basic_checker

a635d9e

cdce8p dismissed Pierre-Sassoulas’s stale review via a635d9e August 30, 2025 16:50

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Aug 30, 2025

View reviewed changes

Add pragma no cover comment

509fe1a

This comment has been minimized.

Sign in to view

Raise AssertionError for uncovered fallback case

afa59eb

cdce8p force-pushed the match-explore-3 branch from 12e9111 to afa59eb Compare August 30, 2025 22:43

This comment has been minimized.

Sign in to view

Cleanup

8ee42d1

cdce8p requested a review from Pierre-Sassoulas August 31, 2025 21:35

Uh oh!

Use match statement in checkers (3) #10529

Are you sure you want to change the base?

Use match statement in checkers (3) #10529

Uh oh!

Conversation

cdce8p commented Aug 29, 2025

Uh oh!

codecov bot commented Aug 29, 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.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

cdce8p commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierre-Sassoulas commented Aug 30, 2025

Uh oh!

cdce8p commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Pierre-Sassoulas commented Aug 30, 2025

Uh oh!

cdce8p commented Aug 30, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Aug 31, 2025

Uh oh!

Uh oh!

codecov bot commented Aug 29, 2025 •

edited

Loading

cdce8p commented Aug 30, 2025 •

edited

Loading

cdce8p commented Aug 30, 2025 •

edited

Loading