bpo-12029: Exception handling now matches subclasses, not subtypes

mbmccoy · mbmccoy · commit 24153ced650e · 2018-04-16T21:21:26.000-07:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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;

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+Fix bug where exception matching fails to match virtual subclasses because`
	`2`	+``PyErr_GivenExceptionMatches`` used ``PyType_IsSubtype`` instead of
	`3`	+``PyObject_IsSubclass``.