Skip to content
New issue

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

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

Already on GitHub? # to your account

Lone unused import in if-statement should not get autofixed, or the if-statement should removed #9472

Open
jakkdl opened this issue Jan 11, 2024 · 4 comments
Labels
rule Implementing or modifying a lint rule

Comments

@jakkdl
Copy link

jakkdl commented Jan 11, 2024

input

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from types import TracebackType

if sys.version_info < (3, 11):
    from exceptiongroup import ExceptionGroup

ruff --fix

from typing import TYPE_CHECKING
import sys

if TYPE_CHECKING:
    pass

if sys.version_info < (3, 11):
    pass

problem

The latter is clearly awful code with fully redundant if statements, and the sys and TYPE_CHECKING imports are in fact unused. There seems to be some custom logic for inserting the pass statement, which I think should be removed and this case should not be treated as an auto-fix - since currently you need to manually fix it anyway (except currently you aren't warned about it, and have to check the diff).
Other solutions include warning and/or autofixing* if [...]: pass statements. If they exist, they should be included by default.

This is running a new, clean, venv with no config. Version 0.1.11

  • probably only autofix if the condition solely contains checks against typing.TYPE_CHECKING, sys.platform, sys.version_info and maybe a few more.
@jakkdl jakkdl changed the title Lone unused import in if-statement should not get autofixed, or the if-statement removed Lone unused import in if-statement should not get autofixed, or the if-statement should removed Jan 11, 2024
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jan 11, 2024
@charliermarsh
Copy link
Member

I think we have a rule for detecting and deleting empty type-checking blocks specifically, but we could make this part of the fix for the general "delete statement" logic.

@jakkdl
Copy link
Author

jakkdl commented Jan 12, 2024

I think we have a rule for detecting and deleting empty type-checking blocks specifically, but we could make this part of the fix for the general "delete statement" logic.

Ah you're right, TCH005. So a partial fix would be enabling that by default.

Searching through the rules, I'm not seeing a rule for "empty/unnecessary if-statement" - even in cases where an if statement does have side-effects and removing it outright is dangerous, if <condition>: pass should probably have a rule.

@gpauloski
Copy link

Is there interest in a PR for an "empty-if-statement" rule (potentially with an unsafe autofix)? I'd really love a way to catch empty if sys.version_info statements when unused imports are removed.

Alternatively, RUF034 (#13218) could be expanded to check statements rather than just expressions to cover this case. This might also depend on if there should be an unsafe autofix (re: #13852).

cc @AlexWaygood @MichaReiser

@MichaReiser
Copy link
Member

@gpauloski I could see such a rule to be useful, but we should think about its scope. E.g what about empty else branches (in for, try etc statements)? And we have to be careful about typing stubs. It's probably best to discuss such a rule in a new issue

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

4 participants