Skip to content

gh-108512: Add and use new replacements for PySys_GetObject() #111035

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

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4d0f508
gh-108512: Add and use new replacements for PySys_GetObject()
serhiy-storchaka Sep 20, 2023
eb42b39
Update Misc/stable_abi.toml
serhiy-storchaka Oct 18, 2023
2d4588d
Merge branch 'main' into capi-PySys_GetAttr
serhiy-storchaka Oct 18, 2023
2eb9533
Add tests.
serhiy-storchaka Oct 19, 2023
9fc2f3d
Apply suggestions from code review
serhiy-storchaka Oct 19, 2023
65713ce
Address review comments.
serhiy-storchaka Oct 19, 2023
e6ecf11
Check that the name is a string.
serhiy-storchaka Oct 19, 2023
8a0f5f2
Merge remote-tracking branch 'refs/remotes/origin/capi-PySys_GetAttr'…
serhiy-storchaka Oct 19, 2023
516829e
Make the new C API not public.
serhiy-storchaka Oct 19, 2023
9503aaf
Remove from Misc/stable_abi.toml.
serhiy-storchaka Oct 19, 2023
e2857ef
Merge branch 'main' into capi-PySys_GetAttr
serhiy-storchaka Jan 28, 2025
dc26ec2
Add to the limited C API.
serhiy-storchaka Jan 28, 2025
104dcc2
Replace few new occurrences of PySys_GetObject().
serhiy-storchaka Jan 28, 2025
cf75fc3
Update the documentation.
serhiy-storchaka Jan 28, 2025
9434659
Clean up.
serhiy-storchaka Jan 28, 2025
56639d8
Fix a typo.
serhiy-storchaka Jan 28, 2025
b40a665
Merge branch 'main' into capi-PySys_GetAttr
serhiy-storchaka Feb 6, 2025
03b9c0a
Merge branch 'main' into capi-PySys_GetAttr
serhiy-storchaka Feb 6, 2025
5d793c5
Merge remote-tracking branch 'refs/remotes/origin/capi-PySys_GetAttr'…
serhiy-storchaka Feb 6, 2025
09869ed
Merge branch 'main' into capi-PySys_GetAttr
serhiy-storchaka Feb 25, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Doc/c-api/init_config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2098,7 +2098,7 @@ initialization::

/* Specify sys.path explicitly */
/* If you want to modify the default set of paths, finish
initialization first and then use PySys_GetObject("path") */
initialization first and then use PySys_GetAttrString("path") */
config.module_search_paths_set = 1;
status = PyWideStringList_Append(&config.module_search_paths,
L"/path/to/stdlib");
Expand Down
49 changes: 47 additions & 2 deletions Doc/c-api/sys.rst
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,55 @@ These are utility functions that make functionality from the :mod:`sys` module
accessible to C code. They all work with the current interpreter thread's
:mod:`sys` module's dict, which is contained in the internal thread state structure.

.. c:function:: PyObject *PySys_GetAttr(PyObject *name)

Get the attribute *name* of the :mod:`sys` module. Return a :term:`strong reference`.
Raise :exc:`RuntimeError` and return ``NULL`` if it does not exist.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to raise AttributeError.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we would need a new pair of functions that raise RuntimeError to use in the CPython code. Or you can add a pair of functions that raise AttributeError later.

Copy link
Member

Choose a reason for hiding this comment

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

Then we would need a new pair of functions that raise RuntimeError to use in the CPython code.

Would you mind to elaborate why RuntimeError is needed by CPython?

Copy link
Member Author

Choose a reason for hiding this comment

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

Status quo. And it looks to me it is justified by the purpose of RuntimeError: "Raised when an error is detected that doesn’t fall in any of the other categories."

If you want to change RuntimeError's in the existing code to something other, you should justify that change.

Copy link
Member

Choose a reason for hiding this comment

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

In my implementation #129369, PySys_SetAttr() raises RuntimeError if the sys module cannot be retrieved. But it raises AttributeError if the attribute doesn't exist.

PySys_GetObject() ignores all errors and returns NULL on error or if the attribute doesn't exist. So I'm confused, which function using PySys_GetObject() currently rely on the RuntimeError? Do you have an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

About a half of functions that use PySys_GetObject() or _PySys_GetAttr() raise RuntimeError if they return NULL. See my statistics in capi-workgroup/decisions#54. Look at usages of PySys_GetAttr() and PySys_GetAttrString() in this PR.


If the non-existing object should not be treated as a failure, you can use
:c:func:`PySys_GetOptionalAttr` instead.

.. versionadded:: next

.. c:function:: PyObject *PySys_GetAttrString(const char *name)

This is the same as :c:func:`PySys_GetAttr`, but *name* is
specified as a :c:expr:`const char*` UTF-8 encoded bytes string,
rather than a :c:expr:`PyObject*`.

If the non-existing object should not be treated as a failure, you can use
:c:func:`PySys_GetOptionalAttrString` instead.

.. versionadded:: next

.. c:function:: int PySys_GetOptionalAttr(PyObject *name, PyObject **result);

Variant of :c:func:`PySys_GetAttr` which doesn't raise
exception if the object does not exist.

If the object exists, set *\*result* to a new :term:`strong reference`
to the object and return ``1``.
If the object does not exist, set *\*result* to ``NULL`` and return ``0``,
without setting an exception.
If other error occurred, set an exception, set *\*result* to ``NULL`` and
return ``-1``.
Comment on lines +287 to +292
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to use a list and start with the return value, so it's easier to see the 3 cases:

Suggested change
If the object exists, set *\*result* to a new :term:`strong reference`
to the object and return ``1``.
If the object does not exist, set *\*result* to ``NULL`` and return ``0``,
without setting an exception.
If other error occurred, set an exception, set *\*result* to ``NULL`` and
return ``-1``.
* Return ``1`` and set *\*result* to a new :term:`strong reference`
to the object if the attribute exists.
* Return ``0`` without setting an exception and set *\*result* to ``NULL``
if the attribute does not exist.
* Return ``-1``, set an exception and set *\*result* to ``NULL``
if an error occurred.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Return and then set exception and variable" looks like a wrong sequence to me. It cannot do anything after returning.


.. versionadded:: next

.. c:function:: int PySys_GetOptionalAttrString(const char *name, PyObject **result);

This is the same as :c:func:`PySys_GetOptionalAttr`, but *name* is
specified as a :c:expr:`const char*` UTF-8 encoded bytes string,
rather than a :c:expr:`PyObject*`.

.. versionadded:: next

.. c:function:: PyObject *PySys_GetObject(const char *name)

Return the object *name* from the :mod:`sys` module or ``NULL`` if it does
not exist, without setting an exception.
Similar to :c:func:`PySys_GetAttrString`, but return a :term:`borrowed
reference` and return ``NULL`` *without* setting exception on failure.

Preserves exception that was set before the call.

.. c:function:: int PySys_SetObject(const char *name, PyObject *v)

Expand Down
4 changes: 4 additions & 0 deletions Doc/data/stable_abi.dat

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Doc/whatsnew/3.14.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1491,6 +1491,11 @@ New features

(Contributed by Sergey B Kirpichev and Victor Stinner in :gh:`102471`.)

* Add :c:func:`PySys_GetAttr`, :c:func:`PySys_GetAttrString`,
:c:func:`PySys_GetOptionalAttr`, and :c:func:`PySys_GetOptionalAttrString`
functions as replacements for :c:func:`PySys_GetObject`.
(Contributed by Serhiy Storchaka in :gh:`108512`.)

* Add :c:func:`PyType_GetBaseByToken` and :c:data:`Py_tp_token` slot for easier
superclass identification, which attempts to resolve the `type checking issue
<https://peps.python.org/pep-0630/#type-checking>`__ mentioned in :pep:`630`
Expand Down
23 changes: 23 additions & 0 deletions Include/cpython/sysmodule.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#ifndef Py_CPYTHON_SYSMODULE_H
# error "this header file must not be included directly"
#endif

typedef int(*Py_AuditHookFunction)(const char *, PyObject *, void *);

PyAPI_FUNC(int) PySys_Audit(
const char *event,
const char *format,
...);
PyAPI_FUNC(int) PySys_AddAuditHook(Py_AuditHookFunction, void*);

typedef struct {
FILE* perf_map;
PyThread_type_lock map_lock;
} PerfMapState;

PyAPI_FUNC(int) PyUnstable_PerfMapState_Init(void);
PyAPI_FUNC(int) PyUnstable_WritePerfMapEntry(
const void *code_addr,
unsigned int code_size,
const char *entry_name);
PyAPI_FUNC(void) PyUnstable_PerfMapState_Fini(void);
5 changes: 0 additions & 5 deletions Include/internal/pycore_sysmodule.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@ extern "C" {
# error "this header requires Py_BUILD_CORE define"
#endif

PyAPI_FUNC(int) _PySys_GetOptionalAttr(PyObject *, PyObject **);
PyAPI_FUNC(int) _PySys_GetOptionalAttrString(const char *, PyObject **);
PyAPI_FUNC(PyObject *) _PySys_GetRequiredAttr(PyObject *);
PyAPI_FUNC(PyObject *) _PySys_GetRequiredAttrString(const char *);

// Export for '_pickle' shared extension
PyAPI_FUNC(size_t) _PySys_GetSizeOf(PyObject *);

Expand Down
6 changes: 6 additions & 0 deletions Include/sysmodule.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
extern "C" {
#endif

#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030e0000
PyAPI_FUNC(PyObject *) PySys_GetAttr(PyObject *);
PyAPI_FUNC(PyObject *) PySys_GetAttrString(const char *);
PyAPI_FUNC(int) PySys_GetOptionalAttr(PyObject *, PyObject **);
PyAPI_FUNC(int) PySys_GetOptionalAttrString(const char *, PyObject **);
#endif
PyAPI_FUNC(PyObject *) PySys_GetObject(const char *);
PyAPI_FUNC(int) PySys_SetObject(const char *, PyObject *);

Expand Down
56 changes: 56 additions & 0 deletions Lib/test/test_capi/test_sys.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,62 @@ class CAPITest(unittest.TestCase):

maxDiff = None

@unittest.skipIf(_testlimitedcapi is None, 'need _testlimitedcapi module')
def test_sys_getattr(self):
# Test PySys_GetAttr()
sys_getattr = _testlimitedcapi.sys_getattr

self.assertIs(sys_getattr('stdout'), sys.stdout)
with support.swap_attr(sys, '\U0001f40d', 42):
self.assertEqual(sys_getattr('\U0001f40d'), 42)

with self.assertRaisesRegex(RuntimeError, r'lost sys\.nonexisting'):
sys_getattr('nonexisting')
self.assertRaises(TypeError, sys_getattr, 1)
self.assertRaises(TypeError, sys_getattr, [])
# CRASHES sys_getattr(NULL)

@unittest.skipIf(_testlimitedcapi is None, 'need _testlimitedcapi module')
def test_sys_getattrstring(self):
# Test PySys_GetAttrString()
getattrstring = _testlimitedcapi.sys_getattrstring

self.assertIs(getattrstring(b'stdout'), sys.stdout)
with support.swap_attr(sys, '\U0001f40d', 42):
self.assertEqual(getattrstring('\U0001f40d'.encode()), 42)

with self.assertRaisesRegex(RuntimeError, r'lost sys\.nonexisting'):
getattrstring(b'nonexisting')
self.assertRaises(UnicodeDecodeError, getattrstring, b'\xff')
# CRASHES getattrstring(NULL)

@unittest.skipIf(_testlimitedcapi is None, 'need _testlimitedcapi module')
def test_sys_getoptionalattr(self):
# Test PySys_GetOptionalAttr()
getoptionalattr = _testlimitedcapi.sys_getoptionalattr

self.assertIs(getoptionalattr('stdout'), sys.stdout)
with support.swap_attr(sys, '\U0001f40d', 42):
self.assertEqual(getoptionalattr('\U0001f40d'), 42)

self.assertIs(getoptionalattr('nonexisting'), AttributeError)
self.assertRaises(TypeError, getoptionalattr, 1)
self.assertRaises(TypeError, getoptionalattr, [])
# CRASHES getoptionalattr(NULL)

@unittest.skipIf(_testlimitedcapi is None, 'need _testlimitedcapi module')
def test_sys_getoptionalattrstring(self):
# Test PySys_GetOptionalAttrString()
getoptionalattrstring = _testlimitedcapi.sys_getoptionalattrstring

self.assertIs(getoptionalattrstring(b'stdout'), sys.stdout)
with support.swap_attr(sys, '\U0001f40d', 42):
self.assertEqual(getoptionalattrstring('\U0001f40d'.encode()), 42)

self.assertIs(getoptionalattrstring(b'nonexisting'), AttributeError)
self.assertRaises(UnicodeDecodeError, getoptionalattrstring, b'\xff')
# CRASHES getoptionalattrstring(NULL)

@support.cpython_only
@unittest.skipIf(_testlimitedcapi is None, 'need _testlimitedcapi module')
def test_sys_getobject(self):
Expand Down
4 changes: 4 additions & 0 deletions Lib/test/test_stable_abi_ctypes.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add functions :c:func:`PySys_GetAttr`, :c:func:`PySys_GetAttrString`,
:c:func:`PySys_GetOptionalAttr` and :c:func:`PySys_GetOptionalAttrString`.
8 changes: 8 additions & 0 deletions Misc/stable_abi.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2545,3 +2545,11 @@
added = '3.14'
[function.Py_PACK_VERSION]
added = '3.14'
[function.PySys_GetAttr]
added = '3.14'
[function.PySys_GetAttrString]
added = '3.14'
[function.PySys_GetOptionalAttr]
added = '3.14'
[function.PySys_GetOptionalAttrString]
added = '3.14'
3 changes: 1 addition & 2 deletions Modules/_cursesmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ static const char PyCursesVersion[] = "2.2";
#include "pycore_capsule.h" // _PyCapsule_SetTraverse()
#include "pycore_long.h" // _PyLong_GetZero()
#include "pycore_structseq.h" // _PyStructSequence_NewType()
#include "pycore_sysmodule.h" // _PySys_GetOptionalAttrString()

#ifdef __hpux
#define STRICT_SYSV_CURSES
Expand Down Expand Up @@ -3543,7 +3542,7 @@ _curses_setupterm_impl(PyObject *module, const char *term, int fd)
if (fd == -1) {
PyObject* sys_stdout;

if (_PySys_GetOptionalAttrString("stdout", &sys_stdout) < 0) {
if (PySys_GetOptionalAttrString("stdout", &sys_stdout) < 0) {
return NULL;
}

Expand Down
6 changes: 3 additions & 3 deletions Modules/_lsprof.c
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ _lsprof_Profiler_enable_impl(ProfilerObject *self, int subcalls,
return NULL;
}

PyObject* monitoring = PyImport_ImportModuleAttrString("sys", "monitoring");
PyObject* monitoring = PySys_GetAttrString("monitoring");
if (!monitoring) {
return NULL;
}
Expand Down Expand Up @@ -861,7 +861,7 @@ _lsprof_Profiler_disable_impl(ProfilerObject *self)
}
if (self->flags & POF_ENABLED) {
PyObject* result = NULL;
PyObject* monitoring = PyImport_ImportModuleAttrString("sys", "monitoring");
PyObject* monitoring = PySys_GetAttrString("monitoring");

if (!monitoring) {
return NULL;
Expand Down Expand Up @@ -980,7 +980,7 @@ profiler_init_impl(ProfilerObject *self, PyObject *timer, double timeunit,
Py_XSETREF(self->externalTimer, Py_XNewRef(timer));
self->tool_id = PY_MONITORING_PROFILER_ID;

PyObject* monitoring = PyImport_ImportModuleAttrString("sys", "monitoring");
PyObject* monitoring = PySys_GetAttrString("monitoring");
if (!monitoring) {
return -1;
}
Expand Down
2 changes: 1 addition & 1 deletion Modules/_pickle.c
Original file line number Diff line number Diff line change
Expand Up @@ -1910,7 +1910,7 @@ whichmodule(PickleState *st, PyObject *global, PyObject *global_name, PyObject *
__module__ can be None. If it is so, then search sys.modules for
the module of global. */
Py_CLEAR(module_name);
modules = _PySys_GetRequiredAttr(&_Py_ID(modules));
modules = PySys_GetAttr(&_Py_ID(modules));
if (modules == NULL) {
return NULL;
}
Expand Down
73 changes: 73 additions & 0 deletions Modules/_testlimitedcapi/sys.c
Original file line number Diff line number Diff line change
@@ -1,7 +1,76 @@
#include "pyconfig.h" // Py_GIL_DISABLED
// Need limited C API version 3.14 for PySys_GetAttr() etc
#if !defined(Py_GIL_DISABLED) && !defined(Py_LIMITED_API)
# define Py_LIMITED_API 0x030e0000
#endif
#include "parts.h"
#include "util.h"


static PyObject *
sys_getattr(PyObject *Py_UNUSED(module), PyObject *name)
{
NULLABLE(name);
return PySys_GetAttr(name);
}

static PyObject *
sys_getattrstring(PyObject *Py_UNUSED(module), PyObject *arg)
{
const char *name;
Py_ssize_t size;
if (!PyArg_Parse(arg, "z#", &name, &size)) {
return NULL;
}
return PySys_GetAttrString(name);
}

static PyObject *
sys_getoptionalattr(PyObject *Py_UNUSED(module), PyObject *name)
{
PyObject *value = UNINITIALIZED_PTR;
NULLABLE(name);

switch (PySys_GetOptionalAttr(name, &value)) {
case -1:
assert(value == NULL);
assert(PyErr_Occurred());
return NULL;
case 0:
assert(value == NULL);
return Py_NewRef(PyExc_AttributeError);
case 1:
return value;
default:
Py_FatalError("PySys_GetOptionalAttr() returned invalid code");
}
}

static PyObject *
sys_getoptionalattrstring(PyObject *Py_UNUSED(module), PyObject *arg)
{
PyObject *value = UNINITIALIZED_PTR;
const char *name;
Py_ssize_t size;
if (!PyArg_Parse(arg, "z#", &name, &size)) {
return NULL;
}

switch (PySys_GetOptionalAttrString(name, &value)) {
case -1:
assert(value == NULL);
assert(PyErr_Occurred());
return NULL;
case 0:
assert(value == NULL);
return Py_NewRef(PyExc_AttributeError);
case 1:
return value;
default:
Py_FatalError("PySys_GetOptionalAttrString() returned invalid code");
}
}

static PyObject *
sys_getobject(PyObject *Py_UNUSED(module), PyObject *arg)
{
Expand Down Expand Up @@ -39,6 +108,10 @@ sys_getxoptions(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(ignored))


static PyMethodDef test_methods[] = {
{"sys_getattr", sys_getattr, METH_O},
{"sys_getattrstring", sys_getattrstring, METH_O},
{"sys_getoptionalattr", sys_getoptionalattr, METH_O},
{"sys_getoptionalattrstring", sys_getoptionalattrstring, METH_O},
{"sys_getobject", sys_getobject, METH_O},
{"sys_setobject", sys_setobject, METH_VARARGS},
{"sys_getxoptions", sys_getxoptions, METH_NOARGS},
Expand Down
3 changes: 1 addition & 2 deletions Modules/_threadmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "pycore_modsupport.h" // _PyArg_NoKeywords()
#include "pycore_pylifecycle.h"
#include "pycore_pystate.h" // _PyThreadState_SetCurrent()
#include "pycore_sysmodule.h" // _PySys_GetOptionalAttr()
#include "pycore_time.h" // _PyTime_FromSeconds()
#include "pycore_weakref.h" // _PyWeakref_GET_REF()

Expand Down Expand Up @@ -2250,7 +2249,7 @@ thread_excepthook(PyObject *module, PyObject *args)
PyObject *thread = PyStructSequence_GET_ITEM(args, 3);

PyObject *file;
if (_PySys_GetOptionalAttr( &_Py_ID(stderr), &file) < 0) {
if (PySys_GetOptionalAttr( &_Py_ID(stderr), &file) < 0) {
return NULL;
}
if (file == NULL || file == Py_None) {
Expand Down
Loading
Loading