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.

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

gh-90501: Add PyErr_GetHandledException and PyErr_SetHandledException #30531

Merged
merged 21 commits into from Apr 15, 2022

Conversation

Copy link
Member

@iritkatriel iritkatriel commented Jan 11, 2022

Now that exc_info was replace by just the exception, we can have simpler versions of the get/set c-api functions.

https://bugs.python.org/issue46343

@iritkatriel iritkatriel requested a review from as a code owner Jan 13, 2022
@iritkatriel iritkatriel requested a review from vstinner Jan 13, 2022
Python/errors.c Outdated Show resolved Hide resolved
Python/errors.c Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Co-authored-by: Victor Stinner <vstinner@python.org>
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Lib/test/test_capi.py Outdated Show resolved Hide resolved
Python/errors.c Outdated Show resolved Hide resolved
Modules/_testcapimodule.c Outdated Show resolved Hide resolved
iritkatriel and others added 2 commits Jan 17, 2022
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@iritkatriel iritkatriel requested a review from encukou Jan 25, 2022
@vstinner
Copy link
Member

@vstinner vstinner commented Feb 1, 2022

Cython defines __Pyx_PyErr_GetTopmostException() in Cython/Utility/Exceptions.c which is a copy of Python PyErr_GetTopmostException(). It seems like Cython cannot use PyErr_GetActiveException() but would need a function taking a tstate parameter.

@iritkatriel iritkatriel requested a review from markshannon Mar 16, 2022
@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Mar 21, 2022

In python you can only access this exception, so it's sys.exception() but in the C api there are two exceptions we can access - the in-flight (raised) exception or this (caught) exception. So I think we need to be clear which one this is.

Is it clear (judging by name only) which exception is the "active" one? Raised/in-flight or caught? :)

Copy link
Member

@vstinner vstinner left a comment

Since this function is an unusal use case, maybe give it an even more explicit name? PyErr_GetCaughtException()?

Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

@vstinner vstinner commented Mar 22, 2022

Would it be possible to use the same wording in the documentation of functions added by this PR, in PyErr_GetExcInfo() and in sys.exception() documentation? My remark is about "the exception instance that is currently being handled" in sys.exception() doc:
https://docs.python.org/dev/library/sys.html#sys.exception

Maybe add also a link from PyErr_GetExcInfo() to PyErr_GetActiveException() in the doc.

Co-authored-by: Victor Stinner <vstinner@python.org>
@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Mar 23, 2022

Since this function is an unusal use case, maybe give it an even more explicit name? PyErr_GetCaughtException()?

How about GetHandledException()?

(I'm not sure Caught accurately describes the timeframe in which an exception is "active").

@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Mar 23, 2022

Would it be possible to use the same wording in the documentation of functions added by this PR, in PyErr_GetExcInfo() and in sys.exception() documentation? My remark is about "the exception instance that is currently being handled" in sys.exception() doc: https://docs.python.org/dev/library/sys.html#sys.exception

sys.exception is in python space, where you don't have access to any other exception so I think the c-level wording might just add confusion. I agree though that the current wording is not great, in particular I'm not sure we need to talk about the frame stack. How about I reword the sys.exception() doc like this:

This function, when called while an exception handler (such as an ``except`` or ``except*`` clause) is executing, returns the exception instance that was caught by this handler. When exception handlers are nested within one another, only the exception handled by the innermost handler is accessible.

If no exception handler is executing, this function returns None.

Maybe add also a link from PyErr_GetExcInfo() to PyErr_GetActiveException() in the doc.

Sure.

Modules/_testcapimodule.c Outdated Show resolved Hide resolved
Include/pyerrors.h Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Mar 23, 2022

I made all the changes suggested in reviews so far (thanks!) except the name change.

I propose SetHandledException/GetHandledException. Does this sound good?

@iritkatriel iritkatriel changed the title bpo-46343: Add PyErr_GetActiveException and PyErr_SetActiveException bpo-46343: Add PyErr_GetHandledException and PyErr_SetHandledException Apr 13, 2022
@iritkatriel iritkatriel changed the title bpo-46343: Add PyErr_GetHandledException and PyErr_SetHandledException gh-90501: Add PyErr_GetHandledException and PyErr_SetHandledException Apr 13, 2022
@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Apr 13, 2022

I propose SetHandledException/GetHandledException. Does this sound good?

IMO , that's easier to understand. +1

@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Apr 13, 2022

@vstinner @encukou @markshannon Any last thoughts on this? Should we include this in 3.11?

@pablogsal FYI.

1 similar comment
@iritkatriel
Copy link
Member Author

@iritkatriel iritkatriel commented Apr 13, 2022

@vstinner @encukou @markshannon Any last thoughts on this? Should we include this in 3.11?

@pablogsal FYI.

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 13, 2022

@vstinner @encukou @markshannon Any last thoughts on this? Should we include this in 3.11?

@pablogsal FYI.

I'm fine including this into 3.11, but please merge it at least one week before the beta to allow to test it a bit.

Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
@iritkatriel iritkatriel merged commit 5d421d7 into python:main Apr 15, 2022
13 checks passed
@iritkatriel iritkatriel deleted the bpo-46343-GetSetException branch Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants