classification
Title: inspect.getmembers and inspect.classify_class_attrs mishandle descriptors
Type: behavior Stage: committed/rejected
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ethan.furman Nosy List: barry, eli.bendersky, ethan.furman, koobs, mjpieters, ncoghlan, neologix, pitrou, python-dev, r.david.murray, skrah
Priority: normal Keywords: patch

Created on 2013-09-16 00:00 by ethan.furman, last changed 2014-02-25 21:05 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
issue19030.stoneleaf.01.patch ethan.furman, 2013-09-16 01:26 review
issue19030.stoneleaf.02.patch ethan.furman, 2013-09-16 14:31 review
issue19030.stoneleaf.03.patch ethan.furman, 2013-09-19 01:23 review
issue19030.stoneleaf.04.patch ethan.furman, 2013-09-22 22:22 review
issue19030.stoneleaf.05.patch ethan.furman, 2013-10-14 17:33 review
koobs-freebsd9-amd64-py3x-build183.log koobs, 2013-10-18 07:49
koobs-freebsd10-amd64-py3x-build588.log koobs, 2013-10-18 07:50
koobs-freebsd9-py3x-build245.log koobs, 2013-10-21 08:56
issue19030-without-docstrings.patch skrah, 2013-10-22 11:30 review
Messages (42)
msg197844 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-16 00:00
Due to the odd nature of Enum classes and instances, the normal methods used by inspect.getmembers and inspect.classify_class_attrs are insufficient.

By special casing Enum inside those two functions the correct information can be returned.

Here is an example of the problem:

=======================================================================================
--> class Test(enum.Enum):
...   this = 'that'
...   these = 'those'
...   whose = 'mine'
...   name = 'Python'
...   value = 'awesome'
...   def what(self):
...     return "%s is %s!" % (self.name, self.value)
... 

--> help(Test)
Help on Test in module __main__ object:

class Test(enum.Enum)
 |  Method resolution order:
 |      Test
 |      enum.Enum
 |      builtins.object
 |  
 |  Data and other attributes defined here:
 |  
 |  these = <Test.these: 'those'>
 |  
 |  this = <Test.this: 'that'>
 |  
 |  whose = <Test.whose: 'mine'>
 |  
 |  ----------------------------------------------------------------------
 |  Data descriptors inherited from enum.Enum:
 |  
 |  name
 |      The name of the Enum member.
 |  
 |  value
 |      The value of the Enum member.
 |  
 |  ----------------------------------------------------------------------
 |  Data descriptors inherited from enum.EnumMeta:
 |  
 |  __members__
 |      Returns a mapping of member name->value.
 |      
 |      This mapping lists all enum members, including aliases. Note that this
 |      is a read-only view of the internal mapping.
(END)
=======================================================================================

As can be seen, 'name' and 'value' are not showing up as enum members.
msg197853 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-16 01:24
Special casing Enum in inspect has a code smell to it.  There may not be a better option, but it sure feels ugly.
msg197854 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-16 01:26
Attached patch yields these results:

=======================================================================================
Help on class Test in module __main__:

class Test(enum.Enum)
 |  Method resolution order:
 |      Test
 |      enum.Enum
 |      builtins.object
 |  
 |  Data and other attributes defined here:
 |  
 |  name = <Test.name: 'Python'>
 |  
 |  these = <Test.these: 'those'>
 |  
 |  this = <Test.this: 'that'>
 |  
 |  value = <Test.value: 'awesome'>
 |  
 |  whose = <Test.whose: 'mine'>
 |  
 |  ----------------------------------------------------------------------
 |  Data descriptors inherited from enum.Enum:
 |  
 |  name
 |      The name of the Enum member.
 |  
 |  value
 |      The value of the Enum member.
 |  
 |  ----------------------------------------------------------------------
 |  Data descriptors inherited from enum.EnumMeta:
 |  
 |  __members__
 |      Returns a mapping of member name->value.
 |      
 |      This mapping lists all enum members, including aliases. Note that this
 |      is a read-only view of the internal mapping.
(END)
=======================================================================================
msg197855 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-16 01:31
R David Murray said:
>
>Special casing Enum in inspect has a code smell to it.

I agree, and I'm certainly open to other options.

The flow at this point is:

  help() --> inspect.classify_class_attrs --> dir() --> Enum.__dir__

Because inspect relies on dir and Enum's dir has been customized, inspect will fail to return the whole story.
msg197857 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-16 01:32
So the real problem is that inspect depends on dir?  Isn't there already a bug open for that issue?
msg197861 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-16 02:11
I do not see one.  I did post to PyDev asking about dir -- perhaps I should have given it a different title.
msg197867 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-16 04:26
Here's a crazy idea.  :)

The only reason the patch is tied to Enum is because of Enum's use of the _RouteClassAttributeToGetattr descriptor.

If we had a module similar to functools, say classtools, we could flesh out _RouteClassAttributeToGetattr, rename it to VirtualAttribute, and then it would no longer be Enum specific.
msg197871 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-09-16 06:18
types is the OO equivalent to functools these days, in addition to its original role of exposing the internal type objects that aren't builtins.

However, it seems to me that the fix for issue 1785 is simply *wrong*: it eliminated the exceptions at the expense of sometimes returning the *wrong answer*. The change in that issue means the inspect module isn't implementing the descriptor protocol correctly, and thus may provide a raw descriptor object when attempting to retrieve that attribute will return something else.

Instead of the current behaviour, the inspect module should be trying getattr *first*, and only falling back to peeking in the __dict__ directly if that throws an exception.
msg197879 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-16 08:36
> However, it seems to me that the fix for issue 1785 is simply *wrong*: it eliminated the exceptions at the expense of sometimes returning the *wrong answer*.

The underlying problem is that those functions are not very well-specified (actually, classify_class_attrs() is not specified at all: it's undocumented).  The main consumer of inspect in the stdlib is pydoc, and pydoc being broken by third-party libraries with non-trivial descriptors was a major nuisance.
msg197883 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-09-16 09:02
Right, we definitely want inspect to swallow the exceptions from
descriptors. My suggestion is merely to switch the order to be:
1. Try getattr
2. If that throws an exception, check __dict__ directly
3. If neither works (e.g. due to a buggy __dir__ method), ignore the
attribute entirely.

The problem at the moment is *working* descriptors that are designed to
fall back on the metaclass lookup are being mishandled.
msg197884 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-16 09:06
> Right, we definitely want inspect to swallow the exceptions from
> descriptors. My suggestion is merely to switch the order to be:
> 1. Try getattr
> 2. If that throws an exception, check __dict__ directly
> 3. If neither works (e.g. due to a buggy __dir__ method), ignore the
> attribute entirely.

Are you talking about descriptors defined on the class or the metaclass? :-)
msg197891 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-09-16 10:51
On the class, since that's the case which is breaking here (instances are
already OK, since they trigger the success path in the custom descriptors)
msg197902 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-16 14:31
Switching the order to try getattr first is going to make getting the doc from the descriptor problematic -- we have no way of knowing if the descriptor doc goes with the object we got back from getattr.

Current patch adds VirtualAttribute to types, and reworks inspect.classify_class_attrs, inspect.getmembers, and Enum to use that instead of _RouteClassAttributeToGetattr (which has been removed).

Tests are still needed for the new VirtualAttribute.

Not sure VirtualAttribute is the best name; other ideas:

  - InstanceProperty
  - AttributeProperty
  - HiddenClassProperty
msg197906 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-09-16 14:51
The current behaviour is broken for *any* descriptor which doesn't
return itself when retrieved from the class. It just so happens that
all the *other* descriptors in the standard library work that way, so
it doesn't matter if you retrieve them directly from __dict__ or
retrieve them with getattr - you'll get the descriptor object either
way.

So we should be calling getattr first, and always taking __doc__ (if
any) from the returned object.
msg198037 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-19 01:23
Okay, some slight reorganizing.  Current patch gives preferential treatment to getattr, falling back on dict lookup if getattr raises an exception or if the resulting object's class is not found in the mro (including the metaclass mro).

Amazingly, nothing appeared to break in the test suite.  :)

If no kibitzing in the next couple days I'll write some tests to better excercise the metaclass portion (I used Enum for interactive testing).
msg198306 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-22 22:22
Current patch has a little more code cleanup and a bunch more tests.

I copied and adapted test_property to test_VirtualAttribute, and VirtualAttribute passes every test except the __slots__ test where __doc__ is not supposed to copy.  I think the problem there is that VA is a python object and already has a __doc__ so having __slots__ not show __doc__ doesn't help.  I put a skipIf(hasattr, obj, '__doc__') around that test.

I'm still not crazy about the name VirtualAttribute... oh well.

If no negative feedback by mid-week I'll commit, as I would really like this code to be the next alpha.
msg198313 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-09-23 03:19
I suggest DynamicClassAttribute for the descriptor name.
msg198387 - (view) Author: Roundup Robot (python-dev) Date: 2013-09-25 14:15
New changeset 436b606ecfe8 by Ethan Furman in branch 'default':
Close #19030: improvements to inspect and Enum.
http://hg.python.org/cpython/rev/436b606ecfe8
msg198481 - (view) Author: Martijn Pieters (mjpieters) Date: 2013-09-27 15:13
Note: there is a comment explaining the point of _RouteClassAttributeToGetattr right above the Enum.name and Enum.value methods you changed (now at line 474). You may want to update that comment now.
msg198506 - (view) Author: Roundup Robot (python-dev) Date: 2013-09-28 06:02
New changeset 96081e7526f0 by Ethan Furman in branch 'default':
Issue19030: fixed comment that was still referring to a changed descriptor.
http://hg.python.org/cpython/rev/96081e7526f0
msg199736 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-13 17:52
New changeset 3752c94368dd by Ethan Furman in branch 'default':
Issue19030: commit tests for DynamicClassAttribute
http://hg.python.org/cpython/rev/3752c94368dd
msg199920 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-14 16:10
Updated and renamed the DynamicClassAttribute tests, and discovered that classify_class_attrs is not handling instance portion correctly.

    class Meta(type):
        def __getattr__(self, name):
            if name == 'ham':
                return 'spam'
            return super().__getattr__(name)

    class VA(metaclass=Meta):
        @types.DynamicClassAttribute
        def ham(self):
            return 'eggs'

We should see both eggs and spam, but only eggs is showing up.
msg199929 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-14 17:33
Updated tests now passing.

Will commit Thursday, or Friday at the latest.
msg200186 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-18 07:28
New changeset 39b06c3fbe2e by Ethan Furman in branch 'default':
Close #19030: inspect.getmembers and inspect.classify_class_attrs
http://hg.python.org/cpython/rev/39b06c3fbe2e
msg200194 - (view) Author: koobs (koobs) Date: 2013-10-18 07:49
Multiple test_pydoc  failures found on koobs-freebsd* buildbots after 39b06c3fbe2e6ef78a540513d4b81f2d095d1e62

Attaching complete logs from both bots to this issue, will reference #16938 as well
msg200230 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-18 08:22
New changeset 02c9d26d231f by Ethan Furman in branch 'default':
Issue #19030:  special-cased __dict__ as the actual dict is not returned, a proxy is.
http://hg.python.org/cpython/rev/02c9d26d231f
msg200699 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-21 05:37
New changeset 2f09a6980e1a by Ethan Furman in branch 'default':
Issue #19030: final pieces for proper location of various class attributes located in the metaclass.
http://hg.python.org/cpython/rev/2f09a6980e1a
msg200726 - (view) Author: koobs (koobs) Date: 2013-10-21 08:56
@Ethan, not sure if you've already seen them, but there are 4 pydoc failures since 2f09a6980e1a

Attaching another complete log from build #245 on the koobs-freebsd9 buildslave here for posterity
msg200768 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-21 12:45
New changeset dad1debba93c by Charles-François Natali in branch 'default':
Fix test_pydoc failure introduced by 2f09a6980e1a (issue #19030).
http://hg.python.org/cpython/rev/dad1debba93c
msg200772 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-21 12:56
I took the freedom to push a fix for the test_pydoc failures (all the buildbots were red). It should fix the failures, but since I'm not familiar with the code, it'd be good to have someone double-check it.

Ethan, you just broke all the buildbots: you're now officialy a core developer! ;-)
msg200803 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-21 15:36
The build --without-doc-strings still fails on the FreeBSD-9.0 bot.

Antoine, could we make that option official on my build slave? Currently
I'm subverting the build system by exporting an environment variable.

It does not seem to be a heavy maintenance burden: This is the first
failure in half a year or so.
msg200805 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-21 16:18
Charles-François Natali added the comment:
> 
> Ethan, you just broke all the buildbots: you're now officialy a core
> developer! ;-)

I was actually hoping to put off this particular honor for a while... drat!  :)
msg200807 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-21 16:22
Thanks, Charles-François!

The problem occurred when I tried to push the commit and was told there was trailing white-space.  Naturally I then ran the de-whitespacing tool which of course removed the whitespace from those lines where you added the \x20s back on.  I'll remember to do those conversions next time, and thanks for fixing it -- I'm not sure I would have found it any time soon.
msg200815 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-21 17:16
> Antoine, could we make that option official on my build slave? Currently
> I'm subverting the build system by exporting an environment variable.

Done. Can you please watch for failures and ensure they get fixed?
msg200824 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-21 18:34
Antoine Pitrou <report@bugs.python.org> wrote:
> Done. Can you please watch for failures and ensure they get fixed?

Thanks! Yes, I'll keep an eye on it.
msg200915 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-22 11:30
Here's the fix for --without-doc-strings (can't commit right now).
msg200949 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-22 13:39
Stefan, do the other tests in PydocWithMetaClasses continue to work on your FreeBSD 9.0 box without docstrings?  Because they all fail on my Linux Ubuntu 13.04 box.
msg200952 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-22 13:52
Ethan Furman <report@bugs.python.org> wrote:
> Stefan, do the other tests in PydocWithMetaClasses continue to work on your FreeBSD 9.0 box without docstrings?  Because they all fail on my Linux Ubuntu 13.04 box.

I tested on Debian, and the remaining tests seem to work (I did not bother to
figure out why though).  Let's just skip all tests if they fail for you.
msg200959 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-10-22 14:26
Actually, you @skipif clued me in as to what was happening with the other pydoc tests and their skipif clauses.

Added appropriate skipifs to the new tests and mimicked the docstring in/exclusion.  All tests now passing with and without docstrings, with and without -OO (simply skipped with -OO).
msg200963 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-22 14:30
New changeset 64d94b21e731 by Ethan Furman in branch 'default':
Issue #19030: fix new pydoc tests for --without-doc-strings
http://hg.python.org/cpython/rev/64d94b21e731
msg212211 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-25 21:03
New changeset 4cd620d8c3f6 by R David Murray in branch 'default':
whatsnew: DynanicClassAttribute (#19030), Py_SetStandardStreamEncoding (#16129)
http://hg.python.org/cpython/rev/4cd620d8c3f6
msg212212 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-25 21:05
I added docs for DynamicClassAttribute by copying the docstring.  I think the doc entry could use some expansion, though, as it isn't obvious how to use it (or what, in fact, it does exactly).
History
Date User Action Args
2014-02-25 21:05:20r.david.murraysetmessages: + msg212212
2014-02-25 21:03:43python-devsetmessages: + msg212211
2013-10-22 14:30:49python-devsetmessages: + msg200963
2013-10-22 14:26:06ethan.furmansetmessages: + msg200959
2013-10-22 13:52:51skrahsetmessages: + msg200952
2013-10-22 13:39:28ethan.furmansetmessages: + msg200949
2013-10-22 11:30:09skrahsetfiles: + issue19030-without-docstrings.patch

messages: + msg200915
2013-10-21 18:34:41skrahsetmessages: + msg200824
2013-10-21 17:16:46pitrousetmessages: + msg200815
2013-10-21 16:22:03ethan.furmansetmessages: + msg200807
2013-10-21 16:18:53ethan.furmansetmessages: + msg200805
2013-10-21 15:36:08skrahsetnosy: + skrah
messages: + msg200803
2013-10-21 12:56:33neologixsetnosy: + neologix
messages: + msg200772
2013-10-21 12:45:52python-devsetmessages: + msg200768
2013-10-21 08:56:35koobssetfiles: + koobs-freebsd9-py3x-build245.log

messages: + msg200726
2013-10-21 05:37:47python-devsetmessages: + msg200699
2013-10-18 08:22:35python-devsetmessages: + msg200230
2013-10-18 07:50:10koobssetfiles: + koobs-freebsd10-amd64-py3x-build588.log
2013-10-18 07:49:33koobssetfiles: + koobs-freebsd9-amd64-py3x-build183.log
nosy: + koobs
messages: + msg200194

2013-10-18 07:39:31ethan.furmanlinkissue16938 superseder
2013-10-18 07:28:05python-devsetstatus: open -> closed
resolution: fixed
messages: + msg200186

stage: patch review -> committed/rejected
2013-10-14 17:33:24ethan.furmansetfiles: + issue19030.stoneleaf.05.patch

messages: + msg199929
stage: patch review
2013-10-14 16:10:10ethan.furmansetstatus: closed -> open
messages: + msg199920

assignee: ethan.furman
resolution: fixed -> (no value)
stage: committed/rejected -> (no value)
2013-10-13 17:52:19python-devsetmessages: + msg199736
2013-09-28 06:02:11python-devsetmessages: + msg198506
2013-09-27 15:13:40mjpieterssetnosy: + mjpieters
messages: + msg198481
2013-09-25 14:15:01python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg198387

resolution: fixed
stage: patch review -> committed/rejected
2013-09-23 03:19:49ncoghlansetmessages: + msg198313
2013-09-22 22:22:58ethan.furmansetfiles: + issue19030.stoneleaf.04.patch
2013-09-22 22:22:41ethan.furmansetmessages: + msg198306
2013-09-19 01:23:52ethan.furmansetfiles: + issue19030.stoneleaf.03.patch

messages: + msg198037
2013-09-16 14:51:17ncoghlansetmessages: + msg197906
2013-09-16 14:31:38ethan.furmansetfiles: + issue19030.stoneleaf.02.patch

messages: + msg197902
stage: patch review
2013-09-16 10:51:05ncoghlansetmessages: + msg197891
title: inspect.getmembers and inspect.classify_class_attrs mishandle descriptors -> inspect.getmembers and inspect.classify_class_attrs mishandle descriptors
2013-09-16 09:06:24pitrousetmessages: + msg197884
title: inspect.getmembers and inspect.classify_class_attrs mishandle descriptors -> inspect.getmembers and inspect.classify_class_attrs mishandle descriptors
2013-09-16 09:02:46ncoghlansetmessages: + msg197883
2013-09-16 08:36:50pitrousetmessages: + msg197879
2013-09-16 06:18:05ncoghlansetmessages: + msg197871
title: Make inspect.getmembers and inspect.classify_class_attrs Enum aware -> inspect.getmembers and inspect.classify_class_attrs mishandle descriptors
2013-09-16 04:26:59ethan.furmansetmessages: + msg197867
2013-09-16 02:11:42ethan.furmansetmessages: + msg197861
2013-09-16 01:32:26r.david.murraysetmessages: + msg197857
2013-09-16 01:31:12ethan.furmansetmessages: + msg197855
2013-09-16 01:26:30ethan.furmansetfiles: + issue19030.stoneleaf.01.patch
keywords: + patch
messages: + msg197854
2013-09-16 01:24:01r.david.murraysetnosy: + r.david.murray
messages: + msg197853
2013-09-16 00:00:54ethan.furmancreate