False-Positive R504 in non-trivial if-else Block

[erikvanderwerf]

• Date you used flake8-return: 2023-02-11
• flake8-return version used, if any: 1.2.0
• Python version, if any: 3.10
• Operating System: Windows 11

Description

The application of R504 unnecessary variable assignment before return statement seems wrong here. I've specifically not inlined my return statement in order to have a single return in this "complicated" if-else block.

def test(x: int) -> str:
    retval: str
    if x == 1:
        retval = "Hello"
    elif x == 2:
        # ... complicated code where I prefer a single return statement at the end.
        retval = "World"
    else:
        retval = "There"
    return retval  # < R504

The example on the homepage was for the trivial case of an unnecessary assignment, but for something more complicated like this I think it should not flag R504 anywhere in the method.

[afonasev]

Thank you for your message. I agree that linter rules can sometimes be subjective. I don't think that in this case it is possible to solve the problem by improving the rules. But you can always explicitly turn off the check using noqa.

[erikvanderwerf]

It looks like my case is an extension of #47 (https://github.com/afonasev/flake8-return/blob/master/tests/test_unnecessary_assign.py#L51). I see the comment # Can be refactored false positives so maybe this particular case was not taken care of yet.

My first thought for a fix would be to not raise the lint rule if the variable is being conditionally assigned in a branch. It seems very strange that even a case like this would flag:

def returns_504(x: int):
    result = ""
    if x == 0:
        result = str(x)
    return result  # <-- R504

Is there a more complicated test case that R504 is meant to check that I'm not seeing? To me it looks like this was only intended to flag in the trivial cases, but is going beyond its purpose.

[calumy]

I think this could be refactored to:

def returns_504(x: int):
    if x == 0:
        return str(x)
    return ""

And the initial case could be refactored to:

def test(x: int) -> str:
    if x == 1:
        return "Hello"

    if x == 2:
        # ... complicated code where I prefer a single return statement at the end.
        return "World"

    return "There"

[erikvanderwerf]

Stylistically I don't like that approach, since my goal was to reduce the number of return statements, and put a single return at the end. If this is not an intended use-case for this lint flag then I will probably disable R504 in all my projects.

[erikvanderwerf]

Since I see that a few people in #47 and probably others had the same goals as myself, I think it would help to put a simple example with the "bad" branching in the project README.md, so that users understand that this is the intended behavior.