[mccabe - too-complex] Add each match case as an edge in the graph

[Pierre-Sassoulas]

Type of Changes

	Type
✓	🐛 Bug fix
✓	📜 Docs

Description

See astral-sh/ruff#11421 for initial reasoning behind this. I think we should modify the way we compute match case. But not sure if we should add a new visit method in pylint or add a get_children method for match case in astroid so we ends up not having to modify anything on the pylint side (and would benefit from it elsewhere too ?).

[cdce8p]

But not sure if we should add a new visit method in pylint or add a get_children method for match case in astroid so we ends up not having to modify anything on the pylint side (and would benefit from it elsewhere too ?).

get_children should already work same as visitMatchCase. I haven't looked into the mccabe extension yet, so not sure I can really help with that at this point.

[cdce8p]

Cross posting #10542 (comment) here.

IMO match in Python is basically an enhanced if statement. So checks should treat it similarly. AFAICT that was also your opinion on the ruff issue, unless I got that wrong.

I.e. each match case should probably increase the complexity by one.

But not sure if we should add a new visit method in pylint or add a get_children method for match case in astroid so we ends up not having to modify anything on the pylint side (and would benefit from it elsewhere too ?).

Astroid get_children() should already work just fine. We'll likely need to add some custom logic to the mccabe extension.

[Pierre-Sassoulas]

Some are asking for the possibility to configure it on the ruff side here : astral-sh/ruff#11421 (comment)

I wish the consideration for match-case statement for PLR rules was configurable to be turned off with lint.pylint setting. Long match-case statements are very common while walking abstract syntax trees and are arguably way less complex than any other solution.

I'm thinking that they possibly want to make match case that are too complex without having to think about design too much (never saw a > 5 case match statement in pylint, probably because we use the visitor pattern). But at the same time pylint is generally very configurable so why not in this case ?

What do you think @DanielNoord @jacobtylerwalls ?

[cdce8p]

I'm thinking that they possibly want to make match case that are too complex without having to think about design too much (never saw a > 5 case match statement in pylint, probably because we use the visitor pattern). But at the same time pylint is generally very configurable so why not in this case ?

Arguably the configurability is also one its the downsides. Nevertheless good defaults go a long way. So why not. I do think though this should only apply to the too-complex check and not for too-many-branches.

[Pierre-Sassoulas]

Arguably the configurability is also one its the downsides. Nevertheless good defaults go a long way.

Yeah I agree with both points. Slightly in favor of adding the option because at this point it's a little hard to rebrand pylint as an opinionated turn-key linter, even if we wanted that. And I strongly think that the default should be that each "case", count as an edge in the McCabe graph.

[jacobtylerwalls]

I never know if the users requesting new settings have fully explored the existing ones. Can't we expect users to just adjust the max-branches setting? If we need better tooling to do that per module, we need better tooling to do that per module.

[Pierre-Sassoulas]

I think their idea is to have max-branches behave differently for match cases because they "are arguably way less complex than any other solution." while still having the default max-branches value for other code constructs.

[jacobtylerwalls]

I think we should keep checkers simple and easy to reason about until we can't. If you only update one setting and then find that your team can cheat by factoring code into or out of match case, that's a surprise.

[Pierre-Sassoulas]

Let's vote:
Option : ❤️
No option: 🚀

[cdce8p]

Let's vote: Option : ❤️ No option: 🚀

I lean towards no option. If we add one, I think it should only be for too-complex.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.89%. Comparing base (c708b6a) to head (a43ce55).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9667      +/-   ##
==========================================
- Coverage   95.90%   95.89%   -0.01%     
==========================================
  Files         177      177              
  Lines       19368    19419      +51     
==========================================
+ Hits        18574    18622      +48     
- Misses        794      797       +3     

Files with missing lines	Coverage Δ
pylint/extensions/mccabe.py	97.47% <100.00%> (+0.20%)	⬆️

... and 10 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]

I wonder if we should instead modify https://github.com/pycqa/mccabe directly so it handle match case or if we should simply drop the dependency and vendor it. It hasn't been upgraded since 2021. And it's hard to optimize performance if we don't understand what's in it (which I don't for the moment, but it's looking like adding handling for match in PathGraphingAstVisitor would simplify things). Also it's a maintenance / supply chain attack risk for a hundred line of codes. (#10551)

[cdce8p]

Not an expert in the mccabe code but this looks about right to me.

[Pierre-Sassoulas]

Not an expert in the mccabe code but this looks about right to me.

Looked at the mccabe code for long enough to see that we're re-using a piece of code intended to generate dot graph, so whatever I did here it's going to be a small detail if we ever merge #10551 and want to optimize later

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on home-assistant:
The following messages are now emitted:

1. too-complex:
   '_reduce_statistics' is too complex. The McCabe rating is 17
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/recorder/statistics.py#L1019
2. too-complex:
   '_get_max_mean_min_statistic_in_sub_period' is too complex. The McCabe rating is 14
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/recorder/statistics.py#L1289
3. too-complex:
   '_get_max_mean_min_statistic' is too complex. The McCabe rating is 11
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/recorder/statistics.py#L1343
4. too-complex:
   'async_setup_entry' is too complex. The McCabe rating is 11
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/pi_hole/__init__.py#L57
5. too-complex:
   'compile_statistics' is too complex. The McCabe rating is 40
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/sensor/recorder.py#L455
6. too-complex:
   '_update_from_device' is too complex. The McCabe rating is 20
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/matter/climate.py#L258
7. too-complex:
   '_get_media_types_from_query' is too complex. The McCabe rating is 11
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/music_assistant/media_browser.py#L464
8. too-complex:
   '_process_search_results' is too complex. The McCabe rating is 12
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/music_assistant/media_browser.py#L515
9. too-complex:
   '_event_listener' is too complex. The McCabe rating is 30
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/home_connect/coordinator.py#L181
10. too-complex:
    'process_update' is too complex. The McCabe rating is 22
    https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/onkyo/media_player.py#L341
11. too-complex:
    '_update_stream_source' is too complex. The McCabe rating is 14
    https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/go2rtc/__init__.py#L306

The following messages are no longer emitted:

1. too-complex:
   '_reduce_statistics' is too complex. The McCabe rating is 15
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/recorder/statistics.py#L1019
2. too-complex:
   'compile_statistics' is too complex. The McCabe rating is 38
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/sensor/recorder.py#L455
3. too-complex:
   '_update_from_device' is too complex. The McCabe rating is 12
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/matter/climate.py#L258
4. too-complex:
   '_event_listener' is too complex. The McCabe rating is 24
   https://github.com/home-assistant/core/blob/410c3df6dd9797d3a835ce4fea1ce6a92d77c409/homeassistant/components/home_connect/coordinator.py#L181

Effect on music21:
The following messages are now emitted:

1. too-complex:
   'elementToMidiEventList' is too complex. The McCabe rating is 12
   https://github.com/cuthbertLab/music21/blob/501e13d0058804471cbfba4b2d848971c86e7582/music21/midi/translate.py#L1344

Effect on django:
The following messages are now emitted:

1. too-complex:
   'normalize_choices' is too complex. The McCabe rating is 11
   https://github.com/django/django/blob/014be2f0dabb1eea4748df528edf65878dcfdecc/django/utils/choices.py#L72

Effect on pandas:
The following messages are now emitted:

1. too-complex:
   '_split_by_backtick' is too complex. The McCabe rating is 14
   https://github.com/pandas-dev/pandas/blob/cc40732889b59d0ebd867b087691f02221e5666c/pandas/core/computation/parsing.py#L145

The following messages are no longer emitted:

1. too-complex:
   '_split_by_backtick' is too complex. The McCabe rating is 11
   https://github.com/pandas-dev/pandas/blob/cc40732889b59d0ebd867b087691f02221e5666c/pandas/core/computation/parsing.py#L145

Effect on sentry:
The following messages are now emitted:

1. too-complex:
   'handle_result' is too complex. The McCabe rating is 24
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/uptime/consumers/results_consumer.py#L278
2. too-complex:
   'create_uptime_detector' is too complex. The McCabe rating is 13
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/uptime/subscriptions/subscriptions.py#L178
3. too-complex:
   'convert_rule_condition_to_snuba_condition' is too complex. The McCabe rating is 15
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/rules/conditions/event_frequency.py#L790
4. too-complex:
   'as_log_message' is too complex. The McCabe rating is 33
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/replays/usecases/summarize.py#L305
5. too-complex:
   'as_trace_item_context' is too complex. The McCabe rating is 36
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/replays/usecases/ingest/event_parser.py#L347
6. too-complex:
   'literal' is too complex. The McCabe rating is 13
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/replays/lib/eap/snuba_transpiler.py#L722
7. too-complex:
   'convert_filter_to_snuba_condition' is too complex. The McCabe rating is 22
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/workflow_engine/handlers/condition/event_frequency_query_handlers.py#L196
8. too-complex:
   'filter_detectors' is too complex. The McCabe rating is 13
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/workflow_engine/endpoints/organization_detector_index.py#L152
9. too-complex:
   'filter_workflows' is too complex. The McCabe rating is 11
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/workflow_engine/endpoints/organization_workflow_index.py#L95

The following messages are no longer emitted:

1. too-complex:
   'handle_result' is too complex. The McCabe rating is 21
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/uptime/consumers/results_consumer.py#L278
2. too-complex:
   'create_uptime_detector' is too complex. The McCabe rating is 11
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/uptime/subscriptions/subscriptions.py#L178
3. too-complex:
   'as_trace_item_context' is too complex. The McCabe rating is 19
   https://github.com/getsentry/sentry/blob/999558bf4b621e4baaaf8ed266bcb66f6dab3106/src/sentry/replays/usecases/ingest/event_parser.py#L347

This comment was generated for commit a43ce55