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

[TCH] moving typing imports into TYPE_CHECKING block does not work as intended #9197

Closed
picnixz opened this issue Dec 19, 2023 · 4 comments · Fixed by #9214
Closed

[TCH] moving typing imports into TYPE_CHECKING block does not work as intended #9197

picnixz opened this issue Dec 19, 2023 · 4 comments · Fixed by #9214
Assignees
Labels
bug Something isn't working linter Related to the linter

Comments

@picnixz
Copy link

picnixz commented Dec 19, 2023

This is a follow-up of #9196 (actually #9196 happened when I was trying to figure out this issue).

Consider the following configuration:

[tool.ruff]
target-version = 'py311'
ignore = ['ALL']
select = ['TCH']

[tool.ruff.lint.flake8-type-checking]
exempt-modules = []

Invoking

python3.11 -m ruff file.py --fix --unsafe-fixes

on

from __future__ import annotations

from typing import TYPE_CHECKING, Final

if TYPE_CHECKING:
    from collections.abc import Sequence

Const: Final[Sequence[str]] = ['a']

has no effect. However, the Final annotation should not be required because of from __future__ import annotations. More precisely, here's what I would expect:

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from collections.abc import Sequence
    from typing import Final

Const: Final[Sequence[str]] = ['a']

I tried to find related issues but most of them were lacking from __future__ import annotations and thus annotations were treated as required at runtime. However, in my case, I really want the Final import to be moved inside a TYPE_CHECKING block, whence

[tool.ruff.lint.flake8-type-checking]
exempt-modules = []

As such, I think the bug is related to flake8-type-checking itself.

Environment

Platform:              linux; (Linux-5.3.18-lp152.106-default-x86_64-with-glibc2.26)
Python version:        3.11.6 (main, Nov 29 2023, 14:46:32) [GCC 7.5.0])
Python implementation: CPython
ruff version:          0.1.8
@picnixz picnixz changed the title [TCH] typing imports into TYPE_CHECKING block are not always moved [TCH] moving typing imports into TYPE_CHECKING block does not work as intended Dec 19, 2023
@charliermarsh charliermarsh added bug Something isn't working linter Related to the linter labels Dec 19, 2023
@charliermarsh charliermarsh self-assigned this Dec 19, 2023
@charliermarsh
Copy link
Member

We definitely shouldn't panic here, but can I ask why you want put your typing imports in the TYPE_CHECKING block? There's no runtime advantage to doing so since you already have to import typing to get TYPE_CHECKING itself, so it doesn't help with startup time or reducing circular dependencies or anything like that.

@eli-schwartz
Copy link
Contributor

As far as I can tell this works as expected according to the documentation.

$ ruff check source.py --fix --unsafe-fixes --diff
--- source.py
+++ source.py
@@ -1,9 +1,9 @@
 from __future__ import annotations
 
 from typing import TYPE_CHECKING
-from typing import Final
 
 if TYPE_CHECKING:
+    from typing import Final
     from collections.abc import Sequence
 
 Const: Final[Sequence[str]] = ['a']

Would fix 1 error.

... however, per the documentation you need to also add the strict = true setting.

Enforce TC001, TC002, and TC003 rules even when valid runtime imports are present for the same module.

In case you're wondering why my example is different from yours, and has Final on its own import line -- without that change, it panicked again. I also had to remove ignore = ['all'] as that just complains it's an unknown rule selector.

@eli-schwartz
Copy link
Contributor

but can I ask why you want put your typing imports in the TYPE_CHECKING block? There's no runtime advantage to doing so since you already have to import typing to get TYPE_CHECKING itself, so it doesn't help with startup time or reducing circular dependencies or anything like that.

On general principle:

  • It avoids binding the name
  • I believe if you have a runtime circular import where typing.py imports from your current module, but after TYPE_CHECKING is defined and before Final is defined, then if you don't actually attempt to forcibly bind Final "right now", you can return to processing the rest of your current module before finishing typing.py. It's kind of sketchy to want to do that but 🤷

@picnixz
Copy link
Author

picnixz commented Dec 20, 2023

We definitely shouldn't panic here, but can I ask why you want put your typing imports in the TYPE_CHECKING block? There's no runtime advantage to doing so since you already have to import typing to get TYPE_CHECKING itself, so it doesn't help with startup time or reducing circular dependencies or anything like that.

It's as eli said, it's to avoid binding the name. Also it's to reduce the amount of imports at the top of the file for clarity purposes (I can collapse the TYPE_CHECKING block).

I also had to remove ignore = ['all'] as that just complains it's an unknown rule selector.

Ah sorry about that, I think it's 'ALL' that I needed to put; I'll edit the issue shortly.

... however, per the documentation you need to also add the strict = true setting.

Oh I am also sorry about this. I usually carefully read the docs before complaining but I think it passed under the radar. However, with that enabled, I also have a linter panic in my case as yopu said and this is probably related to #9196. For instance, the following linter-panicks

from __future__ import annotations

from typing import TYPE_CHECKING, Final

if TYPE_CHECKING:
    from collections.abc import Sequence

Const: Final[Sequence[str]] = ['a']

with

[tool.ruff.lint.flake8-type-checking]
exempt-modules = []
strict = true

So I agree that it works as intended but with 'strict' it's a linter panic. I'll update the title issue as well or we can move the discussion to #9196 if it's better for you.

charliermarsh added a commit that referenced this issue Dec 20, 2023
## Summary

If you remove `typing` from `exempt-modules`, we tend to panic, since we
try to add `TYPE_CHECKING` to `from typing import ...` statements while
concurrently attempting to remove other members from that import. This
PR adds special-casing for typing imports to avoid such panics.

Closes #5331
Closes #9196.
Closes #9197.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working linter Related to the linter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants