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

New check: variable used only for type checking #8111

Open
nickdrozd opened this issue Jan 25, 2023 · 11 comments · May be fixed by #9964
Open

New check: variable used only for type checking #8111

nickdrozd opened this issue Jan 25, 2023 · 11 comments · May be fixed by #9964
Labels
Enhancement ✨ Improvement to a component Import system Needs decision 🔒 Needs a decision before implemention or rejection typing
Milestone

Comments

@nickdrozd
Copy link
Collaborator

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

@nickdrozd nickdrozd added Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling Enhancement ✨ Improvement to a component typing Import system and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jan 25, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the Needs decision 🔒 Needs a decision before implemention or rejection label Jan 25, 2023
@Pierre-Sassoulas
Copy link
Member

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

@DanielNoord
Copy link
Collaborator

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

@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
Copy link
Collaborator

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

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

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 nickdrozd changed the title New check: import used only for type checking New check: variable used only for type checking Apr 13, 2023
@nickdrozd
Copy link
Collaborator Author

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

packages

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

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

@nickdrozd
Copy link
Collaborator Author

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

Don't think (1) is implemented anywhere.

@nickdrozd
Copy link
Collaborator Author

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.

@nickdrozd nickdrozd linked a pull request Sep 24, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Import system Needs decision 🔒 Needs a decision before implemention or rejection typing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants