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-102192: deprecate _PyErr_ChainExceptions #102935

Merged
merged 7 commits into from
Apr 1, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Mar 22, 2023

@erlend-aasland
Copy link
Contributor

I'm not sure we can simply remove this from the C API, as it is unfortunately exposed via Python.h:

$ echo '#include "Python.h"' | gcc -E $(./python.exe ./python-config.py --includes) - | grep _PyErr_ChainExceptions
__attribute__ ((visibility ("default"))) void _PyErr_ChainExceptions(PyObject *, PyObject *, PyObject *);
__attribute__ ((visibility ("default"))) void _PyErr_ChainExceptions1(PyObject *);

cc. @encukou

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

The code looks fine, but it will need documenting.

@@ -98,7 +98,6 @@ PyAPI_FUNC(void) _PyErr_GetExcInfo(PyThreadState *, PyObject **, PyObject **, Py

/* Context manipulation (PEP 3134) */

PyAPI_FUNC(void) _PyErr_ChainExceptions(PyObject *, PyObject *, PyObject *);
Copy link
Member

Choose a reason for hiding this comment

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

This is marked as an API function, so it will need to be added to the "porting to 3.12" section and any other relevant sections in the "what's new".

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@markshannon
Copy link
Member

I'm not sure we can simply remove this from the C API, as it is unfortunately exposed via Python.h:

It is prefixed with an underscore so we can, in theory, remove it. But we will need to document how to upgrade to 3.12.
We could leave _PyErr_ChainExceptions in as a deprecated shim that calls _PyErr_ChainExceptions1 for a couple of releases, if we think removing it will cause breakages.

@iritkatriel
Copy link
Member Author

We could leave _PyErr_ChainExceptions in as a deprecated shim that calls _PyErr_ChainExceptions1 for a couple of releases

Currently it normalises the exception so just calling _PyErr_ChainExceptions1 with the second arg would be a change in semantics.

@erlend-aasland
Copy link
Contributor

Since it is exposed via Python.h, someone is bound to be using it, so we cannot just remove it. I'd say we mark it as deprecated using the Py_DEPRECATED macro. There's practically no maintenance overhead in keeping it around.

BTW, we should consider unexposing the new API (_PyErr_ChainExceptions1) by moving it to Include/internal before 3.12 is frozen.

@iritkatriel iritkatriel changed the title gh-102192: remove unused _PyErr_ChainExceptions gh-102192: deprecate _PyErr_ChainExceptions Mar 31, 2023
@iritkatriel
Copy link
Member Author

iritkatriel commented Mar 31, 2023

I searched on GitHub and found quite a few usages. So I agree with just deprecating it for now.

Not sure it's reasonable to not expose the alternative though, given people are using it.

@erlend-aasland
Copy link
Contributor

Not sure it's reasonable to not expose the alternative though, given people are using it.

Since people are using it, we should consider dropping the underscore, IMO.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM.

We should consider making the new API official and documenting it, in a follow-up PR.

@@ -971,6 +971,10 @@ New Features
This is less error prone and a bit more efficient.
(Contributed by Mark Shannon in :gh:`101578`.)

* Add ``_PyErr_ChainExceptions1``, which takes an exception instance,
to replace the legacy-api ``_PyErr_ChainExceptions``, which is now
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: api => API

Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved
@iritkatriel iritkatriel merged commit 06249ec into python:main Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants