bpo-45083: Include the exception class qualname when formatting an ex… by iritkatriel · Pull Request #28119 · python/cpython

iritkatriel · 2021-09-01T23:52:03Z

…ception

https://bugs.python.org/issue45083

…ception

erlend-aasland

I left some nitpicky code style remarks.

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

bedevere-bot · 2021-09-02T10:35:17Z

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 9781ce5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

erlend-aasland · 2021-09-02T12:20:16Z

In order to free qual name easily in Python/errors.c, I moved PyType_GetQualName and PyUnicode_AsUTF8 down to where class name is actually used. Ditto for Python/pythonrun.c.

patch

diff --git a/Python/errors.c b/Python/errors.c
index e390187215..cfa4275722 100644
--- a/Python/errors.c
+++ b/Python/errors.c
@@ -1287,14 +1287,6 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
     }
 
     assert(PyExceptionClass_Check(exc_type));
-    PyObject *qualName = PyType_GetQualName((PyTypeObject *)exc_type);
-    if (qualName == NULL) {
-        return -1;
-    }
-    const char *className = PyUnicode_AsUTF8(qualName);
-    if (!className) {
-        return -1;
-    }
 
     PyObject *moduleName = _PyObject_GetAttrId(exc_type, &PyId___module__);
     if (moduleName == NULL || !PyUnicode_Check(moduleName)) {
@@ -1319,13 +1311,22 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
             Py_DECREF(moduleName);
         }
     }
-    if (className == NULL) {
+
+    PyObject *qualname = PyType_GetQualName((PyTypeObject *)exc_type);
+    if (qualname == NULL) {
+        return -1;
+    }
+    const char *classname = PyUnicode_AsUTF8(qualname);
+    if (classname == NULL) {
+        Py_DECREF(qualname);
         if (PyFile_WriteString("<unknown>", file) < 0) {
             return -1;
         }
     }
     else {
-        if (PyFile_WriteString(className, file) < 0) {
+        int rc = PyFile_WriteString(classname, file);
+        Py_DECREF(qualname);
+        if (rc < 0) {
             return -1;
         }
     }
diff --git a/Python/pythonrun.c b/Python/pythonrun.c
index b4e34fb504..b5a3b6979b 100644
--- a/Python/pythonrun.c
+++ b/Python/pythonrun.c
@@ -962,23 +962,10 @@ print_exception(PyObject *f, PyObject *value)
     }
     else {
         PyObject* moduleName;
-        PyObject* qualName;
-        const char *className = NULL;
 
         _Py_IDENTIFIER(__module__);
         assert(PyExceptionClass_Check(type));
 
-        qualName = PyType_GetQualName((PyTypeObject *)type);
-        if (!qualName) {
-            PyErr_Clear();
-        }
-        else {
-            className = PyUnicode_AsUTF8(qualName);
-            if (!className) {
-                PyErr_Clear();
-            }
-        }
-
         moduleName = _PyObject_GetAttrId(type, &PyId___module__);
         if (moduleName == NULL || !PyUnicode_Check(moduleName))
         {
@@ -994,10 +981,20 @@ print_exception(PyObject *f, PyObject *value)
             Py_DECREF(moduleName);
         }
         if (err == 0) {
-            if (className == NULL)
+            PyObject *qualname = PyType_GetQualName((PyTypeObject *)type);
+            if (qualname == NULL) {
+                err = PyFile_WriteString("<unknown>", f);
+            }
+            else {
+                const char *classname = PyUnicode_AsUTF8(qualname);
+                if (classname == NULL) {
                       err = PyFile_WriteString("<unknown>", f);
-            else
-                      err = PyFile_WriteString(className, f);
+                }
+                else {
+                      err = PyFile_WriteString(classname, f);
+                }
+                Py_DECREF(qualname);
+            }
         }
     }
     if (err == 0 && (value != Py_None)) {

iritkatriel · 2021-09-02T12:40:42Z

Do we not need to clear the error in this case?

-        else {
-            className = PyUnicode_AsUTF8(qualName);
-            if (!className) {
-                PyErr_Clear();
-            }
-        }

erlend-aasland · 2021-09-02T12:44:24Z

Do we not need to clear the error in this case?

I'm not sure; you probably know that better than me :) IIRC, the old code did not clear the error (on mobile right now, so I can't verify).

erlend-aasland · 2021-09-02T15:06:07Z

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


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.

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.

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

Good points!

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.

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.

ambv · 2021-09-03T07:29:44Z

Confirmed no refleaks in test_exceptions and test_concurrent_futures.

miss-islington · 2021-09-03T07:30:20Z

Thanks @iritkatriel for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-09-03T07:30:43Z

GH-28134 is a backport of this pull request to the 3.10 branch.

bedevere-bot · 2021-09-03T07:30:47Z

GH-28135 is a backport of this pull request to the 3.9 branch.

…ception (pythonGH-28119) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no> (cherry picked from commit b4b6342) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>

… an exception (GH-28119) (GH-28135) * bpo-45083: Include the exception class qualname when formatting an exception (GH-28119) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no> (cherry picked from commit b4b6342) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Co-authored-by: Łukasz Langa <lukasz@langa.pl>

…g an exception (GH-28119) (GH-28134) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no> (cherry picked from commit b4b6342) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> * Use a private version of _PyType_GetQualName Co-authored-by: Łukasz Langa <lukasz@langa.pl>

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

c2ce88e

…ception

iritkatriel requested review from pablogsal and serhiy-storchaka September 1, 2021 23:52

the-knights-who-say-ni added the CLA signed label Sep 1, 2021

bedevere-bot added the awaiting core review label Sep 1, 2021

📜🤖 Added by blurb_it.

39f24c6

erlend-aasland reviewed Sep 2, 2021

View reviewed changes

Comment thread Python/pythonrun.c Outdated

Comment thread Python/pythonrun.c Outdated

Comment thread Python/errors.c Outdated

iritkatriel and others added 3 commits September 2, 2021 10:36

== NULL

b8c09ce

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

newline before else

0da8f5b

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

move * on pointer

9781ce5

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 2, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 2, 2021

erlend-aasland requested changes Sep 2, 2021

View reviewed changes

Comment thread Python/errors.c Outdated

Comment thread Python/pythonrun.c Outdated

Comment thread Python/pythonrun.c Outdated

Comment thread Python/pythonrun.c Outdated

Comment thread Python/pythonrun.c Outdated

Comment thread Python/errors.c Outdated

simplify code and fix qualname refleak

f863ff0

erlend-aasland reviewed Sep 2, 2021

View reviewed changes

ambv added needs backport to 3.9 needs backport to 3.10 only security fixes labels Sep 3, 2021

ambv merged commit b4b6342 into python:main Sep 3, 2021

bedevere-bot removed the awaiting core review label Sep 3, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 3, 2021

bedevere-bot removed the needs backport to 3.9 label Sep 3, 2021

iritkatriel deleted the bpo-45083 branch November 5, 2021 09:54

Uh oh!

Uh oh!

Conversation

iritkatriel commented Sep 1, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Sep 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Sep 2, 2021

Uh oh!

iritkatriel commented Sep 2, 2021

Uh oh!

erlend-aasland commented Sep 2, 2021

Uh oh!

erlend-aasland Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

iritkatriel Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

ambv Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

ambv Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

ambv commented Sep 3, 2021

Uh oh!

miss-islington commented Sep 3, 2021

Uh oh!

bedevere-bot commented Sep 3, 2021

Uh oh!

bedevere-bot commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iritkatriel commented Sep 1, 2021 •

edited by bedevere-bot

Loading