Repository navigation
bpo-44938: Expose PyErr_ChainExceptions in the stable API #27799
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
Changes from 1 commit
33a859b
e9b9724
8bcee51
a6e7254
30135a1
9ffb923
a102b8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,22 +127,24 @@ For convenience, some of these functions will always return a | |
|
|
||
| .. c:function:: void PyErr_ChainExceptions(PyObject *exc, PyObject *val, PyObject *tb) | ||
|
|
||
| Set the provided exception as the cause for the current exception. If there is no exception | ||
| currently set (:c:func:`PyErr_Occurred` returns ``0``) this function behaves | ||
| as like :c:func:`PyErr_Restore`, but if an exception is already set, set the | ||
| context associated with it. | ||
| Set the provided exception as the cause for the current exception. If there | ||
| is no exception currently set (:c:func:`PyErr_Occurred` returns ``0``) this | ||
| function behaves as like :c:func:`PyErr_Restore`, but if an exception is | ||
|
pablogsal marked this conversation as resolved.
Outdated
|
||
| already set, set the provided exception as the context (accessible from | ||
| Python through the :attr:`__context__` attribute of the exception) of the | ||
| one that is currently set. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it the other way around - the existing exception becomes the context of the provided exception?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code calls So unless I am missing something (is late here 😅 ) this means that "The provided exception (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I was thinking we are raising the provided exception in the context of the existing one, but that's what SetObject is for. Can we think of a more descriptive name for this function? I confused the direction, and you confused cause/context because "chain" is ambiguous here. How about PyErr_RestoreContext? IIUC this describes what you use this function for - putting back a fetched exception, as the context of any exception that happened between the fetch and now. (Should something like the previous sentence be in the doc?)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or possibly PyErr_RestoreAsContext?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general authors look for "chain" in the docs because that's the action they want to do, so I would really recommend to have "chain" in the name.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That may be the case, but I think we should promote clear language, and "chain" in this context is vague. The doc can contain "chain" in the prose for the benefit of people who search for it (for now). But should it be part of the function name for all eternity?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is unclear to me. I agree with the ambiguity but what users if the function want is a higher level approach to the python behaviour: an exception is raised when another exception was there and I want to do in C the same thing as Python does. I'm not a native speaker do I trust your judgement regarding how ambiguous this sounds and that we need a better name so let's search for one. Maybe we should make a section in the docs about "exception chaining" and offer two APIs: one for cause and another for context
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iritkatriel Maybe you can suggest a more specific name that still retains "chain" as part of it?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The context is an implicitly chained exception (the cause is explicitly chained). There is no need to have both chained and context in the name. If we have only one of them I think it should be context and not chain.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the equivalence to The key point to emphasise is that the function takes the same arguments as |
||
|
|
||
| The exception type should be a class. Do not pass an invalid exception type or value. This | ||
| function steals a reference to each of its arguments in the same fashion as :c:func:`PyErr_Restore` | ||
| does. | ||
| The exception type should be a class. Do not pass an invalid exception type | ||
| or value. This function steals a reference to each of its arguments in the | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could it not steal its arguments?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It steals the arguments because of what it calls. It had the same calling convention as
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, stealing arguments is not an uncommon calling convention.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I changed my mind on this point, as |
||
| same fashion as :c:func:`PyErr_Restore` does. | ||
|
|
||
| .. warning:: | ||
|
|
||
| The caller is responsible for ensuring that this call won't create | ||
| any cycles in the exception context chain. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the doc should say what happens if you violate the rule. I think in reality it will just create the cycle and if it doesn't bother you then good for you.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is undefined behavior, technically as the contract of this function withing CPython is that cycles should not be created. Some parts of the exception machinery can break (or at least they used to) in presence of cycles.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But when the exception machinery breaks in the presence of a cycle we consider that to be a bug in python.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is the caller supposed to ensure this? Is there any API for checking it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no API to check this, but to create a cycle with exceptions it needs to be s deliberate action. There is not an easy way to accidentally crate an exception cycle
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the movement is in the direction of trying to prevent / avoid the creation of cycles in the first place (as was mentioned in the discussion for this recent ticket, for example). The warning being added here is in line with that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per my comment below, the easiest way to violate this requirement is to call the API without setting a new exception first (or clearing the existing one). Buggy code (using my API naming suggestion from above): Because the exception might not have been instantiated yet when |
||
|
|
||
| See :ref:`tut-exception-chaining` for more information regarding the semantics of exception | ||
| chaining. | ||
| See :ref:`tut-exception-chaining` for more information regarding the | ||
| semantics of exception chaining. | ||
|
|
||
| Example usage: | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.