From ef6f4383b69d2acfedefb261c6cf2a79e9ec7968 Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Sat, 21 Oct 2017 07:22:26 -0700 Subject: Add check for unused exception binding in except: block (#293) Fixes #301 --- NEWS.txt | 3 ++ pyflakes/checker.py | 56 +++++++++++++++++++++++------------ pyflakes/test/test_imports.py | 7 +++-- pyflakes/test/test_other.py | 16 ++++++++++ pyflakes/test/test_undefined_names.py | 29 +++++++++++------- 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/NEWS.txt b/NEWS.txt index ce9cc34..3e9b8c1 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -1,3 +1,6 @@ +UNRELEASED + - Check for unused exception binding in `except:` block + 1.6.0 (2017-08-03) - Process function scope variable annotations for used names - Find Python files without extensions by their shebang diff --git a/pyflakes/checker.py b/pyflakes/checker.py index 8d1887b..d8f093a 100644 --- a/pyflakes/checker.py +++ b/pyflakes/checker.py @@ -1322,34 +1322,52 @@ class Checker(object): self.handleChildren(node) return - # 3.x: the name of the exception, which is not a Name node, but - # a simple string, creates a local that is only bound within the scope - # of the except: block. + # If the name already exists in the scope, modify state of existing + # binding. + if node.name in self.scope: + self.handleNodeStore(node) + + # 3.x: the name of the exception, which is not a Name node, but a + # simple string, creates a local that is only bound within the scope of + # the except: block. As such, temporarily remove the existing binding + # to more accurately determine if the name is used in the except: + # block. for scope in self.scopeStack[::-1]: - if node.name in scope: - is_name_previously_defined = True + try: + binding = scope.pop(node.name) + except KeyError: + pass + else: + prev_definition = scope, binding break else: - is_name_previously_defined = False + prev_definition = None self.handleNodeStore(node) self.handleChildren(node) - if not is_name_previously_defined: - # See discussion on https://github.com/PyCQA/pyflakes/pull/59 - # We're removing the local name since it's being unbound - # after leaving the except: block and it's always unbound - # if the except: block is never entered. This will cause an - # "undefined name" error raised if the checked code tries to - # use the name afterwards. - # - # Unless it's been removed already. Then do nothing. + # See discussion on https://github.com/PyCQA/pyflakes/pull/59 - try: - del self.scope[node.name] - except KeyError: - pass + # We're removing the local name since it's being unbound after leaving + # the except: block and it's always unbound if the except: block is + # never entered. This will cause an "undefined name" error raised if + # the checked code tries to use the name afterwards. + # + # Unless it's been removed already. Then do nothing. + + try: + binding = self.scope.pop(node.name) + except KeyError: + pass + else: + if not binding.used: + self.report(messages.UnusedVariable, node, node.name) + + # Restore. + if prev_definition: + scope, binding = prev_definition + scope[node.name] = binding def ANNASSIGN(self, node): if node.value: diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py index 3e3be88..4723954 100644 --- a/pyflakes/test/test_imports.py +++ b/pyflakes/test/test_imports.py @@ -1,4 +1,3 @@ - from sys import version_info from pyflakes import messages as m @@ -571,11 +570,15 @@ class Test(TestCase): def test_redefinedByExcept(self): as_exc = ', ' if version_info < (2, 6) else ' as ' + expected = [m.RedefinedWhileUnused] + if version_info >= (3,): + # The exc variable is unused inside the exception handler. + expected.append(m.UnusedVariable) self.flakes(''' import fu try: pass except Exception%sfu: pass - ''' % as_exc, m.RedefinedWhileUnused) + ''' % as_exc, *expected) def test_usedInRaise(self): self.flakes(''' diff --git a/pyflakes/test/test_other.py b/pyflakes/test/test_other.py index 9c8462e..ba052f1 100644 --- a/pyflakes/test/test_other.py +++ b/pyflakes/test/test_other.py @@ -1597,6 +1597,22 @@ class TestUnusedAssignment(TestCase): except Exception%se: e ''' % as_exc) + @skipIf(version_info < (3,), + "In Python 2 exception names stay bound after the exception handler") + def test_exceptionUnusedInExcept(self): + self.flakes(''' + try: pass + except Exception as e: pass + ''', m.UnusedVariable) + + def test_exceptionUnusedInExceptInFunction(self): + as_exc = ', ' if version_info < (2, 6) else ' as ' + self.flakes(''' + def download_review(): + try: pass + except Exception%se: pass + ''' % as_exc, m.UnusedVariable) + def test_exceptWithoutNameInFunction(self): """ Don't issue false warning when an unnamed exception is used. diff --git a/pyflakes/test/test_undefined_names.py b/pyflakes/test/test_undefined_names.py index 1464d8e..3d19210 100644 --- a/pyflakes/test/test_undefined_names.py +++ b/pyflakes/test/test_undefined_names.py @@ -25,15 +25,16 @@ class Test(TestCase): @skipIf(version_info < (3,), 'in Python 2 exception names stay bound after the except: block') def test_undefinedExceptionName(self): - """Exception names can't be used after the except: block.""" + """Exception names can't be used after the except: block. + + The exc variable is unused inside the exception handler.""" self.flakes(''' try: raise ValueError('ve') except ValueError as exc: pass exc - ''', - m.UndefinedName) + ''', m.UndefinedName, m.UnusedVariable) def test_namesDeclaredInExceptBlocks(self): """Locals declared in except: blocks can be used after the block. @@ -71,7 +72,8 @@ class Test(TestCase): """Exception names are unbound after the `except:` block. Last line will raise UnboundLocalError on Python 3 but would print out - 've' on Python 2.""" + 've' on Python 2. The exc variable is unused inside the exception + handler.""" self.flakes(''' try: raise ValueError('ve') @@ -79,14 +81,15 @@ class Test(TestCase): pass print(exc) exc = 'Original value' - ''', - m.UndefinedName) + ''', m.UndefinedName, m.UnusedVariable) def test_undefinedExceptionNameObscuringLocalVariableFalsePositive1(self): """Exception names obscure locals, can't be used after. Unless. Last line will never raise UnboundLocalError because it's only entered if no exception was raised.""" + # The exc variable is unused inside the exception handler. + expected = [] if version_info < (3,) else [m.UnusedVariable] self.flakes(''' exc = 'Original value' try: @@ -95,7 +98,7 @@ class Test(TestCase): print('exception logged') raise exc - ''') + ''', *expected) def test_delExceptionInExcept(self): """The exception name can be deleted in the except: block.""" @@ -111,6 +114,8 @@ class Test(TestCase): Last line will never raise UnboundLocalError because `error` is only falsy if the `except:` block has not been entered.""" + # The exc variable is unused inside the exception handler. + expected = [] if version_info < (3,) else [m.UnusedVariable] self.flakes(''' exc = 'Original value' error = None @@ -122,7 +127,7 @@ class Test(TestCase): print(error) else: exc - ''') + ''', *expected) @skip('error reporting disabled due to false positives below') def test_undefinedExceptionNameObscuringGlobalVariable(self): @@ -168,6 +173,8 @@ class Test(TestCase): Last line will never raise NameError because it's only entered if no exception was raised.""" + # The exc variable is unused inside the exception handler. + expected = [] if version_info < (3,) else [m.UnusedVariable] self.flakes(''' exc = 'Original value' def func(): @@ -178,13 +185,15 @@ class Test(TestCase): print('exception logged') raise exc - ''') + ''', *expected) def test_undefinedExceptionNameObscuringGlobalVariableFalsePositive2(self): """Exception names obscure globals, can't be used after. Unless. Last line will never raise NameError because `error` is only falsy if the `except:` block has not been entered.""" + # The exc variable is unused inside the exception handler. + expected = [] if version_info < (3,) else [m.UnusedVariable] self.flakes(''' exc = 'Original value' def func(): @@ -198,7 +207,7 @@ class Test(TestCase): print(error) else: exc - ''') + ''', *expected) def test_functionsNeedGlobalScope(self): self.flakes(''' -- cgit v1.2.1