Add new check: unguarded-typing-import
#9964
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
How about 'unguarded-typing-import' for the name ? Also this need to be disabled by default or an extension (seeing the number of violations in astroid and home assistant alone). Well maybe not as it's the 4.0 branch now, depends if we want to make this default or not long term.
|
I like I think the check makes sense, at least as much sense as vanilla unused-import. You can have bugs from depending on the coincidental importing of one module by another and then be surprised when a less-common code path doesn't import those modules. I think it's likely we want this off by default, but I'm hoping to do away with the concept of extensions in 4.0, so I'm not sure we need to worry about this too much right now. |
|
I'm wondering about the perf implication in astroid. I suppose it should make the startup time better (?). if we fix astroid to always guard typing import we could benchmark it. And if it's a big improvement I would definitely be in favor of making this a default check. But think I remember that Marc or Daniel had reservation about always using typing guard and it's a lot of work to fix those 241 violations. Regarding removing extensions, now that we have a way to disable by default in "standard checkers" we could remove extensions imo. Maybe we could open an issue to see if there are other opinions than ours. |
|
I changed the name and added a test. There are currently some false positives for I also opened a PR against Astroid to move typing imports behind the guard. It takes a little work, but it doesn't take a lot of thought. It should be easy to automate (if Pylint ever gets an autofix feature). |
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
changed the title
import-used-only-for-typecheckingunguarded-typing-impor
Pierre-Sassoulas
changed the title
unguarded-typing-imporunguarded-typing-import
|
Some feedback as a user of pylint. While perhaps there are users who want this, I am not sure how large that number is. I am also not convinced how much it saves in terms of performance. Is there some data to demonstrate the performance savings? You still have to import typing for TYPE_CHECKING. How much is the saving when other types are not imported from typing library? I would like to see concrete data. Also, importing typing without TYPE_CHECKING guards is a pretty common way of writing code. AFAIK it is not discouraged. So not sure why the code needs to be changed. Also, I personally wouldn't want to make this change in my repos at work. The total cost for every developer to turn off this at a large company will be large. I am not sure whether it is worth the cost for the benefit it may provide. At the very minimum, we should not make this the default. |
|
This should definitely be off by default (and to be clear when I say do away with the concept of extensions, I don't mean turn everything on, I mean replace it with a more consistent on/off base default as mused in #3512). I'm also open to the idea of not merging this. (FYI @nickdrozd we should probably wait to polish this until we get more opinions).
I don't think performance is the main motivation. I think the idea is that having unnecessary imports promotes cyclic imports or the hiding of cyclic imports by late imports inside functions, which will emit their own warnings. The best statement of the problem I see is from Carl Meyer:
The reason I suggested we bump main to 4.0 was so that we can start tackling #3512. IMO the main reason people don't use pylint is because it has (to borrow Carl's word) fanned out a bit much. No clutter-free configuration/readme recipe/button-push way (other than just
Pylint has a lot of refactoring messages where nothing is strictly wrong. This proposed message is one of them, just a slight future-proofing advantage against possible import cycles. |
|
I'm not familiar with this, what are the details? |
|
#8893 should be fixed before merging this too. Is there a way to mark that issue as a blocker? |
There's also those that think this is the best thing since sliced bread. And they can enable it if they want. It should probably be by using a mozzilla template / shortcut, because this is something from the Mozzilla style guideline afair. So, yes, this kind of opinionated checks should be disabled by default so the default is acceptable for everyone. Same for #3512 is the main reason pylint is not adopted i.e. a lot of article that say "pylint is nice but you need to configure it before it's nice", the highest priority issue at the moment imo. |
the blocker label exists but it's more for something like "we can't release the milestone if this is not fixed". But we could release without it as long as we don't merge this. We can draft this MR as long as #8893 is not fixed imo |
|
Okay, the check is disabled by default. Everyone will get to keep their unused runtime imports 😆 |
This comment has been minimized.
This comment has been minimized.
|
The new test passes for me locally but not in CI. Is the check being enabled incorrectly? The check is also being run in the primer, which I think it shouldn't be? |
|
I think we still want the primer to test all checks, so we can make the most informed decisions when reviewing features and fixes. |
|
Thank you @jacobtylerwalls for providing the necessary background on this check. I can see the value of this check but yes everyone here realizes that onboarding this check on to is a pretty major ask so leaving this out by default makes sense. Thank you for the update @nickdrozd #3512 is a wonderful feature and definitely worth pursuing. It will be good if some of the checks pylint can do that are useful that some other static checkers can't do make it to this list because pylint can follow imports and do inference based checks. |
|
@nickdrozd I suspect |
|
One thought I have. Is it worth leaving out |
Yeah, there is definitely a bug in there. I think it is closely related to (and maybe even caused by) an existing bug in the consumer logic that is also responsible for problems like #8893.
I don't know if I speak for all users who would want this check, but what I am really interested in knowing is which runtime imports can be avoided. This goes for There is also a function called # XXX REMOVE me :
if context in (Context.Del, Context.Store): # 'Aug' ??
newnode = cast(Union[nodes.AssignName, nodes.DelName], newnode)
self._save_assignment(newnode)Besides all that, if it is going to be disabled by default, it may as well be as strict as possible, since all users will have opted in. |
|
I don't know that segment well enough but |
nickdrozd
force-pushed
the
used-only-for-typechecking
branch
from
70c836f to
af507d7
Compare
This comment has been minimized.
This comment has been minimized.
nickdrozd
force-pushed
the
used-only-for-typechecking
branch
from
af507d7 to
d78d4ee
Compare
Type of Changes
Description
This PR adds a new check to warn about imports that are used only for typechecking but are imported outside of the
TYPE_CHECKINGflag. These are imported at runtime but not used at runtime. As far as I can tell, there is no good reason to do this, and I assume that such imports are a slight drag on speed and possibly memory as well.There is the question of whether or not this check should be enabled by default. As can be seen from the output below, it will raise a lot of warnings on codebases that are large and have a lot of typechecking. These are not false negatives or noise: they really are runtime imports that are not used at runtime. Code that doesn't use type annotations will not be affected. My feeling is that the kind of people who use Python annotations are generally on the fastidious side to begin with, and would be interested to know whether they are doing any useless runtime importing.
The initial form of this PR includes only the check (enabled). There are associated tasks that need to be done (testing, documentation, fixing warnings in Pylint itself, etc), but I will get to these after some discussion.
Surprisingly, it seems to mostly work! I am aware of one bug at the moment, and I don't know the cause yet.
TODO:
Update existing testsAddress warnings in PylintCloses #8111