New check: variable used only for type checking

[nickdrozd]

Current problem

I have no immediate way to tell which imports are used for runtime functionality and which are used only for type checking.

Desired solution

I would like for Pylint to warn when imports are used only for type annotations and not for any runtime functionality. Such imports can then be moved behind the TYPE_CHECKING flag.

Should this check be an extension? I'm not sure. Are there any undeniable benefits to putting type imports behind the type flag? Is it something that everyone really ought to do? If so, then maybe it should be enabled by default, and maybe an extension otherwise.

Only users with type annotations would be affected. I imagine such users are already more fastidious than average, so maybe they would appreciate it.

Additional context

No response

[Pierre-Sassoulas]

I think this is opinionated. This should be an extension.

[DanielNoord]

This is definitely too opinionated I think. By not putting them behind the flag you prevent future import cycles. The flags (imo) main benefit is too avoid cycles that are already present when you start adding typing. Otherwise you should try your best to avoid them.

[nickdrozd]

@DanielNoord Based on what you said, it sounds like putting imports behind the flag is basically a hack that should be avoided if possible. Or in other words, imports ought to be hoisted out from behind the flag if they can be. In that case, should be there be a check for the opposite?

What I would really like to know is whether there are any measurable performance differences either way. I linked above to the issue of dict literals vs the dict constructor, where it turned out that the literals are indeed faster. And in that case the matter of literal vs constructor is decided in favor of literal. Is there anything like that here?

[DanielNoord]

There are a number of concerns with performance and imports. Most notably the use of from module import ... and import module; module.... However, the difference vary depending on the way you use the imported objects so we have refrained from making a check for it.

I think there is a valid use case for the flag so a check doesn't make a lot of sense. I can see how somebody would want to not allow the use of it, but then I think you should just use an import linter. Either way, there doesn't seem to be a real general advice here and thus pylint shouldn't try and create one.

[jacobtylerwalls]

I actually quite like this idea and am surprised it hasn't been proposed before. It's an easy mistake to import things not necessary at runtime.

[nickdrozd]

This check could be expanded to look not just for imports that are only used for type checking, but generally for anything that is used only for type checking. For example:

CompoundType = list[tuple[int, int]]

def f() -> CompoundType:
    return [(1, 2), (3, 4), (5, 6)]

The type alias CompoundType is only used for type checking, so it can be pushed behind the TYPE_CHECKING flag:

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    CompoundType = list[tuple[int, int]]

def f() -> CompoundType:
    return [(1, 2), (3, 4), (5, 6)]

Note that from __future__ import annotations is required for this. I have this in pretty much all my files now; I wish they would just make it standard.

[nickdrozd]

Here is the package import diagram for Astroid (minus brains) generated using #8824:

A dashed line from A to B indicates that A imports B only for type checking, or at least that B is only imported in A from behind the TYPE_CHECKING flag.

There are a few dashed lines in the diagram. For instance, all the arrows outbound from astroid.typing are dashed -- the module only imports for type checking.

But there could be a lot more dashed lines. In particular, it could arranged that all the inbound arrows to astroid.typing are dashed -- the module would only ever be imported for type checking, and never imported at runtime. And maybe other modules too. It could simplify things, and it could also improve performance by reducing import overhead.

Manually reviewing a Pyreverse diagram is not a great way to go about this though. The extension proposed here would be very useful for ferreting out these kinds of imports.

[Pierre-Sassoulas]

Thank you for the visual confirmation that astroid is indeed pure spaghetti, appreciated it.

[nickdrozd]

@jacobtylerwalls How much of the machinery for a check like this is already in place? I guess the basic logic would be something like: for each import, if the import is (1) used only for type checking and (2) not imported under the type checking flag, raise a warning about it. I think the code for (2) exists somewhere, but what about (1)?

[jacobtylerwalls]

Don't think (1) is implemented anywhere.

[nickdrozd]

I think the best way to implement this would be to piggyback off the existing name consumer infrastructure. Modify NamesConsumer to include something like consumed_as_annotation. Then names that are consumed only as annotations but are not imported under TYPE_CHECKING can be flagged.