Skip to content

Handle code guarded by 'if TYPE_CHECKING' correctly #530

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.

Sign up for GitHub

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

Closed
wants to merge 17 commits into from
Closed

Handle code guarded by 'if TYPE_CHECKING' correctly #530

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
44aab70
Omit code guarded by 'if TYPE_CHECKING'
PetterS Apr 15, 2020
d66aeb8
Make the Binding parameter a constructor and use it for all bindings.
PetterS Apr 15, 2020
3b60143
Remove constructor arg.
PetterS Apr 16, 2020
a6f0dd2
Another solution.
PetterS Apr 16, 2020
b938b2b
Revert "Another solution."
PetterS Apr 16, 2020
3474adf
Add during_type_checking to __init__ and set it during construction.
PetterS Apr 19, 2020
b8e687a
Fix Python 2.7
PetterS Apr 19, 2020
a30cfcb
Snake case.
PetterS Apr 19, 2020
7306c2b
Rename to for_annotations
PetterS Apr 19, 2020
12fd45a
Flake8
PetterS Apr 19, 2020
aeaf0b6
Add a test for type comments as well.
PetterS Apr 19, 2020
f5657ba
Merge branch 'master' into omit_type_checking
PetterS Apr 27, 2020
a993d91
Fix: elif and else branches should not be affected.
PetterS Apr 27, 2020
7eb8484
Merge branch 'master' into omit_type_checking
PetterS May 13, 2020
a9d4a7f
Merge branch 'master' into omit_type_checking
PetterS Mar 25, 2021
558914b
Review
PetterS Mar 26, 2021
cbe214a
Move the check for _in_annotation
PetterS Mar 26, 2021
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
90 changes: 62 additions & 28 deletions pyflakes/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,10 +321,12 @@ class Binding(object):
the node that this binding was last used.
"""

def __init__(self, name, source):
def __init__(self, name, source, for_annotations):
self.name = name
self.source = source
self.used = False
assert isinstance(for_annotations, bool)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this assert here?

self.for_annotations = for_annotations

def __str__(self):
return self.name
Expand All @@ -349,7 +351,7 @@ class Builtin(Definition):
"""A definition created for all Python builtins."""

def __init__(self, name):
super(Builtin, self).__init__(name, None)
super(Builtin, self).__init__(name, None, for_annotations=False)

def __repr__(self):
return '<%s object %r at 0x%x>' % (self.__class__.__name__,
Expand Down Expand Up @@ -391,10 +393,11 @@ class Importation(Definition):
@type fullName: C{str}
"""

def __init__(self, name, source, full_name=None):
def __init__(self, name, source, for_annotations, full_name=None):
self.fullName = full_name or name
self.redefined = []
super(Importation, self).__init__(name, source)
super(Importation, self).__init__(name, source,
for_annotations=for_annotations)

def redefines(self, other):
if isinstance(other, SubmoduleImportation):
Expand Down Expand Up @@ -439,11 +442,12 @@ class SubmoduleImportation(Importation):
name is also the same, to avoid false positives.
"""

def __init__(self, name, source):
def __init__(self, name, source, for_annotations):
# A dot should only appear in the name when it is a submodule import
assert '.' in name and (not source or isinstance(source, ast.Import))
package_name = name.split('.')[0]
super(SubmoduleImportation, self).__init__(package_name, source)
super(SubmoduleImportation, self).__init__(
package_name, source, for_annotations=for_annotations)
self.fullName = name

def redefines(self, other):
Expand All @@ -461,7 +465,7 @@ def source_statement(self):

class ImportationFrom(Importation):

def __init__(self, name, source, module, real_name=None):
def __init__(self, name, source, module, for_annotations, real_name=None):
self.module = module
self.real_name = real_name or name

Expand All @@ -470,7 +474,8 @@ def __init__(self, name, source, module, real_name=None):
else:
full_name = module + '.' + self.real_name

super(ImportationFrom, self).__init__(name, source, full_name)
super(ImportationFrom, self).__init__(name, source, full_name=full_name,
for_annotations=for_annotations)

def __str__(self):
"""Return import full name with alias."""
Expand All @@ -492,8 +497,9 @@ def source_statement(self):
class StarImportation(Importation):
"""A binding created by a 'from x import *' statement."""

def __init__(self, name, source):
super(StarImportation, self).__init__('*', source)
def __init__(self, name, source, for_annotations):
super(StarImportation, self).__init__('*', source,
for_annotations=for_annotations)
# Each star importation needs a unique name, and
# may not be the module name otherwise it will be deemed imported
self.name = name + '.*'
Expand All @@ -519,14 +525,17 @@ class FutureImportation(ImportationFrom):
"""

def __init__(self, name, source, scope):
super(FutureImportation, self).__init__(name, source, '__future__')
super(FutureImportation, self).__init__(name, source, '__future__',
for_annotations=False)
self.used = (scope, source)


class Argument(Binding):
"""
Represents binding a name as an argument.
"""
def __init__(self, name, source):
super(Argument, self).__init__(name, source, for_annotations=False)


class Assignment(Binding):
Expand Down Expand Up @@ -576,7 +585,7 @@ class ExportBinding(Binding):
C{__all__} will not have an unused import warning reported for them.
"""

def __init__(self, name, source, scope):
def __init__(self, name, source, scope, for_annotations):
if '__all__' in scope and isinstance(source, ast.AugAssign):
self.names = list(scope['__all__'].names)
else:
Expand Down Expand Up @@ -607,7 +616,7 @@ def _add_to_names(container):
# If not list concatenation
else:
break
super(ExportBinding, self).__init__(name, source)
super(ExportBinding, self).__init__(name, source, for_annotations)


class Scope(dict):
Expand Down Expand Up @@ -871,6 +880,7 @@ class Checker(object):
traceTree = False
_in_annotation = AnnotationState.NONE
_in_deferred = False
_in_type_checking = False

builtIns = set(builtin_vars).union(_MAGIC_GLOBALS)
_customBuiltIns = os.environ.get('PYFLAKES_BUILTINS')
Expand Down Expand Up @@ -1216,6 +1226,10 @@ def handleNodeLoad(self, node):
scope[n.fullName].used = (self.scope, node)
except KeyError:
pass
if n.for_annotations and not self._in_annotation:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not happen before scope[name].used is marked? It's not really being used and the variable should be eligible to be released.

See: https://github.com/PyCQA/pyflakes/pull/622/files#diff-5df44d79406311387e0056faae7a8092d1ce9cc85c7d15420186e45a0c712283L1204-R1218

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, the following test will change:

from typing import TYPE_CHECKING
if TYPE_CHECKING:                                                                                                                                                                              
    from a import b
b()

It will also return 'a.b' imported but unused after this change. Perhaps that is better.

We can do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks.

# Only defined during type-checking; this does not count. Real code
# (not an annotation) using this binding will not work.
continue
except KeyError:
pass
else:
Expand Down Expand Up @@ -1274,17 +1288,18 @@ def handleNodeStore(self, node):

parent_stmt = self.getParent(node)
if isinstance(parent_stmt, ANNASSIGN_TYPES) and parent_stmt.value is None:
binding = Annotation(name, node)
binding = Annotation(name, node, for_annotations=self._in_type_checking)
elif isinstance(parent_stmt, (FOR_TYPES, ast.comprehension)) or (
parent_stmt != node._pyflakes_parent and
not self.isLiteralTupleUnpacking(parent_stmt)):
binding = Binding(name, node)
binding = Binding(name, node, for_annotations=self._in_type_checking)
elif name == '__all__' and isinstance(self.scope, ModuleScope):
binding = ExportBinding(name, node._pyflakes_parent, self.scope)
binding = ExportBinding(name, node._pyflakes_parent, self.scope,
for_annotations=self._in_type_checking)
elif PY2 and isinstance(getattr(node, 'ctx', None), ast.Param):
binding = Argument(name, self.getScopeNode(node))
else:
binding = Assignment(name, node)
binding = Assignment(name, node, for_annotations=self._in_type_checking)
self.addBinding(node, binding)

def handleNodeDelete(self, node):
Expand Down Expand Up @@ -1973,7 +1988,20 @@ def DICT(self, node):
def IF(self, node):
if isinstance(node.test, ast.Tuple) and node.test.elts != []:
self.report(messages.IfTuple, node)
self.handleChildren(node)

prev = self._in_type_checking
if _is_typing(node.test, 'TYPE_CHECKING', self.scopeStack):
self._in_type_checking = True
# Handle the body of this if statement.
body_nodes = node.body
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add another argument only=["body"] to handleChildren to make it clearer.

if not isinstance(body_nodes, list):
body_nodes = [body_nodes]
Comment on lines +1997 to +1998
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this code ever run? I think the body is always a list here (?)

Copy link
Contributor

@terencehonles terencehonles Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in IFEXP body is not a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is a test that will fail if this if statement is removed.

for body_node in body_nodes:
self.handleNode(body_node, node)
self._in_type_checking = prev

# Handle the test of the if statement (elif, else).
self.handleChildren(node, omit=["body"])

IFEXP = IF

Expand All @@ -1994,7 +2022,8 @@ def GLOBAL(self, node):

# One 'global' statement can bind multiple (comma-delimited) names.
for node_name in node.names:
node_value = Assignment(node_name, node)
node_value = Assignment(node_name, node,
for_annotations=self._in_type_checking)

# Remove UndefinedName messages already reported for this name.
# TODO: if the global is not used in this scope, it does not
Expand Down Expand Up @@ -2096,7 +2125,8 @@ def FUNCTIONDEF(self, node):
for deco in node.decorator_list:
self.handleNode(deco, node)
self.LAMBDA(node)
self.addBinding(node, FunctionDefinition(node.name, node))
self.addBinding(node, FunctionDefinition(
node.name, node, for_annotations=self._in_type_checking))
# doctest does not process doctest within a doctest,
# or in nested functions.
if (self.withDoctest and
Expand Down Expand Up @@ -2221,7 +2251,8 @@ def CLASSDEF(self, node):
for stmt in node.body:
self.handleNode(stmt, node)
self.popScope()
self.addBinding(node, ClassDefinition(node.name, node))
self.addBinding(node, ClassDefinition(
node.name, node, for_annotations=self._in_type_checking))

def AUGASSIGN(self, node):
self.handleNodeLoad(node.target)
Expand Down Expand Up @@ -2256,10 +2287,12 @@ def TUPLE(self, node):
def IMPORT(self, node):
for alias in node.names:
if '.' in alias.name and not alias.asname:
importation = SubmoduleImportation(alias.name, node)
importation = SubmoduleImportation(
alias.name, node, for_annotations=self._in_type_checking)
else:
name = alias.asname or alias.name
importation = Importation(name, node, alias.name)
importation = Importation(name, node, full_name=alias.name,
for_annotations=self._in_type_checking)
self.addBinding(node, importation)

def IMPORTFROM(self, node):
Expand All @@ -2284,16 +2317,17 @@ def IMPORTFROM(self, node):
elif alias.name == '*':
# Only Python 2, local import * is a SyntaxWarning
if not PY2 and not isinstance(self.scope, ModuleScope):
self.report(messages.ImportStarNotPermitted,
node, module)
self.report(messages.ImportStarNotPermitted, node, module)
continue

self.scope.importStarred = True
self.report(messages.ImportStarUsed, node, module)
importation = StarImportation(module, node)
importation = StarImportation(
module, node, for_annotations=self._in_type_checking)
else:
importation = ImportationFrom(name, node,
module, alias.name)
importation = ImportationFrom(
name, node, module, real_name=alias.name,
for_annotations=self._in_type_checking)
self.addBinding(node, importation)

def TRY(self, node):
Expand Down
Loading