str(weakref) throws error KeyError: '__name__' if the original obj override __getattr__

[yanboliang]

Bug report

A clear and concise description of what the bug is.
Include a minimal, reproducible example (https://stackoverflow.com/help/minimal-reproducible-example), if possible.

Repo:

class MyConfig(dict):
    def __getattr__(self, x):
        return self[x]

obj = MyConfig(offset=5)
obj_weakref = weakref.ref(obj)
str(obj_weakref)  # raise error: KeyError: '__name__'

Error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __getattr__
KeyError: '__name__'

Your environment

• CPython versions tested on: python 3.8 & 3.9
• Operating system and architecture: Ubuntu Linux and MacOS

• PR: gh-99184: Bypass instance attribute access in repr of weakref.ref #99244

• PR: [3.11] gh-99184: Bypass instance attribute access in repr of weakref.ref (GH-99244) #103789

[sobolevn]

This does not look like a bug to me.
What do you expect to happen?

Right now weakrefobject.c has these lines:

static PyObject *
weakref_repr(PyWeakReference *self)
{
    PyObject *name, *repr;
    PyObject* obj = PyWeakref_GET_OBJECT(self);

    if (obj == Py_None) {
        return PyUnicode_FromFormat("<weakref at %p; dead>", self);
    }

    Py_INCREF(obj);
    if (_PyObject_LookupAttr(obj, &_Py_ID(__name__), &name) < 0) {
        Py_DECREF(obj);
        return NULL;
    }
    // ...

It tries to get __name__ attribute from the passed object. Since __getattr__ tries to get __name__ from self - it fails with a KeyError. It works exactly as it is written.

To fix this you can do something like:

class MyConfig(dict):
    def __getattr__(self, x):
        if x in {'__name__', '__class__', 'whatever_else'}:
             return getattr(type(self), x)
        return self[x]

[yanboliang]

@sobolevn Thanks for your quick response! I do agree this works well in functionality's perspective, but I'd like to say if this is a good user experience. If users are just trying to override __getattr__ to implement their own functionality(like what I'm doing in the example), it means they should handle a lot of other python internal stuff at well. I think this would sort of increase the complexity, and make users can't focus on their own functionality.

[sobolevn]

So, probably you are asking from some kind of fallback for cases like this. Please, correct me if I am wrong :)

I think that repr() of weakref.ref would have a default value if _PyObject_LookupAttr(obj, &_Py_ID(__name__), &name) fails.

Example: <weakref at 0x1085b5800; to __some_default__ at 0x7ffd1071c080 (...)>

However, I don't think this is a good user experience either.
It is better to see an explicit exception early on than very strange and mostly unreadable repr somewhere in logs.

Considering your class, it looks problematic to me. It does not respect basic python's API, like: __dict__, __name__, __qualname__, __class__, __module__, etc.

They are quite basic, many other parts of CPython (and 3rd party projects) do use it.
And you will have a lot of other failure when these fundamentals cannot be accessed 🤔

Do you agree with me?

[yanboliang]

I can understand your point, if other parts of CPython do the same way, it's fine. But if you take a look at this example:

>>> class MyConfig(dict):
...         def __getattr__(self, x):
...             return self[x]
... 
>>> obj = MyConfig(offset=5)
>>> str(obj)
"{'offset': 5}"

If users don't user weakref, it works well even they don't handle __dict__, __name__, __qualname__, __class__, __module__, etc. Do you think the inconsistency will confuse users?

[MojoVampire]

If the goal is to look up __name__ on the class of the instance, shouldn't the weakref code be using _PyObject_LookupSpecial (or _PyObject_LookupSpecialId, though I guess those are the same thing now?), not _PyObject_LookupAttr? The only advantage to the latter is if we wanted to allow instances to change their names by overwriting __name__. Using the LookupSpecial API would bypass the instance and avoid this issue (as well as being faster and likely more correct).

[sobolevn]

@MojoVampire yes, this is a good idea. Thank you! I didn't think about it :)

I've submitted #99244 with your proposal.