If a class implements __dir__ to also return a number of attributes that aren't present in the class __dict__ inspect.classify_class_attrs gets confused and assumes the home class is None.

This in turn confused pydoc, docclass in pydoc assumes that the 'class' slot in the sequence returned by classify_class_attrs is actually a class and fails when it None (because None doesn't have a __name__ attribute).

I've classified this as "low" because I've found a useable workaround: I have implemented a "__objclass__" property for my custom descriptor and with that pydoc works.

It would be nice if pydoc wouldn't crash on this. One possible workaround: assume that the class is the inspected class when a name returned by dir(cls) cannot be found in one of the classes on the mro.

I'll provide a patch if this would be acceptable behavior.

I wonder if it would be better to have inspect.classify_class_attrs be improved instead?

Issue18693 has a patch to `inspect` so that classify_class_attrs will also look in the metaclass.  If that is accepted I think PyDoc is okay as is.

I believe that this describes a problem more general than the problem of Enums from Issue18693: help(x) shouldn't crash in the presence of a __dir__() method that returns unexpected names.

"Crash" is probably too strong.  help() runs, but doesn't return anything useful.

Metaclasses aside, I think the real issue here is objects that don't have __objclass__ defined.  __objclass__ has been around for over a decade (see PEP-0252), and with that `inspect` works fine (aside from metaclasses ;) .

The __objclass__ is a workaround.  I don't understand your point.  I think the original poster is saying that simply putting a __dir__() method on a class can confuse help() more than would be necessary, and if he's correct I agree with him.

dir(obj) is only confused if it returns attributes that are not found in obj.__dict__ (aka virtual attributes).

For these virtual attributes, setting __objclass__ solves the problem.

Armin, when you say it's a workaround do you mean __objclass__ itself, or the way Ronald and I are using it?  It looks like Ronald is using it the way it was intended (on a descriptor), although I may be abusing it by putting it on an Enum member.

Ronald said:
> One possible workaround: assume that the class is the inspected
> class when a name returned by dir(cls) cannot be found in one of
> the classes on the mro.

At some point will this cause help to display an attribute on the wrong class?

=========================================================================
class Boom:
    def __dir__(self):
        return ['BOOM']
    def __getattr__(self, name):
        if name =='BOOM':
            return 42
        return super().__getattr(name)

help(Boom)
=========================================================================
Help on class Boom in module __main__:

class Boom(builtins.object)
 |  Methods defined here:
 |  
 |  __dir__(self)
 |  
 |  __getattr__(self, name)
 |  
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
 |  
 |  __dict__
 |      dictionary for instance variables (if defined)
 |  
 |  __weakref__
 |      list of weak references to the object (if defined)
(END)
=========================================================================

So on an instance there is no problem.

=========================================================================
import inspect

class MetaBoom(type):
    def __dir__(cls):
        return ['__class__', '__module__', '__name__', 'BOOM']
    def __getattr__(self, name):
        if name =='BOOM':
            return 42
        return super().__getattr(name)
    def test(self):
        print('testing...')

class Boom(metaclass=MetaBoom):
    pass

help(Boom())
=========================================================================
Help on Boom in module __main__ object:

<class '__main__.Boom'>
(END)
=========================================================================

Still have problem with metaclasses.

Looking at the result from inspect.classify_class_attrs() we see:
=========================================================================
Attribute(name='BOOM', kind='data', defining_class=None,
    object=<object object at 0x7f061f04d200>)
=========================================================================

The defining class for BOOM should be Boom, not None.

With the attached patch, we get:
=========================================================================
Help on Boom in module __main__ object:

class Boom(builtins.object)
 |  Data and other attributes defined here:
 |  
 |  BOOM = 42
(END)
=========================================================================

A problem with __objclass__ is that it is undocumented other than in PEP 252.  That why I called it a workaround, at the time I created this issue I had just found the problem and workaround and as far as I knew __objclass__ was an undocumented feature of CPython that just happened to fix my problem.

If using __objclass__ is the right solution, as it seems to be, this should be documented somewhere, probably in the section on descriptors.

Part of the issue 19030 patch was incorrect and we missed it in the review. Specifically, the else clause in this bit:

            try:
                get_obj = getattr(cls, name)
            except Exception as exc:
                pass
            else:
                homecls = getattr(get_obj, "__class__")
                homecls = getattr(get_obj, "__objclass__", homecls)
                if homecls not in possible_bases:
                    # if the resulting object does not live somewhere in the
                    # mro, drop it and go with the dict_obj version only
                    homecls = None
                    get_obj = sentinel

The restriction that the __class__ of the object returned by a descriptor must appear in the MRO doesn't make any sense. The entire else block should be simplified to just:

    homecls = getattr(get_obj, "__objclass__", None)

And a "defining class" of None should be documented as a possible result for dynamic attributes that appear in __dir__ but don't set __objclass__ appropriately, and don't correspond to a descriptor.

'None' is not an appropriate response to the "Where does this attribute come from" question.  For one, it's wrong.  For two, it breaks help.

The current patch fixes that particular problem (as a last resort it walks the mro looking for the last class that reported having the attribute, and returns that class as the home class).

The reason it has the __class__, __objclass__ two-step in there was to handle cases like Enum members that do have __class__ set correctly and do not have __objclass__ set at all as it was falsely reporting the home class in that case as None.

Nick Coughlan said:
> The restriction that the __class__ of the object returned by a descriptor
> must appear in the MRO doesn't make any sense.

In the context of finding the home class it does:  if the __class__ returned by a descriptor is not in the mro then just like None it 1) is wrong (it doesn't reflect how it came it be in the class being looked up), and 2) it gives weird results in help.

The added tests in the patch may shed more light if my explanation isn't making sense.

No, __class__ on a descriptor has *NOTHING* to do with how it was looked up. It's the class of the *result*.

>> property.__class__
<class 'type'>
>>> staticmethod.__class__
<class 'type'>
>>> classmethod.__class__
<class 'type'>

It's completely irrelevant to determining *where the attribute came from*.

However, setting __objclass__ on Enum members would be a perfectly reasonable thing to do.

That means __objclass__ can have its meaning broadened to say "this is where this particular instance of this kind of object was defined", and that and __class__ are just coincidentally the same for Enum objects.

We should *not* need to have any Enum specific code in the inspect module, and the current code that tries to find the home class has Enum specific assumptions.

Nick Couphlan added the comment:
>
> No, __class__ on a descriptor has *NOTHING* to do with how it was
> looked up. It's the class of the *result*.

Which is why in most cases it's discarded as the home class.  (I could easily be saying this wrong, which is why I directed you to the tests as better examples of what I am trying to communicate.)


> However, setting __objclass__ on Enum members would be a perfectly
> reasonable thing to do.

I'm glad you think so, but this patch makes that unnecessary.


> We should *not* need to have any Enum specific code in the inspect
> module, and the current code that tries to find the home class has
> Enum specific assumptions.

Only in the sense that Enum is currently the only thing making use of the new DynamicClassAttribute descriptor.

I want to kill the dance in inspect completely. Just use __objclass__
and document it appropriately.

That portion of classify_class_attrs now reads:

    else:
        homecls = getattr(get_obj, "__objclass__", None)
        if homecls not in possible_bases:
            # if the resulting object does not live somewhere in the
            # mro, drop it and go with the dict_obj version only
            homecls = None
            get_obj = sentinel

An example for why the if clause is still there:

    class Life:
        @property
        def answer(self):
            return 42

Without the if clause the home class for answer would be int when it should be Life.

Attached patch is refactored for simplicity as well.

Discovered a problem with one of the tests, moved back to issue19030.  The __class__/__objclass__ is gone, the code is simpler.

When issue19030 is committed I think we can make a doc change to include __objclass__ somewhere and close this one.

Ronald, if you get a chance could you try your descriptor, without the __objclass__ work around, with the latest patch in #19030?

This has been fixed in #19030: every good object will have a home class; non-good objects (the result of buggy __dir__, __getattribute__, or __getattr__ methods) will not be returned and so cannot confuse pydoc.

Some test failures have cropped, I have attached buildbot logs and referenced them in #19030