classification
Title: pydoc confused by __dir__
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder: inspect.getmembers and inspect.classify_class_attrs mishandle descriptors
View: 19030
Assigned To: ethan.furman Nosy List: arigo, eli.bendersky, eric.snow, ethan.furman, koobs, ncoghlan, ronaldoussoren
Priority: normal Keywords: patch

Created on 2013-01-11 22:42 by ronaldoussoren, last changed 2013-10-18 07:59 by koobs. This issue is now closed.

Files
File name Uploaded Description Edit
issue16938.stoneleaf.01.patch ethan.furman, 2013-10-07 03:25 review
issue16938.stoneleaf.02.patch ethan.furman, 2013-10-13 18:04 review
Messages (22)
msg179738 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2013-01-11 22:42
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.
msg196855 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-03 18:25
I wonder if it would be better to have inspect.classify_class_attrs be improved instead?
msg196914 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-04 13:43
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.
msg197075 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2013-09-06 14:57
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.
msg197105 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-06 20:55
"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 ;) .
msg197112 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2013-09-06 21:53
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.
msg197114 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-06 22:14
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.
msg197119 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-06 22:33
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?
msg199126 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-07 03:25
=========================================================================
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)
=========================================================================
msg199381 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2013-10-10 11:40
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.
msg199697 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-13 11:53
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.
msg199711 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-13 14:45
'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.
msg199714 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-13 14:59
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*.
msg199715 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-13 14:59
However, setting __objclass__ on Enum members would be a perfectly reasonable thing to do.
msg199717 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-13 15:03
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.
msg199721 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-13 15:14
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.
msg199722 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-13 15:22
I want to kill the dance in inspect completely. Just use __objclass__
and document it appropriately.
msg199740 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-13 18:04
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.
msg199930 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-14 17:36
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.
msg199969 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-15 00:07
Ronald, if you get a chance could you try your descriptor, without the __objclass__ work around, with the latest patch in #19030?
msg200189 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-18 07:39
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.
msg200228 - (view) Author: Kubilay Kocak (koobs) (Python triager) Date: 2013-10-18 07:59
Some test failures have cropped, I have attached buildbot logs and referenced them in #19030
History
Date User Action Args
2013-10-18 07:59:45koobssetnosy: + koobs
messages: + msg200228
2013-10-18 07:39:31ethan.furmansetstatus: open -> closed
superseder: inspect.getmembers and inspect.classify_class_attrs mishandle descriptors
messages: + msg200189

resolution: fixed
stage: resolved
2013-10-15 00:07:47ethan.furmansetmessages: + msg199969
2013-10-14 17:36:43ethan.furmansetmessages: + msg199930
stage: patch review -> (no value)
2013-10-13 18:04:32ethan.furmansetfiles: + issue16938.stoneleaf.02.patch

messages: + msg199740
2013-10-13 15:22:45ncoghlansetmessages: + msg199722
2013-10-13 15:14:47ethan.furmansetmessages: + msg199721
2013-10-13 15:03:34ncoghlansetmessages: + msg199717
2013-10-13 14:59:52ncoghlansetmessages: + msg199715
2013-10-13 14:59:05ncoghlansetmessages: + msg199714
2013-10-13 14:45:13ethan.furmansetmessages: + msg199711
2013-10-13 11:53:32ncoghlansetnosy: + ncoghlan
messages: + msg199697
2013-10-10 11:40:54ronaldoussorensetmessages: + msg199381
2013-10-07 03:25:29ethan.furmansetfiles: + issue16938.stoneleaf.01.patch
priority: low -> normal

assignee: ethan.furman
versions: - Python 2.7
keywords: + patch
messages: + msg199126
stage: test needed -> patch review
2013-09-06 22:33:17ethan.furmansetmessages: + msg197119
2013-09-06 22:14:13ethan.furmansetmessages: + msg197114
2013-09-06 21:53:09arigosetmessages: + msg197112
2013-09-06 20:55:11ethan.furmansetmessages: + msg197105
2013-09-06 14:57:13arigosetnosy: + arigo
messages: + msg197075
2013-09-04 13:43:54ethan.furmansetmessages: + msg196914
2013-09-03 18:25:52ethan.furmansetmessages: + msg196855
2013-09-02 17:39:47ethan.furmansetnosy: + eli.bendersky, ethan.furman
2013-08-21 19:23:53eric.snowsetnosy: + eric.snow
2013-01-11 22:42:09ronaldoussorencreate