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

F401 false positive (imported but unused) #2044

Closed
dllu opened this issue Jan 20, 2023 · 12 comments · Fixed by #2065
Closed

F401 false positive (imported but unused) #2044

dllu opened this issue Jan 20, 2023 · 12 comments · Fixed by #2065
Labels
bug Something isn't working

Comments

@dllu
Copy link

dllu commented Jan 20, 2023

from functools import reduce

import click

if click.confirm("do you want to disable reduce?", default=False):

    def reduce(f, d):  # noqa: F811
        return d


print(reduce(lambda x, y: x + y, [1, 2, 3, 4, 5]))

It is supposed to work like this:

snippets » python w0611.py
do you want to disable reduce? [y/N]:
15
snippets » python w0611.py
do you want to disable reduce? [y/N]: y
[1, 2, 3, 4, 5]

Here is what ruff outputs:

> ruff w0611.py
w0611.py:1:23: F401 `functools.reduce` imported but unused
Found 1 error(s).
1 potentially fixable with the --fix option.

If we apply the fix, then the code no longer works if the user presses "N" to not disable reduce.

> ruff --version
ruff 0.0.224
@charliermarsh charliermarsh added the bug Something isn't working label Jan 20, 2023
@alonme
Copy link
Contributor

alonme commented Jan 21, 2023

a simpler example without external dependencies

from math import sqrt

if True:
    def sqrt(f):
        print(f)

print(sqrt(5))

Regarding this specific issue:

pyflakes outputs

redefinition of unused 'sqrt' from line 1

pylint outputs

E0102: function already defined line 1 (function-redefined)

ruff outputs

F401 `math.sqrt` imported but unused
F811 Redefinition of unused `sqrt` from line 1

It does make sense to only output F811 in this case (ignoring if the conditional is True or False)

@charliermarsh
Copy link
Member

Yeah -- this was an intentional deviation from Flake8, since it avoids a lot of false negatives, but perhaps the tradeoff here is wrong given that autofixing false positives is more dangerous.

@charliermarsh
Copy link
Member

I'll likely change this today, to avoid these issues.

@charliermarsh
Copy link
Member

Just to prove that this behavior isn't totally without merit, Ruff will flag this as unused (and remove it), unlike Flake8:

import os

def os():
    pass

But, I agree that it's probably wrong to do that right now given the false positives.

@alonme
Copy link
Contributor

alonme commented Jan 21, 2023

Just to prove that this behavior isn't totally without merit, Ruff will flag this as unused (and remove it), unlike Flake8:

import os

def os():
    pass

But, I agree that it's probably wrong to do that right now given the false positives.

just for completeness -
pylint will raise both function-redefined and unused-import

@charliermarsh
Copy link
Member

👍 That makes sense. Pylint does more static analysis than we do right.

@charliermarsh
Copy link
Member

The relevant PR that introduced this behavior is: #1173.

charliermarsh added a commit that referenced this issue Jan 21, 2023
This is effectively a revert of #1173, to favor false-negatives over false-positives in the same-scope case.

Closes #2044.
@MichaelTiemannOSC
Copy link

I came here with a problem in this file: https://github.com/MichaelTiemannOSC/pint-pandas/blob/ducks-unlimited/pint_pandas/testsuite/test_pandas_extensiontests.py

The problem is line 24. as_frame is a fixture I want to import. Later in my code (line 498) I seclare as_frame as a parameter, which is how I used the fixture imported above. I don't understand why this one example is the only thing across all my sources that trigger the problem, but it's blocking my CI/CD right now.

MichaelTiemannOSC added a commit to MichaelTiemannOSC/pint-pandas that referenced this issue Jul 5, 2023
Except that ruff cannot be made happy, see astral-sh/ruff#2044

Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com>
@charliermarsh
Copy link
Member

Is this a pytest fixture issue? Similar to #3295 and #4046?

@MichaelTiemannOSC
Copy link

Yes, #4046 especially. Note: I'm not a maintainer of the project I'm contributing to, nor do I control the CI/CD. I'm just caught in the crossfire between how the developers factored the pytest code, what ruff thinks is OK or not, and when, if ever, the CI/CD will tell the maintainers they should bother looking at my code. I'm surprised that I cannot manually override this somehow.

@charliermarsh
Copy link
Member

I think the error you're seeing is on line 498, not line 24:

foo.py:498:31: F811 Redefinition of unused `as_frame` from line 24

Flake8 gives the same result. You should be able to add a # noqa: F811 to line 498, or just rename that parameter?

@MichaelTiemannOSC
Copy link

Silly me! I didn't put the F811 where it belonged. Now fixed... Thanks!

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

Successfully merging a pull request may close this issue.

4 participants