gh-108512: Add and use new re 8000 placements for PySys_GetObject() #111035
Conversation
Add functions PySys_GetAttr(), PySys_GetAttrString(), PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().
There was a problem hiding this comment.
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
Outdated
| 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``. |
There was a problem hiding this comment.
I suggest to use a list and start with the return value, so it's easier to see the 3 cases:
| 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. |
There was a problem hiding this comment.
"Return and then set exception and variable" looks like a wrong sequence to me. It cannot do anything after returning.
There was a problem hiding this comment.
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 <vstinner@python.org>
Lib/test/test_capi/test_misc.py
Outdated
| with support.swap_attr(sys, '\U0001f40d', 42): | ||
| self.assertEqual(sys_getattr('\U0001f40d'), 42) | ||
|
|
||
| with self.assertRaisesRegex(RuntimeError, r'lost sys\.nonexisting'): |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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,).
Lib/test/test_capi/test_misc.py
Outdated
| # CRASHES sys_getattr(NULL) | ||
|
|
||
| def test_sys_getoptionalattr(self): | ||
| sys_getattr = _testcapi.sys_getoptionalattr |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Lib/test/test_capi/test_misc.py
Outdated
| self.assertEqual(sys_getattr('\U0001f40d'.encode()), 42) | ||
|
|
||
| self.assertIs(sys_getattr(b'nonexisting'), AttributeError) | ||
| self.assertRaises(UnicodeDecodeError, sys_getattr, b'\xff') |
There was a problem hiding this comment.
Can you add a UnicodeDecodeError to test_sys_getattr() as well?
There was a problem hiding this comment.
No, PySys_GetAttr() does not raise UnicodeDecodeError. The wrapper does, but we do not test PyArg_Parse() here.
Modules/_testcapimodule.c
Outdated
|
|
||
| switch (PySys_GetOptionalAttr(name, &value)) { | ||
| case -1: | ||
| assert(value == NULL); |
There was a problem hiding this comment.
I suggest adding: assert(PyErr_Occurred());. Same remark in sys_getoptionalattrstring().
There was a problem hiding this comment.
Isn't there a check that each function that returns NULL should also set an exception? I relied on this in all other tests.
Modules/_testcapimodule.c
Outdated
| return value; | ||
| default: | ||
| Py_FatalError("PySys_GetOptionalAttr() returned invalid code"); | ||
| Py_UNREACHABLE(); |
There was a problem hiding this comment.
It should not be needed, Py_FatalError() is annotated with _Py_NO_RETURN. Same remark in sys_getoptionalattrstring().
There was a problem hiding this comment.
Well, removing.
Modules/_testcapimodule.c
Outdated
| } | ||
| if (result == NULL) { | ||
| result = PyExc_AttributeError; | ||
| Py_INCREF(PyExc_AttributeError); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added it exactly to test that it never happens. But perhaps it can be removed.
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.
These are the words used in the description of the existing function
What exactly do you propose? Isn't it a point that we can suggest them as replacements for
Good point, I forgot about this. Done. |
Well, trying to get |
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. |
There was a problem hiding this comment.
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.
Doc/c-api/sys.rst
Outdated
| 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``. |
There was a problem hiding this comment.
Just say it in the opposite order in this case:
Set an exception, set *\*result* to ``NULL``, and return ``-1``, if an error occurred.
Doc/c-api/sys.rst
Outdated
| .. 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. |
There was a problem hiding this comment.
| 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. |
Include/cpython/sysmodule.h
Outdated
| @@ -0,0 +1,23 @@ | |||
| #ifndef Py_CPYTHON_SYSMODULE_H | |||
There was a problem hiding this comment.
Is this change related to the 4 new functions?
There was a problem hiding this comment.
No, it is a merging error.
Include/cpython/sysmodule.h
Outdated
| @@ -0,0 +1,23 @@ | |||
| #ifndef Py_CPYTHON_SYSMODULE_H | |||
There was a problem hiding this comment.
No, it is a merging error.
Add functions PySys_GetAttr(), PySys_GetAttrString(), PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().
📚 Documentation preview 📚: https://cpython-previews--111035.org.readthedocs.build/