bpo-38337: Change getattr to inspect.getattr_static #16521

3j14 · 2019-10-01T12:07:40Z

When calling inspect.getmembers on a class that has a property (@property), the property will be called by the getattr call in getmembers.

Example:

import inspect

class Example:
    def __init__(self, var):
        self._var = var
        print('__init__')

    def foo(self, bar):
        print(bar)
        print('foo')

    @property
    def var(self):
        print('Access property')
        return self._var


if __name__ == '__main__':
    ex = Example('Hello')
    print('--- getmembers from instance ---')
    print(inspect.getmembers(ex))
    print('--- getmembers from class    ---')
    print(inspect.getmembers(Example))

Result:

__init__
--- getmembers from instance ---
Access property
[('__class__', <attribute '__class__' of 'object' objects>), ..., ('var', 'Hello')]
--- getmembers from class    ---
[('__class__', <attribute '__class__' of 'object' objects>), ..., ('var', <property object at 0x...>)]

Expected (new result):

__init__
--- getmembers from instance ---
[('__class__', <attribute '__class__' of 'object' objects>), ..., ('var', <property object at 0x...>)]
--- getmembers from class    ---
[('__class__', <attribute '__class__' of 'object' objects>), ..., ('var', <property object at 0x...>)]

https://bugs.python.org/issue38337

the-knights-who-say-ni · 2019-10-01T12:07:44Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@jnsdrtlf

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

We also demand... A SHRUBBERY!

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

s-sanjay · 2019-10-01T14:13:23Z

should we add a test case ?

brandtbucher

Thanks @jnsdrtlf, and welcome to CPython! 😎

I have a minor change to suggest for the NEWS entry, but otherwise this looks like a good improvement.

I also agree that adding regression test is probably a good idea.

Misc/NEWS.d/next/Library/2019-10-01-12-23-37.bpo-38337.QmLSXW.rst

brandtbucher · 2019-10-01T16:45:43Z

Actually, from looking at the test failures... I'm not sure if this is the desired behavior (and the docs are ambiguous). Maybe @1st1 can shed some light on the "correct" behavior of this function?

1st1 · 2019-10-01T16:54:38Z

Since inspect.getmembers is an introspection function it shouldn't run any actual code (i.e. executing getters). It should compute its result statically. Using getattr_static is the right idea here, for sure.

brandtbucher · 2019-10-01T17:00:49Z

Okay then. So this probably doesn't need a new test, but the ~4 failing ones should be updated!

1st1 · 2019-10-01T17:11:29Z

So this probably doesn't need a new test, but the ~4 failing ones should be updated!

I think it needs a new test. Basically an object with a property and a test ensuring that the property was discovered by getmembers, but the getter code wasn't run.

@brandtbucher

As suggested by @brandtbucher Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>

3j14 · 2019-10-01T17:38:26Z

I think it needs a new test. Basically an object with a property and a test ensuring that the property was discovered by getmembers, but the getter code wasn't run.

Sounds good! I will get back to work and write a test later today.

Make sure inspect.getmembers won't call properties or other dynamic attributes

3j14 · 2019-10-01T21:49:30Z

The test test_getmembers_VirtualAttribute expects this method

@types.DynamicClassAttribute
    def eggs(self):
        return 'spam'

to be called. It asserts whether ('eggs', 'spam') is in inspect.getmembers. But this is exactly the behavior that I tried to avoid with my PR.

There is also some wierd behaviour when overwriting __getattr__ in a metaclass. This probably needs more insight on how inspect.getmembers should behave.

Failing tests:

test_get_slot_members
test_getmembers_method
test_getmembers_VirtualAttribute

hongweipeng · 2019-10-03T11:32:51Z

Lib/test/test_inspect.py

@@ -1191,6 +1191,27 @@ def f(self):
        self.assertIn(('f', b.f), inspect.getmembers(b))
        self.assertIn(('f', b.f), inspect.getmembers(b, inspect.ismethod))

+    def test_getmembers_static(self):
+        class Foo:
+            def __init__(bar):


Use def __init__(self, bar): please.

Oups 🤦‍♂ Fixed in commit b415224

3j14 · 2019-10-07T08:18:36Z

Another inconsistent behaviour: inspect.isdatadescriptor should return True for properties:

Return true if the object is a data descriptor.

Data descriptors have both a get and a set method. Examples are properties (defined in Python), getsets, and members

(Python Docs)

However, the following code does not return any properties:

class Foo:
    def __init__(self, data):
        self._bar = data
    
    @property
    def bar(self):
        print('--BAR--')
        return self._bar

inspect.getmembers(Foo('test'), inspect.isdatadescriptor)

Result

--BAR--
[]

Expected (new result)

[('__class__', <attribute '__class__' of 'object' objects>), ('__dict__', <attribute '__dict__' of 'Foo' objects>), ('__weakref__', <attribute '__weakref__' of 'Foo' objects>), ('bar', <property object at 0x...>)]

Is this correct so far? I am still confused about how getmembers should behave. See test_getmembers_VirtualAttribute, this test expects a different behaviour.

3j14 · 2019-10-07T08:32:05Z

Additionally,

class Foo:
    pass

inspect.getattr_static(Foo('test'), '__class__')

returns <attribute '__class__' of 'object' objects> whereas getattr would return <class '__main__.Foo'>. What should getmembers return? If it returns <attribute '__class__' of 'object' objects>, the inspect.isclass predicate would not work:

class Foo:
    pass

# Will return [] if getattr_static is used
inspect.getmembers(Foo('test'), inspect.isclass)

1st1 · 2019-10-07T19:27:34Z

Lib/inspect.py

@@ -347,7 +347,10 @@ def getmembers(object, predicate=None):
        # like calling their __get__ (see bug #1785), so fall back to
        # looking in the __dict__.
        try:
-            value = getattr_static(object, key)
+            if isinstance(getattr_static(object, key), property):


Why can't we always use getattr_static? If there's a reason for not using it always, please describe that in a comment.

This is related to my latest comment. Sorry, I didn't make this clear:

getattr_static(...,'__class__') returns <attribute '__class__' of 'object' objects> whereas getattr would return <class '__main__.Foo'>. What should getmembers return? Using getattr_static for every attribute doesn't seem to be the solution. It works fine with methods, functions and properties, but some edge cases seem to require getattr (such as __class__). It is not clear what the expected behaviour is.

Got it, well, describe this in a comment :)

3j14 · 2019-10-08T06:41:20Z

In 3ac2093, I've changed inspect.getmembers to only use inspect.getattr_static for properties, as other members (e.g. __class__) are expected to be returned using getattr(obj,'__class__'):

getattr_static(obj,'__class__') returns <attribute '__class__' of 'object' objects> whereas getattr would return <class '__main__.Foo'>

This feels more like a temporary fix - could be reverted to the first fix I've committed - although I am just not sure which is better.

At this point, I am not sure what the actual expected behaviour should be. Someone should review this and the test case that is currently failing, as it expects a different behaviour (return the actual value of the property).

1st1

Jonas, I thought about this PR for some time and I think we should reject it. Modifying getmembers in any way would break backwards compatibility in a very non-obvious way for many users. The ship to fix it has sailed.

We can add a new function getmembers_static that would use getattr_static instead of getattr. It must be a new distinct function, though.

bedevere-bot · 2019-10-08T12:54:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

3j14 · 2019-10-10T14:47:11Z

@1st1 I share your thoughts on that. It does not seem to be obvious, what getmembers should return - and changing it in any way will break existing code. This has already caused me a lot of headaches.

As for introducing a function getmembers_static: I still have some questions that should be discussed before doing so:

What should getmembers_static return? Is getattr_static appropriate for every member? This would break some predicates (e.g. isclass, see comment above). There does not seem to be a "simple" solution.
A technical question: The new method should pass most of the same tests as getmembers. Would one just copy and paste the old ones? It would also share most of the code - except for maybe 1 line - with getmembers.
Is there a reason for getmembers to return a list of tuples? I would have suggested to use a dict instead, which feels more intuitive on first though.

Is there a better place to discuss such questions?

1st1 · 2019-10-24T18:46:32Z

Is there a better place to discuss such questions?

I think it would be better if you could post this to the python-ideas mailing list. I've very limited time and would hate to get this thing staled because of me.

1st1 · 2019-10-24T18:46:54Z

Or post it here: https://discuss.python.org/c/ideas

csabella · 2020-01-26T01:21:59Z

I added a link to the discussion on the bug tracker.

jaymegordo · 2020-08-30T17:55:59Z

This would be very helpful, running into the same issue with inspect.getmembers evaluating @Property decorated methods.

getmembers_static would solve it.

Change getattr to inspect.getattr_static

ba746bc

the-knights-who-say-ni added the CLA not signed label Oct 1, 2019

bedevere-bot added the awaiting review label Oct 1, 2019

📜🤖 Added by blurb_it.

80fcfb4

brandtbucher added needs backport to 3.7 type-bug An unexpected behavior, bug, or error labels Oct 1, 2019

brandtbucher requested changes Oct 1, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-10-01-12-23-37.bpo-38337.QmLSXW.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Oct 1, 2019

brandtbucher removed needs backport to 3.7 labels Oct 1, 2019

Update NEWS entry

434a3e6

As suggested by @brandtbucher Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 1, 2019

3j14 added 2 commits October 1, 2019 23:46

Add test for inspect.getmembers

db038c5

Make sure inspect.getmembers won't call properties or other dynamic attributes

Assert whether the property is in the list

8a95946

hongweipeng reviewed Oct 3, 2019

View reviewed changes

Fix constructor without self

b415224

This comment has been minimized.

Sign in to view

bpo-38337: Fix whitespace in test_inspect

64f5954

bpo-38337: Only use getattr_static for properties

3ac2093

1st1 reviewed Oct 7, 2019

View reviewed changes

1st1 requested changes Oct 8, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Oct 8, 2019

kernc mentioned this pull request Jun 16, 2020

bpo-30533:Add function inspect.getmembers_static that does not call properties or dynamic properties. #20911

Merged

vstinner closed this May 3, 2021

vstinner deleted the branch python:master May 3, 2021 21:29

jnsdrtlf mannequin mentioned this pull request Apr 10, 2022

inspect: getmembers calls properties #82518

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38337: Change getattr to inspect.getattr_static #16521

bpo-38337: Change getattr to inspect.getattr_static #16521

3j14 commented Oct 1, 2019 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Oct 1, 2019

s-sanjay commented Oct 1, 2019

brandtbucher left a comment

brandtbucher commented Oct 1, 2019

1st1 commented Oct 1, 2019

brandtbucher commented Oct 1, 2019 •

edited

Loading

1st1 commented Oct 1, 2019

3j14 commented Oct 1, 2019

3j14 commented Oct 1, 2019

hongweipeng Oct 3, 2019

3j14 Oct 3, 2019

This comment has been minimized.

3j14 commented Oct 7, 2019 •

edited

Loading

3j14 commented Oct 7, 2019

1st1 Oct 7, 2019

3j14 Oct 7, 2019

1st1 Oct 7, 2019

3j14 commented Oct 8, 2019 •

edited

Loading

1st1 left a comment

bedevere-bot commented Oct 8, 2019

3j14 commented Oct 10, 2019 •

edited

Loading

1st1 commented Oct 24, 2019

1st1 commented Oct 24, 2019

csabella commented Jan 26, 2020

jaymegordo commented Aug 30, 2020

bpo-38337: Change getattr to inspect.getattr_static #16521

bpo-38337: Change getattr to inspect.getattr_static #16521

Conversation

3j14 commented Oct 1, 2019 • edited by bedevere-bot Loading

Result:

Expected (new result):

the-knights-who-say-ni commented Oct 1, 2019

CLA Missing

s-sanjay commented Oct 1, 2019

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher commented Oct 1, 2019

1st1 commented Oct 1, 2019

brandtbucher commented Oct 1, 2019 • edited Loading

1st1 commented Oct 1, 2019

3j14 commented Oct 1, 2019

3j14 commented Oct 1, 2019

hongweipeng Oct 3, 2019

Choose a reason for hiding this comment

3j14 Oct 3, 2019

Choose a reason for hiding this comment

This comment has been minimized.

3j14 commented Oct 7, 2019 • edited Loading

3j14 commented Oct 7, 2019

1st1 Oct 7, 2019

Choose a reason for hiding this comment

3j14 Oct 7, 2019

Choose a reason for hiding this comment

1st1 Oct 7, 2019

Choose a reason for hiding this comment

3j14 commented Oct 8, 2019 • edited Loading

1st1 left a comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 8, 2019

3j14 commented Oct 10, 2019 • edited Loading

1st1 commented Oct 24, 2019

1st1 commented Oct 24, 2019

csabella commented Jan 26, 2020

jaymegordo commented Aug 30, 2020

3j14 commented Oct 1, 2019 •

edited by bedevere-bot

Loading

brandtbucher commented Oct 1, 2019 •

edited

Loading

3j14 commented Oct 7, 2019 •

edited

Loading

3j14 commented Oct 8, 2019 •

edited

Loading

3j14 commented Oct 10, 2019 •

edited

Loading