C901, PLR0912 and PLR0915 treat match/case as one statement

[jaap3]

Using ruff 0.4.4 the following code triggers C901, PLR0912 and PLR0915:

def grades_to_average(grades):
    numbers = []
    for grade in grades:
        if grade in {"F-", "F", "F+", "E-", "E"}:
            numbers.append(0)
        elif grade == "E+":
            numbers.append(.3)
        elif grade == "D-":
            numbers.append(.7)
        elif grade == "D":
            numbers.append(1.0)
        elif grade == "D+":
            numbers.append(1.3)
        elif grade == "C-":
            numbers.append(1.7)
        elif grade == "C":
            numbers.append(2.0)
        elif grade == "C+":
            numbers.append(2.3)
        elif grade == "B-":
            numbers.append(2.7)
        elif grade == "B":
            numbers.append(3.0)
        elif grade == "B+":
            numbers.append(3.3)
        elif grade == "A-":
            numbers.append(3.7)
        elif grade in {"A", "A+"}:
            numbers.append(4.0)
        else:
            raise ValueError(f"Unknown grade: {grade}")

    try:
        avg = sum(numbers) / len(numbers)
    except ZeroDivisionError:
        avg = 0

    if avg < 0.3:
        avg_grade = "F"
    elif avg < .7:
        avg_grade = "E+"
    elif avg < 1.0:
        avg_grade = "D-"
    elif avg < 1.3:
        avg_grade = "D"
    elif avg < 1.7:
        avg_grade = "D+"
    elif avg < 2.0:
        avg_grade = "C-"
    elif avg < 2.3:
        avg_grade = "C"
    elif avg < 2.7:
        avg_grade = "C+"
    elif avg < 3.0:
        avg_grade = "B-"
    elif avg < 3.3:
        avg_grade = "B"
    elif avg < 3.7:
        avg_grade = "B+"
    elif avg < 4.0:
        avg_grade = "A-"
    elif avg >= 4.0:
        avg_grade = "A"
    else:
        raise ValueError(f"Unexpected average: {avg}")
    return avg_grade

The equivalent code that uses match/case however does not:

def grades_to_average(grades):
    numbers = []
    for grade in grades:
        match grade:
            case "F-" | "F" | "F+" | "E-" | "E":
                numbers.append(0)
            case "E+":
                numbers.append(.3)
            case "D-":
                numbers.append(.7)
            case "D":
                numbers.append(1.0)
            case "D+":
                numbers.append(1.3)
            case "C-":
                numbers.append(1.7)
            case "C":
                numbers.append(2.0)
            case "C+":
                numbers.append(2.3)
            case "B-":
                numbers.append(2.7)
            case "B":
                numbers.append(3.0)
            case "B+":
                numbers.append(3.3)
            case "A-":
                numbers.append(3.7)
            case "A" | "A+":
                numbers.append(4.0)
            case _:
                raise ValueError(f"Unknown grade: {grade}")

    try:
        avg = sum(numbers) / len(numbers)
    except ZeroDivisionError:
        avg = 0

    match avg:
        case avg if avg < .3:
            avg_grade = "F"
        case avg if avg < .7:
            avg_grade = "E+"
        case avg if avg < 1.0:
            avg_grade = "D-"
        case avg if avg < 1.3:
            avg_grade = "D"
        case avg if avg < 1.7:
            avg_grade = "D+"
        case avg if avg < 2.0:
            avg_grade = "C-"
        case avg if avg < 2.3:
            avg_grade = "C"
        case avg if avg < 2.7:
            avg_grade = "C+"
        case avg if avg < 3.0:
            avg_grade = "B-"
        case avg if avg < 3.3:
            avg_grade = "B"
        case avg if avg < 3.7:
            avg_grade = "B+"
        case avg if avg < 4.0:
            avg_grade = "A-"
        case avg if avg >= 4.0:
            avg_grade = "A"
        case _:
            raise ValueError(f"Unexpected average: {avg}")
    return avg_grade

Is this intentional? Should each case count as a conditional?

[dhruvmanila]

I think it is intentional as Pylint doesn't detect it either.

Should each case count as a conditional?

Personally, I wouldn't count it as I find the match statement to be more readable than an if statement. Additionally, the semantics of pattern matching is very different then the test expression of an if statement. I'd love to hear others opinion, cc @AlexWaygood @zanieb

[jaap3]

I feel like the same complexity and maintainability arguments apply to match and case. They are definitely a way to achieve branching code and each case (especially the conditional ones containing if) could be considered to be a distinct statement right?

This issue should probably be converted to a discussion.

[dhruvmanila]

This issue should probably be converted to a discussion.

It's fine, we can discuss here.

I don't have any arguments against this, and it does make sense (I think?) to include the match statement similarly to an if statement. I would wait for others to share their opinions before we decide. It would also be useful to hear the thoughts of the Pylint maintainers. Do you want to open an issue on the Pylint repository similar to this? I can also do that :)

[charliermarsh]

\cc @Pierre-Sassoulas

[Pierre-Sassoulas]

Thank you for the ping @charliermarsh :) I see no reason for match to behave differently than if. I think it's an oversight when we added the match statement for python 3.10 we did not think to test the consequences on the already implemented "too-complex" (assumed the "too-complex" code was generic enough).

[Pierre-Sassoulas]

So, I did some light research and there might be a point for 'too-complex" behaving the way it does right now. It is based on Mc Cabe and:

McCabe originally recommended exempting modules consisting of single mul-
tiway decision (“switch” or “case”) statements from the complexity limit. The multiway deci-
sion issue has been interpreted in many ways over the years, sometimes with disastrous
results.

https://web.archive.org/web/20210908120324/https://www.mccabe.com/pdf/mccabe-nist235r.pdf (page 15, notice also that McCabe himself is one of the authors)

Also on page 26 we can see:

A less frequently occurring issue that has greater impact on complexity is the distinction between
“case-labeled statements” and “case labels.” When several case labels apply to the same pro-
gram statement, this is modeled as a single decision outcome edge in the control flow graph,
adding one to complexity. It is certainly possible to make a consistent flow graph model in
which each individual case label contributes a different decision outcome edge and hence also
adds one to complexity, but that is not the typical usage

Imo, the match case in python adds complexity because it's not a simple "case labels", it's behaving more like an if, or even a regex.

[AlexWaygood]

I agree with @Pierre-Sassoulas. For the Mccabe plugin, I believe one of the primary motivations for the rule is to ensure that each function is testable in isolation. If a function has too many branches, it becomes hard to write a unit test for it. I think this is just as much a concern for match/case as it is with if/elif, as each new case statement in the match/case treee can be seen as a wholly distinct branch that you would need to account for when writing tests.

[dhruvmanila]

Thank you @Pierre-Sassoulas for chiming in! I agree with the conclusion. I think we should update all three rules to consider match statement and we can see how many changes we see in the ecosystem checks.

[blueraft]

I can take a stab at this if that's ok.