From 44aab70b8862da08c34193567382a459efce7eaf Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Wed, 15 Apr 2020 19:21:12 +0200 Subject: [PATCH 01/14] Omit code guarded by 'if TYPE_CHECKING' This commit finds a problem that has been observed in read code. Consider the following code: ``` from typing import TYPE_CHECKING ... if TYPE_CHECKING from a import A, B from b import C ... def f() -> "B": ... def f() # Oops! C is actually used here. C() ``` This commit ignores all code that is guarded by TYPE_CHECKING in order to find mistakes like this. This constant is always False when mypy is not running anyway. --- pyflakes/checker.py | 23 ++++++++++++++++++----- pyflakes/test/test_imports.py | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index 5af956cc..9a8ef5e8 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -314,6 +314,7 @@ def __init__(self, name, source): self.name = name self.source = source self.used = False + self.during_type_checking = False def __str__(self): return self.name @@ -829,6 +830,7 @@ class Checker(object): _in_annotation = False _in_typing_literal = False _in_deferred = False + _in_type_checking = False builtIns = set(builtin_vars).union(_MAGIC_GLOBALS) _customBuiltIns = os.environ.get('PYFLAKES_BUILTINS') @@ -1160,11 +1162,15 @@ def handleNodeLoad(self, node): # alias of other Importation and the alias # is used, SubImportation also should be marked as used. n = scope[name] - if isinstance(n, Importation) and n._has_alias(): - try: - scope[n.fullName].used = (self.scope, node) - except KeyError: - pass + if isinstance(n, Importation): + if n._has_alias(): + try: + scope[n.fullName].used = (self.scope, node) + except KeyError: + pass + if n.during_type_checking and not self._in_annotation: + # Only defined during type-checking; this does not count. + continue except KeyError: pass else: @@ -1833,7 +1839,12 @@ def DICT(self, node): def IF(self, node): if isinstance(node.test, ast.Tuple) and node.test.elts != []: self.report(messages.IfTuple, node) + + prev = self._in_type_checking + if _is_typing(node.test, 'TYPE_CHECKING', self.scopeStack): + self._in_type_checking = True self.handleChildren(node) + self._in_type_checking = prev IFEXP = IF @@ -2120,6 +2131,7 @@ def IMPORT(self, node): else: name = alias.asname or alias.name importation = Importation(name, node, alias.name) + importation.during_type_checking = self._in_type_checking self.addBinding(node, importation) def IMPORTFROM(self, node): @@ -2154,6 +2166,7 @@ def IMPORTFROM(self, node): else: importation = ImportationFrom(name, node, module, alias.name) + importation.during_type_checking = self._in_type_checking self.addBinding(node, importation) def TRY(self, node): diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 13e7beff..5ca1e150 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1031,6 +1031,25 @@ def test_futureImportStar(self): from __future__ import * ''', m.FutureFeatureNotDefined) + def test_ignoresTypingImports(self): + """Ignores imports within 'if TYPE_CHECKING' checking normal code.""" + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from a import b + b() + ''', m.UndefinedName) + + def test_usesTypingImportsForAnnotations(self): + """Uses imports within 'if TYPE_CHECKING' checking annotations.""" + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from a import b + def f() -> "b": + pass + ''') + class TestSpecialAll(TestCase): """ From d66aeb8606e31d3ab0c3657af39f155ba0a0c72f Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Wed, 15 Apr 2020 20:53:07 +0200 Subject: [PATCH 02/14] Make the Binding parameter a constructor and use it for all bindings. --- pyflakes/checker.py | 26 +++++++++++++------------- pyflakes/test/test_imports.py | 10 ++++++++++ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index 9a8ef5e8..e9b28458 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -310,11 +310,11 @@ class Binding(object): the node that this binding was last used. """ - def __init__(self, name, source): + def __init__(self, name, source, during_type_checking=False): self.name = name self.source = source self.used = False - self.during_type_checking = False + self.during_type_checking = during_type_checking def __str__(self): return self.name @@ -1074,6 +1074,8 @@ def addBinding(self, node, value): break existing = scope.get(value.name) + value.during_type_checking = self._in_type_checking + if (existing and not isinstance(existing, Builtin) and not self.differentForks(node, existing.source)): @@ -1162,15 +1164,15 @@ def handleNodeLoad(self, node): # alias of other Importation and the alias # is used, SubImportation also should be marked as used. n = scope[name] - if isinstance(n, Importation): - if n._has_alias(): - try: - scope[n.fullName].used = (self.scope, node) - except KeyError: - pass - if n.during_type_checking and not self._in_annotation: - # Only defined during type-checking; this does not count. - continue + if isinstance(n, Importation) and n._has_alias(): + try: + scope[n.fullName].used = (self.scope, node) + except KeyError: + pass + if n.during_type_checking and not self._in_annotation: + # 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: @@ -2131,7 +2133,6 @@ def IMPORT(self, node): else: name = alias.asname or alias.name importation = Importation(name, node, alias.name) - importation.during_type_checking = self._in_type_checking self.addBinding(node, importation) def IMPORTFROM(self, node): @@ -2166,7 +2167,6 @@ def IMPORTFROM(self, node): else: importation = ImportationFrom(name, node, module, alias.name) - importation.during_type_checking = self._in_type_checking self.addBinding(node, importation) def TRY(self, node): diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 5ca1e150..66a32c99 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1040,6 +1040,16 @@ def test_ignoresTypingImports(self): b() ''', m.UndefinedName) + def test_ignoresTypingClassDefinitino(self): + """Ignores definitions within 'if TYPE_CHECKING' checking normal code.""" + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + class T: + ... + t = T() + ''', m.UndefinedName) + def test_usesTypingImportsForAnnotations(self): """Uses imports within 'if TYPE_CHECKING' checking annotations.""" self.flakes(''' From 3b601437549f39b05618b87fb8c8a25aec4001aa Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Thu, 16 Apr 2020 10:49:02 +0200 Subject: [PATCH 03/14] Remove constructor arg. --- pyflakes/checker.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index e9b28458..2ff26d80 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -310,11 +310,11 @@ class Binding(object): the node that this binding was last used. """ - def __init__(self, name, source, during_type_checking=False): + def __init__(self, name, source): self.name = name self.source = source self.used = False - self.during_type_checking = during_type_checking + self.during_type_checking = False def __str__(self): return self.name From a6f0dd230859ad3cd126afaf8d5a2ac5df573afe Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Thu, 16 Apr 2020 11:05:06 +0200 Subject: [PATCH 04/14] Another solution. --- pyflakes/checker.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index 2ff26d80..bb4bac83 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -314,7 +314,6 @@ def __init__(self, name, source): self.name = name self.source = source self.used = False - self.during_type_checking = False def __str__(self): return self.name @@ -588,6 +587,17 @@ def _add_to_names(container): class Scope(dict): importStarred = False # set to True when import * is found + # Special key for checking whether a binding is defined only for type checking. + TYPE_CHECKING_ONLY = object() + + def __init__(self): + super(Scope, self).__init__(self) + self[self.TYPE_CHECKING_ONLY] = collections.defaultdict(bool) + + def items(self): + for key, val in super(Scope, self).items(): + if key != self.TYPE_CHECKING_ONLY: + yield key, val def __repr__(self): scope_cls = self.__class__.__name__ @@ -1074,8 +1084,6 @@ def addBinding(self, node, value): break existing = scope.get(value.name) - value.during_type_checking = self._in_type_checking - if (existing and not isinstance(existing, Builtin) and not self.differentForks(node, existing.source)): @@ -1103,6 +1111,7 @@ def addBinding(self, node, value): # then assume the rebound name is used as a global or within a loop value.used = self.scope[value.name].used + self.scope[Scope.TYPE_CHECKING_ONLY][value.name] = self._in_type_checking self.scope[value.name] = value def _unknown_handler(self, node): @@ -1169,7 +1178,8 @@ def handleNodeLoad(self, node): scope[n.fullName].used = (self.scope, node) except KeyError: pass - if n.during_type_checking and not self._in_annotation: + if (self.scope[Scope.TYPE_CHECKING_ONLY][name] + and not self._in_annotation): # Only defined during type-checking; this does not count. Real code # (not an annotation) using this binding will not work. continue From b938b2be5a9b07324990fc4e47a8210194cd8fe5 Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Thu, 16 Apr 2020 20:50:26 +0200 Subject: [PATCH 05/14] Revert "Another solution." This reverts commit a6f0dd230859ad3cd126afaf8d5a2ac5df573afe. --- pyflakes/checker.py | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index bb4bac83..2ff26d80 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -314,6 +314,7 @@ def __init__(self, name, source): self.name = name self.source = source self.used = False + self.during_type_checking = False def __str__(self): return self.name @@ -587,17 +588,6 @@ def _add_to_names(container): class Scope(dict): importStarred = False # set to True when import * is found - # Special key for checking whether a binding is defined only for type checking. - TYPE_CHECKING_ONLY = object() - - def __init__(self): - super(Scope, self).__init__(self) - self[self.TYPE_CHECKING_ONLY] = collections.defaultdict(bool) - - def items(self): - for key, val in super(Scope, self).items(): - if key != self.TYPE_CHECKING_ONLY: - yield key, val def __repr__(self): scope_cls = self.__class__.__name__ @@ -1084,6 +1074,8 @@ def addBinding(self, node, value): break existing = scope.get(value.name) + value.during_type_checking = self._in_type_checking + if (existing and not isinstance(existing, Builtin) and not self.differentForks(node, existing.source)): @@ -1111,7 +1103,6 @@ def addBinding(self, node, value): # then assume the rebound name is used as a global or within a loop value.used = self.scope[value.name].used - self.scope[Scope.TYPE_CHECKING_ONLY][value.name] = self._in_type_checking self.scope[value.name] = value def _unknown_handler(self, node): @@ -1178,8 +1169,7 @@ def handleNodeLoad(self, node): scope[n.fullName].used = (self.scope, node) except KeyError: pass - if (self.scope[Scope.TYPE_CHECKING_ONLY][name] - and not self._in_annotation): + if n.during_type_checking and not self._in_annotation: # Only defined during type-checking; this does not count. Real code # (not an annotation) using this binding will not work. continue From 3474adf99df54270e5de214333e603c8efbe3db3 Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Sun, 19 Apr 2020 10:14:17 +0200 Subject: [PATCH 06/14] Add during_type_checking to __init__ and set it during construction. --- pyflakes/checker.py | 74 +++++++++++++++++++++-------------- pyflakes/test/test_imports.py | 43 ++++++++++++-------- 2 files changed, 70 insertions(+), 47 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index 2ff26d80..e6ba5488 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -310,11 +310,12 @@ class Binding(object): the node that this binding was last used. """ - def __init__(self, name, source): + def __init__(self, name, source, during_type_checking): self.name = name self.source = source self.used = False - self.during_type_checking = False + assert isinstance(during_type_checking, bool) + self.during_type_checking = during_type_checking def __str__(self): return self.name @@ -339,7 +340,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, during_type_checking=False) def __repr__(self): return '<%s object %r at 0x%x>' % (self.__class__.__name__, @@ -381,10 +382,11 @@ class Importation(Definition): @type fullName: C{str} """ - def __init__(self, name, source, full_name=None): + def __init__(self, name, source, during_type_checking, full_name=None): self.fullName = full_name or name self.redefined = [] - super(Importation, self).__init__(name, source) + super(Importation, self).__init__(name, source, + during_type_checking=during_type_checking) def redefines(self, other): if isinstance(other, SubmoduleImportation): @@ -429,11 +431,12 @@ class SubmoduleImportation(Importation): name is also the same, to avoid false positives. """ - def __init__(self, name, source): + def __init__(self, name, source, during_type_checking): # 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, during_type_checking=during_type_checking) self.fullName = name def redefines(self, other): @@ -451,7 +454,7 @@ def source_statement(self): class ImportationFrom(Importation): - def __init__(self, name, source, module, real_name=None): + def __init__(self, name, source, module, during_type_checking, real_name=None): self.module = module self.real_name = real_name or name @@ -460,7 +463,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, + during_type_checking=during_type_checking) def __str__(self): """Return import full name with alias.""" @@ -482,8 +486,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, during_type_checking): + super(StarImportation, self).__init__('*', source, + during_type_checking=during_type_checking) # Each star importation needs a unique name, and # may not be the module name otherwise it will be deemed imported self.name = name + '.*' @@ -509,7 +514,8 @@ class FutureImportation(ImportationFrom): """ def __init__(self, name, source, scope): - super(FutureImportation, self).__init__(name, source, '__future__') + super(FutureImportation, self).__init__(name, source, '__future__', + during_type_checking=False) self.used = (scope, source) @@ -517,6 +523,8 @@ class Argument(Binding): """ Represents binding a name as an argument. """ + def __init__(self, name, source): + super(Argument, self).__init__(name, source, during_type_checking=False) class Assignment(Binding): @@ -552,7 +560,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, during_type_checking): if '__all__' in scope and isinstance(source, ast.AugAssign): self.names = list(scope['__all__'].names) else: @@ -583,7 +591,7 @@ def _add_to_names(container): # If not list concatenation else: break - super(ExportBinding, self).__init__(name, source) + super(ExportBinding, self).__init__(name, source, during_type_checking) class Scope(dict): @@ -1074,8 +1082,6 @@ def addBinding(self, node, value): break existing = scope.get(value.name) - value.during_type_checking = self._in_type_checking - if (existing and not isinstance(existing, Builtin) and not self.differentForks(node, existing.source)): @@ -1233,13 +1239,15 @@ def handleNodeStore(self, node): if 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, during_type_checking=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, + during_type_checking=self._in_type_checking) elif PY2 and isinstance(getattr(node, 'ctx', None), ast.Param): - binding = Argument(name, self.getScopeNode(node)) + binding = Argument(name, self.getScopeNode(node), + during_type_checking=self._in_type_checking) else: - binding = Assignment(name, node) + binding = Assignment(name, node, during_type_checking=self._in_type_checking) self.addBinding(node, binding) def handleNodeDelete(self, node): @@ -1867,7 +1875,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, + during_type_checking=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 @@ -1969,7 +1978,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, during_type_checking=self._in_type_checking)) # doctest does not process doctest within a doctest, # or in nested functions. if (self.withDoctest and @@ -2094,7 +2104,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, during_type_checking=self._in_type_checking)) def AUGASSIGN(self, node): self.handleNodeLoad(node.target) @@ -2129,10 +2140,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, during_type_checking=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, + during_type_checking=self._in_type_checking) self.addBinding(node, importation) def IMPORTFROM(self, node): @@ -2157,16 +2170,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, during_type_checking=self._in_type_checking) else: - importation = ImportationFrom(name, node, - module, alias.name) + importation = ImportationFrom( + name, node, module, real_name=alias.name, + during_type_checking=self._in_type_checking) self.addBinding(node, importation) def TRY(self, node): diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 66a32c99..14fff280 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -14,78 +14,86 @@ class TestImportationObject(TestCase): def test_import_basic(self): - binding = Importation('a', None, 'a') + binding = Importation('a', None, during_type_checking=False, full_name='a') assert binding.source_statement == 'import a' assert str(binding) == 'a' def test_import_as(self): - binding = Importation('c', None, 'a') + binding = Importation('c', None, during_type_checking=False, full_name='a') assert binding.source_statement == 'import a as c' assert str(binding) == 'a as c' def test_import_submodule(self): - binding = SubmoduleImportation('a.b', None) + binding = SubmoduleImportation('a.b', None, during_type_checking=False) assert binding.source_statement == 'import a.b' assert str(binding) == 'a.b' def test_import_submodule_as(self): # A submodule import with an as clause is not a SubmoduleImportation - binding = Importation('c', None, 'a.b') + binding = Importation('c', None, during_type_checking=False, full_name='a.b') assert binding.source_statement == 'import a.b as c' assert str(binding) == 'a.b as c' def test_import_submodule_as_source_name(self): - binding = Importation('a', None, 'a.b') + binding = Importation('a', None, during_type_checking=False, full_name='a.b') assert binding.source_statement == 'import a.b as a' assert str(binding) == 'a.b as a' def test_importfrom_relative(self): - binding = ImportationFrom('a', None, '.', 'a') + binding = ImportationFrom('a', None, '.', + during_type_checking=False, real_name='a') assert binding.source_statement == 'from . import a' assert str(binding) == '.a' def test_importfrom_relative_parent(self): - binding = ImportationFrom('a', None, '..', 'a') + binding = ImportationFrom('a', None, '..', + during_type_checking=False, real_name='a') assert binding.source_statement == 'from .. import a' assert str(binding) == '..a' def test_importfrom_relative_with_module(self): - binding = ImportationFrom('b', None, '..a', 'b') + binding = ImportationFrom('b', None, '..a', + during_type_checking=False, real_name='b') assert binding.source_statement == 'from ..a import b' assert str(binding) == '..a.b' def test_importfrom_relative_with_module_as(self): - binding = ImportationFrom('c', None, '..a', 'b') + binding = ImportationFrom('c', None, '..a', + during_type_checking=False, real_name='b') assert binding.source_statement == 'from ..a import b as c' assert str(binding) == '..a.b as c' def test_importfrom_member(self): - binding = ImportationFrom('b', None, 'a', 'b') + binding = ImportationFrom('b', None, 'a', + during_type_checking=False, real_name='b') assert binding.source_statement == 'from a import b' assert str(binding) == 'a.b' def test_importfrom_submodule_member(self): - binding = ImportationFrom('c', None, 'a.b', 'c') + binding = ImportationFrom('c', None, 'a.b', + during_type_checking=False, real_name='c') assert binding.source_statement == 'from a.b import c' assert str(binding) == 'a.b.c' def test_importfrom_member_as(self): - binding = ImportationFrom('c', None, 'a', 'b') + binding = ImportationFrom('c', None, 'a', + during_type_checking=False, real_name='b') assert binding.source_statement == 'from a import b as c' assert str(binding) == 'a.b as c' def test_importfrom_submodule_member_as(self): - binding = ImportationFrom('d', None, 'a.b', 'c') + binding = ImportationFrom('d', None, 'a.b', + during_type_checking=False, real_name='c') assert binding.source_statement == 'from a.b import c as d' assert str(binding) == 'a.b.c as d' def test_importfrom_star(self): - binding = StarImportation('a.b', None) + binding = StarImportation('a.b', None, during_type_checking=False) assert binding.source_statement == 'from a.b import *' assert str(binding) == 'a.b.*' def test_importfrom_star_relative(self): - binding = StarImportation('.b', None) + binding = StarImportation('.b', None, during_type_checking=False) assert binding.source_statement == 'from .b import *' assert str(binding) == '.b.*' @@ -1040,16 +1048,17 @@ def test_ignoresTypingImports(self): b() ''', m.UndefinedName) - def test_ignoresTypingClassDefinitino(self): + def test_ignoresTypingClassDefinition(self): """Ignores definitions within 'if TYPE_CHECKING' checking normal code.""" self.flakes(''' from typing import TYPE_CHECKING if TYPE_CHECKING: class T: - ... + pass t = T() ''', m.UndefinedName) + @skipIf(version_info < (3,), 'has type annotations') def test_usesTypingImportsForAnnotations(self): """Uses imports within 'if TYPE_CHECKING' checking annotations.""" self.flakes(''' From b8e687a8f9598c8e9a67bed1c0dd799ac9228cbb Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Sun, 19 Apr 2020 10:22:05 +0200 Subject: [PATCH 07/14] Fix Python 2.7 --- pyflakes/checker.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index e6ba5488..f42d5a13 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -1244,8 +1244,7 @@ def handleNodeStore(self, node): binding = ExportBinding(name, node._pyflakes_parent, self.scope, during_type_checking=self._in_type_checking) elif PY2 and isinstance(getattr(node, 'ctx', None), ast.Param): - binding = Argument(name, self.getScopeNode(node), - during_type_checking=self._in_type_checking) + binding = Argument(name, self.getScopeNode(node)) else: binding = Assignment(name, node, during_type_checking=self._in_type_checking) self.addBinding(node, binding) From a30cfcba5eb20e18f340a303fe37dcde66d0c1e3 Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Sun, 19 Apr 2020 22:59:57 +0200 Subject: [PATCH 08/14] Snake case. --- pyflakes/test/test_imports.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 14fff280..51fbceb5 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1039,7 +1039,7 @@ def test_futureImportStar(self): from __future__ import * ''', m.FutureFeatureNotDefined) - def test_ignoresTypingImports(self): + def test_ignores_typing_imports(self): """Ignores imports within 'if TYPE_CHECKING' checking normal code.""" self.flakes(''' from typing import TYPE_CHECKING @@ -1048,7 +1048,7 @@ def test_ignoresTypingImports(self): b() ''', m.UndefinedName) - def test_ignoresTypingClassDefinition(self): + def test_ignores_typing_class_definition(self): """Ignores definitions within 'if TYPE_CHECKING' checking normal code.""" self.flakes(''' from typing import TYPE_CHECKING @@ -1059,7 +1059,7 @@ class T: ''', m.UndefinedName) @skipIf(version_info < (3,), 'has type annotations') - def test_usesTypingImportsForAnnotations(self): + def test_uses_typing_imports_for_annotations(self): """Uses imports within 'if TYPE_CHECKING' checking annotations.""" self.flakes(''' from typing import TYPE_CHECKING @@ -1069,7 +1069,6 @@ def f() -> "b": pass ''') - class TestSpecialAll(TestCase): """ Tests for suppression of unused import warnings by C{__all__}. From 7306c2b50316e9241ed353fe69108d4555ab0007 Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Sun, 19 Apr 2020 23:02:10 +0200 Subject: [PATCH 09/14] Rename to for_annotations --- pyflakes/checker.py | 54 +++++++++++++++++------------------ pyflakes/test/test_imports.py | 30 +++++++++---------- 2 files changed, 42 insertions(+), 42 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index f42d5a13..feba3654 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -310,12 +310,12 @@ class Binding(object): the node that this binding was last used. """ - def __init__(self, name, source, during_type_checking): + def __init__(self, name, source, for_annotations): self.name = name self.source = source self.used = False - assert isinstance(during_type_checking, bool) - self.during_type_checking = during_type_checking + assert isinstance(for_annotations, bool) + self.for_annotations = for_annotations def __str__(self): return self.name @@ -340,7 +340,7 @@ class Builtin(Definition): """A definition created for all Python builtins.""" def __init__(self, name): - super(Builtin, self).__init__(name, None, during_type_checking=False) + super(Builtin, self).__init__(name, None, for_annotations=False) def __repr__(self): return '<%s object %r at 0x%x>' % (self.__class__.__name__, @@ -382,11 +382,11 @@ class Importation(Definition): @type fullName: C{str} """ - def __init__(self, name, source, during_type_checking, 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, - during_type_checking=during_type_checking) + for_annotations=for_annotations) def redefines(self, other): if isinstance(other, SubmoduleImportation): @@ -431,12 +431,12 @@ class SubmoduleImportation(Importation): name is also the same, to avoid false positives. """ - def __init__(self, name, source, during_type_checking): + 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, during_type_checking=during_type_checking) + package_name, source, for_annotations=for_annotations) self.fullName = name def redefines(self, other): @@ -454,7 +454,7 @@ def source_statement(self): class ImportationFrom(Importation): - def __init__(self, name, source, module, during_type_checking, real_name=None): + def __init__(self, name, source, module, for_annotations, real_name=None): self.module = module self.real_name = real_name or name @@ -464,7 +464,7 @@ def __init__(self, name, source, module, during_type_checking, real_name=None): full_name = module + '.' + self.real_name super(ImportationFrom, self).__init__(name, source, full_name=full_name, - during_type_checking=during_type_checking) + for_annotations=for_annotations) def __str__(self): """Return import full name with alias.""" @@ -486,9 +486,9 @@ def source_statement(self): class StarImportation(Importation): """A binding created by a 'from x import *' statement.""" - def __init__(self, name, source, during_type_checking): + def __init__(self, name, source, for_annotations): super(StarImportation, self).__init__('*', source, - during_type_checking=during_type_checking) + 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 + '.*' @@ -515,7 +515,7 @@ class FutureImportation(ImportationFrom): def __init__(self, name, source, scope): super(FutureImportation, self).__init__(name, source, '__future__', - during_type_checking=False) + for_annotations=False) self.used = (scope, source) @@ -524,7 +524,7 @@ class Argument(Binding): Represents binding a name as an argument. """ def __init__(self, name, source): - super(Argument, self).__init__(name, source, during_type_checking=False) + super(Argument, self).__init__(name, source, for_annotations=False) class Assignment(Binding): @@ -560,7 +560,7 @@ class ExportBinding(Binding): C{__all__} will not have an unused import warning reported for them. """ - def __init__(self, name, source, scope, during_type_checking): + def __init__(self, name, source, scope, for_annotations): if '__all__' in scope and isinstance(source, ast.AugAssign): self.names = list(scope['__all__'].names) else: @@ -591,7 +591,7 @@ def _add_to_names(container): # If not list concatenation else: break - super(ExportBinding, self).__init__(name, source, during_type_checking) + super(ExportBinding, self).__init__(name, source, for_annotations) class Scope(dict): @@ -1175,7 +1175,7 @@ def handleNodeLoad(self, node): scope[n.fullName].used = (self.scope, node) except KeyError: pass - if n.during_type_checking and not self._in_annotation: + if n.for_annotations and not self._in_annotation: # Only defined during type-checking; this does not count. Real code # (not an annotation) using this binding will not work. continue @@ -1239,14 +1239,14 @@ def handleNodeStore(self, node): if isinstance(parent_stmt, (FOR_TYPES, ast.comprehension)) or ( parent_stmt != node._pyflakes_parent and not self.isLiteralTupleUnpacking(parent_stmt)): - binding = Binding(name, node, during_type_checking=self._in_type_checking) + 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, - during_type_checking=self._in_type_checking) + 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, during_type_checking=self._in_type_checking) + binding = Assignment(name, node, for_annotations=self._in_type_checking) self.addBinding(node, binding) def handleNodeDelete(self, node): @@ -1875,7 +1875,7 @@ 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, - during_type_checking=self._in_type_checking) + 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 @@ -1978,7 +1978,7 @@ def FUNCTIONDEF(self, node): self.handleNode(deco, node) self.LAMBDA(node) self.addBinding(node, FunctionDefinition( - node.name, node, during_type_checking=self._in_type_checking)) + 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 @@ -2104,7 +2104,7 @@ def CLASSDEF(self, node): self.handleNode(stmt, node) self.popScope() self.addBinding(node, ClassDefinition( - node.name, node, during_type_checking=self._in_type_checking)) + node.name, node, for_annotations=self._in_type_checking)) def AUGASSIGN(self, node): self.handleNodeLoad(node.target) @@ -2140,11 +2140,11 @@ def IMPORT(self, node): for alias in node.names: if '.' in alias.name and not alias.asname: importation = SubmoduleImportation( - alias.name, node, during_type_checking=self._in_type_checking) + alias.name, node, for_annotations=self._in_type_checking) else: name = alias.asname or alias.name importation = Importation(name, node, full_name=alias.name, - during_type_checking=self._in_type_checking) + for_annotations=self._in_type_checking) self.addBinding(node, importation) def IMPORTFROM(self, node): @@ -2175,11 +2175,11 @@ def IMPORTFROM(self, node): self.scope.importStarred = True self.report(messages.ImportStarUsed, node, module) importation = StarImportation( - module, node, during_type_checking=self._in_type_checking) + module, node, for_annotations=self._in_type_checking) else: importation = ImportationFrom( name, node, module, real_name=alias.name, - during_type_checking=self._in_type_checking) + for_annotations=self._in_type_checking) self.addBinding(node, importation) def TRY(self, node): diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 51fbceb5..0d814e21 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -14,86 +14,86 @@ class TestImportationObject(TestCase): def test_import_basic(self): - binding = Importation('a', None, during_type_checking=False, full_name='a') + binding = Importation('a', None, for_annotations=False, full_name='a') assert binding.source_statement == 'import a' assert str(binding) == 'a' def test_import_as(self): - binding = Importation('c', None, during_type_checking=False, full_name='a') + binding = Importation('c', None, for_annotations=False, full_name='a') assert binding.source_statement == 'import a as c' assert str(binding) == 'a as c' def test_import_submodule(self): - binding = SubmoduleImportation('a.b', None, during_type_checking=False) + binding = SubmoduleImportation('a.b', None, for_annotations=False) assert binding.source_statement == 'import a.b' assert str(binding) == 'a.b' def test_import_submodule_as(self): # A submodule import with an as clause is not a SubmoduleImportation - binding = Importation('c', None, during_type_checking=False, full_name='a.b') + binding = Importation('c', None, for_annotations=False, full_name='a.b') assert binding.source_statement == 'import a.b as c' assert str(binding) == 'a.b as c' def test_import_submodule_as_source_name(self): - binding = Importation('a', None, during_type_checking=False, full_name='a.b') + binding = Importation('a', None, for_annotations=False, full_name='a.b') assert binding.source_statement == 'import a.b as a' assert str(binding) == 'a.b as a' def test_importfrom_relative(self): binding = ImportationFrom('a', None, '.', - during_type_checking=False, real_name='a') + for_annotations=False, real_name='a') assert binding.source_statement == 'from . import a' assert str(binding) == '.a' def test_importfrom_relative_parent(self): binding = ImportationFrom('a', None, '..', - during_type_checking=False, real_name='a') + for_annotations=False, real_name='a') assert binding.source_statement == 'from .. import a' assert str(binding) == '..a' def test_importfrom_relative_with_module(self): binding = ImportationFrom('b', None, '..a', - during_type_checking=False, real_name='b') + for_annotations=False, real_name='b') assert binding.source_statement == 'from ..a import b' assert str(binding) == '..a.b' def test_importfrom_relative_with_module_as(self): binding = ImportationFrom('c', None, '..a', - during_type_checking=False, real_name='b') + for_annotations=False, real_name='b') assert binding.source_statement == 'from ..a import b as c' assert str(binding) == '..a.b as c' def test_importfrom_member(self): binding = ImportationFrom('b', None, 'a', - during_type_checking=False, real_name='b') + for_annotations=False, real_name='b') assert binding.source_statement == 'from a import b' assert str(binding) == 'a.b' def test_importfrom_submodule_member(self): binding = ImportationFrom('c', None, 'a.b', - during_type_checking=False, real_name='c') + for_annotations=False, real_name='c') assert binding.source_statement == 'from a.b import c' assert str(binding) == 'a.b.c' def test_importfrom_member_as(self): binding = ImportationFrom('c', None, 'a', - during_type_checking=False, real_name='b') + for_annotations=False, real_name='b') assert binding.source_statement == 'from a import b as c' assert str(binding) == 'a.b as c' def test_importfrom_submodule_member_as(self): binding = ImportationFrom('d', None, 'a.b', - during_type_checking=False, real_name='c') + for_annotations=False, real_name='c') assert binding.source_statement == 'from a.b import c as d' assert str(binding) == 'a.b.c as d' def test_importfrom_star(self): - binding = StarImportation('a.b', None, during_type_checking=False) + binding = StarImportation('a.b', None, for_annotations=False) assert binding.source_statement == 'from a.b import *' assert str(binding) == 'a.b.*' def test_importfrom_star_relative(self): - binding = StarImportation('.b', None, during_type_checking=False) + binding = StarImportation('.b', None, for_annotations=False) assert binding.source_statement == 'from .b import *' assert str(binding) == '.b.*' From 12fd45adb71aa44df63175848a87c2708d76fb0d Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Sun, 19 Apr 2020 23:23:56 +0200 Subject: [PATCH 10/14] Flake8 --- pyflakes/test/test_imports.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 0d814e21..808c1ffd 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1069,6 +1069,7 @@ def f() -> "b": pass ''') + class TestSpecialAll(TestCase): """ Tests for suppression of unused import warnings by C{__all__}. From aeaf0b69e40e67a7a9d38db8a228ce197806150b Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Sun, 19 Apr 2020 23:49:04 +0200 Subject: [PATCH 11/14] Add a test for type comments as well. --- pyflakes/test/test_imports.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 808c1ffd..77c78c29 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1069,6 +1069,18 @@ def f() -> "b": pass ''') + def test_uses_typing_imports_for_annotations_in_comments(self): + """Uses imports within 'if TYPE_CHECKING' checking annotations.""" + if self.withDoctest: + return + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from a import b + def f(): # type: () -> b + pass + ''') + class TestSpecialAll(TestCase): """ From a993d91b6485b46da441ad2757250cee1517c48a Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Mon, 27 Apr 2020 10:03:17 +0200 Subject: [PATCH 12/14] Fix: elif and else branches should not be affected. --- pyflakes/checker.py | 10 +++++++++- pyflakes/test/test_type_annotations.py | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index feba3654..2425797b 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -1852,9 +1852,17 @@ def IF(self, node): prev = self._in_type_checking if _is_typing(node.test, 'TYPE_CHECKING', self.scopeStack): self._in_type_checking = True - self.handleChildren(node) + # Handle the body of this if statement. + body_nodes = node.body + if not isinstance(body_nodes, list): + body_nodes = [body_nodes] + 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 def ASSERT(self, node): diff --git a/pyflakes/test/test_type_annotations.py b/pyflakes/test/test_type_annotations.py index abe5473f..854c6928 100644 --- a/pyflakes/test/test_type_annotations.py +++ b/pyflakes/test/test_type_annotations.py @@ -596,3 +596,23 @@ class C(Protocol): def f(): # type: () -> int pass """) + + def test_typing_guard_with_elif_branch(self): + # This test will actually not raise an error, even though it by analysis can + # be shown to have an error (Protocol is not defined outside TYPE_CHECKING). + # Pyflakes currently does not to case analysis. + + self.flakes(""" + from typing import TYPE_CHECKING + + if TYPE_CHECKING: + from typing import Protocol + elif False: + Protocol = object + else: + pass + + class C(Protocol): + def f(): # type: () -> int + pass + """) From 558914bc567580ff938e1cd72d7b819d3a676749 Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Fri, 26 Mar 2021 16:59:02 +0100 Subject: [PATCH 13/14] Review --- pyflakes/test/test_imports.py | 42 ------------------------ pyflakes/test/test_type_annotations.py | 44 +++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 43 deletions(-) diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 67565b79..9b82204f 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1039,48 +1039,6 @@ def test_futureImportStar(self): from __future__ import * ''', m.FutureFeatureNotDefined) - def test_ignores_typing_imports(self): - """Ignores imports within 'if TYPE_CHECKING' checking normal code.""" - self.flakes(''' - from typing import TYPE_CHECKING - if TYPE_CHECKING: - from a import b - b() - ''', m.UndefinedName) - - def test_ignores_typing_class_definition(self): - """Ignores definitions within 'if TYPE_CHECKING' checking normal code.""" - self.flakes(''' - from typing import TYPE_CHECKING - if TYPE_CHECKING: - class T: - pass - t = T() - ''', m.UndefinedName) - - @skipIf(version_info < (3,), 'has type annotations') - def test_uses_typing_imports_for_annotations(self): - """Uses imports within 'if TYPE_CHECKING' checking annotations.""" - self.flakes(''' - from typing import TYPE_CHECKING - if TYPE_CHECKING: - from a import b - def f() -> "b": - pass - ''') - - def test_uses_typing_imports_for_annotations_in_comments(self): - """Uses imports within 'if TYPE_CHECKING' checking annotations.""" - if self.withDoctest: - return - self.flakes(''' - from typing import TYPE_CHECKING - if TYPE_CHECKING: - from a import b - def f(): # type: () -> b - pass - ''') - class TestSpecialAll(TestCase): """ diff --git a/pyflakes/test/test_type_annotations.py b/pyflakes/test/test_type_annotations.py index 4a05900c..5a43716e 100644 --- a/pyflakes/test/test_type_annotations.py +++ b/pyflakes/test/test_type_annotations.py @@ -707,7 +707,7 @@ def f(): # type: () -> int def test_typing_guard_with_elif_branch(self): # This test will actually not raise an error, even though it by analysis can # be shown to have an error (Protocol is not defined outside TYPE_CHECKING). - # Pyflakes currently does not to case analysis. + # Pyflakes currently does not do case analysis. self.flakes(""" from typing import TYPE_CHECKING @@ -765,3 +765,45 @@ class X(TypedDict): class Y(NamedTuple): y: NamedTuple("v", [("vv", int)]) """) + + def test_ignores_typing_imports(self): + """Ignores imports within 'if TYPE_CHECKING' checking normal code.""" + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from a import b + b() + ''', m.UndefinedName) + + def test_ignores_typing_class_definition(self): + """Ignores definitions within 'if TYPE_CHECKING' checking normal code.""" + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + class T: + pass + t = T() + ''', m.UndefinedName) + + @skipIf(version_info < (3,), 'has type annotations') + def test_uses_typing_imports_for_annotations(self): + """Uses imports within 'if TYPE_CHECKING' checking annotations.""" + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from a import b + def f() -> "b": + pass + ''') + + def test_uses_typing_imports_for_annotations_in_comments(self): + """Uses imports within 'if TYPE_CHECKING' checking annotations.""" + if self.withDoctest: + return + self.flakes(''' + from typing import TYPE_CHECKING + if TYPE_CHECKING: + from a import b + def f(): # type: () -> b + pass + ''') From cbe214af12844be3747cca99d01e9c6a9edf1417 Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Fri, 26 Mar 2021 17:07:34 +0100 Subject: [PATCH 14/14] Move the check for _in_annotation --- pyflakes/checker.py | 12 ++++++------ pyflakes/test/test_type_annotations.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pyflakes/checker.py b/pyflakes/checker.py index 391758e1..feab3a42 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -1215,21 +1215,21 @@ def handleNodeLoad(self, node): self.report(messages.InvalidPrintSyntax, node) try: - scope[name].used = (self.scope, node) + n = scope[name] + if n.for_annotations and not self._in_annotation: + # Only defined during type-checking; this does not count. Real code + # (not an annotation) using this binding will not work. + continue + n.used = (self.scope, node) # if the name of SubImportation is same as # alias of other Importation and the alias # is used, SubImportation also should be marked as used. - n = scope[name] if isinstance(n, Importation) and n._has_alias(): try: scope[n.fullName].used = (self.scope, node) except KeyError: pass - if n.for_annotations and not self._in_annotation: - # 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: diff --git a/pyflakes/test/test_type_annotations.py b/pyflakes/test/test_type_annotations.py index 5a43716e..ab457238 100644 --- a/pyflakes/test/test_type_annotations.py +++ b/pyflakes/test/test_type_annotations.py @@ -773,7 +773,7 @@ def test_ignores_typing_imports(self): if TYPE_CHECKING: from a import b b() - ''', m.UndefinedName) + ''', m.UndefinedName, m.UnusedImport) def test_ignores_typing_class_definition(self): """Ignores definitions within 'if TYPE_CHECKING' checking normal code."""