bpo-33668: Fix inspect.getmembers to traverse mro when object is class by corona10 · Pull Request #8284 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation51 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable [WIP] bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable
[WIP] bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable
bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable [WIP] bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable
What is the purpose of iterating the instance attribute __bases__?
[WIP] bpo-33668: Fix inspect.getmembers to ignore when __bases__ is non iterable bpo-33668: Fix inspect.getmembers to ignore when __bases__ is not a tuple
@serhiy-storchaka
AFAIK To handle this case,
import inspect import types
class Base(object): @types.DynamicClassAttribute def fried(self): return self._x _x = object()
class A(Base): @types.DynamicClassAttribute def eggs(self): return self._x _x = object()
inspect.getmembers(A)
I found the case of the related to this code.
"Lib/test/test_enum.py", line 2560, in test_inspect_getmembers
I think that we can update this code into
@@ -331,7 +331,8 @@ def getmembers(object, predicate=None):
for base in object.__bases__:
for k, v in base.__dict__.items():
if isinstance(v, types.DynamicClassAttribute):
- names.append(k)
+ if k not in names:
+ names.append(k)
except AttributeError:
Current implementation returns dulplicated members,
, ('eggs', <types.DynamicClassAttribute object at 0x10ed38970>), ('fried', <types.DynamicClassAttribute object at 0x10ed38940>), ('fried', <types.DynamicClassAttribute object at 0x10ed38940>)]
But with this PR
('_x', <object object at 0x10e1691a0>), ('eggs', <types.DynamicClassAttribute object at 0x10e2c0970>), ('fried', <types.DynamicClassAttribute object at 0x10e2c0940>)]
It works well.
Shouldn't this be executed only for classes?
import enum import inspect
class Color(enum.Enum): red = 1 green = 2 blue = 3
values = dict(( ('class', enum.EnumMeta), ('doc', 'An enumeration.'), ('members', Color.members), ('module', name), ('blue', Color.blue), ('green', Color.green), ('name', enum.Enum.dict['name']), ('red', Color.red), ('value', enum.Enum.dict['value']), ))
print(dict(inspect.getmembers(Color)).keys()) c = Color.red print(c.value)
Here is the case,
Without traversing __base__, it can not get value and name from inspect.getmembers.
In all your examples object is a class.
bpo-33668: Fix inspect.getmembers to ignore when __bases__ is not a tuple bpo-33668: Fix inspect.getmembers to traverse __bases__ when object is class
@serhiy-storchaka
I am now understanding!
I've updated to check the object is class before traversing.
Current implementation returns dulplicated members,
The comment above the loop over object.__bases__ explicitly says about duplicated entries. And the code below handles them.
| names.append(k) |
|---|
| except AttributeError: |
| pass |
| if isclass(object): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to move the above comment inside the if branch. Seems :dd is a typo of Add.
This if isclass(object): can be merged with other if isclass(object): at the beginning of the function.
| for k, v in base.__dict__.items(): |
|---|
| if isinstance(v, types.DynamicClassAttribute) and k not in names: |
| names.append(k) |
| except AttributeError: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed?
| @@ -0,0 +1 @@ |
|---|
| Fix inspect.getmembers to travering __bases__ when the object is a class. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
travering -> traversing
And I would add "only" before "traversing".
And maybe add a markup: :func:`inspect.getmembers`.
| @@ -1136,6 +1136,18 @@ class C(metaclass=M): |
|---|
| attrs = [a[0] for a in inspect.getmembers(C)] |
| self.assertNotIn('missing', attrs) |
| def test_getmembers_with_module(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to change the name (and perhaps the code) of the test. There is nothing specific for modules. Instead we need to test that the instance attribute __bases__ doesn't break inspect.getmembers().
Thank you for your review despite the busy situation.
Following your guide, I 've added a configuration to my .vimrc also! Thanks!
set colorcolumn=79 autocmd BufWritePre *.py :%s/\s+$//e
I 've reflected your review.
PTAL
@serhiy-storchaka Are you sure you want to backport this to 2.7? I'm not; it might accidentally fix some things that weren't working for a decade and there are now workarounds in place that might fail if we fix this.
Isn't this true for 3.6 and 3.7 as well, except for the "decade" part?
IMO this should only go into 3.8, and have a "What's New" entry mentioning the change.
This strikes me as being similar to bpo-33899, in that this is probably how it should have been to begin with, but changing it now in released versions would likely unexpectedly break existing code that uses it.
This PR should be backported at least to 3.7, because a module-level __getattr__ is new in 3.7, and this increase the chance to encounter this bug.
But the bug can be encountered in older Python versions for classes with __getattr__. I don't think that fixing it will break somebody's code. Do you have any examples?
Do you have any examples?
No, I don't. You're probably right about backporting this to 3.x.
| # Add any DynamicClassAttributes to the list of names |
|---|
| # if object is a class; |
| # this may result in duplicate entries if, for example, a virtual |
| # attribute with the same name as a DynamicClassAttribute exists |
| for base in object.__bases__: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not it iterate mro instead of object.__bases__? Or _static_getmro(object)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it should apparently use one of those.
bpo-33668: Fix inspect.getmembers to traverse __bases__ when object is class bpo-33668: Fix inspect.getmembers to traverse mro when object is class
@1st1
Can you take a look, please?
Could you please add a test for your last change? I.e. the test that passed when iterate mro, but fails when iterate __bases__. It can be a class with indirectly inherited DynamicClassAttribute.
class A: # A DynamicClassAttribute should be defined here class B(A): pass class C(B): pass
test inspect.getmembers(C)
@serhiy-storchaka
Thanks
I 've added a test test_getmembers_with_dynamic_class_attribute which was failed when
iterate __bases__.
| class C(B): pass |
|---|
| attrs = [c[0] for c in inspect.getmembers(C) if c[0] == 'ham'] |
| self.assertEqual(len(attrs), 2) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not self.assertIn('ham', attrs)?
Add also test for inspect.getmembers(B).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@serhiy-storchaka
Just checking self.assertIn('ham', attrs) does not fail when iterate __bases__.
I updated the unit test and to understand more deeply.
Please reference below script and outputs.
As I am not an expert on inspect module. I don't know which is right output.
import types import inspect
class A: @types.DynamicClassAttribute def ham(self): return 'eggs'
class B(A): pass class C(B): pass
print([a for a in inspect.getmembers(A) if a[0] == 'ham']) print([b for b in inspect.getmembers(B) if b[0] == 'ham']) print([c for c in inspect.getmembers(C) if c[0] == 'ham'])
AS-IS -> iterate __bases__
[('ham', <types.DynamicClassAttribute object at 0x10d2f8198>)] [('ham', <types.DynamicClassAttribute object at 0x10d2f8198>), ('ham', <types.DynamicClassAttribute object at 0x10d2f8198>)] [('ham', <types.DynamicClassAttribute object at 0x10d2f8198>)]
TO-BE 98d79c7 -> iterate mro
[('ham', <types.DynamicClassAttribute object at 0x103365130>), ('ham', <types.DynamicClassAttribute object at 0x103365130>)] [('ham', <types.DynamicClassAttribute object at 0x103365130>), ('ham', <types.DynamicClassAttribute object at 0x103365130>)] [('ham', <types.DynamicClassAttribute object at 0x103365130>), ('ham', <types.DynamicClassAttribute object at 0x103365130>)]
Should not inspect.getmembers() remove duplicates?
Seems using mro instead of __bases__ doesn't fix any bug. In this case it is better to restore the original code.
@serhiy-storchaka
I revert the code to use __bases__.
But I was wondering if this PR was the right way.
Do you think this PR would not be merged and should we find out another way?
If so, I am going to close this PR :)
Maybe other good people will find the way.
@1st1 Can you take a look, please?