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

F841 ignores redefinitions of local assignments #6704

Open
mertcangokgoz opened this issue Aug 20, 2023 · 7 comments
Open

F841 ignores redefinitions of local assignments #6704

mertcangokgoz opened this issue Aug 20, 2023 · 7 comments
Labels
bug Something isn't working needs-decision Awaiting a decision from a maintainer

Comments

@mertcangokgoz
Copy link

While developing complex code in Django, I realised that F841 was not working properly.

I am leaving a small code example. under normal conditions flake8 says that the "teams" in this code are not used, but I do not encounter any warning in ruff.

Or even if it detects it, it shows the last one found, it must show all three

def test():
    if user.amin_user:
        teams = user.teams.all()
        if user.teams.filter(name="admin").exists():
            print("admin")
        else:
            print("not admin")

    if user.related_user:
        teams = user.teams.all()
        if user.teams.filter(name="related").exists():
            print("related")
        else:
            print("not related")

    if user.test_user:
        teams = user.teams.all()
        if user.teams.filter(name="test").exists():
            print("test")
        else:
            print("tests")

    if user.related_user:
        teams = user.teams.all()
        if user.teams.filter(name="related").exists():
            print("related")
        else:
            print("not related")

Ruff version: 0.0.285

@charliermarsh
Copy link
Member

I see the same output here for Ruff and Flake8.

Ruff:

❯ ruff check foo.py -n --select F841 --isolated
foo.py:24:9: F841 [*] Local variable `teams` is assigned to but never used

Flake8:

❯ flake8 foo.py --select F841
foo.py:24:9: F841 local variable 'teams' is assigned to but never used

What am I missing here? :)

@charliermarsh charliermarsh added the needs-info More information is needed from the issue author label Aug 20, 2023
@mertcangokgoz
Copy link
Author

Sorry, my example is wrong, the trouble starts exactly as follows.

def test():
    if user.admin_user:
        teams = user.teams.all()
        if user.teams.filter(name="admin").exists():
            print("admin")
        else:
            print("not admin")

    if user.related_user:
        if user.teams.filter(name="related").exists():
            print("related")
        else:
            print("not related")

    if user.test_user:
        if user.teams.filter(name="test").exists():
            print("test")
        else:
            print("tests")

    if user.related_user:
        if user.teams.filter(name="related").exists():
            print("related")
        else:
            print("not related")

    if user.org_user:
        teams = user.teams.all()
        if teams.filter(booking=True).exists():
            return self.filter(models.Q(booking__isnull=False))
        return self.filter()

    return self.none()

@charliermarsh
Copy link
Member

Thanks, though I still don't see F841 errors for Ruff or Flake8 with that code:

❯ flake8 foo.py --select F841
❯ ruff check foo.py -n --select F841

@mertcangokgoz
Copy link
Author

Yes, but normally you should see it, because the teams in line 3 have never been used :S

@charliermarsh
Copy link
Member

There are a bunch of Pyflakes issues about this like PyCQA/pyflakes#498 or the parent issue PyCQA/pyflakes#715. It's not trivial because we need to handle cases like:

a = 1
if ...:
  a = 2
print(a)

Or, above:

def test():
    if user.admin_user:
        teams = user.teams.all()
        if user.teams.filter(name="admin").exists():
            print("admin")
        else:
            print("not admin")

    if user.org_user:
        # Redefined `teams`, unused.
        teams = user.teams.all()

    # But it _might_ be used here.
    if teams.filter(booking=True).exists():
        return self.filter(models.Q(booking__isnull=False))

    return self.filter()

Undecided on whether to keep this open as it does have parity with Pyflakes and this kind of branch analysis is probably a bigger project beyond this issue.

@charliermarsh charliermarsh added bug Something isn't working needs-decision Awaiting a decision from a maintainer and removed needs-info More information is needed from the issue author labels Aug 20, 2023
@charliermarsh charliermarsh changed the title F841 Not working properly F841 ignores redefinitions of local assignments Aug 20, 2023
@mertcangokgoz
Copy link
Author

It makes it very difficult for us to find the definitions that the developer overlooked in the ci/cd process. Thank you for the information and tagging. <3

@MichaReiser
Copy link
Member

I think Ruff should support this but it requires a branch sensitive analysis which Ruff doesn't support today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

No branches or pull requests

3 participants