Skip to content

bpo-45083: Include the exception class qualname when formatting an ex… #28119

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
ambv merged 6 commits into from
Sep 3, 2021
Merged

bpo-45083: Include the exception class qualname when formatting an ex… #28119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c2ce88e
bpo-45083: Include the exception class qualname when formatting an ex…
iritkatriel Sep 1, 2021
39f24c6
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 1, 2021
b8c09ce
== NULL
iritkatriel Sep 2, 2021
0da8f5b
newline before else
iritkatriel Sep 2, 2021
9781ce5
move * on pointer
iritkatriel Sep 2, 2021
f863ff0
simplify code and fix qualname refleak
iritkatriel Sep 2, 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
14 changes: 14 additions & 0 deletions Lib/test/test_sys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,20 @@ def __del__(self):
self.assertIn("del is broken", report)
self.assertTrue(report.endswith("\n"))

def test_original_unraisablehook_exception_qualname(self):
class A:
class B:
class X(Exception):
pass

with test.support.captured_stderr() as stderr, \
test.support.swap_attr(sys, 'unraisablehook',
sys.__unraisablehook__):
expected = self.write_unraisable_exc(
A.B.X(), "msg", "obj");
report = stderr.getvalue()
testName = 'test_original_unraisablehook_exception_qualname'
self.assertIn(f"{testName}.<locals>.A.B.X", report)

def test_original_unraisablehook_wrong_type(self):
exc = ValueError(42)
Expand Down
13 changes: 13 additions & 0 deletions Lib/test/test_traceback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,19 @@ def test_syntax_error_various_offsets(self):
exp = "\n".join(expected)
self.assertEqual(exp, err)

def test_format_exception_only_qualname(self):
class A:
class B:
class X(Exception):
def __str__(self):
return "I am X"
pass
err = self.get_report(A.B.X())
str_value = 'I am X'
str_name = '.'.join([A.B.X.__module__, A.B.X.__qualname__])
exp = "%s: %s\n" % (str_name, str_value)
self.assertEqual(exp, err)


class PyExcReportingTests(BaseExceptionReportingTests, unittest.TestCase):
#
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2021-09-01-23-55-49.bpo-45083.cLi9G3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
When the interpreter renders an exception, its name now has a complete qualname. Previously only the class name was concatenated to the module name, which sometimes resulted in an incorrect full name being displayed.

(This issue impacted only the C code exception rendering, the :mod:`traceback` module was using qualname already).
33 changes: 16 additions & 17 deletions Python/errors.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,46 +1287,45 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
}

assert(PyExceptionClass_Check(exc_type));
const char *className = PyExceptionClass_Name(exc_type);
if (className != NULL) {
const char *dot = strrchr(className, '.');
if (dot != NULL) {
className = dot+1;
}
}

PyObject *moduleName = _PyObject_GetAttrId(exc_type, &PyId___module__);
if (moduleName == NULL || !PyUnicode_Check(moduleName)) {
Py_XDECREF(moduleName);
PyObject *modulename = _PyObject_GetAttrId(exc_type, &PyId___module__);
if (modulename == NULL || !PyUnicode_Check(modulename)) {
Py_XDECREF(modulename);
_PyErr_Clear(tstate);
if (PyFile_WriteString("<unknown>", file) < 0) {
return -1;
}
}
else {
if (!_PyUnicode_EqualToASCIIId(moduleName, &PyId_builtins)) {
if (PyFile_WriteObject(moduleName, file, Py_PRINT_RAW) < 0) {
Py_DECREF(moduleName);
if (!_PyUnicode_EqualToASCIIId(modulename, &PyId_builtins)) {
if (PyFile_WriteObject(modulename, file, Py_PRINT_RAW) < 0) {
Py_DECREF(modulename);
return -1;
}
Py_DECREF(moduleName);
Py_DECREF(modulename);
if (PyFile_WriteString(".", file) < 0) {
return -1;
}
}
else {
Py_DECREF(moduleName);
Py_DECREF(modulename);
}
}
if (className == NULL) {

PyObject *qualname = PyType_GetQualName((PyTypeObject *)exc_type);
if (qualname == NULL || !PyUnicode_Check(qualname)) {

@erlend-aasland erlend-aasland Sep 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, one last nitpick :) Unless I've missed something in typeobject.c, qualname is always a unicode object. I suggest using an assert for the unicode check. Ditto for the check in pythonrun.c.

@iritkatriel iritkatriel Sep 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that's correct, but I wasn't sure we want to assume it here (since this is error handling code, it needs to be very safe while it's not performance sensitive).

There is the same check above for modulename, I think there it is actually necessary.

I'm on the fence, wonder what others think.

@erlend-aasland erlend-aasland Sep 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[...] since this is error handling code, it needs to be very safe while it's not performance sensitive

Good points!

@ambv ambv Sep 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, you can't make __qualname__ anything else than a string from Python code:

>>> class C: ...
...
>>> C.__name__ = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only assign string to C.__name__, not 'int'
>>> C.__qualname__ = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only assign string to C.__qualname__, not 'int'
>>> C.__module__ = 1
>>>

I don't mind keeping the check though as it's less dramatic than an assert failure. You're right that error handling isn't performance-sensitive.

@ambv ambv Sep 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Additionally, this isn't even bona fide "error handling", it's literally about printing the exception out so totally no issue with keeping the additional check.

Py_XDECREF(qualname);
_PyErr_Clear(tstate);
if (PyFile_WriteString("<unknown>", file) < 0) {
return -1;
}
}
else {
if (PyFile_WriteString(className, file) < 0) {
if (PyFile_WriteObject(qualname, file, Py_PRINT_RAW) < 0) {
Py_DECREF(qualname);
return -1;
}
Py_DECREF(qualname);
}

if (exc_value && exc_value != Py_None) {
Expand Down
37 changes: 19 additions & 18 deletions Python/pythonrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,36 +961,37 @@ print_exception(PyObject *f, PyObject *value)
/* Don't do anything else */
}
else {
PyObject* moduleName;
const char *className;
PyObject* modulename;

_Py_IDENTIFIER(__module__);
assert(PyExceptionClass_Check(type));
className = PyExceptionClass_Name(type);
if (className != NULL) {
const char *dot = strrchr(className, '.');
if (dot != NULL)
className = dot+1;
}

moduleName = _PyObject_GetAttrId(type, &PyId___module__);
if (moduleName == NULL || !PyUnicode_Check(moduleName))
modulename = _PyObject_GetAttrId(type, &PyId___module__);
if (modulename == NULL || !PyUnicode_Check(modulename))
{
Py_XDECREF(moduleName);
Py_XDECREF(modulename);
PyErr_Clear();
err = PyFile_WriteString("<unknown>", f);
}
else {
if (!_PyUnicode_EqualToASCIIId(moduleName, &PyId_builtins))
if (!_PyUnicode_EqualToASCIIId(modulename, &PyId_builtins))
{
err = PyFile_WriteObject(moduleName, f, Py_PRINT_RAW);
err = PyFile_WriteObject(modulename, f, Py_PRINT_RAW);
err += PyFile_WriteString(".", f);
}
Py_DECREF(moduleName);
Py_DECREF(modulename);
}
if (err == 0) {
if (className == NULL)
err = PyFile_WriteString("<unknown>", f);
else
err = PyFile_WriteString(className, f);
PyObject* qualname = PyType_GetQualName((PyTypeObject *)type);
if (qualname == NULL || !PyUnicode_Check(qualname)) {
Py_XDECREF(qualname);
PyErr_Clear();
err = PyFile_WriteString("<unknown>", f);
}
else {
err = PyFile_WriteObject(qualname, f, Py_PRINT_RAW);
Py_DECREF(qualname);
}
}
}
if (err == 0 && (value != Py_None)) {
Expand Down