New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
inspect.getmembers and inspect.classify_class_attrs mishandle descriptors #63230
Comments
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: ======================================================================================= --> help(Test) 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: As can be seen, 'name' and 'value' are not showing up as enum members. |
Special casing Enum in inspect has a code smell to it. There may not be a better option, but it sure feels ugly. |
Attached patch yields these results: ======================================================================================= 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: |
R David Murray said:
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. |
So the real problem is that inspect depends on dir? Isn't there already a bug open for that issue? |
I do not see one. I did post to PyDev asking about dir -- perhaps I should have given it a different title. |
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. |
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 bpo-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. |
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. |
Right, we definitely want inspect to swallow the exceptions from
The problem at the moment is *working* descriptors that are designed to |
Are you talking about descriptors defined on the class or the metaclass? :-) |
On the class, since that's the case which is breaking here (instances are |
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:
|
The current behaviour is broken for *any* descriptor which doesn't So we should be calling getattr first, and always taking __doc__ (if |
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). |
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. |
I suggest DynamicClassAttribute for the descriptor name. |
New changeset 436b606ecfe8 by Ethan Furman in branch 'default': |
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. |
New changeset 96081e7526f0 by Ethan Furman in branch 'default': |
New changeset 3752c94368dd by Ethan Furman in branch 'default': |
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. |
Updated tests now passing. Will commit Thursday, or Friday at the latest. |
New changeset 39b06c3fbe2e by Ethan Furman in branch 'default': |
Multiple test_pydoc failures found on koobs-freebsd* buildbots after 39b06c3fbe2e6ef78a540513d4b81f2d095d1e62 Attaching complete logs from both bots to this issue, will reference bpo-16938 as well |
New changeset 02c9d26d231f by Ethan Furman in branch 'default': |
New changeset 2f09a6980e1a by Ethan Furman in branch 'default': |
New changeset dad1debba93c by Charles-François Natali in branch 'default': |
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! ;-) |
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 It does not seem to be a heavy maintenance burden: This is the first |
Charles-François Natali added the comment:
I was actually hoping to put off this particular honor for a while... drat! :) |
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. |
Done. Can you please watch for failures and ensure they get fixed? |
Antoine Pitrou <report@bugs.python.org> wrote:
Thanks! Yes, I'll keep an eye on it. |
Here's the fix for --without-doc-strings (can't commit right now). |
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. |
Ethan Furman <report@bugs.python.org> wrote:
I tested on Debian, and the remaining tests seem to work (I did not bother to |
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). |
New changeset 64d94b21e731 by Ethan Furman in branch 'default': |
New changeset 4cd620d8c3f6 by R David Murray in branch 'default': |
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). |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: