FA100 issue: Ruff doesnt suggest adding `from __future__ import annotations`

HappyCthulhu commented 1 month ago

Search keywords

"TCH rules", "move under TYPE_CHECKING", "FA100"

Ruff Version

0.5.7

Config

Part of pyproject.toml

``` [tool.poetry.group.dev.dependencies] ruff = "0.5.7" [tool.ruff] line-length = 120 extend-exclude = ["venv", "stubs"] [tool.ruff.lint] select = ["ALL"] unfixable = ["ERA001", "COM819"] # remove comment out code lines # COM819 prohibited-trailing-comma. Это фиксить должна format команда, чтоб как в black было # если есть запятая в конце - раскрывает на несколько строк. Если нет - оставляет в одной ignore = [ "Q", # flake8-quotes "D100", # undocumented-public-module Missing docstring in public module "D101", # undocumented-public-class "Missing" # docstring in public class "D102", # undocumented-public-method Missing docstring "in" # public method "D103", # undocumented-public-function Missing docstring in "public" # function "D104", # undocumented-public-package Missing docstring in public package "🛠" "D105", # undocumented-magic-method Missing docstring in magic method "D106", # # undocumented-public-nested-class Missing docstring in public nested class "D107", # # undocumented-public-init Missing docstring in __init__ "ANN101", # missing-type-self "TD003", # missing-todo-link missing issue link on the line following this TODO "TD002", # missing-todo-author missing author in TODO; try: `# TODO(): ...` or `# TODO @: ...`Ruff "S101", # assert Use of assert detected "ANN002", # missing-type-args Missing type annotation for *{name} "ANN003", # missing-type-kwargs Missing type annotation for **{name} "RUF001", # ambiguous-unicode-character-string String contains ambiguous {}. Did you mean {}? Может на русскую "с" в комменте ругаться "RUF003", # ambiguous-unicode-character-comment "SLF001", # private-member-access Private member accessed: {access} | учитывая, что в Django постоянно дергаем _Meta моделей и поля в духе _prefetched_objects_cache и т.д. # "FA100", # future-rewritable-type-annotation Missing from __future__ import annotations, but uses {name} # "FA102", # future-required-type-annotation Missing from __future__ import annotations, but uses "DJ001", # django-nullable-model-string-field Avoid using null=True on string-based fields such as "PGH004", # Use specific rule codes when using `noqa` "TRY003", # Avoid specifying long messages outside the exception class "EM102", # Exception must not use an f-string literal, assign to variable first "RUF002", # Docstring contains ambiguous `у` (CYRILLIC SMALL LETTER U). Did you mean `y` (LATIN SMALL LETTER Y)? "FBT001", # Boolean-typed positional argument in function definition "FBT002", # Boolean default positional argument in function definition "ANN204", # Missing return type annotation for special method `__init__` ] [tool.ruff.lint.flake8-annotations] mypy-init-return = true # ANN204 | checkers often allow you to omit the return type annotation for __init__ methods, as long as at least one argument has a type annotation [tool.ruff.lint.isort] force-wrap-aliases = true combine-as-imports = true ```

Bug Report

My request:

I would like all imports that are used only for type hints to be placed under a TYPE_CHECKING block. If you have not yet implemented this function, please consider this issue as a feature request.

Sometimes ruff doesn't detect imports, that are used only for type-hints without from __future__ import anotations. I know, that existing of from __future__ import anotations is one of requirements for ruff to make give TCH-warnings. But sometimes FA100 rules doesnt detect cases, when import is used only for type-hints. I will make an example for django-migration:

# Generated by Django 4.0.2 on 2022-09-19 12:51

from django.db import migrations
from django.db.backends.base.schema import BaseDatabaseSchemaEditor
from django.db.migrations.state import StateApps

def migrate(apps: StateApps, _schema: BaseDatabaseSchemaEditor) -> None:
    apps.get_model("app_name", "ModelName").objects.all().delete()

class Migration(migrations.Migration):
    dependencies = []  # noqa: RUF012

    operations = [  # noqa: RUF012
        migrations.RunPython(
            code=migrate,
            reverse_code=migrations.RunPython.noop,
        ),
    ]

No ruff-warnings are shown in this code.

MichaReiser commented 1 month ago

I think there are multiple things at play here.

Ruff doesn't emit any TCH error for the provided example even if the __future__ import annotations is present. It has to do with the migrations.RunPython call but I'm not sure why. See Playground Maybe @charliermarsh knows why?
Whether Ruff should add from __future__ import annotations I'm not so sure about. It could be a configuration option but the general idea is that these comments are explicit opt ins by the user.

AlexWaygood commented 1 month ago

If you know you want from __future__ import annotations added to all your Python files, you can use Ruff's I002 rule in combination with this setting: https://docs.astral.sh/ruff/settings/#lint_isort_required-imports

HappyCthulhu commented 1 month ago

I think there are multiple things at play here.

Ruff doesn't emit any TCH error for the provided example even if the __future__ import annotations is present. It has to do with the migrations.RunPython call but I'm not sure why. See Playground Maybe @charliermarsh knows why?

I dont think, thats true:

https://github.com/user-attachments/assets/4bc6bc04-712c-41e6-8baf-a10d0927fcab

As you can see, error appears after adding from __future__ import annotations

Whether Ruff should add from __future__ import annotations I'm not so sure about. It could be a configuration option but the general idea is that these comments are explicit opt ins by the user.

Im not talking about autofixing this rule (ruff have autofix option). Im talking, that i want ruff to tell me every time when some import is used only for type hint

HappyCthulhu commented 1 month ago

If you know you want from __future__ import annotations added to all your Python files, you can use Ruff's I002 rule in combination with this setting: https://docs.astral.sh/ruff/settings/#lint_isort_required-imports

I dont rly want from __future__ import annotations present in my files. But, if that is the only way to force ruff tell me, if some import is used only for type-hint, i will apply this rule. So... This way is the only one, right?

MichaReiser commented 1 month ago

I dont think, thats true:

You're right. The problem was that the original code snipped contained a syntax error. It works as expected if I fix the syntax error playground

eli-schwartz commented 1 month ago

The same file with flake8 and flake8-type-checking emits the necessary error codes:

$ flake8 test.py
test.py:5:1: TC002 Move third-party import 'django.db.backends.base.schema.BaseDatabaseSchemaEditor' into a type-checking block
test.py:6:1: TC002 Move third-party import 'django.db.migrations.state.StateApps' into a type-checking block

$ ruff check --select TCH test.py
All checks passed!

flake8-type-checking doesn't care whether you have imported future annotations yet. If you fix the imports by moving them into T.TYPE_CHECKING, it will still emit an error telling you how to fix your use of annotations that are evaluated at runtime and aren't available, via the "how to handle forward references" (there's an option to have it always advise you to use quoted strings instead):

TC100 Add 'from __future__ import annotations' import

It might be nice for flake8-type-checking to warn you about both at the same time. I just recently opened https://github.com/snok/flake8-type-checking/issues/190 which is related to this but not exactly the same thing.

In the same situation, by the way, ruff reports:

test.py:8:44: TCH004 Move import `django.db.migrations.state.StateApps` out of type-checking block. Import is used for more than type hinting.
test.py:9:48: TCH004 Move import `django.db.backends.base.schema.BaseDatabaseSchemaEditor` out of type-checking block. Import is used for more than type hinting.
Found 2 errors.

It's not used for more than type hinting, but whatever. ;)

eli-schwartz commented 1 month ago

By the way my reading of FA100 is that it is specifically applicable to cases where an annotation could be rewritten in a manner that ordinarily requires a new version of CPython, but where __future__.annotations would allow "backporting" support to an earlier version of python, i.e. PEP 585 / 604.

It is not about unlocking additional ways to discover TCH001-TCH003 "move import into a type-checking block" issues.

HappyCthulhu commented 1 month ago

It is not about unlocking additional ways to discover TCH001-TCH003 "move import into a type-checking block" issues.

So, can we treat this issue as feature request or should i create another one?

HappyCthulhu commented 1 month ago

BTW, ruff has a feature that automatically moves imports under the TYPE_CHECKING block, but this can cause issues with tools like drf-spectacular, which rely on type hints for certain classes (e.g., Serializers). When Ruff moves these type hints to the TYPE_CHECKING block, it causes Swagger to crash.

eli-schwartz commented 1 month ago

Please take a look at:

astral-sh / ruff