Skip to content

bpo-45635: refactor print_exception_recursive into smaller functions to standardize error handling #30015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 10, 2021
Merged

bpo-45635: refactor print_exception_recursive into smaller functions to standardize error handling #30015

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fec85a6
bpo-45635: refactor print_exception_recursive into smaller functions …
iritkatriel Dec 9, 2021
5d68eb5
extract print_exception_seen_lookup
iritkatriel Dec 10, 2021
e0d0f8a
Apply suggestions from code review
iritkatriel Dec 10, 2021
a386833
a couple more review comments
iritkatriel Dec 10, 2021
5dc1936
newline
iritkatriel Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
336 changes: 187 additions & 149 deletions Python/pythonrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 frame 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 frame 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 */
}
Comment on lines +1456 to +1458
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be clearer to just return if we don't need to do anything else? Actually, seeing that err is only set twice, I wonder if it could be worth it to just inline the error handling (PyErr_Clear()) and remove err 🤔 Nitpicking 😎

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will happen, once this function stops swallowing errors (that's a behaviour change rather than a refactor). Right now we have the PyErr_Clear() at the end.

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)
Expand Down