Add new checks for except (...): clauses (#105)

Author: Zac-HD

README.rst

@@ -106,6 +106,13 @@ To silence an exception, do it explicitly in the `except` block. To properly use
 a `break`, `continue` or `return` refactor your code so these statements are not
 in the `finally` block.
 
+**B013**: A length-one tuple literal is redundant.  Write `except SomeError:`
+instead of `except (SomeError,):`.
+
+**B014**: Redundant exception types in `except (Exception, TypeError):`.
+Write `except Exception:`, which catches exactly the same exceptions.
+
+
 Python 3 compatibility warnings
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -228,6 +235,12 @@ MIT
 Change Log
 ----------
 
+Future
+~~~~~~
+
+* For B001, also check for ``except ():``
+* Introduce B013 and B014 to check tuples in ``except (..., ):`` statements
+
 20.1.0
 ~~~~~~
 

bugbear.py

@@ -1,4 +1,5 @@
 import ast
+import builtins
 from collections import namedtuple
 from contextlib import suppress
 from functools import lru_cache, partial
@@ -136,7 +137,50 @@ def visit(self, node):
 
     def visit_ExceptHandler(self, node):
         if node.type is None:
-            self.errors.append(B001(node.lineno, node.col_offset))
+            self.errors.append(
+                B001(node.lineno, node.col_offset, vars=("bare `except:`",))
+            )
+        elif isinstance(node.type, ast.Tuple):
+            names = []
+            for e in node.type.elts:
+                if isinstance(e, ast.Name):
+                    names.append(e.id)
+                else:
+                    assert isinstance(e, ast.Attribute)
+                    names.append("{}.{}".format(e.value.id, e.attr))
+            as_ = " as " + node.name if node.name is not None else ""
+            if len(names) == 0:
+                vs = ("`except (){}:`".format(as_),)
+                self.errors.append(B001(node.lineno, node.col_offset, vars=vs))
+            elif len(names) == 1:
+                self.errors.append(B013(node.lineno, node.col_offset, vars=names))
+            else:
+                # See if any of the given exception names could be removed, e.g. from:
+                #    (MyError, MyError)  # duplicate names
+                #    (MyError, BaseException)  # everything derives from the Base
+                #    (Exception, TypeError)  # builtins where one subclasses another
+                # but note that other cases are impractical to hande from the AST.
+                # We expect this is mostly useful for users who do not have the
+                # builtin exception hierarchy memorised, and include a 'shadowed'
+                # subtype without realising that it's redundant.
+                good = sorted(set(names), key=names.index)
+                if "BaseException" in good:
+                    good = ["BaseException"]
+                for name, other in itertools.permutations(tuple(good), 2):
+                    if issubclass(
+                        getattr(builtins, name, type), getattr(builtins, other, ())
+                    ):
+                        if name in good:
+                            good.remove(name)
+                if good != names:
+                    desc = good[0] if len(good) == 1 else "({})".format(", ".join(good))
+                    self.errors.append(
+                        B014(
+                            node.lineno,
+                            node.col_offset,
+                            vars=(", ".join(names), as_, desc),
+                        )
+                    )
         self.generic_visit(node)
 
     def visit_UAdd(self, node):
@@ -459,7 +503,7 @@ def visit(self, node):
 
 
 B001 = Error(
-    message="B001 Do not use bare `except:`, it also catches unexpected "
+    message="B001 Do not use {}, it also catches unexpected "
     "events like memory errors, interrupts, system exit, and so on.  "
     "Prefer `except Exception:`.  If you're sure what you're doing, "
     "be explicit and write `except BaseException:`."
@@ -542,6 +586,14 @@ def visit(self, node):
     "to be silenced. Exceptions should be silenced in except blocks. Control "
     "statements can be moved outside the finally block."
 )
+B013 = Error(
+    message="B013 A length-one tuple literal is redundant.  "
+    "Write `except {0}:` instead of `except ({0},):`."
+)
+B014 = Error(
+    message="B014 Redundant exception types in `except ({0}){1}:`.  "
+    "Write `except {2}{1}:`, which catches exactly the same exceptions."
+)
 
 # Those could be false positives but it's more dangerous to let them slip
 # through if they're not.

tests/b001.py

@@ -1,6 +1,6 @@
 """
 Should emit:
-B001 - on lines 8 and 40
+B001 - on lines 8, 40, and 54
 """
 
 try:
@@ -47,3 +47,10 @@ def func(**kwargs):
     except:  # noqa
         # warning silenced
         return
+
+
+try:
+    pass
+except ():
+    # Literal empty tuple is just like bare except:
+    pass

tests/b013.py

@@ -0,0 +1,30 @@
+"""
+Should emit:
+B013 - on lines 10 and 28
+"""
+
+import re
+
+try:
+    pass
+except (ValueError,):
+    # pointless use of tuple
+    pass
+
+try:
+    pass
+except (ValueError):
+    # not using a tuple means it's OK (if odd)
+    pass
+
+try:
+    pass
+except ValueError:
+    # no warning here, all good
+    pass
+
+try:
+    pass
+except (re.error,):
+    # pointless use of tuple with dotted attribute
+    pass

tests/b014.py

@@ -0,0 +1,50 @@
+"""
+Should emit:
+B014 - on lines 10, 16, 27, 41, and 48
+"""
+
+import re
+
+try:
+    pass
+except (Exception, TypeError):
+    # TypeError is a subclass of Exception, so it doesn't add anything
+    pass
+
+try:
+    pass
+except (OSError, OSError) as err:
+    # Duplicate exception types are useless
+    pass
+
+
+class MyError(Exception):
+    pass
+
+
+try:
+    pass
+except (MyError, MyError):
+    # Detect duplicate non-builtin errors
+    pass
+
+
+try:
+    pass
+except (MyError, Exception) as e:
+    # Don't assume that we're all subclasses of Exception
+    pass
+
+
+try:
+    pass
+except (MyError, BaseException) as e:
+    # But we *can* assume that everything is a subclass of BaseException
+    pass
+
+
+try:
+    pass
+except (re.error, re.error):
+    # Duplicate exception types as attributes
+    pass

tests/test_bugbear.py

@@ -16,6 +16,8 @@
     B010,
     B011,
     B012,
+    B013,
+    B014,
     B301,
     B302,
     B303,
@@ -39,7 +41,12 @@ def test_b001(self):
         filename = Path(__file__).absolute().parent / "b001.py"
         bbc = BugBearChecker(filename=str(filename))
         errors = list(bbc.run())
-        self.assertEqual(errors, self.errors(B001(8, 0), B001(40, 4)))
+        expected = self.errors(
+            B001(8, 0, vars=("bare `except:`",)),
+            B001(40, 4, vars=("bare `except:`",)),
+            B001(54, 0, vars=("`except ():`",)),
+        )
+        self.assertEqual(errors, expected)
 
     def test_b002(self):
         filename = Path(__file__).absolute().parent / "b002.py"
@@ -138,6 +145,28 @@ def test_b012(self):
         ]
         self.assertEqual(errors, self.errors(*all_errors))
 
+    def test_b013(self):
+        filename = Path(__file__).absolute().parent / "b013.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B013(10, 0, vars=("ValueError",)), B013(28, 0, vars=("re.error",))
+        )
+        self.assertEqual(errors, expected)
+
+    def test_b014(self):
+        filename = Path(__file__).absolute().parent / "b014.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B014(10, 0, vars=("Exception, TypeError", "", "Exception")),
+            B014(16, 0, vars=("OSError, OSError", " as err", "OSError")),
+            B014(27, 0, vars=("MyError, MyError", "", "MyError")),
+            B014(41, 0, vars=("MyError, BaseException", " as e", "BaseException")),
+            B014(48, 0, vars=("re.error, re.error", "", "re.error")),
+        )
+        self.assertEqual(errors, expected)
+
     def test_b301_b302_b305(self):
         filename = Path(__file__).absolute().parent / "b301_b302_b305.py"
         bbc = BugBearChecker(filename=str(filename))