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

Add new check: unguarded-typing-import #9964

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 68 additions & 11 deletions pylint/checkers/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from pylint.checkers.utils import (
in_type_checking_block,
is_module_ignored,
is_node_in_type_annotation_context,
is_postponed_evaluation_enabled,
is_sys_guard,
overridden_method,
Expand Down Expand Up @@ -409,6 +410,14 @@ def _has_locals_call_after_node(stmt: nodes.NodeNG, scope: nodes.FunctionDef) ->
"Used when an imported module or variable is not used from a "
"`'from X import *'` style import.",
),
"R0615": (
"`%s` used only for typechecking but imported outside of a typechecking block",
"unguarded-typing-import",
"Used when an import is used only for typechecking but imported outside of a typechecking block.",
{
"default_enabled": False,
},
),
"W0621": (
"Redefining name %r from outer scope (line %s)",
"redefined-outer-name",
Expand Down Expand Up @@ -482,6 +491,7 @@ class NamesConsumer:

to_consume: Consumption
consumed: Consumption
consumed_as_type: Consumption
consumed_uncertain: Consumption
"""Retrieves nodes filtered out by get_next_to_consume() that may not
have executed.
Expand All @@ -498,6 +508,7 @@ def __init__(self, node: nodes.NodeNG, scope_type: str):

self.to_consume = copy.copy(node.locals)
self.consumed = {}
self.consumed_as_type = {}
self.consumed_uncertain = defaultdict(list)

self.names_under_always_false_test: set[str] = set()
Expand All @@ -506,30 +517,46 @@ def __init__(self, node: nodes.NodeNG, scope_type: str):
def __repr__(self) -> str:
_to_consumes = [f"{k}->{v}" for k, v in self.to_consume.items()]
_consumed = [f"{k}->{v}" for k, v in self.consumed.items()]
_consumed_as_type = [f"{k}->{v}" for k, v in self.consumed_as_type.items()]
_consumed_uncertain = [f"{k}->{v}" for k, v in self.consumed_uncertain.items()]
to_consumes = ", ".join(_to_consumes)
consumed = ", ".join(_consumed)
consumed_as_type = ", ".join(_consumed_as_type)
consumed_uncertain = ", ".join(_consumed_uncertain)
return f"""
to_consume : {to_consumes}
consumed : {consumed}
consumed_as_type : {consumed_as_type}
consumed_uncertain: {consumed_uncertain}
scope_type : {self.scope_type}
"""

def mark_as_consumed(self, name: str, consumed_nodes: list[nodes.NodeNG]) -> None:
def mark_as_consumed(
self,
name: str,
consumed_nodes: list[nodes.NodeNG],
consumed_as_type: bool = False,
) -> None:
"""Mark the given nodes as consumed for the name.

If all of the nodes for the name were consumed, delete the name from
the to_consume dictionary
"""
unconsumed = [n for n in self.to_consume[name] if n not in set(consumed_nodes)]
self.consumed[name] = consumed_nodes
consumed = self.consumed_as_type if consumed_as_type else self.consumed
consumed[name] = consumed_nodes

if unconsumed:
self.to_consume[name] = unconsumed
else:
del self.to_consume[name]
if name in self.to_consume:
unconsumed = [
n for n in self.to_consume[name] if n not in set(consumed_nodes)
]

if unconsumed:
self.to_consume[name] = unconsumed
else:
del self.to_consume[name]

if not consumed_as_type and name in self.consumed_as_type:
del self.consumed_as_type[name]

def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
"""Return a list of the nodes that define `node` from this scope.
Expand Down Expand Up @@ -572,6 +599,9 @@ def get_next_to_consume(self, node: nodes.Name) -> list[nodes.NodeNG] | None:
if VariablesChecker._comprehension_between_frame_and_node(node):
return found_nodes

if found_nodes is None:
found_nodes = self.consumed_as_type.get(name)

# Filter out assignments in ExceptHandlers that node is not contained in
if found_nodes:
found_nodes = [
Expand Down Expand Up @@ -1356,7 +1386,8 @@ def leave_module(self, node: nodes.Module) -> None:
assert len(self._to_consume) == 1

self._check_metaclasses(node)
not_consumed = self._to_consume.pop().to_consume
consumer = self._to_consume.pop()
not_consumed = consumer.to_consume
# attempt to check for __all__ if defined
if "__all__" in node.locals:
self._check_all(node, not_consumed)
Expand All @@ -1368,7 +1399,7 @@ def leave_module(self, node: nodes.Module) -> None:
if not self.linter.config.init_import and node.package:
return

self._check_imports(not_consumed)
self._check_imports(not_consumed, consumer.consumed_as_type)
self._type_annotation_names = []

def visit_classdef(self, node: nodes.ClassDef) -> None:
Expand Down Expand Up @@ -1672,7 +1703,11 @@ def _undefined_and_used_before_checker(
# They will have already had a chance to emit used-before-assignment.
# We check here instead of before every single return in _check_consumer()
nodes_to_consume += current_consumer.consumed_uncertain[node.name]
current_consumer.mark_as_consumed(node.name, nodes_to_consume)
current_consumer.mark_as_consumed(
node.name,
nodes_to_consume,
consumed_as_type=is_node_in_type_annotation_context(node),
)
if action is VariableVisitConsumerAction.CONTINUE:
continue
if action is VariableVisitConsumerAction.RETURN:
Expand Down Expand Up @@ -3163,7 +3198,11 @@ def _check_globals(self, not_consumed: Consumption) -> None:
self.add_message("unused-variable", args=(name,), node=node)

# pylint: disable = too-many-branches
def _check_imports(self, not_consumed: Consumption) -> None:
def _check_imports(
self,
not_consumed: Consumption,
consumed_as_type: Consumption,
) -> None:
local_names = _fix_dot_imports(not_consumed)
checked = set()
unused_wildcard_imports: defaultdict[
Expand Down Expand Up @@ -3251,8 +3290,26 @@ def _check_imports(self, not_consumed: Consumption) -> None:
self.add_message(
"unused-wildcard-import", args=(arg_string, module[0]), node=module[1]
)

self._check_type_imports(consumed_as_type)

del self._to_consume

def _check_type_imports(
self,
consumed_as_type: dict[str, list[nodes.NodeNG]],
) -> None:
for name, import_node in _fix_dot_imports(consumed_as_type):
if import_node.names[0][0] == "*":
continue

if not in_type_checking_block(import_node):
self.add_message(
"unguarded-typing-import",
args=name,
node=import_node,
)

def _check_metaclasses(self, node: nodes.Module | nodes.FunctionDef) -> None:
"""Update consumption analysis for metaclasses."""
consumed: list[tuple[Consumption, str]] = []
Expand Down
21 changes: 21 additions & 0 deletions tests/functional/u/unguarded_typing_import.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# pylint: disable = import-error, missing-module-docstring, missing-function-docstring, missing-class-docstring, too-few-public-methods,

from mod import A # [unguarded-typing-import]
from mod import B

def f(_: A):
pass

def g(x: B):
assert isinstance(x, B)

class C:
pass

class D:
c: C

def h(self):
# --> BUG <--
# pylint: disable = undefined-variable
return [C() for _ in self.c]
2 changes: 2 additions & 0 deletions tests/functional/u/unguarded_typing_import.rc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[MESSAGES CONTROL]
enable = unguarded-typing-import
1 change: 1 addition & 0 deletions tests/functional/u/unguarded_typing_import.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
unguarded-typing-import:3:0:3:17::`A` used only for typechecking but imported outside of a typechecking block:UNDEFINED
Loading