Move typechecking imports under TYPE_CHECKING flag

[nickdrozd]

Type of Changes

	Type
✓	🔨 Refactoring

Description

Putting #9964 to the test. Move typechecking imports behind TYPE_CHECKING. Also move a few type aliases. The idea is to move out anything that isn't used at runtime.

Similar to pylint-dev/astroid#2585

[cdce8p]

Please don't. I've seen PRs similar to this in other projects before. Overall the performance benefit, if at all, is likely minor since most of the imports need to happen at one point or another anyway.

However, it does impose a greater workload on developers and us maintainer. If we change something, we constantly need to make sure to double check all imports or run the risk of something failing.

In the end I think it isn't worth it.

[nickdrozd]

However, it does impose a greater workload on developers and us maintainer. If we change something, we constantly need to make sure to double check all imports or run the risk of something failing.

I don't understand what you mean. There is the minor one-time cost of reviewing this PR (and the other one). What other workload is there?

[nickdrozd]

#8893 presents a problem for this kind of change.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.76%. Comparing base (88e4bc6) to head (8d455ab).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9970      +/-   ##
==========================================
- Coverage   95.80%   95.76%   -0.05%     
==========================================
  Files         174      174              
  Lines       18940    18750     -190     
==========================================
- Hits        18146    17956     -190     
  Misses        794      794              

Files with missing lines	Coverage Δ
pylint/checkers/bad_chained_comparison.py	100.00% <ø> (ø)
pylint/checkers/base/basic_checker.py	98.07% <100.00%> (-0.01%)	⬇️
pylint/checkers/base/basic_error_checker.py	95.43% <100.00%> (ø)
pylint/checkers/base/docstring_checker.py	97.64% <100.00%> (ø)
pylint/checkers/base/name_checker/checker.py	98.62% <ø> (-0.02%)	⬇️
pylint/checkers/base/name_checker/naming_style.py	100.00% <100.00%> (ø)
pylint/checkers/base_checker.py	94.87% <100.00%> (-0.17%)	⬇️
pylint/checkers/classes/class_checker.py	93.71% <100.00%> (-0.04%)	⬇️
pylint/checkers/classes/special_methods_checker.py	95.37% <100.00%> (-0.08%)	⬇️
pylint/checkers/deprecated.py	99.03% <100.00%> (-0.01%)	⬇️
... and 92 more

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 8d455ab

[cdce8p]

However, it does impose a greater workload on developers and us maintainer. If we change something, we constantly need to make sure to double check all imports or run the risk of something failing.

I don't understand what you mean. There is the minor one-time cost of reviewing this PR (and the other one). What other workload is there?

This is the just the first change here.

I assume at the moment nodes is only used for type annotations so it's "safe" in the if TYPE_CHECKING block. What happens if you go ahead and modify / add a new check that uses isinstance(nodes, nodes.ClassDef) for example? You would run the tests only to notice that they fail since nodes isn't defined at runtime. So you have to go back, adjust the import, and run it again.

On its own maybe not that much of work but now you move that check to another file. So the imports need to change again. Keeping that in sync will be quite frustrating, even if it can be automated with dev tools.

More importantly though, it isn't really worth it IMO.

In the example, nodes is probably already cached as it's used at runtime in a different checker. Guarding an import behind if TYPE_CHECKING for that purpose only really makes sense if you can avoid having to import a package in the first place.

[nickdrozd]

In the scenario you're proposing, you're taking a module which does not have any runtime dependency on astroid.nodes and adding a new runtime dependency on it. A new dependency where before there was none. Is a new module dependency a good idea? Can the feature be implemented without adding a new module dependency? These are questions that are worth taking a few seconds to think about. If the code is structured in such a way as to highlight when a new runtime module dependency is added, well that seems like a good thing to me. Currently the true cost of adding a new dependency is hidden because typechecking imports and runtime imports are not distinguished.

Pylint remains bedeviled by two severe problems: it is very slow and it is not type-correct. Fixing the type errors would help the speed problem, since it would allow compiling with Mypyc. But the typing problem is difficult due to an internal problem, namely a gnarly module structure based on circular imports.

In light of the these well-known and long-standing problems, your idea is that the code should be optimized to make it as easy as possible to add new runtime module dependencies? That is a very strange thing to prioritize. Personally I think adding a little friction to the process might be a good thing, if it can stanch the complexity.