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

serhiy-storchaka · 2023-10-18T12:11:24Z

Add functions PySys_GetAttr(), PySys_GetAttrString(), PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().

Issue: C API: Add a replacement for PySys_GetObject #108512

📚 Documentation preview 📚: https://cpython-previews--111035.org.readthedocs.build/

Add functions PySys_GetAttr(), PySys_GetAttrString(), PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().

vstinner

Would it be possible to leave changes to use these changes aside, in a separated PR, so we can focus only on the API, doc and tests on the new functions?

The function is called "GetAttr" but in the doc, you wrote "if the object exists". IMO "if the attribute exists" is more appropriate.

Is it required to add these new functions to the stable ABI in Python 3.13? Can we wait one Python release to see how it does, before add them to the stable ABI?

Would it be possible to add tests?

Doc/c-api/sys.rst

vstinner · 2023-10-18T16:05:06Z

Doc/c-api/sys.rst

+   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``.


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.

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

Just say it in the opposite order in this case:

Set an exception, set *\*result* to ``NULL``, and return ``-1``, if an error occurred.

Co-authored-by: Victor Stinner <[email protected]>

vstinner · 2023-10-19T08:18:53Z

Lib/test/test_capi/test_misc.py

+        with support.swap_attr(sys, '\U0001f40d', 42):
+            self.assertEqual(sys_getattr('\U0001f40d'), 42)
+
+        with self.assertRaisesRegex(RuntimeError, r'lost sys\.nonexisting'):


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'

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,).

vstinner · 2023-10-19T08:21:06Z

Lib/test/test_capi/test_misc.py

+        # CRASHES sys_getattr(NULL)
+
+    def test_sys_getoptionalattr(self):
+        sys_getattr = _testcapi.sys_getoptionalattr


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.

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.

vstinner · 2023-10-19T08:21:51Z

Lib/test/test_capi/test_misc.py

+            self.assertEqual(sys_getattr('\U0001f40d'.encode()), 42)
+
+        self.assertIs(sys_getattr(b'nonexisting'), AttributeError)
+        self.assertRaises(UnicodeDecodeError, sys_getattr, b'\xff')


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

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

vstinner · 2023-10-19T08:23:50Z

Modules/_testcapimodule.c

+
+    switch (PySys_GetOptionalAttr(name, &value)) {
+        case -1:
+            assert(value == NULL);


I suggest adding: assert(PyErr_Occurred());. Same remark in sys_getoptionalattrstring().

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

vstinner · 2023-10-19T08:24:20Z

Modules/_testcapimodule.c

+            return value;
+        default:
+            Py_FatalError("PySys_GetOptionalAttr() returned invalid code");
+            Py_UNREACHABLE();


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

Well, removing.

vstinner · 2023-10-19T08:26:27Z

Modules/_testcapimodule.c

+    }
+    if (result == NULL) {
+        result = PyExc_AttributeError;
+        Py_INCREF(PyExc_AttributeError);


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.

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

serhiy-storchaka · 2023-10-19T08:31:24Z

Would it be possible to leave changes to use these changes aside, in a separated PR, so we can focus only on the API, doc and tests on the new functions?

It is easy to see the effect of these changes if they are in a single PR. Also, it replaces old private functions with new public API that is impossible if they are still used. If you prefer, I will split this PR into two parts after the development of the new API is complete, immediately before merging.

The function is called "GetAttr" but in the doc, you wrote "if the object exists". IMO "if the attribute exists" is more appropriate.

These are the words used in the description of the existing function PySys_GetObject(). Module attributes are rarely referred to as "attributes". They are more often referred as module variables, module constants, module globals. Sphynx has a special role :data: for this instead of more general :attr:.

Is it required to add these new functions to the stable ABI in Python 3.13? Can we wait one Python release to see how it does, before add them to the stable ABI?

What exactly do you propose? Isn't it a point that we can suggest them as replacements for PySys_GetObject() and deprecate the latter in distant future? Should not all new public API be in the stable ABI or not be public at all?

Would it be possible to add tests?

Good point, I forgot about this. Done.

vstinner · 2023-10-19T08:39:25Z

Module attributes are rarely referred to as "attributes".

Well, trying to get sys.x raises an AttributeError, not a NameError :-)

vstinner · 2023-10-19T08:41:13Z

Should not all new public API be in the stable ABI or not be public at all?

I'm asking if we should only add the API to Include/cpython/ in Python 3.13, and wait for Python 3.14 to add it to the limited API. Just in case if something goes wrong, if the API changes when we notice issues. If it lands directly in the stable ABI, we can no longer change it, it's too late.

… into capi-PySys_GetAttr

vstinner

About the exception, I see two options:

We consider that we get an object from a namespace, similar to LOAD_GLOBAL, and so NameError should be raised. IMO the function should be called PySys_GetVar() in this case. Example: https://docs.python.org/dev/c-api/frame.html#c.PyFrame_GetVar raises NameError.
Or we consider that we are getting an attribute, PySys_GetAttr() name is good, and AttributeError should be raised in this case.

For me, RuntimeError is just meaningless and it should be avoided. RuntimeError means everything and nothing: "something failed". Thanks Python...

In Python, usually I consider that the sys module is an object and I get sys attributes with getattr(sys, "stdout") which raises AttributeError.

In Python, I'm not even sure how to treat sys as a namespace. sys.__dict__['stdout'] raises KeyError, not NameError.

serhiy-storchaka · 2025-01-28T11:33:38Z

Many of existing uses of PySys_GetObject() and _PySys_GetAttr() raise RuntimeError (if they raise an exception at all). None raises AttributeError. If you want an AttributeError, use PyObject_GetAttr(). I do not think there are many cases for this.

Doc/c-api/sys.rst

… into capi-PySys_GetAttr

vstinner · 2025-05-21T19:58:42Z

Doc/c-api/sys.rst

+   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``.


Just say it in the opposite order in this case:

Set an exception, set *\*result* to ``NULL``, and return ``-1``, if an error occurred.

Modules/_testlimitedcapi/sys.c

Python/sysmodule.c

vstinner · 2025-05-21T20:09:07Z

Doc/c-api/sys.rst

+.. 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.


Suggested change

Raise :exc:`RuntimeError` and return ``NULL`` if it does not exist.

Raise :exc:`RuntimeError` and return ``NULL`` if it does not exist or if the :mod:`sys` module cannot be found.

vstinner · 2025-05-21T20:12:52Z

Include/cpython/sysmodule.h

@@ -0,0 +1,23 @@
+#ifndef Py_CPYTHON_SYSMODULE_H


Is this change related to the 4 new functions?

No, it is a merging error.

Include/sysmodule.h

serhiy-storchaka

Thank you for review @vstinner.

serhiy-storchaka · 2025-05-22T08:22:13Z

Include/cpython/sysmodule.h

@@ -0,0 +1,23 @@
+#ifndef Py_CPYTHON_SYSMODULE_H


No, it is a merging error.

Include/sysmodule.h

Modules/_testlimitedcapi/sys.c

vstinner

LGTM

Modules/_testlimitedcapi/sys.c

pythongh-108512: Add and use new replacements for PySys_GetObject()

4d0f508

Add functions PySys_GetAttr(), PySys_GetAttrString(), PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().

serhiy-storchaka added the topic-C-API label Oct 18, 2023

serhiy-storchaka requested a review from vstinner October 18, 2023 12:11

serhiy-storchaka requested review from kumaraditya303 and iritkatriel as code owners October 18, 2023 12:11

bedevere-app bot mentioned this pull request Oct 18, 2023

C API: Add a replacement for PySys_GetObject #108512

Open

bedevere-app bot added the awaiting core review label Oct 18, 2023

Update Misc/stable_abi.toml

eb42b39

serhiy-storchaka requested review from a team and encukou as code owners October 18, 2023 14:10

Merge branch 'main' into capi-PySys_GetAttr

2d4588d

vstinner reviewed Oct 18, 2023

View reviewed changes

serhiy-storchaka and others added 2 commits October 19, 2023 10:57

Add tests.

2eb9533

Apply suggestions from code review

9fc2f3d

Co-authored-by: Victor Stinner <[email protected]>

vstinner reviewed Oct 19, 2023

View reviewed changes

serhiy-storchaka added 5 commits October 19, 2023 12:00

Address review comments.

65713ce

Check that the name is a string.

e6ecf11

Merge remote-tracking branch 'refs/remotes/origin/capi-PySys_GetAttr'…

8a0f5f2

… into capi-PySys_GetAttr

Make the new C API not public.

516829e

Remove from Misc/stable_abi.toml.

9503aaf

vstinner reviewed Oct 20, 2023

View reviewed changes

serhiy-storchaka marked this pull request as draft January 10, 2024 14:25

bedevere-app bot removed the awaiting core review label Jan 10, 2024

serhiy-storchaka added 4 commits January 28, 2025 12:00

Merge branch 'main' into capi-PySys_GetAttr

e2857ef

Add to the limited C API.

dc26ec2

Replace few new occurrences of PySys_GetObject().

104dcc2

Update the documentation.

cf75fc3

serhiy-storchaka added 2 commits January 28, 2025 12:47

Clean up.

9434659

Fix a typo.

56639d8

vstinner reviewed Jan 28, 2025

View reviewed changes

Doc/c-api/sys.rst Outdated Show resolved Hide resolved

serhiy-storchaka marked this pull request as ready for review January 28, 2025 11:38

serhiy-storchaka requested review from ericsnowcurrently, FFY00 and markshannon as code owners January 28, 2025 11:38

bedevere-app bot added the awaiting core review label Jan 28, 2025

serhiy-storchaka mentioned this pull request Jan 28, 2025

Add replacements for PySys_GetObject() capi-workgroup/decisions#54

Open

vstinner mentioned this pull request Jan 28, 2025

gh-129367: Add PySys_GetAttr() function #129369

Closed

serhiy-storchaka added 3 commits February 6, 2025 16:33

Merge branch 'main' into capi-PySys_GetAttr

b40a665

Merge branch 'main' into capi-PySys_GetAttr

03b9c0a

Merge remote-tracking branch 'refs/remotes/origin/capi-PySys_GetAttr'…

5d793c5

… into capi-PySys_GetAttr

serhiy-storchaka mentioned this pull request Feb 6, 2025

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

Draft

kumaraditya303 removed their request for review February 21, 2025 18:15

This was referenced Feb 24, 2025

gh-130163: Fix possible crashes related to PySys_GetObject() #130503

Merged

Crash when concurrently writing with print and concurrently modifying sys.stdout #130163

Closed

Merge branch 'main' into capi-PySys_GetAttr

09869ed

serhiy-storchaka marked this pull request as draft May 4, 2025 16:22

bedevere-app bot removed the awaiting core review label May 4, 2025

serhiy-storchaka added 2 commits May 21, 2025 22:02

Merge branch 'main' into capi-PySys_GetAttr

439bc3c

Move to 3.15.

93ab31b

vstinner reviewed May 21, 2025

View reviewed changes

serhiy-storchaka added 2 commits May 22, 2025 11:47

Address review comments.

154a82a

Improve tests.

81c7605

serhiy-storchaka commented May 22, 2025

View reviewed changes

vstinner approved these changes May 22, 2025

View reviewed changes

Modules/_testlimitedcapi/sys.c Show resolved Hide resolved

bedevere-app bot added the awaiting merge label May 22, 2025

	Raise :exc:`RuntimeError` and return ``NULL`` if it does not exist.
	Raise :exc:`RuntimeError` and return ``NULL`` if it does not exist or if the :mod:`sys` module cannot be found.

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

serhiy-storchaka commented Oct 18, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Oct 19, 2023

Uh oh!

vstinner commented Oct 19, 2023

Uh oh!

vstinner commented Oct 19, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jan 28, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Oct 18, 2023 •

edited by github-actions bot

Loading