astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32k stars 1.07k forks source link

PLR0912: Pylint counts `try: ... except:` statements as single branch #11205

Closed Kakadus closed 5 months ago

Kakadus commented 5 months ago

PLR0912 is not equivalent to pylint. The following function is accepted by pylint but rejected by ruff:

def capital(country):
    if country == "Australia":
        return "Canberra"
    elif country == "Brazil":
        return "Brasilia"
    elif country == "Canada":
        return "Ottawa"
    elif country == "England":
        return "London"
    elif country == "France":
        return "Paris"
    elif country == "Germany":
        return "Berlin"
    elif country == "Poland":
        return "Warsaw"
    elif country == "Romania":
        return "Bucharest"
    elif country == "Spain":
        return "Madrid"
    elif country == "Thailand":
        return "Bangkok"
    elif country == "Turkey":
        return "Ankara"
    try:
        return None
    except:
        return "Some"

Pylint only detects 12 branches, while ruff counts 13. Is this a bug or a feature? If the latter, it should probably be documented. I can also imagine a setting, enabling/disabling this behavior

MichaReiser commented 5 months ago

The rule was added in https://github.com/astral-sh/ruff/pull/2550 but there's no conversation in that PR nor is there any change to the rule explaining if that divergence is indeed intentional.

@chanman3388 as author of the rule. What's your take on this?

To me, counting except as its own branch does make sense. Because the except might or might not be executed. So the try block has two possible exit branches.

chanman3388 commented 5 months ago

The rule was added in #2550 but there's no conversation in that PR nor is there any change to the rule explaining if that divergence is indeed intentional.

@chanman3388 as author of the rule. What's your take on this?

To me, counting except as its own branch does make sense. Because the except might or might not be executed. So the try block has two possible exit branches.

I don't believe that this divergence was intentional, pylint does some odd things, I'd be inclined to agree with @MichaReiser that is is its own branch or brances, but I guess the question to me is whether we should remain faithful to pylint's implementation which looks like:

    def visit_try(self, node: nodes.Try) -> None:
        """Increments the branches counter."""
        branches = len(node.handlers)
        if node.orelse:
            branches += 1
        if node.finalbody:
            branches += 1
        self._inc_branch(node, branches)
        self._inc_all_stmts(branches)

or if we should go with our own interpretation. Looking at the python AST I can't tell if this was oversight or if the intention was to consider exceptions differently?

charliermarsh commented 5 months ago

It looks like the difference is whether the try body counts as a branch? I'd say it should, probably. Although it's perhaps strange that finally counts as a branch, since it always runs? Ultimately, though, the rule is trying to capture some measure of complexity, and both the try body and the finally contribute to complexity in a way...

charliermarsh commented 5 months ago

I'd be fine to remove the body from the count though. It's a bit more consistent with Pylint and is pretty marginal either way.

dhruvmanila commented 5 months ago

Related: https://github.com/astral-sh/ruff/issues/11421