diff --git a/README.rst b/README.rst index e2e821c..8fe5020 100644 --- a/README.rst +++ b/README.rst @@ -179,6 +179,8 @@ It is therefore recommended to use a stacklevel of 2 or greater to provide more **B029**: Using ``except: ()`` with an empty tuple does not handle/catch anything. Add exceptions to handle. +**B030**: Except handlers should only be exception classes or tuples of exception classes. + Opinionated warnings ~~~~~~~~~~~~~~~~~~~~ diff --git a/bugbear.py b/bugbear.py index 12d4718..7f9f80b 100644 --- a/bugbear.py +++ b/bugbear.py @@ -189,6 +189,51 @@ def _is_identifier(arg): return re.match(r"^[A-Za-z_][A-Za-z0-9_]*$", arg.s) is not None +def _flatten_excepthandler(node): + if isinstance(node, ast.Tuple): + for elt in node.elts: + yield from _flatten_excepthandler(elt) + else: + yield node + + +def _check_redundant_excepthandlers(names, node): + # 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 + # (IOError, OSError) # IOError is an alias of OSError since Python3.3 + # but note that other cases are impractical to handle 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"] + # Remove redundant exceptions that the automatic system either handles + # poorly (usually aliases) or can't be checked (e.g. it's not an + # built-in exception). + for primary, equivalents in B014.redundant_exceptions.items(): + if primary in good: + good = [g for g in good if g not in equivalents] + + for name, other in itertools.permutations(tuple(good), 2): + if _typesafe_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)) + as_ = " as " + node.name if node.name is not None else "" + return B014( + node.lineno, + node.col_offset, + vars=(", ".join(names), as_, desc), + ) + return None + + def _to_name_str(node): # Turn Name and Attribute nodes to strings, e.g "ValueError" or # "pkg.mod.error", handling any depth of attribute accesses. @@ -196,6 +241,7 @@ def _to_name_str(node): return node.id if isinstance(node, ast.Call): return _to_name_str(node.func) + assert isinstance(node, ast.Attribute), f"Unexpected node type: {type(node)}" try: return _to_name_str(node.value) + "." + node.attr except AttributeError: @@ -277,48 +323,27 @@ def visit(self, node): def visit_ExceptHandler(self, node): if node.type is None: self.errors.append(B001(node.lineno, node.col_offset)) - elif isinstance(node.type, ast.Tuple): - names = [_to_name_str(e) for e in node.type.elts] - as_ = " as " + node.name if node.name is not None else "" - if len(names) == 0: - self.errors.append(B029(node.lineno, node.col_offset)) - elif len(names) == 1: - self.errors.append(B013(node.lineno, node.col_offset, vars=names)) + self.generic_visit(node) + return + handlers = _flatten_excepthandler(node.type) + good_handlers = [] + bad_handlers = [] + for handler in handlers: + if isinstance(handler, (ast.Name, ast.Attribute)): + good_handlers.append(handler) 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 - # (IOError, OSError) # IOError is an alias of OSError since Python3.3 - # but note that other cases are impractical to handle 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"] - # Remove redundant exceptions that the automatic system either handles - # poorly (usually aliases) or can't be checked (e.g. it's not an - # built-in exception). - for primary, equivalents in B014.redundant_exceptions.items(): - if primary in good: - good = [g for g in good if g not in equivalents] - - for name, other in itertools.permutations(tuple(good), 2): - if _typesafe_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), - ) - ) + bad_handlers.append(handler) + if bad_handlers: + self.errors.append(B030(node.lineno, node.col_offset)) + names = [_to_name_str(e) for e in good_handlers] + if len(names) == 0 and not bad_handlers: + self.errors.append(B029(node.lineno, node.col_offset)) + elif len(names) == 1 and not bad_handlers and isinstance(node.type, ast.Tuple): + self.errors.append(B013(node.lineno, node.col_offset, vars=names)) + else: + maybe_error = _check_redundant_excepthandlers(names, node) + if maybe_error is not None: + self.errors.append(maybe_error) self.generic_visit(node) def visit_UAdd(self, node): @@ -1533,6 +1558,7 @@ def visit_Lambda(self, node): "anything. Add exceptions to handle." ) ) +B030 = Error(message="B030 Except handlers should only be names of exception classes") # Warnings disabled by default. B901 = Error( diff --git a/tests/b030.py b/tests/b030.py new file mode 100644 index 0000000..4b66f04 --- /dev/null +++ b/tests/b030.py @@ -0,0 +1,14 @@ +try: + pass +except (ValueError, (RuntimeError, (KeyError, TypeError))): # ok + pass + +try: + pass +except 1: # error + pass + +try: + pass +except (1, ValueError): # error + pass diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index bbd1c2b..4751e73 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -41,6 +41,7 @@ B027, B028, B029, + B030, B901, B902, B903, @@ -448,6 +449,16 @@ def test_b029(self): ) self.assertEqual(errors, expected) + def test_b030(self): + filename = Path(__file__).absolute().parent / "b030.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors( + B030(8, 0), + B030(13, 0), + ) + self.assertEqual(errors, expected) + @unittest.skipIf(sys.version_info < (3, 8), "not implemented for <3.8") def test_b907(self): filename = Path(__file__).absolute().parent / "b907.py"