classification
Title: __dir__ semantics changed in Python 2.7.2
Type: behavior Stage:
Components: Interpreter Core Versions: Python 2.7
process
Status: closed Resolution: works for me
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: Arfrever, Trundle, barry, benjamin.peterson, eric.araujo, jcea, michael.foord, ncoghlan, r.david.murray, rhettinger, soren
Priority: release blocker Keywords: patch

Created on 2011-06-02 23:32 by barry, last changed 2011-06-10 16:19 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
foo.py barry, 2011-06-02 23:32
dir.patch rhettinger, 2011-06-04 18:13 Candidate patch for dir() in Py2.7 -- removes unnecessary restriction on __dir__ return type.
12248.diff barry, 2011-06-09 19:48 review
Messages (23)
msg137500 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2011-06-02 23:32
I'm making this a release blocker for 2.7.2 because I want to ensure that the change is deliberate and appropriate.  This is triggered by a bug report in Ubuntu where the change in behavior was noticed:

https://bugs.launchpad.net/nova/+bug/791221/+index

I have two problems with the change that I'd like to describe.  First is the NEWS file entry:

- Correct lookup of __dir__ on objects. Among other things, this causes errors
  besides AttributeError found on lookup to be propagated.

This isn't associated with a tracker issue, so it's hard to know why this change was made, or whether there was any discussion of its impact.

Second, is this change appropriate for a maintenance release?  What problem does it fix and is that worth breaking existing packages for it?

I'll attach a simple test program.  Run this under 2.7.1 and it works, run it under 2.7.2rc1 and you get a TypeError.
msg137501 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-02 23:56
As I recall the issue that triggered the change was reported by Michael Foord, so I'm adding him as nosy too.
msg137503 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-06-03 00:14
Hmm, that behaviour looks unrelated to the specific problem Michael reported.

The initial problem in this space was that defining __dir__() completely determined the result of dir() calls, but object.__dir__() didn't actually work, so you couldn't easily get the standard list of attributes in order to supplement it.

I don't believe there is any reason to have tightened up the type constraints while fixing that - dir() should be returning sorted(obj.__dir__()) and not caring about the exact return type of the magic method.
msg137504 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-03 01:42
> I don't believe there is any reason to have tightened up
> the type constraints while fixing that - dir() should be
> returning sorted(obj.__dir__()) and not caring about the 
> exact return type of the magic method.

+1
msg137587 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-03 21:03
No additional type-checking was added. The problem is that __dir__ didn't work on old-style classes at all in 2.7.1:

$ python
Python 2.7.1 (r271:86832, Mar 24 2011, 22:44:47)
[GCC 4.4.5] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
... def __dir__(self):
... return ['a', 'b', 'c']
...
>>> class Bar:
... def __dir__(self):
... return ('a', 'b', 'c')
...
>>> print dir(Foo())
['__dir__', '__doc__', '__module__']
>>> print dir(Bar())
['__dir__', '__doc__', '__module__']

Loosening type-checks on dir() is fine with me but is really a 3.3 issue.
msg137618 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-06-04 01:48
Ah, I wondered about that when I saw Barry was using old-style classes in his example. Perhaps the answer then is to add a PyInstance_Check() to skip invocation of __dir__() completely for old-style classes?
msg137619 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-04 01:52
2011/6/3 Nick Coghlan <report@bugs.python.org>:
>
> Nick Coghlan <ncoghlan@gmail.com> added the comment:
>
> Ah, I wondered about that when I saw Barry was using old-style classes in his example. Perhaps the answer then is to add a PyInstance_Check() to skip invocation of __dir__() completely for old-style classes?

Uh, well, part of point of my checkin was to fix old style class
__dir__(). I think this is a bug in the package we're just exposing.
msg137620 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-04 02:21
I would guess that if you instead skipped __dir__ completely for old style classes it would expose a different bug in this or some other package.
msg137621 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-06-04 02:26
It would be broken in the same way that it was broken in 2.7.1 though. That can be a plus when it comes to maintenance releases.

OTOH, this does turn a silent failure (__dir__() ignored on old-style classes) into a noisy failure (must return a list).

If you make Barry's classes new-style, they break in 2.7.1 as well, so I'm coming around to a point of view that this is a legitimate fix that reveals a real bug in third party code (i.e. anyone that hits this had a __dir__ that previously wasn't getting invoked)
msg137622 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-04 02:40
I believe the type check was gratuitous to begin with and should be removed.  There's no reason the result has to be a list.
msg137623 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-04 02:43
2011/6/3 Raymond Hettinger <report@bugs.python.org>:
>
> Raymond Hettinger <raymond.hettinger@gmail.com> added the comment:
>
> I believe the type check was gratuitous to begin with and should be removed.  There's no reason the result has to be a list.

The reason for it is that the sort() method is called on it.
msg137625 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-04 04:46
Can sorted() be used instead?
msg137627 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-04 05:03
or something like:

   result = obj.__dir__()
   if not isinstance(result, list):
       result = list(result)
   result.sort()
   return result
msg137648 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2011-06-04 15:29
Using sorted() makes sense to me.  

Note that I've at least accomplished one goal, which is to have a tracker issue that discusses the merits of the change.  That way, no matter what the RM decides, I can at least point to an issue for justification when I resolve the downstream bug. :)

Thanks!
msg137658 - (view) Author: Soren Hansen (soren) Date: 2011-06-04 18:42
When I first investigated this problem (I reported the original bug on Launchpad), my first attempt to address this issue in pymox had me quite stumped. The class in question has a __getattr__ method. Up until now, this hasn't affected the use of dir(), but it does now. I really just wanted it return whatever it used to return (since that has worked so far), but realising that this was an old-style class, I couldn't just call super(TheClass, self).__dir__().

So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?
msg137659 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-04 18:53
2011/6/4 Soren Hansen <report@bugs.python.org>:
>
> Soren Hansen <soren@linux2go.dk> added the comment:
>
> When I first investigated this problem (I reported the original bug on Launchpad), my first attempt to address this issue in pymox had me quite stumped. The class in question has a __getattr__ method. Up until now, this hasn't affected the use of dir(), but it does now. I really just wanted it return whatever it used to return (since that has worked so far), but realising that this was an old-style class, I couldn't just call super(TheClass, self).__dir__().
>
> So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?

Yes, this is a limitation of magic methods on old style classes. The
usual method is something like this:

    def __getattr__(self, name):
        if name == "__dir__":
            return self.__dir__
        # Other stuff

Of course, the best fix is to use new-style classes. :)
msg137663 - (view) Author: Soren Hansen (soren) Date: 2011-06-04 22:13
2011/6/4 Benjamin Peterson <report@bugs.python.org>:
> 2011/6/4 Soren Hansen <report@bugs.python.org>:
>> So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?
>
> Yes, this is a limitation of magic methods on old style classes. The
> usual method is something like this:
>
>    def __getattr__(self, name):
>        if name == "__dir__":
>            return self.__dir__
>        # Other stuff
>
> Of course, the best fix is to use new-style classes. :)

If I do this:

===== test.py ======
class Foo:
    def __getattr__(self, name):
        if name == '__dir__':
            return self.__dir__
        return 'something else'

a = Foo()
print dir(a)
====================

After a lot of this:
  File "test.py", line 4, in __getattr__
    return self.__dir__
  File "test.py", line 4, in __getattr__
    return self.__dir__
  File "test.py", line 4, in __getattr__
    return self.__dir__

...I end up with a "RuntimeError: maximum recursion depth exceeded". I
can't say I'm surprised :)
msg137664 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-04 22:18
2011/6/4 Soren Hansen <report@bugs.python.org>:
>
> Soren Hansen <soren@linux2go.dk> added the comment:
>
> 2011/6/4 Benjamin Peterson <report@bugs.python.org>:
>> 2011/6/4 Soren Hansen <report@bugs.python.org>:
>>> So my question is: If this change stays (which seems clear given that the only changes proposed here are ways of relaxing the type requirement of the __dir__ method's return value, not reverting the change altogether), and I have an old-style class with a __getattr__ defined, how do I make that class return whatever it would have usually returned for __dir__()?
>>
>> Yes, this is a limitation of magic methods on old style classes. The
>> usual method is something like this:
>>
>>    def __getattr__(self, name):
>>        if name == "__dir__":
>>            return self.__dir__
>>        # Other stuff
>>
>> Of course, the best fix is to use new-style classes. :)
>
> If I do this:
>
> ===== test.py ======
> class Foo:
>    def __getattr__(self, name):
>        if name == '__dir__':
>            return self.__dir__
>        return 'something else'
>
> a = Foo()
> print dir(a)
> ====================
>
> After a lot of this:
>  File "test.py", line 4, in __getattr__
>    return self.__dir__
>  File "test.py", line 4, in __getattr__
>    return self.__dir__
>  File "test.py", line 4, in __getattr__
>    return self.__dir__
>
> ...I end up with a "RuntimeError: maximum recursion depth exceeded". I
> can't say I'm surprised :)

Ah, sorry I should have thought before writing that. :)
self.__class__.__dir__.__get__(self, self.__class__) should work,
though.
msg137665 - (view) Author: Soren Hansen (soren) Date: 2011-06-04 22:30
2011/6/5 Benjamin Peterson <report@bugs.python.org>:
> 2011/6/4 Soren Hansen <report@bugs.python.org>:
>> ...I end up with a "RuntimeError: maximum recursion depth exceeded". I
>> can't say I'm surprised :)
> Ah, sorry I should have thought before writing that. :)
> self.__class__.__dir__.__get__(self, self.__class__) should work,
> though.

Yeah, that one does seem to work. Excellent, thanks!
msg138023 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2011-06-09 19:48
Raymond, I like your patch and I think it addresses the issue nicely.  I made one small change, which is to add a test for non-list-sequenceness instead of changing the existing __dir__is_list test.  I think both tests are useful to keep.

Benjamin, what do you think about this for 2.7.2?
msg138030 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-09 22:43
2011/6/9 Barry A. Warsaw <report@bugs.python.org>:
>
> Barry A. Warsaw <barry@python.org> added the comment:
>
> Raymond, I like your patch and I think it addresses the issue nicely.  I made one small change, which is to add a test for non-list-sequenceness instead of changing the existing __dir__is_list test.  I think both tests are useful to keep.
>
> Benjamin, what do you think about this for 2.7.2?

My preference is to leave 2.7 alone (actually all maintenance
releases) and apply the patch to 3.3. It seems to me the type
restricts are tangentially only related to the issue. There's no need
to change the behavior for new-style classes, too.
msg138048 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-06-10 04:46
My preference is to remove the type check.  It hasn't and won't serve a useful purpose so there's no reason to freeze it into the API and have it live on.

That being said, it probably doesn't matter one bit how this report gets resolved.
msg138102 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-06-10 16:19
I look at it this way: If I was browsing the 2.7 code and saw that type check, would I remove it in the bugfix release? No.

I'm going to mark this resolved. Thanks everyone.
History
Date User Action Args
2011-06-10 16:19:47benjamin.petersonsetstatus: open -> closed
resolution: works for me
messages: + msg138102
2011-06-10 04:46:57rhettingersetmessages: + msg138048
2011-06-09 22:43:40benjamin.petersonsetmessages: + msg138030
2011-06-09 19:48:24barrysetfiles: + 12248.diff

messages: + msg138023
2011-06-04 22:30:59sorensetmessages: + msg137665
2011-06-04 22:18:55benjamin.petersonsetmessages: + msg137664
2011-06-04 22:13:07sorensetmessages: + msg137663
2011-06-04 18:53:52benjamin.petersonsetmessages: + msg137659
2011-06-04 18:42:44sorensetnosy: + soren
messages: + msg137658
2011-06-04 18:13:29rhettingersetfiles: + dir.patch
keywords: + patch
2011-06-04 15:29:51barrysetmessages: + msg137648
2011-06-04 05:03:03rhettingersetmessages: + msg137627
2011-06-04 04:46:26rhettingersetmessages: + msg137625
2011-06-04 02:43:14benjamin.petersonsetmessages: + msg137623
2011-06-04 02:40:46rhettingersetmessages: + msg137622
2011-06-04 02:26:31ncoghlansetmessages: + msg137621
2011-06-04 02:21:45r.david.murraysetmessages: + msg137620
2011-06-04 01:52:41benjamin.petersonsetmessages: + msg137619
2011-06-04 01:48:55ncoghlansetmessages: + msg137618
2011-06-03 22:11:13Arfreversetnosy: + Arfrever
2011-06-03 21:03:02benjamin.petersonsetmessages: + msg137587
2011-06-03 18:03:04jceasetnosy: + jcea
2011-06-03 15:51:06eric.araujosetnosy: + eric.araujo
2011-06-03 01:42:43rhettingersetnosy: + rhettinger
messages: + msg137504
2011-06-03 00:46:13Trundlesetnosy: + Trundle
2011-06-03 00:14:51ncoghlansetnosy: + ncoghlan
messages: + msg137503
2011-06-02 23:56:22r.david.murraysetnosy: + r.david.murray, michael.foord
messages: + msg137501
2011-06-02 23:32:33barrycreate