Functions must not suppress exceptions

[serhiy-storchaka]

The following functions suppress arbitrary exceptions raised inside. In some cases they can also clear exceptions raised before the call.

• PyDict_GetItem and PyDict_GetItemString
• PyMapping_HasKey and PyMapping_HasKeyString
• PyObject_HasAttr and PyObject_HasAttrString
• PySys_GetObject

They can call Python code (via __hash__, __eq__, __getattr__ or __getattribute__) which can raise arbitrary exception which will be silenced anyway. PyDict_GetItem has a counterpart which keeps exception (PyDict_GetItemWithError), but other functions do not have any.

These function should not be used in modern code, except in cases in which any error is silenced anyway (and it is bad that we still have such places). They should be deprecated in future, both at compile time and at run time. But first we should introduce replacements. Should we just introduce new functions with WithError suffix? In CPython code all occurrences of PyObject_HasAttr were replaced with _PyObject_LookupAttr.

Historical reference: hasattr() never raised exceptions in Python 2.

[serhiy-storchaka]

Yes, PyIter_Next is unrelated.

[vstinner]

It was unclear to me what was the discussed problem, so I changed the title to: "Functions must not suppress exceptions". Is it what you mean?

Is the problem that if a function is called with an exception set, the function can supress it?

[serhiy-storchaka]

The problem is that if an unexpected exception is raised inside a function, it should be reported to the caller. Functions mentioned above have no way to do this, and leaving exception set after the function would break an existing code down the line.

We should introduce the replacement C API. For example, PyObject_GetAttr() can be used instead of PyObject_HasAttr().

if (PyObject_HasAttr(obj, attr_name)) {
    // has attribute
}
else {
    // does not have attribute
}

should be correctly rewritten as

PyObject *dummy = PyObject_GetAttr(obj, attr_name);
if (dummy != NULL) {
    Py_DECREF(dummy);
    // has attribute
}
else if (PyErr_ExceptionMatches(PyExc_AttributeError)) {
    PyErr_Clear();
    // does not have attribute
}
else {
    // error
}

The new function PyObject_GetOptionalAttr() is more convenient, allowing to get rid of PyErr_ExceptionMatches(PyExc_AttributeError) and PyErr_Clear():

PyObject *dummy;
int rc = PyObject_GetOptionalAttr(obj, attr_name, &dummy);
if (rc < 0) {
    // error
}
else if (rc) {
    Py_DECREF(dummy);
    // has attribute
}
else {
    // does not have attribute
}

Perhaps it is worth to introduce yet one new function PyObject_HasAttrEx() to get rid of the dummy variable:

int rc = PyObject_HasAttrEx(obj, attr_name);
if (rc < 0) {
    // error
}
else if (rc) {
    // has attribute
}
else {
    // does not have attribute
}

[encukou]

Another option is to allow the result pointer to be NULL, in which case it won't be set/incref'd.
(Unlike an “input” argument, users aren't likely to pass NULL by mistake here.)

int rc = PyObject_GetOptionalAttr(obj, attr_name, NULL);
if (rc < 0) {
    // error
}
else if (rc) {
    // has attribute
}
else {
    // does not have attribute
}

The old Lookup name suggestion would fit this case better, though.

[serhiy-storchaka]

It would add a tiny overhead for every use of PyObject_GetOptionalAttr():

    if (result != NULL) {
        result = value;
    }

instead of simply

    result = value;

[encukou]

Will that even be measurable?

Edit: We should find the best API for users, then make it fast.
If performance matters here, IMO we should make the function (or relevant parts of it) inline and let the compiler sort it out. (Obviously, this'll help C/C++ and version-specific ABI only, but IMO that's fine).
cc @markshannon

[vstinner]

I would prefer to store the information "has an attibute" in a separated variable:

int has_attr;
if (PyObject_HasAttrEx(obj, attr_name, &has_attr) < 0) {
    // error
    return -1;
}

if (has_attr) {
    // has attribute
}
else {
    // does not have attribute
}

About the Ex suffix, I created #52 about naming convention to add new C API functions. So far, I didn't see a clear consensus.

[vstinner]

int rc = PyObject_GetOptionalAttr(obj, attr_name, NULL);

Hum, I dislike passing NULL to a function to get a different behavior. It reminds me #47 problem (even if here it's a little bit different).

I would prefer adding new functions for "hasattr".

[vstinner]

If you change PySys_GetObject(), please return a new strong reference in the new API.

[encukou]

Hum, I dislike passing NULL to a function to get a different behavior. It reminds me #47 problem (even if here it's a little bit different).

IMO, the main point of #47 -- "what if creating a value fails and the code pass NULL instead of a Python object" -- doesn't apply at all here. Is there any other danger?

[encukou]

Proposed guideline issue: capi-workgroup/api-evolution#35

[vstinner]

Proposed guideline issue: capi-workgroup/api-evolution#35

Let's continue the discussion there, I close the issue.

@serhiy-storchaka added multiple variants of listed functions which don't suppress exceptions anymore.

By the way, see also capi-workgroup/decisions#21 about PyIter_Next().