Use match statement in checkers (1) #10514

cdce8p · 2025-08-22T16:36:38Z

After we added the first match statement in #10424 (comment), I checked a few other files to see which checks can be converted. There are actually quite a lot. Before we go ahead with it though, I think we should decide on some design choices and good practices first. I'll create comments for the specific sections. Feel free to add new ones as well in case I missed anything.

There are also mypy issues as astroid isn't fully typed yet. Opened PRs for it upstream already but those will be included in v1.18 at the earliest. Until then it might be best to just disable the offending error code(s) or add type ignores.

codecov · 2025-08-22T16:50:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.84%. Comparing base (e737d0c) to head (b3d1654).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10514   +/-   ##
=======================================
  Coverage   95.84%   95.84%           
=======================================
  Files         177      177           
  Lines       19284    19288    +4     
=======================================
+ Hits        18483    18487    +4     
  Misses        801      801

Files with missing lines	Coverage Δ
pylint/checkers/match_statements_checker.py	`100.00% <100.00%> (ø)`
pylint/extensions/_check_docs_utils.py	`93.72% <100.00%> (+0.01%)`	⬆️
pylint/extensions/for_any_all.py	`100.00% <100.00%> (ø)`
pylint/extensions/private_import.py	`99.17% <100.00%> (+<0.01%)`	⬆️

🚀 New features to boost your workflow:

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

cdce8p · 2025-08-22T16:45:53Z

pylint/checkers/match_statements_checker.py

        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 (


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.

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.

cdce8p · 2025-08-22T16:48:47Z

pylint/checkers/match_statements_checker.py

                        "bare-name-capture-pattern",
                        node=case.pattern,
-                        args=case.pattern.name.name,
+                        args=n,


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.

cdce8p · 2025-08-22T17:09:09Z

pylint/extensions/code_style.py

-            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


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): ...

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.

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

cdce8p · 2025-08-22T17:10:46Z

pylint/extensions/private_import.py

+            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
+                )


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

For me it would!

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.

pyproject.toml

Pierre-Sassoulas · 2025-08-22T18:40:39Z

Love it, I like the elegance of match and it's proper in pylint with all the isinstance check we do all the time. I'm not sure if we should upgrade everything though. This kind of changes could tell us if our tests are actually covering or if we're just protected by years of live testing on real code bases and by not touching some part of the code without a good reason. Could be a lot of hassle if the latter is true and we go all in.

Regarding the standard I don't think we're at the step of making rules yet. Or at least I'm not : I'm not using match a lot. Seems like for a generic node n could be nice, like f for open and file. On other it would be nice to have a real name like "enumerate_start_kwargs" for clarity. Maybe think about it each time for now, use it for some time, then think about a rule later ?

cdce8p · 2025-08-22T19:17:02Z

Love it, I like the elegance of match and it's proper in pylint with all the isinstance check we do all the time.

Me too! I've used match once or twice before in the custom pylint plugin for HomeAssistant, so I knew it might work but I was honestly surprised at how good it actually works.

I'm not sure if we should upgrade everything though. This kind of changes could tell us if our tests are actually covering or if we're just protected by years of live testing on real code bases and by not touching some part of the code without a good reason. Could be a lot of hassle if the latter is true and we go all in.

True. So far I'd say the functional tests have caught most errors during conversion and helped a lot. There is always the risk that we introduce a regression though. The biggest issue I see though is that it will probably make backports much more difficult.

Regarding the standard I don't think we're at the step of making rules yet. Or at least I'm not : I'm not using match a lot. [...] Maybe think about it each time for now, use it for some time, then think about a rule later ?

Works for me. Just wanted to check what you guys think before doing more changes.

DanielNoord · 2025-08-25T09:56:12Z

pylint/checkers/match_statements_checker.py

        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 (


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.

DanielNoord · 2025-08-25T09:58:45Z

pylint/extensions/code_style.py

-            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


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.

DanielNoord · 2025-08-25T09:59:01Z

pylint/extensions/private_import.py

+            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
+                )


For me it would!

pyproject.toml

cdce8p · 2025-08-29T21:47:08Z

I think this is ready now.

@DanielNoord Mostly adopted your suggestions. Left some comments above.

--
Edit: Needed to push two more commit. Now it's ready 🤞🏻

github-actions · 2025-08-29T22:29:42Z

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

This comment was generated for commit b3d1654

cdce8p · 2025-08-30T20:26:37Z

@Pierre-Sassoulas Anything to add here? Otherwise I'd like to start merging the PRs with this one to avoid conflicts.

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 22, 2025

This comment has been minimized.

Sign in to view

cdce8p commented Aug 22, 2025

View reviewed changes

cdce8p requested review from Pierre-Sassoulas, DanielNoord and jacobtylerwalls August 22, 2025 17:17

DanielNoord previously approved these changes Aug 25, 2025

View reviewed changes

cdce8p mentioned this pull request Aug 29, 2025

Switch to mypy-dev 1.18.0a6 #10527

Merged

Pierre-Sassoulas closed this in #10527 Aug 29, 2025

cdce8p reopened this Aug 29, 2025

Use match statement in checkers (1)

ffed4b5

cdce8p force-pushed the match-explore-1 branch from 1996d57 to ffed4b5 Compare August 29, 2025 21:10

cdce8p added 3 commits August 29, 2025 23:11

Revert mypy disable

ed93da5

Revert questionable tuple match

3b460ee

Use better variable names

b7938b1

cdce8p dismissed DanielNoord’s stale review via b7938b1 August 29, 2025 21:18

This comment has been minimized.

Sign in to view

cdce8p added 2 commits August 30, 2025 00:02

Remove unnecessary variable binding

e734457

Add back code comment

b3d1654

DanielNoord approved these changes Aug 30, 2025

View reviewed changes

cdce8p merged commit 2d60c59 into pylint-dev:main Aug 31, 2025
44 checks passed

cdce8p deleted the match-explore-1 branch August 31, 2025 19:07

cdce8p temporarily deployed to Backport August 31, 2025 19:07 — with GitHub Actions Inactive

Uh oh!

Use match statement in checkers (1) #10514

Use match statement in checkers (1) #10514

Uh oh!

Conversation

cdce8p commented Aug 22, 2025

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas commented Aug 22, 2025

Uh oh!

cdce8p commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

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

Uh oh!

Uh oh!

github-actions bot commented Aug 29, 2025

Uh oh!

cdce8p commented Aug 30, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 22, 2025 •

edited

Loading

cdce8p Aug 29, 2025 •

edited

Loading

cdce8p commented Aug 29, 2025 •

edited

Loading