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

FA100 issue: Ruff doesnt suggest adding from __future__ import annotations #13273

Open
HappyCthulhu opened this issue Sep 6, 2024 · 10 comments

Comments

@HappyCthulhu
Copy link

HappyCthulhu commented Sep 6, 2024

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(<author_name>): ...` or `# TODO @<author_name>: ...`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,
        ),
    ]


image

No ruff-warnings are shown in this code.

@MichaReiser MichaReiser changed the title FA100 issue: Roof doesnt suggest adding from __future__ import annotations FA100 issue: Ruff doesnt suggest adding from __future__ import annotations Sep 7, 2024
@MichaReiser
Copy link
Member

I think there are multiple things at play here.

  1. 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?
  2. 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
Copy link
Member

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
Copy link
Author

I think there are multiple things at play here.

  1. 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:

2024-09-07.14-28-56.mp4

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

  1. 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
Copy link
Author

HappyCthulhu commented Sep 7, 2024

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
Copy link
Member

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
Copy link
Contributor

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 snok/flake8-type-checking#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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants