Skip to content

gh-131151: Refactor use of lookup_maybe_method to propagate lookup errors #131187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+116 −48
Open

gh-131151: Refactor use of lookup_maybe_method to propagate lookup errors #131187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43322b6
Refactor use of lookup_maybe_method to propagate lookup errors
kcdodd Mar 13, 2025
ef8e9ed
Add news, testing asserts
kcdodd Mar 13, 2025
2059c7f
fixup news, modify test assertions
kcdodd Mar 14, 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
8 changes: 8 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-03-13-16-21-09.gh-issue-131151.KaIHys.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Change the handling of errors raised from descriptor ``__get__``
accessing some special (dunder) methods.

* Raising ``AttributeError`` from ``__get__`` will continue to treat the method *as though it was not defined*, and special methods that had a fallback will continue to fallback.
* Other errors will be propagated as-is.
* Directly affects ``__eq__`` (and other comparisons), ``__hash__``, ``__repr__``, ``__iter__``.
* ``__del__`` will report as un-raisable.
* May affect ``__contains__``, ``__bool__``, ``__await__``, ``__aiter__``, ``__anext__``.
156 changes: 108 additions & 48 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ static PyObject *
slot_tp_new(PyTypeObject *type, PyObject *args, PyObject *kwds);

static PyObject *
lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound);
lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound, PyObject* exc_ignored);

static int
slot_tp_setattro(PyObject *self, PyObject *name, PyObject *value);
Expand Down Expand Up @@ -1163,13 +1163,13 @@ type_mro_modified(PyTypeObject *type, PyObject *bases) {
if (custom) {
PyObject *mro_meth, *type_mro_meth;
mro_meth = lookup_maybe_method(
(PyObject *)type, &_Py_ID(mro), &unbound);
if (mro_meth == NULL) {
(PyObject *)type, &_Py_ID(mro), &unbound, PyExc_AttributeError);
if (mro_meth == NULL || mro_meth == Py_NotImplemented) {
goto clear;
}
type_mro_meth = lookup_maybe_method(
(PyObject *)&PyType_Type, &_Py_ID(mro), &unbound);
if (type_mro_meth == NULL) {
(PyObject *)&PyType_Type, &_Py_ID(mro), &unbound, PyExc_AttributeError);
if (type_mro_meth == NULL || type_mro_meth == Py_NotImplemented) {
Py_DECREF(mro_meth);
goto clear;
}
Expand Down Expand Up @@ -2800,12 +2800,24 @@ _PyObject_LookupSpecialMethod(PyObject *self, PyObject *attr, PyObject **self_or
return res;
}

/* lookup_maybe_method returns NotImplemented if lookup fails
Exception 'exc_ignored' (if not NULL) is suppressed and converted to NotImplemented,
All other errors during lookup return NULL with the error set

NOTE: technically this doesn't check if the descriptor itself returned NotImplemented,
when that would have been an error since its not callable.
The logic is messier downstream to check combinations of NULL and PyErr_Occurred.
NotImplemented <-> res == NULL && !PyErr_Occurred()
Error <-> res == NULL && PyErr_Occurred()
*/
static PyObject *
lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound)
lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound, PyObject* exc_ignored)
{
PyObject *res = _PyType_LookupRef(Py_TYPE(self), attr);
assert(!PyErr_Occurred());

if (res == NULL) {
return NULL;
Py_RETURN_NOTIMPLEMENTED;
}

if (_PyType_HasFeature(Py_TYPE(res), Py_TPFLAGS_METHOD_DESCRIPTOR)) {
Expand All @@ -2817,16 +2829,32 @@ lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound)
descrgetfunc f = Py_TYPE(res)->tp_descr_get;
if (f != NULL) {
Py_SETREF(res, f(res, self, (PyObject *)(Py_TYPE(self))));

assert((!res)^(!PyErr_Occurred()));

if(res == NULL && exc_ignored && PyErr_ExceptionMatches(exc_ignored)){
// TODO: even though the docs say check before calling PyErr_ExceptionMatches,
// it seems like it could handle both being NULL?

// the descriptor caused exception, suppress only "exc_ignored" errors
PyErr_Clear();
Py_RETURN_NOTIMPLEMENTED;
}
}
}

return res;
}

/* lookup_method is complement of lookup_maybe_method
Does *not* supress any errors, and NotImplemented is converted to AttributeError
*/
static PyObject *
lookup_method(PyObject *self, PyObject *attr, int *unbound)
{
PyObject *res = lookup_maybe_method(self, attr, unbound);
if (res == NULL && !PyErr_Occurred()) {
PyObject *res = lookup_maybe_method(self, attr, unbound, NULL);
if (res == Py_NotImplemented) {
res = NULL;
PyErr_SetObject(PyExc_AttributeError, attr);
}
return res;
Expand Down Expand Up @@ -2891,11 +2919,10 @@ vectorcall_maybe(PyThreadState *tstate, PyObject *name,

int unbound;
PyObject *self = args[0];
PyObject *func = lookup_maybe_method(self, name, &unbound);
if (func == NULL) {
if (!PyErr_Occurred())
Py_RETURN_NOTIMPLEMENTED;
return NULL;
// TODO: PyExc_AttributeError shouldn't be ignored in this case for some reason?
PyObject *func = lookup_maybe_method(self, name, &unbound, PyExc_AttributeError);
if (func == NULL || func == Py_NotImplemented) {
return func;
}
PyObject *retval = vectorcall_unbound(tstate, unbound, func, args, nargs);
Py_DECREF(func);
Expand Down Expand Up @@ -9801,15 +9828,20 @@ slot_sq_contains(PyObject *self, PyObject *value)
PyObject *func, *res;
int result = -1, unbound;

func = lookup_maybe_method(self, &_Py_ID(__contains__), &unbound);
func = lookup_maybe_method(self, &_Py_ID(__contains__), &unbound, PyExc_AttributeError);
if (func == NULL){
return -1;
}

if (func == Py_None) {
Py_DECREF(func);
PyErr_Format(PyExc_TypeError,
"'%.200s' object is not a container",
Py_TYPE(self)->tp_name);
return -1;
}
if (func != NULL) {

if (func != Py_NotImplemented) {
PyObject *args[2] = {self, value};
res = vectorcall_unbound(tstate, unbound, func, args, 2);
Py_DECREF(func);
Expand All @@ -9818,7 +9850,7 @@ slot_sq_contains(PyObject *self, PyObject *value)
Py_DECREF(res);
}
}
else if (! PyErr_Occurred()) {
else {
/* Possible results: -1 and 1 */
result = (int)_PySequence_IterSearch(self, value,
PY_ITERSEARCH_CONTAINS);
Expand Down Expand Up @@ -9890,19 +9922,21 @@ slot_nb_bool(PyObject *self)
int result, unbound;
int using_len = 0;

func = lookup_maybe_method(self, &_Py_ID(__bool__), &unbound);
func = lookup_maybe_method(self, &_Py_ID(__bool__), &unbound, PyExc_AttributeError);
if (func == NULL) {
if (PyErr_Occurred()) {
return -1;
}

if (func == Py_NotImplemented) {
func = lookup_maybe_method(self, &_Py_ID(__len__), &unbound, PyExc_AttributeError);
if (func == NULL) {
return -1;
}

func = lookup_maybe_method(self, &_Py_ID(__len__), &unbound);
if (func == NULL) {
if (PyErr_Occurred()) {
return -1;
}
if (func == Py_NotImplemented) {
return 1;
}

using_len = 1;
}

Expand Down Expand Up @@ -9982,13 +10016,17 @@ slot_tp_repr(PyObject *self)
PyObject *func, *res;
int unbound;

func = lookup_maybe_method(self, &_Py_ID(__repr__), &unbound);
if (func != NULL) {
func = lookup_maybe_method(self, &_Py_ID(__repr__), &unbound, PyExc_AttributeError);
if (func == NULL) {
return NULL;
}

if (func != Py_NotImplemented) {
res = call_unbound_noarg(unbound, func, self);
Py_DECREF(func);
return res;
}
PyErr_Clear();

return PyUnicode_FromFormat("<%s object at %p>",
Py_TYPE(self)->tp_name, self);
}
Expand All @@ -10002,13 +10040,12 @@ slot_tp_hash(PyObject *self)
Py_ssize_t h;
int unbound;

func = lookup_maybe_method(self, &_Py_ID(__hash__), &unbound);

if (func == Py_None) {
Py_SETREF(func, NULL);
func = lookup_maybe_method(self, &_Py_ID(__hash__), &unbound, PyExc_AttributeError);
if (func == NULL) {
return -1;
}

if (func == NULL) {
if (func == Py_None || func == Py_NotImplemented) {
return PyObject_HashNotImplemented(self);
}

Expand Down Expand Up @@ -10192,10 +10229,9 @@ slot_tp_richcompare(PyObject *self, PyObject *other, int op)
PyThreadState *tstate = _PyThreadState_GET();

int unbound;
PyObject *func = lookup_maybe_method(self, name_op[op], &unbound);
if (func == NULL) {
PyErr_Clear();
Py_RETURN_NOTIMPLEMENTED;
PyObject *func = lookup_maybe_method(self, name_op[op], &unbound, PyExc_AttributeError);
if (func == NULL || func == Py_NotImplemented){
return func;
}

PyObject *stack[2] = {self, other};
Expand All @@ -10210,7 +10246,11 @@ slot_tp_iter(PyObject *self)
int unbound;
PyObject *func, *res;

func = lookup_maybe_method(self, &_Py_ID(__iter__), &unbound);
func = lookup_maybe_method(self, &_Py_ID(__iter__), &unbound, PyExc_AttributeError);
if (func == NULL) {
return NULL;
}

if (func == Py_None) {
Py_DECREF(func);
PyErr_Format(PyExc_TypeError,
Expand All @@ -10219,15 +10259,18 @@ slot_tp_iter(PyObject *self)
return NULL;
}

if (func != NULL) {
if (func != Py_NotImplemented) {
res = call_unbound_noarg(unbound, func, self);
Py_DECREF(func);
return res;
}

PyErr_Clear();
func = lookup_maybe_method(self, &_Py_ID(__getitem__), &unbound);
func = lookup_maybe_method(self, &_Py_ID(__getitem__), &unbound, PyExc_AttributeError);
if (func == NULL) {
return NULL;
}

if (func == Py_NotImplemented) {
PyErr_Format(PyExc_TypeError,
"'%.200s' object is not iterable",
Py_TYPE(self)->tp_name);
Expand Down Expand Up @@ -10346,8 +10389,13 @@ slot_tp_finalize(PyObject *self)
PyObject *exc = PyErr_GetRaisedException();

/* Execute __del__ method, if any. */
del = lookup_maybe_method(self, &_Py_ID(__del__), &unbound);
if (del != NULL) {
del = lookup_maybe_method(self, &_Py_ID(__del__), &unbound, PyExc_AttributeError);
if (del == NULL) {
PyErr_FormatUnraisable(
"Exception ignored while getting deallocator for %s object",
Py_TYPE(self)->tp_name);

}else if (del != Py_NotImplemented) {
res = call_unbound_noarg(unbound, del, self);
if (res == NULL) {
PyErr_FormatUnraisable("Exception ignored while "
Expand Down Expand Up @@ -10611,8 +10659,12 @@ slot_am_await(PyObject *self)
int unbound;
PyObject *func, *res;

func = lookup_maybe_method(self, &_Py_ID(__await__), &unbound);
if (func != NULL) {
func = lookup_maybe_method(self, &_Py_ID(__await__), &unbound, PyExc_AttributeError);
if (func == NULL){
return NULL;
}

if (func != Py_NotImplemented) {
res = call_unbound_noarg(unbound, func, self);
Py_DECREF(func);
return res;
Expand All @@ -10629,8 +10681,12 @@ slot_am_aiter(PyObject *self)
int unbound;
PyObject *func, *res;

func = lookup_maybe_method(self, &_Py_ID(__aiter__), &unbound);
if (func != NULL) {
func = lookup_maybe_method(self, &_Py_ID(__aiter__), &unbound, PyExc_AttributeError);
if (func == NULL){
return NULL;
}

if (func != Py_NotImplemented) {
res = call_unbound_noarg(unbound, func, self);
Py_DECREF(func);
return res;
Expand All @@ -10647,8 +10703,12 @@ slot_am_anext(PyObject *self)
int unbound;
PyObject *func, *res;

func = lookup_maybe_method(self, &_Py_ID(__anext__), &unbound);
if (func != NULL) {
func = lookup_maybe_method(self, &_Py_ID(__anext__), &unbound, PyExc_AttributeError);
if (func == NULL){
return NULL;
}

if (func != Py_NotImplemented) {
res = call_unbound_noarg(unbound, func, self);
Py_DECREF(func);
return res;
Expand Down
Loading