From 65e7c1f90e9136fc61f4af029b065d9f6c5664c3 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Sun, 2 Jan 2022 23:22:42 +0000 Subject: [PATCH] bpo-46219, 46221: simplify except* implementation following exc_info changes. Move helpers to exceptions.c. Do not assume that exception groups are truthy. (GH-30289) --- .github/CODEOWNERS | 8 ++ Doc/library/dis.rst | 4 +- Include/internal/pycore_pyerrors.h | 6 +- Lib/importlib/_bootstrap_external.py | 3 +- Lib/test/test_except_star.py | 28 ++++ .../2022-01-01-14-23-57.bpo-46221.7oGp-I.rst | 1 + Objects/exceptions.c | 120 +++++++++++++++- Python/ceval.c | 133 +----------------- Python/compile.c | 25 ++-- 9 files changed, 179 insertions(+), 149 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-01-01-14-23-57.bpo-46221.7oGp-I.rst diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index ce5121e7ac8..f484664f7b7 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -21,6 +21,14 @@ Python/ceval.c @markshannon Python/compile.c @markshannon Python/ast_opt.c @isidentical +# Exceptions +Lib/traceback.py @iritkatriel +Lib/test/test_except*.py @iritkatriel +Lib/test/test_traceback.py @iritkatriel +Objects/exceptions.c @iritkatriel +Python/traceback.c @iritkatriel +Python/pythonrun.c @iritkatriel + # Hashing **/*hashlib* @python/crypto-team @tiran **/*pyhash* @python/crypto-team @tiran diff --git a/Doc/library/dis.rst b/Doc/library/dis.rst index ffade3c9bfe..14de191265c 100644 --- a/Doc/library/dis.rst +++ b/Doc/library/dis.rst @@ -911,8 +911,8 @@ All of the following opcodes use their arguments. Combines the raised and reraised exceptions list from TOS, into an exception group to propagate from a try-except* block. Uses the original exception group from TOS1 to reconstruct the structure of reraised exceptions. Pops - two items from the stack and pushes 0 (for lasti, which is unused) followed - by the exception to reraise or ``None`` if there isn't one. + two items from the stack and pushes the exception to reraise or ``None`` + if there isn't one. .. versionadded:: 3.11 diff --git a/Include/internal/pycore_pyerrors.h b/Include/internal/pycore_pyerrors.h index b9fc36cf067..f375337a405 100644 --- a/Include/internal/pycore_pyerrors.h +++ b/Include/internal/pycore_pyerrors.h @@ -87,9 +87,9 @@ PyAPI_FUNC(PyObject *) _PyExc_CreateExceptionGroup( const char *msg, PyObject *excs); -PyAPI_FUNC(PyObject *) _PyExc_ExceptionGroupProjection( - PyObject *left, - PyObject *right); +PyAPI_FUNC(PyObject *) _PyExc_PrepReraiseStar( + PyObject *orig, + PyObject *excs); PyAPI_FUNC(int) _PyErr_CheckSignalsTstate(PyThreadState *tstate); diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 5ead6caf9f3..095c1274beb 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -375,6 +375,7 @@ _code_type = type(_write_atomic.__code__) # Python 3.11a4 3467 (Change CALL_xxx opcodes) # Python 3.11a4 3468 (Add SEND opcode) # Python 3.11a4 3469 (bpo-45711: remove type, traceback from exc_info) +# Python 3.11a4 3470 (bpo-46221: PREP_RERAISE_STAR no longer pushes lasti) # # MAGIC must change whenever the bytecode emitted by the compiler may no @@ -384,7 +385,7 @@ _code_type = type(_write_atomic.__code__) # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array # in PC/launcher.c must also be updated. -MAGIC_NUMBER = (3469).to_bytes(2, 'little') + b'\r\n' +MAGIC_NUMBER = (3470).to_bytes(2, 'little') + b'\r\n' _RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c _PYCACHE = '__pycache__' diff --git a/Lib/test/test_except_star.py b/Lib/test/test_except_star.py index b03de9c1de3..490b159e3b7 100644 --- a/Lib/test/test_except_star.py +++ b/Lib/test/test_except_star.py @@ -952,6 +952,34 @@ class TestExceptStarExceptionGroupSubclass(ExceptStarTest): self.assertEqual(teg.code, 42) self.assertEqual(teg.exceptions[0].code, 101) + def test_falsy_exception_group_subclass(self): + class FalsyEG(ExceptionGroup): + def __bool__(self): + return False + + def derive(self, excs): + return FalsyEG(self.message, excs) + + try: + try: + raise FalsyEG("eg", [TypeError(1), ValueError(2)]) + except *TypeError as e: + tes = e + raise + except *ValueError as e: + ves = e + pass + except Exception as e: + exc = e + + for e in [tes, ves, exc]: + self.assertFalse(e) + self.assertIsInstance(e, FalsyEG) + + self.assertExceptionIsLike(exc, FalsyEG("eg", [TypeError(1)])) + self.assertExceptionIsLike(tes, FalsyEG("eg", [TypeError(1)])) + self.assertExceptionIsLike(ves, FalsyEG("eg", [ValueError(2)])) + class TestExceptStarCleanup(ExceptStarTest): def test_exc_info_restored(self): diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-01-01-14-23-57.bpo-46221.7oGp-I.rst b/Misc/NEWS.d/next/Core and Builtins/2022-01-01-14-23-57.bpo-46221.7oGp-I.rst new file mode 100644 index 00000000000..0cb3e90a28d --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-01-01-14-23-57.bpo-46221.7oGp-I.rst @@ -0,0 +1 @@ +:opcode:`PREP_RERAISE_STAR` no longer pushes ``lasti`` to the stack. diff --git a/Objects/exceptions.c b/Objects/exceptions.c index d82340b8e78..403d2d4a3fd 100644 --- a/Objects/exceptions.c +++ b/Objects/exceptions.c @@ -1207,8 +1207,8 @@ collect_exception_group_leaves(PyObject *exc, PyObject *leaves) * of eg which contains all leaf exceptions that are contained * in any exception group in keep. */ -PyObject * -_PyExc_ExceptionGroupProjection(PyObject *eg, PyObject *keep) +static PyObject * +exception_group_projection(PyObject *eg, PyObject *keep) { assert(_PyBaseExceptionGroup_Check(eg)); assert(PyList_CheckExact(keep)); @@ -1245,6 +1245,122 @@ _PyExc_ExceptionGroupProjection(PyObject *eg, PyObject *keep) return result; } +static bool +is_same_exception_metadata(PyObject *exc1, PyObject *exc2) +{ + assert(PyExceptionInstance_Check(exc1)); + assert(PyExceptionInstance_Check(exc2)); + + PyBaseExceptionObject *e1 = (PyBaseExceptionObject *)exc1; + PyBaseExceptionObject *e2 = (PyBaseExceptionObject *)exc2; + + return (e1->note == e2->note && + e1->traceback == e2->traceback && + e1->cause == e2->cause && + e1->context == e2->context); +} + +/* + This function is used by the interpreter to calculate + the exception group to be raised at the end of a + try-except* construct. + + orig: the original except that was caught. + excs: a list of exceptions that were raised/reraised + in the except* clauses. + + Calculates an exception group to raise. It contains + all exceptions in excs, where those that were reraised + have same nesting structure as in orig, and those that + were raised (if any) are added as siblings in a new EG. + + Returns NULL and sets an exception on failure. +*/ +PyObject * +_PyExc_PrepReraiseStar(PyObject *orig, PyObject *excs) +{ + assert(PyExceptionInstance_Check(orig)); + assert(PyList_Check(excs)); + + Py_ssize_t numexcs = PyList_GET_SIZE(excs); + + if (numexcs == 0) { + return Py_NewRef(Py_None); + } + + if (!_PyBaseExceptionGroup_Check(orig)) { + /* a naked exception was caught and wrapped. Only one except* clause + * could have executed,so there is at most one exception to raise. + */ + + assert(numexcs == 1 || (numexcs == 2 && PyList_GET_ITEM(excs, 1) == Py_None)); + + PyObject *e = PyList_GET_ITEM(excs, 0); + assert(e != NULL); + return Py_NewRef(e); + } + + PyObject *raised_list = PyList_New(0); + if (raised_list == NULL) { + return NULL; + } + PyObject* reraised_list = PyList_New(0); + if (reraised_list == NULL) { + Py_DECREF(raised_list); + return NULL; + } + + /* Now we are holding refs to raised_list and reraised_list */ + + PyObject *result = NULL; + + /* Split excs into raised and reraised by comparing metadata with orig */ + for (Py_ssize_t i = 0; i < numexcs; i++) { + PyObject *e = PyList_GET_ITEM(excs, i); + assert(e != NULL); + if (Py_IsNone(e)) { + continue; + } + bool is_reraise = is_same_exception_metadata(e, orig); + PyObject *append_list = is_reraise ? reraised_list : raised_list; + if (PyList_Append(append_list, e) < 0) { + goto done; + } + } + + PyObject *reraised_eg = exception_group_projection(orig, reraised_list); + if (reraised_eg == NULL) { + goto done; + } + + if (!Py_IsNone(reraised_eg)) { + assert(is_same_exception_metadata(reraised_eg, orig)); + } + Py_ssize_t num_raised = PyList_GET_SIZE(raised_list); + if (num_raised == 0) { + result = reraised_eg; + } + else if (num_raised > 0) { + int res = 0; + if (!Py_IsNone(reraised_eg)) { + res = PyList_Append(raised_list, reraised_eg); + } + Py_DECREF(reraised_eg); + if (res < 0) { + goto done; + } + result = _PyExc_CreateExceptionGroup("", raised_list); + if (result == NULL) { + goto done; + } + } + +done: + Py_XDECREF(raised_list); + Py_XDECREF(reraised_list); + return result; +} + static PyMemberDef BaseExceptionGroup_members[] = { {"message", T_OBJECT, offsetof(PyBaseExceptionGroupObject, msg), READONLY, PyDoc_STR("exception message")}, diff --git a/Python/ceval.c b/Python/ceval.c index 9976bdeffbe..43925e6db26 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1092,7 +1092,6 @@ fail: static int do_raise(PyThreadState *tstate, PyObject *exc, PyObject *cause); -static PyObject *do_reraise_star(PyObject *excs, PyObject *orig); static int exception_group_match( PyObject* exc_value, PyObject *match_type, PyObject **match, PyObject **rest); @@ -2777,7 +2776,7 @@ check_eval_breaker: assert(PyList_Check(excs)); PyObject *orig = POP(); - PyObject *val = do_reraise_star(excs, orig); + PyObject *val = _PyExc_PrepReraiseStar(orig, excs); Py_DECREF(excs); Py_DECREF(orig); @@ -2785,8 +2784,6 @@ check_eval_breaker: goto error; } - PyObject *lasti_unused = Py_NewRef(_PyLong_GetZero()); - PUSH(lasti_unused); PUSH(val); DISPATCH(); } @@ -6313,134 +6310,6 @@ exception_group_match(PyObject* exc_value, PyObject *match_type, return 0; } -/* Logic for the final raise/reraise of a try-except* contruct - (too complicated for inlining). -*/ - -static bool -is_same_exception_metadata(PyObject *exc1, PyObject *exc2) -{ - assert(PyExceptionInstance_Check(exc1)); - assert(PyExceptionInstance_Check(exc2)); - - PyObject *tb1 = PyException_GetTraceback(exc1); - PyObject *ctx1 = PyException_GetContext(exc1); - PyObject *cause1 = PyException_GetCause(exc1); - PyObject *tb2 = PyException_GetTraceback(exc2); - PyObject *ctx2 = PyException_GetContext(exc2); - PyObject *cause2 = PyException_GetCause(exc2); - - bool result = (Py_Is(tb1, tb2) && - Py_Is(ctx1, ctx2) && - Py_Is(cause1, cause2)); - - Py_XDECREF(tb1); - Py_XDECREF(ctx1); - Py_XDECREF(cause1); - Py_XDECREF(tb2); - Py_XDECREF(ctx2); - Py_XDECREF(cause2); - return result; -} - -/* - excs: a list of exceptions to raise/reraise - orig: the original except that was caught - - Calculates an exception group to raise. It contains - all exceptions in excs, where those that were reraised - have same nesting structure as in orig, and those that - were raised (if any) are added as siblings in a new EG. - - Returns NULL and sets an exception on failure. -*/ -static PyObject * -do_reraise_star(PyObject *excs, PyObject *orig) -{ - assert(PyList_Check(excs)); - assert(PyExceptionInstance_Check(orig)); - - Py_ssize_t numexcs = PyList_GET_SIZE(excs); - - if (numexcs == 0) { - return Py_NewRef(Py_None); - } - - if (!_PyBaseExceptionGroup_Check(orig)) { - /* a naked exception was caught and wrapped. Only one except* clause - * could have executed,so there is at most one exception to raise. - */ - - assert(numexcs == 1 || (numexcs == 2 && PyList_GET_ITEM(excs, 1) == Py_None)); - - PyObject *e = PyList_GET_ITEM(excs, 0); - assert(e != NULL); - return Py_NewRef(e); - } - - - PyObject *raised_list = PyList_New(0); - if (raised_list == NULL) { - return NULL; - } - PyObject* reraised_list = PyList_New(0); - if (reraised_list == NULL) { - Py_DECREF(raised_list); - return NULL; - } - - /* Now we are holding refs to raised_list and reraised_list */ - - PyObject *result = NULL; - - /* Split excs into raised and reraised by comparing metadata with orig */ - for (Py_ssize_t i = 0; i < numexcs; i++) { - PyObject *e = PyList_GET_ITEM(excs, i); - assert(e != NULL); - if (Py_IsNone(e)) { - continue; - } - bool is_reraise = is_same_exception_metadata(e, orig); - PyObject *append_list = is_reraise ? reraised_list : raised_list; - if (PyList_Append(append_list, e) < 0) { - goto done; - } - } - - PyObject *reraised_eg = _PyExc_ExceptionGroupProjection(orig, reraised_list); - if (reraised_eg == NULL) { - goto done; - } - - if (!Py_IsNone(reraised_eg)) { - assert(is_same_exception_metadata(reraised_eg, orig)); - } - - Py_ssize_t num_raised = PyList_GET_SIZE(raised_list); - if (num_raised == 0) { - result = reraised_eg; - } - else if (num_raised > 0) { - int res = 0; - if (!Py_IsNone(reraised_eg)) { - res = PyList_Append(raised_list, reraised_eg); - } - Py_DECREF(reraised_eg); - if (res < 0) { - goto done; - } - result = _PyExc_CreateExceptionGroup("", raised_list); - if (result == NULL) { - goto done; - } - } - -done: - Py_XDECREF(raised_list); - Py_XDECREF(reraised_list); - return result; -} - /* Iterate v argcnt times and store the results on the stack (via decreasing sp). Return 1 for success, 0 if error. diff --git a/Python/compile.c b/Python/compile.c index 8e90b1ab370..48250b5dba9 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -1134,7 +1134,7 @@ stack_effect(int opcode, int oparg, int jump) return jump ? 1 : 0; case PREP_RERAISE_STAR: - return 0; + return -1; case RERAISE: return -1; case PUSH_EXC_INFO: @@ -3488,12 +3488,16 @@ compiler_try_except(struct compiler *c, stmt_ty s) [orig, res, rest] Ln+1: LIST_APPEND 1 ) add unhandled exc to res (could be None) [orig, res] PREP_RERAISE_STAR - [i, exc] POP_JUMP_IF_TRUE RER - [i, exc] POP - [i] POP + [exc] DUP_TOP + [exc, exc] LOAD_CONST None + [exc, exc, None] COMPARE_IS + [exc, is_none] POP_JUMP_IF_FALSE RER + [exc] POP_TOP [] JUMP_FORWARD L0 - [i, exc] RER: POP_EXCEPT_AND_RERAISE + [exc] RER: ROT_TWO + [exc, prev_exc_info] POP_EXCEPT + [exc] RERAISE 0 [] L0: */ @@ -3657,18 +3661,21 @@ compiler_try_star_except(struct compiler *c, stmt_ty s) compiler_use_next_block(c, reraise_star); ADDOP(c, PREP_RERAISE_STAR); ADDOP(c, DUP_TOP); - ADDOP_JUMP(c, POP_JUMP_IF_TRUE, reraise); + ADDOP_LOAD_CONST(c, Py_None); + ADDOP_COMPARE(c, Is); + ADDOP_JUMP(c, POP_JUMP_IF_FALSE, reraise); NEXT_BLOCK(c); - /* Nothing to reraise - pop it */ - ADDOP(c, POP_TOP); + /* Nothing to reraise */ ADDOP(c, POP_TOP); ADDOP(c, POP_BLOCK); ADDOP(c, POP_EXCEPT); ADDOP_JUMP(c, JUMP_FORWARD, end); compiler_use_next_block(c, reraise); ADDOP(c, POP_BLOCK); - ADDOP(c, POP_EXCEPT_AND_RERAISE); + ADDOP(c, ROT_TWO); + ADDOP(c, POP_EXCEPT); + ADDOP_I(c, RERAISE, 0); compiler_use_next_block(c, cleanup); ADDOP(c, POP_EXCEPT_AND_RERAISE); compiler_use_next_block(c, orelse);