GeezerVison is showing github.com — C API: Add a replacement for PySys_GetObject · Issue #108512 · python/cpython
C API: Add a replacement for PySys_GetObject #108512DescriptionFeature or enhancement
Proposal:
The PySys_GetObject() function has two flaws:
- It clears any error raised inside the function, including important and critical errors.
- It returns a borrowed reference.
We need to add a replacement free from these flaws. Any ideas about the API and the name?
Links to previous discussion of this feature:
Linked PRs
Activityvstinner commented on Sep 14, 2023 I propose the name PySys_GetAttr(). By the way, we already have a private _PySys_GetAttr() function.
The existing name PySys_GetObject() doesn't give any hint on the purpose of the function. It gets an object from where? Which object?
By the way, the special Py_TRACE_REFS debug build has a sys.getobjects() function which is unrelated: it gives the list of all Python objects :-) serhiy-storchaka commented on Sep 17, 2023 The name is only the part of the question. Other questions are:
- Should it return a borrowed or new reference?
- Should it raise an exception if
sys has no such attribute and what type of exception?
Currently PySys_GetObject() returns a borrowed reference and NULL if sys has no such attribute. But the code that calls PySys_GetObject() has different reaction on the returned NULL:
- Some code raises some exception.
- Some code returns error (and the caller of that code may expect an exception be set, but it is not in this case).
- Some code expects the attribute be optional and does not expect any exception. It would need to silence it if it was raised.
Case 2 looks like a bug. We should raise explicit exceptions in these cases. But what exceptions? encukou commented on Sep 18, 2023 IMO this is similar to the new PyDict_GetItemRef and PyObject_GetOptionalAttr and should have the same API. That is, int PySys_GetObjectStringRef(char *name, PyObject **result), returning
- -1: internal error or API misuse; exception raised
- 0: lookup succeeded; no item was found
- 1: lookup succeeded;
result set
It gets an object from where? Which object?
Well, from the sys module (see PySys_), and the one associated with the given key :) Reacted by Erlend E. Aasland vstinner commented on Sep 18, 2023 @encukou:
Well, from the sys module (see PySys_), and the one associated with the given key :)
I was a rhetorical question. I was just argument in favor of my proposed PySys_GetAttr() name :-) vstinner commented on Sep 18, 2023 @serhiy-storchaka: I'm interested to work on such API, but if you prefer to write it, just tell me.
For many years, PySys_GetObject() annoyed me, but I was not motivated enough to change it. For example, I dislike the fact that it returns a borrowed reference. I also dislikes the fact that it returns NULL if there is no attribute and if an error occurs. Also, it never raises any exception, it's bad in many ways :-( serhiy-storchaka commented on Sep 18, 2023
@serhiy-storchaka: I'm interested to work on such API, but if you prefer to write it, just tell me.
I have not started to write a code yet.
There is a similar function _PyEval_GetBuiltin() which is much more used. It is similar to PyObject_GetAttr() -- returns a new reference or NULL, and always sets error if returns NULL. This interface is more convenient when passed to Py_BuildValue("N").
I think that convenient functions for getting an attribute of sys and builtins should be in the public API, and they should have the same interface. One of differences is that PySys_GetObject() takes name as a C string, and it is never used in performance sensitive code, while _PyEval_GetBuiltin() takes name as a Python string, so we perhaps need two variants of each functions. serhiy-storchaka commented on Sep 20, 2023 I started to work on preliminary patch and found that PySys_GetObject() is used so little because more sites use private _PySys_GetAttr() which take a name as PyObject*. So now I have much more cases to consider. vstinner commented on Sep 21, 2023 We should add PySys_GetAttr() with PyObject name and PySys_GetAttrString() with char* name. It would be consistent with other APIs, no? serhiy-storchaka commented on Oct 18, 2023 There are at least 53 sites in CPython code that can use new functions (currently they use PySys_GetObject(), private functions _PySys_GetObject() and _PySys_GetAttr(), direct access to sysdict, _PyImport_GetModuleAttrString(), etc). Approximately 23 of them treat non-existing object as error, and 29 tolerates the absence. It is approximately because in some sites any errors are silenced, so any variant can be used. 20 sites pass the names as a C string, and 20 sites pass it as a Python string (_Py_ID()). Again, the difference is not strict, because every site that use literal C string can be reritten to use _Py_ID() and vice verse. _Py_ID() is faster, but in most cases it does not matter.
More detailed, per function:
11 PySys_GetAttr()
12 PySys_GetAttrString()
21 PySys_GetOptionalAttr()
8 PySys_GetOptionalAttrString()
All 4 variants are used in almost equal proportions. 4 new functions to replace PySys_GetObject() may be too much. We can get rid of Python string variants in favor of C string variants at some performance cost (but in most cases the difference is negligible). It is difficult to chose between "optional" and "required" variants, because when we left only one we will need to add either PyErr_ExceptionMatches(PyExc_RuntimeError)+PyErr_Clear() or PyErr_SetString(PyExc_RuntimeError, "...") in half of cases. Reacted by Erlend E. Aasland vstinner commented on Jan 27, 2025 Oh, I forgot about this old issue! I just created #129367 to propose adding PySys_GetAttr() function. Reacted by Erlend E. Aasland vstinner commented on Jan 27, 2025
All 4 variants are used in almost equal proportions. 4 new functions to replace PySys_GetObject() may be too much.
IMO PySys_GetOptionalAttrString functions are too much (adding 2 functions is already "a lot") and not worth it.
If you want to support optional attribute, you should expect AttributeError and ignore it (PyErr_Clear()). vstinner commented on Jan 28, 2025
IMO PySys_GetOptionalAttrString functions are too much (adding 2 functions is already "a lot") and not worth it.
I read the PR #111035 which replaces PySys_GetObject() with new functions and I changed my mind. There are many functions which do nothing if a sys attribute is missing: having to call PyErr_ExceptionMatches(PyExc_AttributeError) + PyErr_Clear() sounds annoying in this case. vstinner commented on Jun 3, 2025
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|