Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

possible false positive with RET504 and multiple-assignements #4236

Closed
Borda opened this issue May 5, 2023 · 3 comments · Fixed by #4997
Closed

possible false positive with RET504 and multiple-assignements #4236

Borda opened this issue May 5, 2023 · 3 comments · Fixed by #4997
Labels
bug Something isn't working

Comments

@Borda
Copy link

Borda commented May 5, 2023

I have the following code sample, and here I do think it is correct:

    def consume_result(self) -> T:
        if self._result is None:
            raise MisconfigurationException()
        result, self._result = self._result, None  # free memory
        return result
@Borda Borda mentioned this issue May 5, 2023
Merged
7 tasks
@charliermarsh charliermarsh added the bug Something isn't working label May 5, 2023
@charliermarsh
Copy link
Member

Yeah makes sense. This rule is so unreliable, it's just really hard to avoid false positives while retaining much utility.

@MichaReiser
Copy link
Member

Yeah makes sense. This rule is so unreliable, it's just really hard to avoid false positives while retaining much utility.

I agree that this code per se is correct and we should, if possible, detect and support it. But this can also be a case where a slight restructuring of the code fixes the false-positive (by splitting the code into two assignments)

@Borda
Copy link
Author

Borda commented May 8, 2023

But this can also be a case where a slight restructuring of the code fixes the false-positive (by splitting the code into two assignments)

Yes, but then you drop the beauty of puthon and memory efficiency too... :)
(in the end all can be just C code 🐿️)

@charliermarsh charliermarsh mentioned this issue Jun 10, 2023
Merged
charliermarsh added a commit that referenced this issue Jun 10, 2023
@charliermarsh
Refactor RET504 to only enforce assignment-then-return pattern (#4997)
02b8ce8 
## Summary

The `RET504` rule, which looks for unnecessary assignments before return
statements, is a frequent source of issues (#4173, #4236, #4242, #1606,
#2950). Over time, we've tried to refine the logic to handle more cases.
For example, we now avoid analyzing any functions that contain any
function calls or attribute assignments, since those operations can
contain side effects (and so we mark them as a "read" on all variables
in the function -- we could do a better job with code graph analysis to
handle this limitation, but that'd be a more involved change.) We also
avoid flagging any variables that are the target of multiple
assignments. Ultimately, though, I'm not happy with the implementation
-- we just can't do sufficiently reliable analysis of arbitrary code
flow given the limited logic herein, and the existing logic is very hard
to reason about and maintain.

This PR refocuses the rule to only catch cases of the form:

```py
def f():
    x = 1
    return x
```

That is, we now only flag returns that are immediately preceded by an
assignment to the returned variable. While this is more limiting, in
some ways, it lets us flag more cases vis-a-vis the previous
implementation, since we no longer "fully eject" when functions contain
function calls and other effect-ful operations.

Closes #4173.

Closes #4236.

Closes #4242.
konstin pushed a commit that referenced this issue Jun 13, 2023
@charliermarsh @konstin
Refactor RET504 to only enforce assignment-then-return pattern (#4997)
1aa4093 
## Summary

The `RET504` rule, which looks for unnecessary assignments before return
statements, is a frequent source of issues (#4173, #4236, #4242, #1606,
#2950). Over time, we've tried to refine the logic to handle more cases.
For example, we now avoid analyzing any functions that contain any
function calls or attribute assignments, since those operations can
contain side effects (and so we mark them as a "read" on all variables
in the function -- we could do a better job with code graph analysis to
handle this limitation, but that'd be a more involved change.) We also
avoid flagging any variables that are the target of multiple
assignments. Ultimately, though, I'm not happy with the implementation
-- we just can't do sufficiently reliable analysis of arbitrary code
flow given the limited logic herein, and the existing logic is very hard
to reason about and maintain.

This PR refocuses the rule to only catch cases of the form:

```py
def f():
    x = 1
    return x
```

That is, we now only flag returns that are immediately preceded by an
assignment to the returned variable. While this is more limiting, in
some ways, it lets us flag more cases vis-a-vis the previous
implementation, since we no longer "fully eject" when functions contain
function calls and other effect-ful operations.

Closes #4173.

Closes #4236.

Closes #4242.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor RET504 to only enforce assignment-then-return pattern astral-sh/ruff
3 participants
@MichaReiser @charliermarsh @Borda