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.85%. Comparing base (4f98c18) to head (1996d57).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10514      +/-   ##
==========================================
- Coverage   95.85%   95.85%   -0.01%     
==========================================
  Files         177      177              
  Lines       19198    19289      +91     
==========================================
+ Hits        18402    18489      +87     
- Misses        796      800       +4

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/code_style.py	`100.00% <100.00%> (ø)`
pylint/extensions/for_any_all.py	`100.00% <100.00%> (ø)`
pylint/extensions/private_import.py	`99.17% <100.00%> (+<0.01%)`	⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

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

github-actions · 2025-08-22T17:02:33Z

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

This comment was generated for commit 1996d57

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

pylint/checkers/match_statements_checker.py

@@ -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 (


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.

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

pylint/checkers/match_statements_checker.py

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

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?

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

pyproject.toml

+  # TODO: remove once match false-positives are fixed, probably in v1.18
+  # https://github.com/python/mypy/pull/19708
+  "has-type",


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.

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.

Use match statement in checkers (1)

1996d57

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

cdce8p commented Aug 22, 2025

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use match statement in checkers (1) #10514

Use match statement in checkers (1) #10514

cdce8p commented Aug 22, 2025

Uh oh!

codecov bot commented Aug 22, 2025

Uh oh!

github-actions bot commented Aug 22, 2025

Uh oh!

cdce8p Aug 22, 2025

Uh oh!

cdce8p Aug 22, 2025

Uh oh!

cdce8p Aug 22, 2025

Uh oh!

cdce8p Aug 22, 2025

Uh oh!

cdce8p Aug 22, 2025

Uh oh!

Pierre-Sassoulas commented Aug 22, 2025

Uh oh!

cdce8p commented Aug 22, 2025

Uh oh!

Uh oh!

Uh oh!

Use match statement in checkers (1) #10514

Are you sure you want to change the base?

Use match statement in checkers (1) #10514

Conversation

cdce8p commented Aug 22, 2025

Uh oh!

codecov bot commented Aug 22, 2025

Codecov Report

Uh oh!

github-actions bot commented Aug 22, 2025

Uh oh!

cdce8p Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

cdce8p Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas commented Aug 22, 2025

Uh oh!

cdce8p commented Aug 22, 2025

Uh oh!

Uh oh!