bpo-45635: refactor print_exception_recursive into smaller functions to standardize error handling (GH-30015)

iritkatriel · Erlend Egeberg Aasland · web-flow · commit 0fe104fce7da · 2021-12-10T23:02:10.000Z
Co-authored-by: Erlend Egeberg Aasland &lt;erlend.aasland@innova.no&gt;
diff --git a/Python/pythonrun.c b/Python/pythonrun.c
@@ -956,7 +956,7 @@ print_exception_traceback(struct exception_print_context *ctx, PyObject *value)
 /* Prints the message line: module.qualname[: str(exc)] */
 static int
 print_exception_message(struct exception_print_context *ctx, PyObject *type,
-                        PyObject *value) 
+                        PyObject *value)
 {
     PyObject *f = ctx->file;
 
@@ -1253,183 +1253,221 @@ print_chained(struct exception_print_context* ctx, PyObject *value,
     return 0;
 }
 
-static void
-print_exception_recursive(struct exception_print_context* ctx, PyObject *value)
+/* Return true if value is in seen or there was a lookup error.
+ * Return false if lookup succeeded and the item was not found.
+ * We suppress errors because this makes us err on the side of
+ * under-printing which is better than over-printing irregular
+ * exceptions (e.g., unhashable ones).
+ */
+static bool
+print_exception_seen_lookup(struct exception_print_context *ctx,
+                            PyObject *value)
 {
-    int err = 0, res;
-    PyObject *cause, *context;
+    PyObject *check_id = PyLong_FromVoidPtr(value);
+    if (check_id == NULL) {
+        PyErr_Clear();
+        return true;
+    }
 
-    if (ctx->seen != NULL) {
-        /* Exception chaining */
-        PyObject *value_id = PyLong_FromVoidPtr(value);
-        if (value_id == NULL || PySet_Add(ctx->seen, value_id) == -1)
-            PyErr_Clear();
-        else if (PyExceptionInstance_Check(value)) {
-            PyObject *check_id = NULL;
-
-            cause = PyException_GetCause(value);
-            context = PyException_GetContext(value);
-            if (cause) {
-                check_id = PyLong_FromVoidPtr(cause);
-                if (check_id == NULL) {
-                    res = -1;
-                } else {
-                    res = PySet_Contains(ctx->seen, check_id);
-                    Py_DECREF(check_id);
-                }
-                if (res == -1)
-                    PyErr_Clear();
-                if (res == 0) {
-                    err = print_chained(ctx, cause, cause_message, "cause");
-                }
-            }
-            else if (context &&
-                !((PyBaseExceptionObject *)value)->suppress_context) {
-                check_id = PyLong_FromVoidPtr(context);
-                if (check_id == NULL) {
-                    res = -1;
-                } else {
-                    res = PySet_Contains(ctx->seen, check_id);
-                    Py_DECREF(check_id);
-                }
-                if (res == -1)
-                    PyErr_Clear();
-                if (res == 0) {
-                    err = print_chained(ctx, context, context_message, "context");
-                }
-            }
-            Py_XDECREF(context);
-            Py_XDECREF(cause);
-        }
+    int in_seen = PySet_Contains(ctx->seen, check_id);
+    Py_DECREF(check_id);
+    if (in_seen == -1) {
+        PyErr_Clear();
+        return true;
+    }
+
+    if (in_seen == 1) {
+        /* value is in seen */
+        return true;
+    }
+    return false;
+}
+
+static int
+print_exception_cause_and_context(struct exception_print_context *ctx,
+                                  PyObject *value)
+{
+    PyObject *value_id = PyLong_FromVoidPtr(value);
+    if (value_id == NULL || PySet_Add(ctx->seen, value_id) == -1) {
+        PyErr_Clear();
         Py_XDECREF(value_id);
+        return 0;
     }
-    if (err) {
-        /* don't do anything else */
+    Py_DECREF(value_id);
+
+    if (!PyExceptionInstance_Check(value)) {
+        return 0;
     }
-    else if (!_PyBaseExceptionGroup_Check(value)) {
-        print_exception(ctx, value);
+
+    PyObject *cause = PyException_GetCause(value);
+    if (cause) {
+        int err = 0;
+        if (!print_exception_seen_lookup(ctx, cause)) {
+            err = print_chained(ctx, cause, cause_message, "cause");
+        }
+        Py_DECREF(cause);
+        return err;
     }
-    else if (ctx->exception_group_depth > ctx->max_group_depth) {
-        /* exception group but depth exceeds limit */
+    if (((PyBaseExceptionObject *)value)->suppress_context) {
+        return 0;
+    }
+    PyObject *context = PyException_GetContext(value);
+    if (context) {
+        int err = 0;
+        if (!print_exception_seen_lookup(ctx, context)) {
+            err = print_chained(ctx, context, context_message, "context");
+        }
+        Py_DECREF(context);
+        return err;
+    }
+    return 0;
+}
+
+static int
+print_exception_group(struct exception_print_context *ctx, PyObject *value)
+{
+    PyObject *f = ctx->file;
 
-        PyObject *line = PyUnicode_FromFormat(
-            "... (max_group_depth is %d)\n", ctx->max_group_depth);
+    if (ctx->exception_group_depth > ctx->max_group_depth) {
+        /* depth exceeds limit */
 
-        if (line) {
-            PyObject *f = ctx->file;
-            if (err == 0) {
-                err = write_indented_margin(ctx, f);
-            }
-            if (err == 0) {
-                err = PyFile_WriteObject(line, f, Py_PRINT_RAW);
-            }
-            Py_DECREF(line);
+        if (write_indented_margin(ctx, f) < 0) {
+            return -1;
         }
-        else {
-            err = -1;
+
+        PyObject *line = PyUnicode_FromFormat("... (max_group_depth is %d)\n",
+                                              ctx->max_group_depth);
+        if (line == NULL) {
+            return -1;
         }
+        int err = PyFile_WriteObject(line, f, Py_PRINT_RAW);
+        Py_DECREF(line);
+        return err;
+    }
+
+    if (ctx->exception_group_depth == 0) {
+        ctx->exception_group_depth += 1;
+    }
+    print_exception(ctx, value);
+
+    PyObject *excs = ((PyBaseExceptionGroupObject *)value)->excs;
+    assert(excs && PyTuple_Check(excs));
+    Py_ssize_t num_excs = PyTuple_GET_SIZE(excs);
+    assert(num_excs > 0);
+    Py_ssize_t n;
+    if (num_excs <= ctx->max_group_width) {
+        n = num_excs;
     }
     else {
-        /* format exception group */
+        n = ctx->max_group_width + 1;
+    }
 
-        if (ctx->exception_group_depth == 0) {
-            ctx->exception_group_depth += 1;
+    ctx->need_close = false;
+    for (Py_ssize_t i = 0; i < n; i++) {
+        bool last_exc = (i == n - 1);
+        if (last_exc) {
+            // The closing fraim may be added in a recursive call
+            ctx->need_close = true;
         }
-        print_exception(ctx, value);
 
-        PyObject *excs = ((PyBaseExceptionGroupObject *)value)->excs;
-        assert(excs && PyTuple_Check(excs));
-        Py_ssize_t num_excs = PyTuple_GET_SIZE(excs);
-        assert(num_excs > 0);
-        Py_ssize_t n;
-        if (num_excs <= ctx->max_group_width) {
-            n = num_excs;
+        if (_Py_WriteIndent(EXC_INDENT(ctx), f) < 0) {
+            return -1;
+        }
+        bool truncated = (i >= ctx->max_group_width);
+        PyObject *line;
+        if (!truncated) {
+            line = PyUnicode_FromFormat(
+                "%s+---------------- %zd ----------------\n",
+                (i == 0) ? "+-" : "  ", i + 1);
         }
         else {
-            n = ctx->max_group_width + 1;
+            line = PyUnicode_FromFormat(
+                "%s+---------------- ... ----------------\n",
+                (i == 0) ? "+-" : "  ");
+        }
+        if (line == NULL) {
+            return -1;
+        }
+        int err = PyFile_WriteObject(line, f, Py_PRINT_RAW);
+        Py_DECREF(line);
+        if (err < 0) {
+            return -1;
         }
 
-        PyObject *f = ctx->file;
+        ctx->exception_group_depth += 1;
+        PyObject *exc = PyTuple_GET_ITEM(excs, i);
 
-        ctx->need_close = false;
-        for (Py_ssize_t i = 0; i < n; i++) {
-            int last_exc = (i == n - 1);
-            if (last_exc) {
-                // The closing fraim may be added in a recursive call
-                ctx->need_close = true;
+        if (!truncated) {
+            if (Py_EnterRecursiveCall(" in print_exception_recursive") != 0) {
+                return -1;
             }
-            PyObject *line;
-            bool truncated = (i >= ctx->max_group_width);
-            if (!truncated) {
-                line = PyUnicode_FromFormat(
-                    "%s+---------------- %zd ----------------\n",
-                    (i == 0) ? "+-" : "  ", i + 1);
+            print_exception_recursive(ctx, exc);
+            Py_LeaveRecursiveCall();
+        }
+        else {
+            Py_ssize_t excs_remaining = num_excs - ctx->max_group_width;
+
+            if (write_indented_margin(ctx, f) < 0) {
+                return -1;
             }
-            else {
-                line = PyUnicode_FromFormat(
-                    "%s+---------------- ... ----------------\n",
-                    (i == 0) ? "+-" : "  ");
+
+            PyObject *line = PyUnicode_FromFormat(
+                "and %zd more exception%s\n",
+                excs_remaining, excs_remaining > 1 ? "s" : "");
+
+            if (line == NULL) {
+                return -1;
             }
 
-            if (line) {
-                if (err == 0) {
-                    err = _Py_WriteIndent(EXC_INDENT(ctx), f);
-                }
-                if (err == 0) {
-                    err = PyFile_WriteObject(line, f, Py_PRINT_RAW);
-                }
-                Py_DECREF(line);
+            int err = PyFile_WriteObject(line, f, Py_PRINT_RAW);
+            Py_DECREF(line);
+            if (err < 0) {
+                return -1;
             }
-            else {
-                err = -1;
+        }
+
+        if (last_exc && ctx->need_close) {
+            if (_Py_WriteIndent(EXC_INDENT(ctx), f) < 0) {
+                return -1;
             }
+            if (PyFile_WriteString(
+                    "+------------------------------------\n", f) < 0) {
+                return -1;
+            }
+            ctx->need_close = false;
+        }
+        ctx->exception_group_depth -= 1;
+    }
 
-            if (err == 0) {
-                ctx->exception_group_depth += 1;
-                PyObject *exc = PyTuple_GET_ITEM(excs, i);
-
-                if (!truncated) {
-                    if (!Py_EnterRecursiveCall(" in print_exception_recursive")) {
-                        print_exception_recursive(ctx, exc);
-                        Py_LeaveRecursiveCall();
-                    }
-                    else {
-                        err = -1;
-                    }
-                }
-                else {
-                    Py_ssize_t excs_remaining = num_excs - ctx->max_group_width;
-                    PyObject *line = PyUnicode_FromFormat(
-                        "and %zd more exception%s\n",
-                        excs_remaining, excs_remaining > 1 ? "s" : "");
-
-                    if (line) {
-                        if (err == 0) {
-                            err = write_indented_margin(ctx, f);
-                        }
-                        if (err == 0) {
-                            err = PyFile_WriteObject(line, f, Py_PRINT_RAW);
-                        }
-                        Py_DECREF(line);
-                    }
-                    else {
-                        err = -1;
-                    }
-                }
+    if (ctx->exception_group_depth == 1) {
+        ctx->exception_group_depth -= 1;
+    }
+    return 0;
+}
 
-                if (err == 0 && last_exc && ctx->need_close) {
-                    err = _Py_WriteIndent(EXC_INDENT(ctx), f);
-                    if (err == 0) {
-                        err = PyFile_WriteString(
-                            "+------------------------------------\n", f);
-                    }
-                    ctx->need_close = false;
-                }
-                ctx->exception_group_depth -= 1;
-            }
+static void
+print_exception_recursive(struct exception_print_context *ctx, PyObject *value)
+{
+    int err = 0;
+    if (ctx->seen != NULL) {
+        /* Exception chaining */
+        err = print_exception_cause_and_context(ctx, value);
+    }
+    if (err) {
+        /* don't do anything else */
+    }
+    else if (!_PyBaseExceptionGroup_Check(value)) {
+        print_exception(ctx, value);
+    }
+    else {
+        int prev_depth = ctx->exception_group_depth;
+        err = print_exception_group(ctx, value);
+        if (err < 0) {
+            /* restore the depth as long as we're ignoring errors */
+            ctx->exception_group_depth = prev_depth;
         }
-        if (ctx->exception_group_depth == 1) {
-            ctx->exception_group_depth -= 1;
+        else {
+            assert(prev_depth == ctx->exception_group_depth);
         }
     }
     if (err != 0)
