Skip to content
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

Jump to bottom

gh-92119: Print exception class name instead of its representation #98302

Merged
merged 6 commits into from
Nov 8, 2022
Merged

gh-92119: Print exception class name instead of its representation #98302

merged 6 commits into from
Nov 8, 2022

Conversation

kamilturek
Copy link
Contributor

@kamilturek kamilturek commented Oct 15, 2022 •
edited by bedevere-bot

This PR updates the exception messages raised from ctypes calls to print the exception class name instead of its string representation. Also, it removes obsolete exceptions. prefixes from documentation examples.

Closes #92119.

kamilturek
kamilturek commented Oct 15, 2022
View reviewed changes
Doc/library/ctypes.rst Outdated
@@ -487,7 +487,7 @@ single character Python bytes object into a C char::
>>> strchr(b"abcdef", b"def")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ArgumentError: argument 2: exceptions.TypeError: one character string expected
ArgumentError: argument 2: TypeError: wrong type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the actual error message.

Copy link
Member

Choose a reason for hiding this comment

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

Probably best kept for a separate PR, but I wonder why we don't run these tests as part of the doctest suite.

Copy link
Contributor Author

@kamilturek kamilturek Nov 3, 2022 •
edited

Choose a reason for hiding this comment

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

OK, I will create a separate PR for this. Reverted the change for now.

@kamilturek kamilturek marked this pull request as ready for review October 15, 2022 21:40
@kumaraditya303 kumaraditya303 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-bug An unexpected behavior, bug, or error labels Oct 28, 2022
JelleZijlstra
JelleZijlstra reviewed Oct 30, 2022
View reviewed changes
Doc/library/ctypes.rst Outdated
@@ -487,7 +487,7 @@ single character Python bytes object into a C char::
>>> strchr(b"abcdef", b"def")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ArgumentError: argument 2: exceptions.TypeError: one character string expected
ArgumentError: argument 2: TypeError: wrong type
Copy link
Member

Choose a reason for hiding this comment

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

Probably best kept for a separate PR, but I wonder why we don't run these tests as part of the doctest suite.

Modules/_ctypes/callproc.c Outdated
@@ -1019,7 +1019,7 @@ void _ctypes_extend_error(PyObject *exc_class, const char *fmt, ...)

PyErr_Fetch(&tp, &v, &tb);
PyErr_NormalizeException(&tp, &v, &tb);
cls_str = PyObject_Str(tp);
cls_str = PyType_GetName((PyTypeObject *)tp);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's guaranteed that tp is actually a type here, although of course it should be. Perhaps we can execute this block only if (PyType_Check(tb)).

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://docs.python.org/3/c-api/exceptions.html?highlight=pyerr_fetch#c.PyErr_Fetch, tp is guaranteed to be a typeobject here.

Copy link
Member

Choose a reason for hiding this comment

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

There's no explicit statement that says that, and there is no check in the C code in errors.c that ensures that the type is really a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Corrected 👍

@bedevere-bot bedevere-bot mentioned this pull request Nov 3, 2022
Closed
@kamilturek kamilturek requested review from kumaraditya303 and JelleZijlstra and removed request for kumaraditya303 and JelleZijlstra November 3, 2022 10:18
@JelleZijlstra JelleZijlstra self-assigned this Nov 3, 2022
@chgnrdv chgnrdv mentioned this pull request Nov 6, 2022
Closed
@JelleZijlstra JelleZijlstra merged commit b9dedfe into python:main Nov 8, 2022
@miss-islington
Copy link
Contributor

Thanks @kamilturek for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 8, 2022
@kamilturek @miss-islington
pythongh-92119: ctypes: Print exception class name instead of its rep…
b01a7c2 
…resentation (pythonGH-98302)

(cherry picked from commit b9dedfe)

Co-authored-by: Kamil Turek <kamil.turek@hotmail.com>
@bedevere-bot
Copy link

GH-99234 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Nov 8, 2022
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Nov 8, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 8, 2022
@kamilturek @miss-islington
pythongh-92119: ctypes: Print exception class name instead of its rep…
bf3be15 
…resentation (pythonGH-98302)

(cherry picked from commit b9dedfe)

Co-authored-by: Kamil Turek <kamil.turek@hotmail.com>
miss-islington added a commit that referenced this pull request Nov 8, 2022
@miss-islington @kamilturek
gh-92119: ctypes: Print exception class name instead of its represent…
2d00190 
…ation (GH-98302)

(cherry picked from commit b9dedfe)

Co-authored-by: Kamil Turek <kamil.turek@hotmail.com>
@kamilturek kamilturek deleted the gh-92119 branch November 8, 2022 23:02
@bedevere-bot
Copy link

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

kamilturek added a commit to kamilturek/cpython that referenced this pull request Nov 13, 2022
@kamilturek
pythongh-92119: ctypes: Print exception class name instead of its rep…
9411a84 
…resentation (python#98302)

(cherry picked from commit b9dedfe)
JelleZijlstra pushed a commit that referenced this pull request Nov 13, 2022
@kamilturek
[3.10] gh-92119: ctypes: Print exception class name instead of its re…
bc2cdfc 
…presentation (GH-98302) (#99452)

gh-92119: ctypes: Print exception class name instead of its representation (#98302)

(cherry picked from commit b9dedfe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Assignees

@JelleZijlstra JelleZijlstra

Labels
topic-ctypes type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

_ctypes_extend_error prints exception class string representation instead of exception class name
6 participants
@kamilturek @miss-islington @bedevere-bot @JelleZijlstra @kumaraditya303 @AlexWaygood