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.

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

R504 false positive when variable is a function parameter #133

Open
jakkdl opened this issue Mar 16, 2023 · 2 comments
Open

R504 false positive when variable is a function parameter #133

jakkdl opened this issue Mar 16, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@jakkdl
Copy link

jakkdl commented Mar 16, 2023

  • Date you used flake8-return: 2023-03-16
  • flake8-return version used, if any: 1.2.0
  • Python version, if any: 3.10.9
  • Operating System: Linux

Description

def foo(a):
    if ...:
        a = 2
    return a
$ flake8 foo.py --select=R
foo.py:4:12: R504 unnecessary variable assignment before return statement.

variables that are function parameters should not be treated as unnecessary, cursory skim of https://github.com/afonasev/flake8-return/blob/master/flake8_return/mixins/unnecessary_assign.py looks like it doesn't look at FunctionDef or AsyncFunctionDef at all which explains it.

@afonasev
Copy link
Owner

Thank you for your issue! My personal opinion is that it is bad practice to change the value of an argument within a function, it is better to use a separate variable. But I will be glad to accept your contribution, which will correctly handle FunctionDef or AsyncFunctionDef.

@afonasev afonasev self-assigned this Mar 16, 2023
@afonasev afonasev added the enhancement New feature or request label Mar 16, 2023
@jakkdl
Copy link
Author

jakkdl commented Mar 16, 2023

Sure, it's got typing issues too - but the code that inspired the above example was actually an attempt to get rid of R504 for a more complex case - where the underlying complexity is in class attribute modifications where adding a new variable doesn't help:

class A:
    def foo(self, a):
        b = a
        if ...:
            b = function(self.class_var)
        self.class_var = None

        return b

this will also raise R504, and only way to get around that is with a different temporary variable (or even one per class variable you're caring about, which quickly becomes a vastly inferior solution, or in my case modifying a lot of state with a method call).

class A:
    def foo(self, a):
        tmp_var = self.class_var
        self.class_var = None

        if ...:
            return function(tmp_var)

        return a

I'm much less sure about how to go about attempting to get rid of that false positive though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants