Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 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
439bc3c
Merge branch 'main' into capi-PySys_GetAttr
serhiy-storchaka May 21, 2025
93ab31b
Move to 3.15.
serhiy-storchaka May 21, 2025
154a82a
Address review comments.
serhiy-storchaka May 22, 2025
81c7605
Improve tests.
serhiy-storchaka May 22, 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
49 changes: 49 additions & 0 deletions Lib/test/test_capi/test_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,55 @@ class Data(_testcapi.ObjExtraData):
del d.extra
self.assertIsNone(d.extra)

def test_sys_getattr(self):
sys_getattr = _testcapi.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'):
Copy link
Member

Choose a reason for hiding this comment

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

This error message is surprising. sys has no attribute "nonexisting". It's not "lost", it simply doesn't exist.

I would prefer to always raise AttributeError with a message like "module 'sys' has no attribute 'x'", similar than in Python:

>>> import sys
>>> sys.x
AttributeError: module 'sys' has no attribute 'x'

Copy link
Member Author

Choose a reason for hiding this comment

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

It leaves no use cases for PySys_GetAttr(). They all can be replaced with PySys_GetOptionalAttr() followed by PyErr_SetString(PyExc_RuntimeError,).

If you leave PySys_GetAttr(), you will always use it with PyErr_ExceptionMatches(PyExc_AttributeError) followed by PyErr_SetString(PyExc_RuntimeError,).

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

def test_sys_getattrstring(self):
sys_getattr = _testcapi.sys_getattrstring

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

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

def test_sys_getoptionalattr(self):
sys_getattr = _testcapi.sys_getoptionalattr
Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that in all tests, the function is called "sys_getattr", whereas here you test PySys_GetOptionalAttr(), not PySys_GetAttr().

I suggest to rename the variable t o"sys_getoptionalattr", or even PySys_GetOptionalAttr() since this is a C API test. It would be more explicit to use the name of the C API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it was exactly what I wrote initially, but in last minute I replaced all names with the same name to make reading easy. But if you think that it does not help, I'll restore previous names.


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

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

def test_sys_getoptionalattrstring(self):
sys_getattr = _testcapi.sys_getoptionalattrstring

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

self.assertIs(sys_getattr(b'nonexisting'), AttributeError)
self.assertRaises(UnicodeDecodeError, sys_getattr, b'\xff')
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a UnicodeDecodeError to test_sys_getattr() as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, PySys_GetAttr() does not raise UnicodeDecodeError. The wrapper does, but we do not test PyArg_Parse() here.

# CRASHES sys_getattr(NULL)

def test_sys_getobject(self):
getobject = _testcapi.sys_getobject

Expand Down
84 changes: 84 additions & 0 deletions Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -3231,6 +3231,86 @@ test_weakref_capi(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
}


static PyObject *
sys_getattr(PyObject *Py_UNUSED(module), PyObject *name)
{
NULLABLE(name);
PyObject *result = PySys_GetAttr(name);
if (result == NULL && PyErr_Occurred()) {
return NULL;
}
if (result == NULL) {
result = PyExc_AttributeError;
Py_INCREF(PyExc_AttributeError);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this code path. The function must return NULL and raise an exception if the attribute does not exist. This code path must never be reached according to the API doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it exactly to test that it never happens. But perhaps it can be removed.

}
return result;
}

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;
}
PyObject *result = PySys_GetAttrString(name);
if (result == NULL && PyErr_Occurred()) {
return NULL;
}
if (result == NULL) {
result = PyExc_AttributeError;
Py_INCREF(PyExc_AttributeError);
}
return result;
}

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);
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 adding: assert(PyErr_Occurred());. Same remark in sys_getoptionalattrstring().

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't there a check that each function that returns NULL should also set an exception? I relied on this in all other tests.

return NULL;
case 0:
assert(value == NULL);
return Py_NewRef(PyExc_AttributeError);
case 1:
return value;
default:
Py_FatalError("PySys_GetOptionalAttr() returned invalid code");
Py_UNREACHABLE();
Copy link
Member

Choose a reason for hiding this comment

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

It should not be needed, Py_FatalError() is annotated with _Py_NO_RETURN. Same remark in sys_getoptionalattrstring().

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, removing.

}
}

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);
return NULL;
case 0:
assert(value == NULL);
return Py_NewRef(PyExc_AttributeError);
case 1:
return value;
default:
Py_FatalError("PySys_GetOptionalAttrString() returned invalid code");
Py_UNREACHABLE();
}
}

static PyObject *
sys_getobject(PyObject *Py_UNUSED(module), PyObject *arg)
{
Expand Down Expand Up @@ -3392,6 +3472,10 @@ static PyMethodDef TestMethods[] = {
{"function_set_kw_defaults", function_set_kw_defaults, METH_VARARGS, NULL},
{"check_pyimport_addmodule", check_pyimport_addmodule, METH_VARARGS},
{"test_weakref_capi", test_weakref_capi, METH_NOARGS},
{"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},
{NULL, NULL} /* sentinel */
Expand Down