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

Fix for false positive on F401 breaks valid code #4521

Closed
RUrlus opened this issue May 19, 2023 · 8 comments · Fixed by #4885
Closed

Fix for false positive on F401 breaks valid code #4521

RUrlus opened this issue May 19, 2023 · 8 comments · Fixed by #4885
Assignees
Labels
bug Something isn't working

Comments

@RUrlus
Copy link

RUrlus commented May 19, 2023

  • ruff v0.0.269
  • CPython 3.10.9

Issue

ruff flags a false positive F401 for imports that are used in __all__ that is guarded by a conditional statement.
Running ruff with --fix on will result in broken imports.

MRE

import random


def some_dependency_check():
    return random.uniform(0.0, 1.0) > 0.49999

if some_dependency_check():
    import math

    __all__ = ["math"]
else:
    __all__ = []
-> ruff mre.py
mre.py:9:12: F401 [*] `math` imported but unused
Found 1 error.
[*] 1 potentially fixable with the --fix option.
@charliermarsh charliermarsh self-assigned this May 19, 2023
@charliermarsh
Copy link
Member

So, can and should avoid flagging this as unused...

I do feel obligated to mention that you should try to avoid using conditionally-defined __all__ assignments :) They will cause issues with all sorts of tooling, since many conditions are undecidable at "compile" time, e.g., Pyright treats x as undefined if you use a wildcard import on this file:

x = 1

if x > 0:
    __all__ = ["x"]
else:
    __all__ = []

@charliermarsh charliermarsh added the bug Something isn't working label May 19, 2023
@JonathanPlasse
Copy link
Contributor

We should maybe merge all __all__s.

@RUrlus
Copy link
Author

RUrlus commented May 19, 2023

@charliermarsh Thanks for the quick response!

In the example you gave, x is an ast.Constant and thus pyright could/ should conclude that the else branch is unreachable at import time .

I do feel obligated to mention that you should try to avoid using conditionally-defined all assignments

I agree, but I don't know how to avoid it in the pattern where I encountered this. I want to support different backends/types for dispatching without making all of them dependencies:

from foo.backends import pandas, spark

@singledispatch
def strip_punctuation(data):
    raise NotImplementedError(f"no backend with support for type {type(data)} was found")

if pandas._supported:
    strip_punctuation.register(pandas.strip_punctuation)

if spark._supported:
    strip_punctuation.register(spark.strip_punctuation)

Where the imports are guarded, foo.backends.pandas.__init__.py:

_supported = can_import("pandas")
if _supported:
    from foo.backends.pandas.string import strip_punctuation
    
    __all__ = ["strip_punctuation"]
else:
    __all__ = []

@charliermarsh
Copy link
Member

@JonathanPlasse - Yeah I think that's probably what we need to do.

@JonathanPlasse
Copy link
Contributor

JonathanPlasse commented May 19, 2023

I can work on it if you want.

@MichaReiser
Copy link
Member

@JonathanPlasse - Yeah I think that's probably what we need to do.

I'm not sure if merging gives us the desired result in all situations:

  • When testing for public docstrings -> Merging them is what we want
  • When testing for invalid imports -> Merging is unsound because it then wont flag imports that may not exist at runtime. A better approach for this use case is to explicitly determine the "conditional" exports and potentially warn about them (but not flag any that are in both sets)
  • others?

@charliermarsh
Copy link
Member

Yeah that's true. Another option could be to iterate over all Export bindings (even those that are overridden) when we do these checks.

@charliermarsh
Copy link
Member

I'm looking into this.

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