ruff icon indicating copy to clipboard operation
ruff copied to clipboard

F401: Ruff Incorrectly Flags Quoted Class used as Generic Parameter as Unused

Open max-muoto opened this issue 7 months ago • 6 comments

To give some context, I have from __future__ import annotations required in every-single file, and the TCH rule-set enabled across my codebase, along with the full Pyflakes rule-set.

I'm running into a situation like this:

from __future__ import annotations

from typing import TypeVar, Generic
from my_module import ModelB

T = TypeVar("T")

class BaseModel(Generic[T]):
   ...

class ModelA(BaseClass[ModelB]]
   ...

Here, even though ModelB is only being passed in as the generic parameter, TCH moves it out of the type-checking block. If I try and quote ModelB.

from __future__ import annotations

from typing import TypeVar, Generic, TYPE_CHECKING

if TYPE_CHECKING:
    from my_module import ModelB  # Ruff flags `ModelB` as unused.

T = TypeVar("T")

class BaseModel(Generic[T]):
   ...

class ModelA(BaseClass["ModelB"]]
   ...

In this case my type-checker (Pyright) still works as expected, however F401 flags ModelB as unused and removes it. Unfortunately in my situtation, importing ModelB causes a circular import, so it would need it to go under the TYPE_CHECKING block . As a note I also have UP037 enabled, and it doesn't attempt to remove the quotes in this case.

Lot of intertwining rules here, so happy to clarify anything about my configuration. Also, I'm not sure if TCH moving ModelB when non-quoted out of the TYPE_CHECKING block is also a bug.

Ruff Version: 0.1.9 Ruff Command: ruff check .

max-muoto avatar Dec 28 '23 06:12 max-muoto

The general problem here is that, when looking at BaseClass[ModelB], we don't have a great way to know whether [Model] is a generic annotation or an actual subscript access (like BASE_CLASSES[0], as a counterexample). We could come up with some better heuristics, but right now, we effectively scope generics to those that are known in the standard library (like Sequence).

We do have a mechanism for marking classes as generics: extend-generics. Maybe that can help here?

charliermarsh avatar Dec 28 '23 13:12 charliermarsh

The general problem here is that, when looking at BaseClass[ModelB], we don't have a great way to know whether [Model] is a generic annotation or an actual subscript access (like BASE_CLASSES[0], as a counterexample). We could come up with some better heuristics, but right now, we effectively scope generics to those that are known in the standard library (like Sequence).

We do have a mechanism for marking classes as generics: extend-generics. Maybe that can help here?

Makes sense on why this is then!

Yes, that actually solves my problem, I didn't know there was a configuration setting for this. Really appreciate it!

max-muoto avatar Jan 01 '24 17:01 max-muoto

I'm assuming not, but does it support wildcards? For example, if you wanted to treat all of your Django models as valid generics you might want to do extend-generics = ["src.my_app.models.*"]

max-muoto avatar Jan 01 '24 17:01 max-muoto

I don't believe so... but we could extend it to support wildcards.

charliermarsh avatar Jan 02 '24 18:01 charliermarsh

Hi!

I have the same problem with generic annotations. I read suggestions above and I think the "store all unused imports in settings" way is a bit odd

M1troll avatar Jan 24 '24 08:01 M1troll

extend-generics is a good solution for general case. But... Would the heuristic "subscript expression in parent classes list" work? I agree that it can cause a false positive (below), but such case would be marginally less popular, I guess (I do not remember ever inheriting from dict lookup example), and can be easily overcome without linter silencing (below-2).

So, I suggest to treat any string that is a valid identifier as a name when used in a subscript in class parents list.

FP example:

BASES = {"foo": int, "bar": str}
class A(Bases["foo"]): ...  # Name "foo" not found

To overcome:

BASES = {"foo": int, "bar": str}
A_BASE = BASES["foo"]
class A(A_BASE): ...

We may also want to extend this a bit, so that "name" should be interpreted as a name:

  • class X(Foo["name"])
  • X: TypeAlias = Foo["name"] and right-hand side of new 3.12 type expression
  • Anything else I'm missing?

Alternatively, this may be introduced as some kind of "soft name" that may be absent from namespace, but marks the name as "used" in scope. With such approach the FP example above is no longer failing, and I do not see any backwards-incompatible consequences from doing that.

sterliakov avatar Mar 30 '24 23:03 sterliakov