From 24153ced650eeca564bb5e4bdf3d2cf4e4b9c8f4 Mon Sep 17 00:00:00 2001 From: Michael McCoy Date: Fri, 13 Apr 2018 00:41:37 -0700 Subject: [PATCH] bpo-12029: Exception handling now matches subclasses, not subtypes --- Doc/library/exceptions.rst | 10 ++-- Lib/test/test_exceptions.py | 48 +++++++++++++++++++ Lib/unittest/test/test_assertions.py | 11 +++++ .../2018-04-13-10-38-00.bpo-12029.851NV8.rst | 3 ++ Python/errors.c | 16 ++++++- 5 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-04-13-10-38-00.bpo-12029.851NV8.rst diff --git a/Doc/library/exceptions.rst b/Doc/library/exceptions.rst index 42b99cc47e05f1..ad445893d6138a 100644 --- a/Doc/library/exceptions.rst +++ b/Doc/library/exceptions.rst @@ -8,11 +8,11 @@ Built-in Exceptions statement: except In Python, all exceptions must be instances of a class that derives from -:class:`BaseException`. In a :keyword:`try` statement with an :keyword:`except` -clause that mentions a particular class, that clause also handles any exception -classes derived from that class (but not exception classes from which *it* is -derived). Two exception classes that are not related via subclassing are never -equivalent, even if they have the same name. +:class:`BaseException`. A :keyword:`try` statement with an :keyword:`except` +clause that mentions a class handles exceptions of that class and its +subclasses (as defined by :func:`issubclass`). Two exception classes that +are not related via subclassing are never equivalent, even if they have the +same name. .. index:: statement: raise diff --git a/Lib/test/test_exceptions.py b/Lib/test/test_exceptions.py index 2a9ec706467f3e..4a8371b80f9878 100644 --- a/Lib/test/test_exceptions.py +++ b/Lib/test/test_exceptions.py @@ -7,6 +7,7 @@ import pickle import weakref import errno +import abc from test.support import (TESTFN, captured_stderr, check_impl_detail, check_warnings, cpython_only, gc_collect, run_unittest, @@ -1337,6 +1338,53 @@ def test_copy_pickle(self): self.assertEqual(exc.name, orig.name) self.assertEqual(exc.path, orig.path) + def test_exception_registration(self): + # See issue12029 + class A(Exception, metaclass=abc.ABCMeta): + pass + class B(Exception): + pass + A.register(B) + try: + raise B + except A: + pass + except B: + self.fail("Caught B, should have caught A") + + def test_exception_subclasscheck(self): + # Test that a bad __subclasscheck__ does not cause failure + class RaisingMeta(type): + def __subclasscheck__(self, cls): + raise RuntimeError + class RaisingCatchingMeta(type): + def __subclasscheck__(self, cls): + try: + raise RuntimeError + except RuntimeError: + pass + def recursion(): + return recursion() + class RecursingMeta(type): + def __subclasscheck__(self, cls): + recursion() + reclimit = sys.getrecursionlimit() + try: + sys.setrecursionlimit(1000) + for metaclass in [RaisingMeta, RaisingCatchingMeta, RecursingMeta]: + BadException = metaclass('BadException', (Exception,), {}) + try: + raise ValueError + # this handler should cause an exception that is ignored + except BadException: + self.fail('caught a bad exception but shouldn\'t') + except ValueError: + pass + else: + self.fail('ValueError not raised') + finally: + sys.setrecursionlimit(reclimit) + if __name__ == '__main__': unittest.main() diff --git a/Lib/unittest/test/test_assertions.py b/Lib/unittest/test/test_assertions.py index f5e64d68e7b101..11030978a719c5 100644 --- a/Lib/unittest/test/test_assertions.py +++ b/Lib/unittest/test/test_assertions.py @@ -1,3 +1,4 @@ +import abc import datetime import warnings import weakref @@ -370,6 +371,16 @@ def testAssertRaises(self): '^TypeError not raised$', '^TypeError not raised : oops$']) + def testAssertRaisesWithMetaClass(self): + # See issue33271 + class A(Exception, metaclass=abc.ABCMeta): + pass + class B(Exception): + pass + A.register(B) + with self.assertRaises(A, msg="Exception B should be interpreted as A"): + raise B + def testAssertRaisesRegex(self): # test error not raised self.assertMessagesCM('assertRaisesRegex', (TypeError, 'unused regex'), diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-04-13-10-38-00.bpo-12029.851NV8.rst b/Misc/NEWS.d/next/Core and Builtins/2018-04-13-10-38-00.bpo-12029.851NV8.rst new file mode 100644 index 00000000000000..a3b99acbf77f56 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2018-04-13-10-38-00.bpo-12029.851NV8.rst @@ -0,0 +1,3 @@ +Fix bug where exception matching fails to match virtual subclasses because +``PyErr_GivenExceptionMatches`` used ``PyType_IsSubtype`` instead of +``PyObject_IsSubclass``. diff --git a/Python/errors.c b/Python/errors.c index 15e6ba05714337..01b7fbb38049dc 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -203,8 +203,20 @@ PyErr_GivenExceptionMatches(PyObject *err, PyObject *exc) if (PyExceptionInstance_Check(err)) err = PyExceptionInstance_Class(err); - if (PyExceptionClass_Check(err) && PyExceptionClass_Check(exc)) { - return PyType_IsSubtype((PyTypeObject *)err, (PyTypeObject *)exc); + if (PyExceptionClass_Check(err) && PyExceptionClass_Check(exc)) { + int res = 0; + PyObject *exception, *value, *tb; + PyErr_Fetch(&exception, &value, &tb); + /* PyObject_IsSubclass() can recurse and therefore is + not safe (see test_bad_getattr in test.pickletester). */ + res = PyObject_IsSubclass(err, exc); + /* This function must not fail, so print the error here */ + if (res == -1) { + PyErr_WriteUnraisable(err); + res = 0; + } + PyErr_Restore(exception, value, tb); + return res; } return err == exc;