invalid-repr-returned false positive with match statement

[CobaltCause]

Bug description

It looks like pylint isn't checking exhaustiveness before triggering this lint.

pylint_e0306_false_positive.py:

# pylint: disable=missing-docstring,too-few-public-methods

from typing import Optional


class FalsePositive:
    def __init__(self) -> None:
        self.opt_field: Optional[int] = 5

    def __repr__(self) -> str:
        match self.opt_field:
            case None:
                return "None"
            case some_value:
                return f"Some({repr(some_value)}"

Likely related: #7470 (comment)

Configuration

No response

Command used

pylint pylint_e0306_false_positive.py

Pylint output

************* Module pylint_e0306_false_positive
pylint_e0306_false_positive.py:10:4: E0306: __repr__ does not return str (invalid-repr-returned)

Expected behavior

No error

Pylint version

pylint 2.14.5
astroid 2.11.7
Python 3.10.7 (main, Sep  5 2022, 13:12:31) [GCC 11.3.0]

OS / Environment

NixOS 22.05

Additional dependencies

No response

[Pierre-Sassoulas]

If nothing is matched this function would return None. This is hard to understand for pylint but also for human. Even for human you can make a mistake and not realize something can end up unmatched and get an error. I would suggest using a default.

[CobaltCause]

In this particular case, every possibility is covered and all branches return the correct type. Under no circumstances will this function return None.

And generally, I understand that humans make mistakes, which is why I also employ static analysis tools like Pyright, which do have exhaustiveness checking for match statements. I don't believe it's correct to require a default value for an unreachable branch, at best that's just adding dead code for no reason. It may even be detrimental in that it may cause one to forget to handle new cases, an error that would have otherwise been caught by static analysis tooling, but which would be wrongfully silenced/ignored by adding a default case _: pattern.

That said, it's my understanding that exhaustiveness checking is a difficult problem. If this particular case were marked as wontfix, that would be understandable.

On the other hand, maybe this lint shouldn't be the responsibility of pylint, but that of a typechecker, since this is fundamentally about the type system and not code style.

[Pierre-Sassoulas]

I don't believe it's correct to require a default value for an unreachable branch, at best that's just adding dead code for no reason.

The reason would be that you know within 10ms when reading the code that it can't return None instead of having to think about each possible case or blindly trust the tooling. I don't think a codebase should rely too much on trust in their tooling (trust but verify instead ?).

That being said, this is a false positive that should be fixed, but this rely on control flow (#4795 ?). Control flow understanding is one of the weak point of pylint.

[CobaltCause]

I "know within 10ms when reading the code that it can't return None" because I don't see any errors in my editor informing me that my match was nonexhaustive. I trust this result, but can easily verify it by scanning the cases and comparing to the type in the match. Generally, the failure mode for tools around exhaustiveness checking is false negatives rather than false positives, so I think it's pretty safe to rely on the tool here. I don't think this line of reasoning holds any water.

---

On the other hand, maybe this lint shouldn't be the responsibility of pylint, but that of a typechecker, since this is fundamentally about the type system and not code style.

I'd like to take a stronger stance on this and say the proper solution to this issue is to simply remove this lint and let a typechecker handle it instead.

[Pierre-Sassoulas]

We have a lot of legacy checkers that half-ass checks that should be done by a type-checker in a very particular scenario. On the top of my head this is one of the worst offender : https://pylint.pycqa.org/en/latest/user_guide/messages/error/invalid-envvar-value.html. This is a lot of cleanup and/or documentation to do if it come to it.

[udifuchs]

Another example of this issue is with enum:

import enum

class Foo(enum.Enum):
    ONE = 1
    TWO = 2

    def __repr__(self) -> str:
        match self:
            case Foo.ONE:
                return "One"
            case Foo.TWO:
                return "Two"

The point is that I do not want to have a default case _: pattern because I want my linter (pylint or mypy) to warn me if there is an enum value that is missing a case pattern.

[jacobtylerwalls]

Verified OP and #7645 (comment) are fixed with pylint-dev/astroid#2042, so closing as a duplicate of #5288, which we'll close with a regression test when updating pylint to use the next astroid release.