Emit 'catching-non-exception' for non-class nodes. Closes issue #303.

4 files changed, 80 insertions, 6 deletions

diff --git a/ChangeLog b/ChangeLog
index dc85ee4..cd25ca8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -71,6 +71,8 @@ ChangeLog for Pylint
     * Check that a class has an explicitly defined metaclass before
       emitting 'old-style-class' for Python 2.
 
+    * Emit 'catching-non-exception' for non-class nodes. Closes issue #303.
+
 
 2014-07-26  --  1.3.0
 
diff --git a/checkers/exceptions.py b/checkers/exceptions.py
index 81520ce..ecbf4a4 100644
--- a/checkers/exceptions.py
+++ b/checkers/exceptions.py
@@ -19,7 +19,7 @@ import sys
 from logilab.common.compat import builtins
 BUILTINS_NAME = builtins.__name__
 import astroid
-from astroid import YES, Instance, unpack_infer
+from astroid import YES, Instance, unpack_infer, List, Tuple
 
 from pylint.checkers import BaseChecker
 from pylint.checkers.utils import (
@@ -28,6 +28,36 @@ from pylint.checkers.utils import (
     EXCEPTIONS_MODULE)
 from pylint.interfaces import IAstroidChecker
 
+def _annotated_unpack_infer(stmt, context=None):
+    """
+    Recursively generate nodes inferred by the given statement.
+    If the inferred value is a list or a tuple, recurse on the elements.
+    Returns an iterator which yields tuples in the format
+    ('original node', 'infered node').
+    """
+    # TODO: the same code as unpack_infer, except for the annotated
+    # return. We need this type of annotation only here and
+    # there is no point in complicating the API for unpack_infer.
+    # If the need arises, this behaviour can be promoted to unpack_infer
+    # as well.
+    if isinstance(stmt, (List, Tuple)):
+        for elt in stmt.elts:
+            for infered_elt in unpack_infer(elt, context):
+                yield elt, infered_elt
+        return
+    # if infered is a final node, return it and stop
+    infered = next(stmt.infer(context))
+    if infered is stmt:
+        yield stmt, infered
+        return
+    # else, infer recursivly, except YES object that should be returned as is
+    for infered in stmt.infer(context):
+        if infered is YES:
+            yield stmt, infered
+        else:
+            for inf_inf in unpack_infer(infered, context):
+                yield stmt, inf_inf
+
 def infer_bases(klass):
     """ Fully infer the bases of the klass node.
 
@@ -255,13 +285,34 @@ class ExceptionsChecker(BaseChecker):
                                  node=handler, args=handler.type.op)
             else:
                 try:
-                    excs = list(unpack_infer(handler.type))
+                    excs = list(_annotated_unpack_infer(handler.type))
                 except astroid.InferenceError:
                     continue
-                for exc in excs:
-                    # XXX skip other non class nodes
-                    if exc is YES or not isinstance(exc, astroid.Class):
+                for part, exc in excs:
+                    if exc is YES:
                         continue
+                    if not isinstance(exc, astroid.Class):
+                        # Don't emit the warning if the infered stmt
+                        # is None, but the exception handler is something else,
+                        # maybe it was redefined.
+                        if (isinstance(exc, astroid.Const) and
+                                exc.value is None):
+                            if ((isinstance(handler.type, astroid.Const) and
+                                     handler.type.value is None) or
+                                     handler.type.parent_of(exc)):
+                                # If the exception handler catches None or
+                                # the exception component, which is None, is
+                                # defined by the entire exception handler, then
+                                # emit a warning.
+                                self.add_message('catching-non-exception',
+                                                 node=handler.type,
+                                                 args=(part.as_string(), ))
+                        else:
+                            self.add_message('catching-non-exception',
+                                             node=handler.type,
+                                             args=(part.as_string(), ))
+                        continue
+
                     exc_ancestors = [anc for anc in exc.ancestors()
                                      if isinstance(anc, astroid.Class)]
                     for previous_exc in exceptions_classes:
@@ -289,7 +340,7 @@ class ExceptionsChecker(BaseChecker):
                                              node=handler.type,
                                              args=(exc.name, ))
 
-                exceptions_classes += excs
+                exceptions_classes += [exc for _, exc in excs]
 
 
 def register(linter):
diff --git a/test/functional/invalid_exceptions_caught.py b/test/functional/invalid_exceptions_caught.py
index 99872d2..0bd0f7b 100644
--- a/test/functional/invalid_exceptions_caught.py
+++ b/test/functional/invalid_exceptions_caught.py
@@ -45,3 +45,21 @@ try:
     1 + 3
 except (SkipException, SecondSkipException):
     print "caught"
+
+try:
+    1 + 42
+# +1:[catching-non-exception,catching-non-exception]
+except (None, list()):
+    print "caught"
+
+try:
+    1 + 24
+except None: # [catching-non-exception]
+    print "caught"
+
+EXCEPTION = None
+EXCEPTION = ZeroDivisionError
+try:
+    1 + 46
+except EXCEPTION:
+    print "caught"
diff --git a/test/functional/invalid_exceptions_caught.txt b/test/functional/invalid_exceptions_caught.txt
index 530ef9a..55b653b 100644
--- a/test/functional/invalid_exceptions_caught.txt
+++ b/test/functional/invalid_exceptions_caught.txt
@@ -1,3 +1,6 @@
 catching-non-exception:25::Catching an exception which doesn't inherit from BaseException: MyException
 catching-non-exception:31::Catching an exception which doesn't inherit from BaseException: MyException
 catching-non-exception:31::Catching an exception which doesn't inherit from BaseException: MySecondException
+catching-non-exception:52::Catching an exception which doesn't inherit from BaseException: None
+catching-non-exception:52::Catching an exception which doesn't inherit from BaseException: list()
+catching-non-exception:57::Catching an exception which doesn't inherit from BaseException: None