Subtle error in `getattr_hook` example in the Descriptor Guide

[LunaRood]

Documentation

The example below, from the Descriptor Guide, does not seem to reflect the behaviour of the actual implementation.

cpython/Doc/howto/descriptor.rst

Lines 733 to 740 in ced2691

	def getattr_hook(obj, name):
	"Emulate slot_tp_getattr_hook() in Objects/typeobject.c"
	try:
	return obj.__getattribute__(name)
	except AttributeError:
	if not hasattr(type(obj), '__getattr__'):
	raise
	return type(obj).__getattr__(obj, name) # __getattr__

Unless I'm mistaken, line 736 return obj.__getattribute__(name) should actually read return type(obj).__getattribute__(obj, name).

It's easy to miss when thinking of class instances, but the issue becomes more evident when the example is applied to an attribute lookup on a class object instead. The example would call the __getattribute__ method of the class itself, while the actual implementation calls __getattribute__ on the metaclass (not to mention that in this case the call in the example would fail, due to a missing argument, as it is calling a plain function, rather than a bound method).

[LunaRood]

As an aside, even in the case of an attribute lookup on a class instance, where (potential recursion issues and instance attributes aside) the example could, at a glance, appear to match the actual behaviour, it would be relying on a descriptor invocation to call __getattribute__ as a bound method, which makes the example rather circular, in an amusing way.

[rhettinger]

Given:

class M(type):
    def __getattribute__(cls, name):
        print('M', name)
        return type.__getattribute__(cls, name)

class C(object, metaclass=M):
    def __getattribute__(self, name):
        print('C', name)
        return object.__getattribute__(self, name)
    def __getattr__(self, name):
        return 'GA' + name

def getattr_hook(obj, name):
    try:
        return obj.__getattribute__(name)
    except AttributeError:
        if not hasattr(type(obj), '__getattr__'):
            raise
    return type(obj).__getattr__(obj, name)

def getattr_hook_proposed(obj, name):
    try:
        return type(obj).__getattribute__(obj, name)
    except AttributeError:
        if not hasattr(type(obj), '__getattr__'):
            raise
    return type(obj).__getattr__(obj, name)
    
c = C()
c.x = 20

Compare the following calls:

>>> getattr(c, 'x')
C x
20
>>> getattr_hook(c, 'x')
C __getattribute__
C x
20
>>> getattr_hook_proposed(c, 'x')
M __getattribute__
C x
20

When starting with an instance (the focus of this section), it looka like proposed edit adds an unexpected jump to the metaclass with the current code and actual getattr() function call do not.

The C code is convoluted and has alternative code paths depending on whether __getattr__ is defined. It does however has this call on the simple path:

    if (getattr == NULL) {
        /* No __getattr__ hook: use a simpler dispatcher */
        tp->tp_getattro = _Py_slot_tp_getattro;
        return _Py_slot_tp_getattro(self, name);
    }

which jumps to:

PyObject *
_Py_slot_tp_getattro(PyObject *self, PyObject *name)
{
    PyObject *stack[2] = {self, name};
    return vectorcall_method(&_Py_ID(__getattribute__), stack, 2);
}

I don't see a jump from obj to type(obj) at this stage.

[LunaRood]

This ended up getting much more lengthy than I hoped, for which I apologise, but I felt it necessary to make my case thoroughly, as this is a fairly nuanced area of Python.

In my report, I used the example of class objects (rather than instances), as it most clearly demonstrates the issue, however, I'll explain below why the example is fundamentally wrong, and how this issue has subtle implications in the case of instances as well. Additionally, I'll argue why having the correct behaviour for class objects is also relevant and important in that part of the documentation.

Overview

That call to __getattribute__ on M is indeed slightly misleading, however, I'd say that your code actually demonstrates that both variants are equally misleading, since the call to __getattribute__ on C also should not happen (and is arguably more problematic), as the actual implementation looks __getattribute__ up directly, without itself going through __getattribute__ (this avoids recursive lookups).

In both versions of the example, the dot lookup for __getattribute__ in getattr_hook fundamentally is not (and cannot be) representative of the actual C implementation, which uses a specialised lookup mechanism here. Ultimately, as you pointed out, the C code is complex, and the example cannot be fully representative, so compromises need to be made.

But the key point that is made clear with my proposed change, is that __getattribute__ is called on type(obj) rather than on obj itself (I'll justify this below, both by looking at live Python code, as well as a breakdown of the C implementation). Perhaps that last bit of clarity and accuracy could be achieved by means of a comment, noting that the lookup of __getattribute__ itself within getattr_hook is special, and bypasses the standard lookup mechanism.

Additionally, as far as inaccuracies go, I'd argue that deferring to the metaclass to look up __getattribute__ is a much better compromise than having the __getattribute__ lookup go through the class's own __getattribute__, as the user is much more likely to implement a custom __getattribute__ on their class, than to use a custom metaclass. The only case where the output of my proposed version does not match native behaviour is if a custom metaclass with a custom __getattribute__ is used. In all other cases, it will result in the expected outcome.

Python Examples

With my proposed change, not only is it arguably more logically representative, but also, the result of a lookup is at least correct in almost all major cases (with the only exception being cases where __getattribute__ on the metaclass does something unexpected as noted above, but then again, the same is true in the current version if __getattribute__ on the class does something unexpected). On the other hand, the current version behaves incorrectly in a much larger number of cases, as I'll demonstrate here.

Given:

class M(type):
    def __getattribute__(cls, name):
        print('M', name)
        return type.__getattribute__(cls, name)

class C(object, metaclass=M):
    def __getattribute__(self, name):
        print('C', name)
        return object.__getattribute__(self, name)
    def __getattr__(self, name):
        return 'GA' + name
    @staticmethod
    def method():
    	pass

def getattr_hook(obj, name):
    try:
        return obj.__getattribute__(name)
    except AttributeError:
        if not hasattr(type(obj), '__getattr__'):
            raise
    return type(obj).__getattr__(obj, name)

def getattr_hook_class(obj, name):
    try:
        return obj.__getattribute__(obj, name)
    except AttributeError:
        if not hasattr(type(obj), '__getattr__'):
            raise
    return type(obj).__getattr__(obj, name)

def getattr_hook_proposed(obj, name):
    try:
        return type(obj).__getattribute__(obj, name)
    except AttributeError:
        if not hasattr(type(obj), '__getattr__'):
            raise
    return type(obj).__getattr__(obj, name)

c = C()
c.x = 20
c.__getattribute__ = lambda *args: print(f"c {args}")

We can make the following comparison:

>>> getattr(c, "x")
C x
20
>>> getattr_hook(c, "x")
C __getattribute__
c ('x',)
>>> getattr_hook_proposed(c, "x")
M __getattribute__
C x
20

Yes, overriding __getattribute__ on the instance is admittedly contrived, however, it demonstrates that the actual C implementation bypasses the instance, and performs the call on the class's __getattribute__.

As for the behaviour with classes, as you said, it is not the focus of that section, however, that is only relevant to the object_getattribute example, and not the getattr_hook example. That is because (as far as I can tell) there is no significant functional difference in the various code paths for getattr, while there is a significant difference in the __getattribute__ implementation of type vs object (which is indeed, in part, mentioned in the next section, regarding invocation from a class). In fact, those sections could be reorganised altogether, and the getattr_hook example could be moved to the general "Overview of descriptor invocation" section above, to reflect that this is not part of the specialised logic for instances. Though making this change would make it difficult to keep the flow of the explanation well structured.

With my change the example works universally, which reflects the C code, as it doesn't actually make a distinction here (note that the example is meant to reflect slot_tp_getattr_hook in Objects/typeobject.c, which is where the generic logic is defined).

For completeness, here's an example on classes, which demonstrates some of the inconsistencies with the current version of the example code (getattr_hook_class fixes the arguments in the __getattribute__ call, to demonstrate the behaviour when we avoid the exception):

>>> getattr(C, "method")
M method
<function C.method at 0x743b411c0a40>
>>> getattr_hook(C, "method")
M __getattribute__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in getattr_hook
TypeError: C.__getattribute__() missing 1 required positional argument: 'name'
>>> getattr_hook_class(C, "method")
M __getattribute__
C method
<staticmethod(<function C.method at 0x743b411c0a40>)>
>>> getattr_hook_proposed(C, "method")
M method
<function C.method at 0x743b411c0a40>

This shows that the current version breaks the descriptor invocation, as it calls __getattribute__ on the class itself, which (as expected) doesn't resolve its own descriptors (unless an instance is passed to it, which is what it expects). This is because only the __getattribute__ implementation of type (not object) resolves descriptors on the passed object itself, rather than its type.

I would say this is a significant issue in the documentation, since the same documentation page describes implementations of descriptors such as staticmethod and classmethod, which simply do not work with the given explanation of attribute lookup and descriptor invocation.

C Code Analysis

Indeed, as you pointed out, getattr may end up following several code paths, but ultimately it should generally end up in either _Py_slot_tp_getattro or _Py_slot_tp_getattr_hook (quick edit for the sake of pedantry: yes, I am aware of the cases where PyObject_GenericGetAttr gets hooked directly into tp_getattro, but that's not really relevant, as it circumvents the whole __getattribute__ lookup altogether, making it moot for our purposes. Additionally I'll also point out that PyObject_GetAttr, which decides which slot to call, also operates on type(obj).).

When looking at _Py_slot_tp_getattr_hook, it is obvious that the implementation looks up __getattribute__ on type(obj).

_Py_slot_tp_getattr_hook(PyObject *self, PyObject *name)
{
    PyTypeObject *tp = Py_TYPE(self);
    PyObject *getattr, *getattribute, *res;
    ...
    getattribute = _PyType_LookupRef(tp, &_Py_ID(__getattribute__));
    ...

As you pointed out, if __getattr__ is not present, it jumps to a simpler code path. There is indeed no explicit jump to the type in _Py_slot_tp_getattro itself, however, if we follow the calls downstream, we find that the lookup for __getattribute__ ultimately takes place in lookup_maybe_method, where we do find the jump to type(obj):

lookup_maybe_method(PyObject *self, PyObject *attr, int *unbound)
{
    PyObject *res = _PyType_LookupRef(Py_TYPE(self), attr);
    ...

Conclusion

I hope that this extensive analysis demonstrates why my proposed change is not only more logically sound and clearer in explaining how this mechanism works, but also is more faithful to the C implementation.

I really hope this change can be implemented. It took me a long time and much confusion trying to understand how this mechanism works, as I couldn't square the actual observed behaviour with what was documented, and assumed my understanding must have been flawed. It was only when I finally decided to look into the C implementation that I realised that it was an issue with the documentation all along.

Since this is such a nuanced area of Python, with many subtleties, I hope this kind of confusion can be avoided for others in the future, as there is already enough to be confused about when getting into the weeds in this area.