RET504 false-positive when variable built up with assignments and returned

[gdub]

Example:

def get_options(option1, option2):
    options = []
    if option1:
        options = options + ["option1"]
    if option2:
        options = options + ["option2"]
    return options

Output:

$ ruff check --isolated --select RET ret_test.py 
ret_test2.py:16:12: RET504 Unnecessary variable assignment before `return` statement
Found 1 error.

In this case, there is no unnecessary variable assignment that could be removed and cannot do an earlier return as need to get through all the conditionals.

That said, the error does not trigger if one were to instead use an in-place operator such as +=:

def get_options(option1, option2):
    options = []
    if option1:
        options += ["option1"]
    if option2:
        options += ["option2"]
    return options

Version:

$ ruff --version
ruff 0.0.247

[charliermarsh]

Possibly not respecting overrides, does look like a bug.

[henryiii]

Just noticed this too!

def f(y):
    x = 1
    if y:
        x = 2

    return x

$ ruff tmp.py --isolated --select RET
tmp.py:6:12: RET504 Unnecessary variable assignment before `return` statement
Found 1 error.

It seems the RET checks don't handle branches very well (the "else after" ones also are broken when you have a chain).

[charliermarsh]

It may wants you to do this, even though it's awkward:

def f(y):
    if y:
        return 2

    return 1

the "else after" ones also are broken when you have a chain

Is this on main? I removed enforcement for elif chains in the most recent release.

[charliermarsh]

(I find these rules really tedious lol. Maybe they should just be removed.)

[henryiii]

They will come back again when adding pylint rules. :) I like these in pylint a lot, which I is why I'm happy to see them showing up in a faster pre-commit friendly form, but the pylint version is solid and doesn't trigger falsely, while RET seems to not properly handle branching (I expect it's a problem with the flake8 plugin).

It may wants you to do this, even though it's awkward

It's not practical in the actual code that caused me to look into this. The fix should be to just only enforce x = value; return value and not if the assignment is in an if statement (with statement is 90% of the time fine, just not if statements)

[henryiii]

I haven't rerun with the most recent release yet for the RET 507 and similar rules, quite possibly fixed!

[charliermarsh]

Ah yeah, I mostly meant that I have issues with the implementation. It's a direct port of the flake8 plugin, and it just has a bunch of limitations (in addition to being hard to reason about).

[henryiii]

Ahh, I see what you mean.

def f(y):
    x = 1
    if y:
        return 2

    return x

Would pass. I could live with that in some/most cases.

[charliermarsh]

I think it will still complain about x=1 :(

[henryiii]

In a realistic example (where other things are happening to the x before the final if), it seems pretty happy. I didn't end up with any false positives that I couldn't handle knowing that it was fine just changing the assignment in the if to a return. There were several when converting Plumbum and scikit-hep/particle@10caa90 was the one that prompted me to search for this issue.

[martinlehoux]

Just to add one more (almost) real life example: I have this pattern that seems reasonable in Django to build a queryset based on different inputs

def get_queryset(option_1, option_2):
    queryset: Any = None

    queryset = queryset.filter(a=1)

    if option_1:
        queryset = queryset.annotate(b=Value(2))

    if option_2:
        queryset = queryset.filter(c=3)

    return queryset

main.py:15:12: RET504 Unnecessary variable assignment before return statement

[henryiii]

This would be how you’d make it happy:

def get_queryset(option_1, option_2):
    queryset: Any = None

    queryset = queryset.filter(a=1)

    if option_1:
        queryset = queryset.filter(b=2)

    if option_2:
        return queryset.filter(c=3)

    return queryset

Not quite as symmetric, but not really “worse” from a readers standpoint either. I’d probably disable the check if this was a lot of very symmetric conditions (like you have described), though in practice most of them are not that symmetric and are fine written with the extra return. Overriding a variable is a bit ugly, so avoiding an unnecessary one isn’t that bad.

[gdub]

Personally, I dislike the resulting work-around because:

• The lack of symmetry feels unnatural.
• If you need to add an additional condition block, you would have churn turning the return statement back to an assignment.
• It does not save any lines, as is the case for simple assignment then return.

One idea would be to only flag the single-assignment and return scenarios, e.g.:

def get_queryset():
    queryset = Model.filter(a=1)
    return queryset

...and not flag scenarios where there are multiple assignments to the same variable before returning, e.g.:

def get_queryset(option_1, option_2):
    queryset = Model.filter(a=1)
    if option_1:
        queryset = queryset.filter(b=2)
    if option_2:
        queryset = queryset.filter(c=3)
    return queryset

Though, I guess that would also mean that something like this would pass not get flagged:

def get_queryset():
    queryset = Model.filter(a=1)
    queryset = queryset.filter(c=3)
    return queryset

Which raises the question of what is it that this rule is really meant to be targeting? Cases where would save a line of code due to an unnecessary assignment? Should check be simplified to only look for the simple assign/return case that happens at the same nesting level, and ignore if there is more complex branching/looping involved?

[Czaki]

Though, I guess that would also mean that something like this would pass not get flagged:

If the rule should only be ignored if the assignment is under if clause.