Add check for unused exception binding in except: block (#293)

Fixes #301

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('''